| rkflx |
| Gwenview |
Gwenview does not display a custom folder icon which is configured
by an absolute path and the folder does not contain any images.
This is caused by KIconLoader::loadMimeTypeIcon() failing for
absolute path entries. If the folder contains images the preview
job corrects the icon later.
Using KIconLoader::loadIcon() loads the correct icon.
BUG: 315983
FIXED-IN: 18.04.2
| Before: | After: |
 |  |
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Dolphin also uses KIconLoader::loadIcon() - this version loads the same mime icons as KIconLoader::loadMimeTypeIcon() but correctly handles absolute path entries.
| lib/thumbnailview/thumbnailview.cpp | ||
|---|---|---|
| 618 | There's another issue with small custom icons (< ThumbnailGroup size; 128/256 pixel). If the folder contains images the overlay of mini thumbnails is created but immediately overdrawn with the base icon from KIconLoader::loadIcon(). This is caused by if (... thumbnail.mGroupPix.height() < groupSize) here (introduced in: 2a098d9a7874). I'm not sure why this was or is necessary - how could a bigger thumbnail appear later? The overlay is recreated and overdrawn repeatedly while moving the mouse over this thumbnail. | |
LGTM, apart from the mentioned small custom icon issue (see inline).
| lib/thumbnailview/thumbnailview.cpp | ||
|---|---|---|
| 618 |
The reason is hinted at lines 632-634 just below. I.e., without that clause, if I increase the icon size, the existing thumbnails don't get updated with the higher res icon. This results in the following. Tested by changing to smallest icons, opening the folder, then increasing to largest icons.  How to solve the issue with small custom icons though, I'm not sure. | |
| lib/thumbnailview/thumbnailview.cpp | ||
|---|---|---|
| 618 | Ahh - a user changing of the icon size - that's what I missed here. Thanks for the hint. :) | |
This is caused by KIconLoader::loadMimeTypeIcon() failing for absolute path entries.
Have not looked into it too deeply, but I think an important question to ask here is if this difference in behaviour compared to KIconLoader::loadIcon() is by design, or if it is a bug. If it is the latter, maybe we should actually fix the library?
I don't think it's a bug. KIconLoader::loadMimeTypeIcon() replaces the first / with - and calls loadIcon(). Maybe to avoid loading icons by absolute paths?
Sure, the implementation you linked to behaves as you observed. However, what's relevant here is what the API promises: "This is basically like loadIcon except that extra desktop themes are loaded if necessary." To me it sounds like it should find more icons, not less. Maybe at some point loading absolute paths was added to one function, but forgotten to be added to the other?
The question to ask here is whether there can be cases where loading the intended icon won't work anymore after the patch, because "extra desktop themes" supposedly aren't loaded anymore. OTOH it may very well turn out that most of KIconLoader is in some kind of buggy state in KF5, so your patch is probably okay too. Let me think more about it.
Looked more into this: 936683a40006 shows the basic relationship between mimetype and icon, e.g. application/pdf becomes application-pdf. This is not at all about absolute paths at this point.
f76fd2bb07a8 then adds something related to the above to loadMimeTypeIcon, however as soon as that function is called with an absolute path, it will fail. I suspect this should also test for whether the string starts with a slash (i.e. it is an absolute path), or at least the API docs should be changed to clarify the string argument should be a mimetype.
04724820c879 introduced a call to iconName(). The API docs claim that it returns "the full path name", but with qDebug() we see it returns either a mimetype or a full path (e.g. for a custom folder icon), the latter of which then leads to failure.
I don't think there's a problem in that regard with 7debda890fc2 or 2133d72e00ac.
Back to loadMimeTypeIcon(): This assumes loadIcon() can return an invalid pixmap, and falls back to application-octet-stream. Do we risk assigning a null pixmap if we directly ported to loadIcon()? No, because of canReturnNull defaulting to false.
As far as I can see, the slashindex check is not relevant for us, and when testing, addExtraDesktopThemes() (i.e. 527553685f32 and 1ba48c45f68e) did not seem to be necessary.
I'd say the patch is fine (although I'm not sure I fully understood all of this yet…).
Thanks for the profound explanations. :)
I also tested with a couple of mimetype icons from various icon themes that exists in both or only one theme and did not get any different results for loadMimeTypeIcon()/loadIcon().