Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

tsujan
Copy link
Member

@tsujan tsujan commented May 19, 2021

Closes #258

Notes:

  • For respecting Freedesktop standards, Qt's fallback search paths are taken into account only after inherited paths.
  • Colorizing of SVG icons is ignored with Qt's fallback paths because it depends on icon themes, not on paths.
  • To test it, you could do something like this in a code:
    QIcon::setFallbackSearchPaths(QIcon::fallbackSearchPaths() << PARENT_DIR_OF_EXTRA_ICON);
    anAction->setIcon(QIcon::fromTheme(EXTRA_ICON_NAME));

Closes #258

Notes:

 * For respecting Freedesktop standards, Qt's fallback search paths are taken into account only after inherited paths.
 * Colorizing of SVG icons is ignored with Qt's fallback paths because it depends on icon themes, not on paths.
 * To test it, you could do something like this in a code:

```c++
    QIcon::setFallbackSearchPaths(QIcon::fallbackSearchPaths() << PARENT_DIR_OF_EXTRA_ICON);
    anAction->setIcon(QIcon::fromTheme(EXTRA_ICON_NAME));
```
if (info.entries.isEmpty()) {
// Also, consider Qt's fallback search paths (which are not defined by Freedesktop)
// if the icon is not found in any inherited theme
const auto fallbackPaths = QIcon::fallbackSearchPaths();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should surround it with:

#if QT_VERSION >= QT_VERSION_CHECK(5, 11, 0)
...
#endif

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dropped Qt < 5.15.

Copy link
Member

@luis-pereira luis-pereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only took a glance, yet
Shouldn't

QIcon::fallbackSearchPaths()

just be added to the search paths when calling

QThemeIconInfo XdgIconLoader::unthemedFallback(const QString &iconName, const QStringList &searchPaths)

and that's it ?
I'm I missing something ?

@tsujan
Copy link
Member Author

tsujan commented May 19, 2021

@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
Copy link
Member

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
Copy link
Member Author

tsujan commented May 20, 2021

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
Copy link
Member Author

tsujan commented May 21, 2021

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
Copy link
Member

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
Copy link
Member Author

tsujan commented Oct 25, 2021

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

@tsujan tsujan merged commit 44038d7 into master Oct 25, 2021
@tsujan tsujan deleted the qt_fallback_search branch October 25, 2021 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XdgIconLoader ignores QIcon::fallbackSearchPaths()
3 participants