Review Request: Do not fallback to Hicolor when Oxygen provides a "-x-generic" version of an icon

[Aurélien Gâteau]

> On 2010-06-20 18:39:13, Michael Pyne wrote:
> > I'll not that KIconLoader is not "incorrect" in that the icon-theme-spec does specify that hicolor should be searched if the desired icon does not exist in a theme. shared-mime-info kind of overrides that by saying that if no specific icon is located, to search for a generic fallback icon, but that should only be mime icons.
> > 
> > One question is what do we do if hicolor happens to have an exact match of the required size, do we accept hicolor or try to fallback to generic? I would say accept hicolor's more specific video-mp4 in preference to video-x-generic, but that could look ugly in several themes.
> 
> Riccardo Iaconelli wrote:
>     As an artist, I'd say ship it. If the name spec says something different, I think we should change it.
>     I've not reviewed this patch code-wise though.
>     
>     For the second problem of yours, but still as an artistic advice, I'd say it's better to load the generic mime icon over accepting hicolor's specific version.

The "Icon Naming Specification" says this:
"The dash “-” character is used to separate levels of specificity in icon names, for all contexts other than MimeTypes. For instance, we use “input-mouse” as the generic item for all mouse devices, and we use “input-mouse-usb” for a USB mouse device. However, if the more specific item does not exist in the current theme, and does exist in a parent theme, the generic icon from the current theme is preferred, in order to keep consistent style."

If we interpret the part which says "for all contexts other than MimeTypes" as an explanation that for mimetypes, "/" is used rather than "-" (but actually "-" is also used), then it says that oxygen "video" (and I think, "video-x-generic" as well) should be preferred over hicolor "video-mp4".

Another way to fix this bug would be to copy/move "video-x-generic" to "video" in the oxygen theme. This is probably safer code-wise, but it does not really match the spec.


> On 2010-06-20 18:39:13, Michael Pyne wrote:
> > trunk/KDE/kdelibs/kdeui/icons/kiconloader.cpp, line 932
> > <http://reviewboard.kde.org/r/4403/diff/1/?file=29183#file29183line932>
> >
> >     Be careful with this, I'm pretty sure this code path can be run for any non-User icon, not just mime icons, so doing this replace would break icons under subdirectories that are currently found.

I thought "name" was always an icon name at this point. If it can be a path, then it's definitly wrong.


> On 2010-06-20 18:39:13, Michael Pyne wrote:
> > trunk/KDE/kdelibs/kdeui/icons/kiconloader.cpp, line 1068
> > <http://reviewboard.kde.org/r/4403/diff/1/?file=29183#file29183line1068>
> >
> >     Is there a reason to move generic fallback support into findMatchingIcon where it must be used for all icons? KIconLoader is plenty slow as it currently stands, I'm not sure this would help matters.

That's the whole point of the patch. findMatchingIcon() works like this:

findMatchingIcon():
 for theme in themes:
   while not name is empty:
     if theme has name:
       return name
     make name more generic

It is necessary to move the "-x-generic" code in here so that the "make name more generic" step can try the "-x-generic" version. Before the patch, findMatchingIconWithGenericFallbacks() worked like this:

findMatchingIconWithGenericFallbacks():
  icon = findMatchingIcon(name)
  if icon:
    return icon
  ...
  icon = findMatchingIcon(name + "-x-generic")
  if icon:
    return icon

This would not use "video-x-generic" for "video-mp4" because the first call to findMatchingIcon() finds the "video" icon in the "hicolor" theme.


- Aurélien


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4403/#review6198
-----------------------------------------------------------


On 2010-06-20 09:18:37, Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4403/
> -----------------------------------------------------------
> 
> (Updated 2010-06-20 09:18:37)
> 
> 
> Review request for kdelibs and Rafael Fernández López.
> 
> 
> Summary
> -------
> 
> When loading an icon for video-mp4, KIconLoader incorrectly returns "video.png" from Hicolor, instead of returning "video-x-generic.png" from Oxygen. Since Hicolor only has a 16x16 version, this results in blurry icons in Dolphin or Gwenview when icons are bigger than 16x16.
> 
> This is because the current code fallbacks to Hicolor before looking for a "-x-generic" version.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdeui/icons/kiconloader.cpp 1140302 
>   trunk/KDE/kdelibs/kdeui/tests/kiconloader_unittest.cpp 1140302 
> 
> Diff: http://reviewboard.kde.org/r/4403/diff
> 
> 
> Testing
> -------
> 
> - Added a unit-test entry to check we get video-x-generic.
> - Icons are no longer blurry after the patch.
> 
> 
> Thanks,
> 
> Aurélien
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100620/829409b9/attachment.htm>