search

lxqt / libqtxdg

Qt implementation of freedesktop.org xdg specs
https://lxqt.github.io
GNU Lesser General Public License v2.1
72 stars 35 forks source link

Consider Qt's fallback search paths when finding icons #259

Closed tsujan closed 2 years ago

tsujan commented 3 years ago

Closes https://github.com/lxqt/libqtxdg/issues/258

Notes:

    QIcon::setFallbackSearchPaths(QIcon::fallbackSearchPaths() << PARENT_DIR_OF_EXTRA_ICON);
    anAction->setIcon(QIcon::fromTheme(EXTRA_ICON_NAME));
tsujan commented 3 years ago

@luis-pereira unthemedFallback() is used only when findIconHelper() fails. But findIconHelper() may find a dash fallback, so that unthemedFallback() isn't called anymore. IMO, it's much better to find the whole name in Qt's fallback search paths, after it isn't found in the inherited themes.

luis-pereira commented 3 years ago

There's nothing special about the QIcon user provided fallback paths. We already have fallback search (unlike QIconLoader). By that logic all fallback paths should be searched before the dash fallbacks.

If a dash fallback is found in the current theme it will keep a consistent style. If it doesn't then it's a theme design fault. The icon naming guidelines states it. An unthemed fallback icon will most likely break consistency than a theme fallback dashed one.

tsujan commented 3 years ago

We had this discussion years ago.

Suppose a developer needs both drive-harddisk and drive-harddisk-encrypted but he wants to use theme icons as far as possible (which is good practice). He adds the second icon to Qt's fallback search paths because the gnome and adwaita icon themes lack it.

Then, this patch guarantees that drive-harddisk-encrypted is found. If unthemedFallback() was used, drive-harddisk would be found instead of it without Breeze, which would defeat the purpose.

Dash fallbacks should be used as the last resort; otherwise, they'll damge UX.

tsujan commented 3 years ago

Oh, while reading the comments again, I saw that I'd missed this good statement:

By that logic all fallback paths should be searched before the dash fallbacks.

Not all but, with QIcon::themeSearchPaths(), yes, I think it should come before dash fallbacks because, like QIcon::fallbackSearchPaths(), it's specific to Qt. I was aware of it but, since it was beyond the goal of this PR, I didn't touch it.

luis-pereira commented 2 years ago

Suppose a developer needs both drive-harddisk and drive-harddisk-encrypted but he wants to use theme icons as far as possible (which is good practice). He adds the second icon to Qt's fallback search paths because the gnome and adwaita icon themes lack it.

Then, this patch guarantees that drive-harddisk-encrypted is found. If unthemedFallback() was used, drive-harddisk would be found instead of it without Breeze, which would defeat the purpose.

Yes, it's found but there's no guarantee that it will match the theme. How can a developer guarantee that the drive-harddisk-encrypted will follow the visual style of the icon theme in use ? There is no way that I know of.

Dash fallbacks should be used as the last resort; otherwise, they'll damge UX.

Themes inheritances guarantee some visual similarities. Much more than a random drive-harddisk-encrypted.

I thought that this was already merged. We came to a "tie". In that situation I advocate that the one that writes the code gets to decide. Feel free to merge it.

tsujan commented 2 years ago

Giving priority to "meaning" over appearance, as it was done before. Merging, to prepare a release…