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

[Michael Pyne]

> 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.
> 
> Aurélien Gâteau wrote:
>     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.

OK, I didn't see the wording about icons other than mime type icons. It sounds perfectly cromulent to prefer Oxygen's video-x-generic over the parent theme's video-mp4, and your reasoning about findMatchingIcons() makes sense, so my only concern is the mangling of the icon relative path.


> 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.
> 
> Aurélien Gâteau wrote:
>     I thought "name" was always an icon name at this point. If it can be a path, then it's definitly wrong.

Looking at KIconLoader, the KIconLoader::iconPath() seems most relevant, and it appears to me that the icon name as passed to findMatchingIcon() can be a relative path still, although absolute paths have already been handled by that point. This is the only concern I have left, it may be easy to handle this case manually in KIconLoader::loadMimeTypeIcon() though.


- Michael


-----------------------------------------------------------
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/20100621/d68526b6/attachment.htm>