Fix insufficient icon theme issues
ClosedPublic

Authored by nmel on Feb 6 2018, 10:26 PM.

Details

Reviewers
martinkostolny
asensi
Group Reviewers
Krusader
Commits
R167:575f3e00a54d: Switched krLoader->loadIcon to Icon in case it's used as QIcon
R167:5f5f7477a603: Replaced QIcon::hasThemeIcon calls with Icon::exists; refactored icon search
R167:0a53a37417e2: Removed old commented code for handling icons
R167:02100ceabaa2: Switched from KIconLoader to Icon in ActionMan
R167:c741229623c1: Switched krLoader->loadIcon to Icon in case it's used as QPixmap
R167:9b5d31904fde: Fixed misuse of FL_LOADICON
R167:ab5226f0aeba: Switched app icon to Icon
R167:218489c8cd1b: Added Icon class that implements per-icon fallback logic
R167:10caec54a1d1: Replaced QIcon::fromTheme with Icon instances
R167:aeae6781ef02: Implemented icon cache and system theme change handling
R167:5d64608bc6a8: Removed KrGlobal::iconLoader and cleaned up KIconLoader includes
R167:4864e38a23be: Fixed names of variables, functions, members, methods related to icons
R167:94ea5b69af14: Refactored and unified icon rendering, implemented per-icon fallback logic
R167:9da2a28d6dd8: Used KIconLoader in icon search to improve mime icons
R167:0b4df738799f: Replaced FL_LOADICON with FileListIcon::pixmap
R167:44d37ecbada1: Replaced SmallIcon with Icon
R167:b475b1ac15f0: Fixed icons on Synchronizer buttons
R167:0ccf0748e534: Icon: improved comments and messages
R167:e90ea0ec104d: Icon: don't apply fallback in case of empty icon name
R167:8612eddd542e: Fixed icon for dragging multiple items between panels
R167:27d4082dcc6e: Fixed icons in User Action Examples
R167:e1828f60ccc8: Added loading fallback icon from resource file
R167:580ac5efe6b0: + Konfigurator option to specify a fallback icon theme
R167:85de0cec2734: Icon: Added overlay support and replaced KDE::icon in the project
Summary

Ultimate goal is to implement per-icon fallback logic:

  1. Search icon in the active icon theme — if found, use it.
  2. Search it in the icon set that is specified in Krusader config — if found, use it.
  3. Search it in Breeze (in case it's installed) — if found, use it.
  4. Otherwise use a default "not-found" icon (like a question mark). Opinions about what icon to use here are welcome!

FIXED: [ 372964 ] At least one Oxygen icon in Krusader is not correctly seen (using Kubuntu 16.04)
BUG: 372964

FIXED: [ 388691 ] Problems with "edit-select-none" icon and "edit-select" icon?
BUG: 388691

Test Plan

// TODO: update the test plan once the patch is ready

For now, you can check if it works as expected in your install. You may check what happens if you temporary uninstall Breeze.

Diff Detail

Repository
R167 Krusader
Branch
fix-missing-icons_arc
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes

I strongly disagree with this decision (and with Kexi ones too): remember that we should be able to use the application in any environment, and definitely Gnome users are not going to have those themes. Application specific icons should be provided by the application. Please go with 3 or at most 2b.

Application specific icons should be provided by the application.

Mmm... "edit-select-none" and "edit-select-invert" icons are included in Breeze and Oxygen, they are not Krusader-specific icons ( :-? ).

Alex Bikadorov wrote:

mixing different icon sets in one application is just ugly and should also never happen. I can't even remember I have ever seen this anywhere.

and people agreed.

Please go with 3 or at most 2b.

Although many people agree with 2a including me, I have to admit, that when any custom icon theme (compatible with Krusader needs) is created, it will not be used by Krusader when Breeze or Oxygen are also installed. Therefore I propose this:

2c) When Oxygen or Breeze are set as default, use them. When Oxygen and Breeze are not set as default, show a warning that icons may be missing. If one of them or both are installed but not used as default, show also a non-mandatory selectbox with them along with the warning. So the user can choose one of them, if he/she wants to. This way user will be notified about possible missing icons but not forced to use anything.

2d) Lighter version of the above would be just a warning message if Breeze or Oxygen are not set as default.

What do you think?

nmel added a comment.Feb 18 2018, 3:46 AM

Actually, I think it's a valid point that the app should work in any environment. In my opinion, the ideal solution would be something like per-icon fallback logic. First, search icon in the active icon theme — if found, use it. Second, search it in the icon set that is specified in Krusader config — if found, use it. Third, search it in Breeze (in case it's installed) — if found, use it. Otherwise use a default "not-found" icon (like a question mark). This way will make the Breeze set a weak dependency of Krusader and in the same time we don't have to copy (and maintain later!) every icon to the Krusader repository. This solves incomplete set problem (due to fallback logic) and icon set inconsistency problem (as user can always set Oxygen or Breeze in Krusader settings to have a consistent style while having his/her own system style). What do you think about this idea? How hard would it be to implement it?

In addition, do we know how other KDE apps solve this type of problem? It must be pretty common.

nmel added a comment.Feb 18 2018, 4:23 AM

BTW, just found a user's hassle and opinion about fallback logic for icons:
https://bugs.kde.org/show_bug.cgi?id=372966#c14
https://bugs.kde.org/show_bug.cgi?id=372966#c18

Please add BUG: 372966 to the commit message in case we're able to solve it.

Mixing different type of icons is not the end of the world, and that's why applications contains application-specific icons as the "hicolor" theme, which should more neutral. They can also ship application specific icons for other themes. It's like code: is the code generic enough that can be useful for others and put in a shared library? The difference here is that the common "code" (icons) should be added to all the implementation of the same library (the themes). Good or bad? That user commented that he prefer to force the dependency. I, on the other side, would prefer to have consistency as much as possible with the icon theme that I use (Oxygen in my case, but think about Gnome users). We are trying to do a lot to ensure consistency for applications (the entire reason for having Qt QPA, which should work out of the box and not be forced), icons should be the same.

I don't like the warning too - it should not be an invasive blocker at the first run. It's not a fatal error, so the program can start and this may be a question mark (which prompts the dialog box if clicked, and then goes away), or a KMessageWidget. That's IMHO. The main point is: don't force a specific icon theme.

(and please raise the question on more generic technical lists if you want to improve the icon loading and the fallback system).

In my opinion, the ideal solution would be something like per-icon fallback logic.

I spent some time to investigate this idea but it looks like a quite difficult task so far. One can set QIcon or QPixmap to buttons, tabs and other widgets. In case of QPixmap one you paint whatever you want. So you can query if icon exists and if it doesn't, set a different theme and create the icon from that. But you need to know its size and it is fixed since the creation. You sometimes probably want the application to decide the size dynamically, but I'm not sure about that. Also when system wide setting of icon theme changes, your pixmaps are not repainted. QIcon on the other hand is dynamic - widgets will paint them how they want and they will change when system-wide icon theme is changed.

KIconLoader will try to load icons from current icon theme. If they are not found, it will try to fallback to hicolor theme icons provided by the application. I believe this is currently the only proper way to solve incomplete icon-sets situations. It is like Luigi said. We should provide all the icons missing from https://specifications.freedesktop.org/icon-naming-spec/latest/ar01s04.html (please correct me if I'm wrong).

One can set extraSearchPaths to KIconLoader to provide additional paths to look for icons. But this did not work for me, don't know why.

Anyway I think one cannot set an ordered list of theme names to KIconLoader for fallback lookups. So we have yet another crossroad :)

  1. no patch
  2. add icons missing from standard icon set (e.g. copy from breeze / oxygen / else?)
  3. figure out how to set custom fallback icon theme for missing icons - meaing already installed theme like breeze or oxygen

I like the last option most (I'm also fine with the other ones), but for now I'm puzzled. If nobody knows here, I'll ask on some KDE technical forum for a guidance, like Luigi advised.

nmel added a comment.Feb 22 2018, 8:27 AM

Martin, thanks for investigation and for writing details about it. I propose we ask at the appropriate mailing list or forum anyway, just to make sure we are not trying to reinvent the wheel. There should be a lot of other KDE apps with the same problem and it's likely their devs already solved the issue. If not, we can try to extend KIconLoader for supporting the logic we need.

nmel requested changes to this revision.Mar 4 2018, 5:35 AM

Hi Martin,

Have you asked about this on any board or list? If so, please share the link or a summary. I can start a thread if you want, just let me know if you don't have time for this. It would be nice to include the fix into v2.7.

Thanks,
Nikita.

This revision now requires changes to proceed.Mar 4 2018, 5:35 AM
nmel commandeered this revision.Apr 2 2018, 7:15 AM
nmel edited reviewers, added: martinkostolny; removed: nmel.

Taking control per agreement with Martin.

nmel planned changes to this revision.Apr 2 2018, 7:29 AM

I was working on a proof of concept for per-icon fallback logic that I've described earlier and I had been 2/3 way done when I did a regular update of my system. I've got Qt, KF5 updates as newer versions got stabilized and the issue had stopped reproducing. I reproduced it the following way:

  1. Select Adwaita icon theme.
  2. Reopen Krusader.
  3. Finding whether Bookmarks icon is displayed or not.

It was not on a system before upgrade. After upgrade it shows Breeze variant although almost all other icons are from Adwaita.

Mu upgrade looks like this:
Qt: 5.7.1 -> 5.9.4
KF: 5.40 -> 5.43
kde-apps: 17.08.3 -> 17.12.3

Can anyone confirm whether the icon issues are resolved with these lib versions in non-KDE environment or not?

asensi added a comment.EditedApr 3 2018, 8:10 AM

My upgrade looks like this:
Qt: 5.7.1 -> 5.9.4
KF: 5.40 -> 5.43
kde-apps: 17.08.3 -> 17.12.3

Can anyone confirm whether the icon issues are resolved with these lib versions in non-KDE environment or not?

If it may be useful:

I attach a screenshot of Krusader in a Ubuntu 16.04 virtual machine (with old versions of Qt and KF):

I attach a screenshot of Krusader in a Ubuntu 18.04 daily build (from http://cdimage.ubuntu.com/daily-live/20180402/bionic-desktop-amd64.iso) virtual machine:


In that Ubuntu 18.04 daily build virtual machine, Qt 5.9.4 and KF 5.44 are used:

$ dpkg -l | grep qt5core
ii libqt5core5a:amd64 5.9.4+dfsg-0ubuntu4 amd64 Qt 5 core module

$ dpkg -l | grep -i libkf5coreaddons5
ii libkf5coreaddons5:amd64 5.44.0a-0ubuntu1 amd64 KDE Frameworks 5 addons to QtCore

asensi added a comment.EditedApr 3 2018, 11:53 PM

In the same Ubuntu 18.04 daily build virtual machine, after applying the present differential:

If Breeze is not installed, this is a screenshot of what happens when executing Krusader:

and after the user accepts:

Though, after executing sudo apt install breeze, Krusader seems to look as it was intended:

In D10352#211406, @nmel wrote:

Martin, thanks for investigation and for writing details about it. I propose we ask at the appropriate mailing list or forum anyway, just to make sure we are not trying to reinvent the wheel. There should be a lot of other KDE apps with the same problem and it's likely their devs already solved the issue. If not, we can try to extend KIconLoader for supporting the logic we need.

Hey there, I was just randomly wandering around in Phabricator and stumbled across this. I'm going to stick my neck out here... Please, do not force icons in your app. As a user of a "3rd party" theme, I would be very unhappy, as it violates Freedesktop specs.

The various icon themes out there follow the freedesktop theme spec which defines an "Inherits" key. So for any icon not found in the currently enabled theme, it will search the other specified themes, if installed. So for example, Papirus inherits breeze, ubuntu-mono-dark, gnome, and hicolor. That covers most bases for ensuring the proper icon is loaded. Breeze inherits hicolor as an additional example. If someone is running a theme that does not include a proper inherit key, that is on the icon theme to fix that in the index.theme file.

All you need to do is let KIconLoader or QIcon::fromTheme do its thing, and it will handle the searching of the icons based on the index.theme file.

nmel added a comment.Apr 4 2018, 4:33 AM

Hi Andrew,

Please, do not force icons in your app. As a user of a "3rd party" theme, I would be very unhappy, as it violates Freedesktop specs.

Don't worry. We already agreed not to force Breeze. This is just the old patch, I haven't updated it yet because I haven't complete my work. I will update the patch and the title shortly to avoid confusion. Currently I'm trying to implement a proof of concept of the fallback logic I proposed in D10352#208536:

In D10352#208536, @nmel wrote:

In my opinion, the ideal solution would be something like per-icon fallback logic. First, search icon in the active icon theme — if found, use it. Second, search it in the icon set that is specified in Krusader config — if found, use it. Third, search it in Breeze (in case it's installed) — if found, use it. Otherwise use a default "not-found" icon (like a question mark). This way will make the Breeze set a weak dependency of Krusader and in the same time we don't have to copy (and maintain later!) every icon to the Krusader repository. This solves incomplete set problem (due to fallback logic) and icon set inconsistency problem (as user can always set Oxygen or Breeze in Krusader settings to have a consistent style while having his/her own system style).

Then I'll post it here to gather opinions.

The various icon themes out there follow the freedesktop theme spec which defines an "Inherits" key. So for any icon not found in the currently enabled theme, it will search the other specified themes, if installed. So for example, Papirus inherits breeze, ubuntu-mono-dark, gnome, and hicolor. That covers most bases for ensuring the proper icon is loaded. Breeze inherits hicolor as an additional example. If someone is running a theme that does not include a proper inherit key, that is on the icon theme to fix that in the index.theme file.

I'm aware of this, however it doesn't solve the problem we have: by default non-KDE users experience the lack of icons. Why? Because we use a set that is larger than Freedesktop set and we don't control theme inheritance as it's governed by a theme of interest. In the same time, this larger set is covered by Breeze or Oxygen in full.

All you need to do is let KIconLoader or QIcon::fromTheme do its thing, and it will handle the searching of the icons based on the index.theme file.

If you could help us set this up, it will be awesome! Both Martin and I haven't solved it though a standard interface. I read some code, mainly of KIconLoader, and it seems these classes don't support this case. I encourage you to read the whole thread to understand the context. We don't want to copy icons from Breeze to Krusader repository.

nmel retitled this revision from Force Breeze icon theme. to Fix insufficient icon theme issues.Apr 4 2018, 4:37 AM
nmel edited the summary of this revision. (Show Details)
nmel edited the test plan for this revision. (Show Details)
nmel added a project: Krusader.
nmel updated this revision to Diff 31256.Apr 4 2018, 4:42 AM
  • Removed code that forces Breeze
nmel added a comment.Apr 4 2018, 5:05 AM

Since the discussion is a bit scattered, I want to mention that Martin did ask on kde-frameworks-devel: https://mail.kde.org/pipermail/kde-frameworks-devel/2018-March/058241.html

Martin:

Hi!

We at Krusader would like to address a rather standard issue of applications - missing icons. And we would like an opinion of you as experts :).

Starter discussion (which led in here) is on Krusader's phabricator: https://phabricator.kde.org/D10352

Krusader is using a lot of icons existing in Breeze or Oxygen themes. But e.g. for Gnome users there are a lot of missing icons. We don't want to add all the non-standard icons to hicolor theme attached to Krusader. Especially since they already exist in Breeze or Oxygen. Instead we would like to specify a fallback theme - e.g. Breeze, so every missing icon from currently set theme would be loaded from the fallback theme (if it is installed of course).

I believe this functionality is currently not supported by KIconLoader but I may be wrong. I have only found a possibility to add an additional path to icons (extraSearchPaths) which is probably not what we want.

To give an example, I'd expect something like this to be a possibility for a programmer:

KIconLoader *iconLoader = KIconLoader::global();
QStringList fallbackThemes;
fallbackThemes << "breeze-dark";
fallbackThemes << "oxygen";
iconLoader->setFallbackThemes(fallbackThemes);

Any guidance or another opinion is appreciated. Thanks!
Martin

Albert:
Yeah, i don't think such functionality exists, some people may even say that's
undesirable since it may cause weird visual "identity" on when mixing icon
themes.

Once functionality that does exist is forcing the icon theme (in QIcon),
that's also a bit weird since it overrides the user selection which can be
seen as a bit aggresive, but if you *really* need the icons it may be a
possibility.

That's the other question, do you really need the icons or is it just a nice
to have?

Cheers,
Albert

Matrin:
Thanks, Albert, for your answer and point of view!

We actually need the icons since quite a lot of them is misisng and Krusader looks (e.g. with Adwaita) quite unfinished with default configuration. We also get bug reports about missing icons.

From your opinions and opinions I've already heard - mixing icon themes is not usually preferable and forcing breeze or oxygen is not what user expects or wants either. Therefore the best would be allowing a user to configure icon theme specifically for Krusader from Krusader's config. And, ideally (in the long run), design all the missing non-standard icon images for hicolor theme in Krusader.

If there is a better way of solving our situation, please tell us :).

Thanks!
Martin

Harald:
Someone needs to file bug reports against adwaita so the missing icons
can get added.
That is the proper solution to an incomplete icon set.

It's a game of pick your poison:

  • Inject breeze as "fallback": icons are now an inconsistent mix of adwaita and breeze making for poor integration with workspace
  • Don't inject breeze: icons are consistent but some are missing
  • Force breeze: icons are consistent and none are missing but now the integration with the workspace is poor

    If adwaita grows the missing icons: the icons will be consistent, none will be missing and the icons will match the rest of the workspace.

    HS
nmel added a comment.Apr 4 2018, 6:18 AM

I attach a screenshot of Krusader in a Ubuntu 16.04 virtual machine (with old versions of Qt and KF):

I attach a screenshot of Krusader in a Ubuntu 18.04 daily build (from http://cdimage.ubuntu.com/daily-live/20180402/bionic-desktop-amd64.iso) virtual machine:


In that Ubuntu 18.04 daily build virtual machine, Qt 5.9.4 and KF 5.44 are used:

Toni, thanks a lot for testing this! Quick question: is Breeze installed in both of these cases or not?

It appears to be a good idea to have VMs with various systems for testing, especially given that testing from KDE on my machine is problematic now.

asensi added a comment.Apr 4 2018, 8:44 AM

Toni, thanks a lot for testing this! Quick question: is Breeze installed in both of these cases or not?

Hi, Nikita! Thanks for asking in order to make it clearer. Breeze was not installed in neither case. After the aforementioned comment I added another one (https://phabricator.kde.org/D10352#239438) .

It appears to be a good idea to have VMs with various systems for testing, especially given that testing from KDE on my machine is problematic now.

Yes :-) . In those cases it's also very useful the "go back to the previous state"-"restore snapshot" feature that virtual machines have :-)

nmel added a comment.Apr 5 2018, 6:05 AM

Toni, thanks a lot for testing this! Quick question: is Breeze installed in both of these cases or not?

Hi, Nikita! Thanks for asking in order to make it clearer. Breeze was not installed in neither case. After the aforementioned comment I added another one (https://phabricator.kde.org/D10352#239438) .

Thanks for clarification. The following comment was about applying the current patch, which enforces Breeze. I was interested to see if new libs load Breeze variant (in case installed) without the patch. Anyway, I almost done setting up VM, I can check by myself soon. Thanks for your help!

nmel added a comment.Apr 6 2018, 5:52 AM

Core functionality is ready — I'll update the diff in the CR shortly for a preview. It provides a new Icon class that handles the fallback logic. I only replaced a couple of icons for testing: bookmark icon and folder history icon that are missing on Ubuntu. Please check the screenshots from my Ubuntu VM:

This is baseline (v2.6.0), Breeze is installed:

This is treatment, i.e. this patch on master, Breeze is installed:

This is treatment, i.e. this patch on master, Breeze is not installed:

In case you like the way my code works, I will implement the rest: settings, fallback icon file, replace every QIcon in the app with an Icon instance.
Please be honest about what you think about this work early, especially if you don't like it! It will save my time. :)

nmel updated this revision to Diff 31461.Apr 6 2018, 5:56 AM

preview of the Icon class and its usage

nmel edited the summary of this revision. (Show Details)Apr 6 2018, 6:05 AM
nmel edited the test plan for this revision. (Show Details)
nmel added a reviewer: asensi.
asensi added a comment.Apr 6 2018, 2:43 PM

When using Kubuntu 17.10:

Tests went as expected .

When using Ubuntu 18.04 daily build:

Tests went as expected (the results were like in the screenshots that Nmel published).

One thing: if Breeze was not installed, but the Oxygen icons were installed (using the "oxygen-icon-theme" package), then the result was this:


but if after

_themeFallbackList << "breeze"

it was added

<< "oxygen"

then {the bookmark icon and folder history icon} were seen correctly:


:-)

Let's notice that mantainers of the Oxygen icons have added Krusader icons to Oxygen when it was requested.

Nikita, I like this approach, thanks for working on this! Sorry it took me so long.

I'm a bit worried if this will work when there is theoretically more then one thread taking care of painting icons across the application. Since QIcon::setThemeName and QIcon::fromTheme are static, there could be races. So maybe we can add a few QMutex::lock() / QMutex::unlock() later if issues arise.

And I actually have one issue. Although I have breeze installed, I don't get "chronometer" icon with this patch. It is always the fallback one. The "bookmarks" icon is fine though. This is most weird. I've added the mutexes and debug messages to narrow down the problem but nothing changed. My use case is exactly this:

  1. I have Plasma (git master version) and Krusader master with this patch
  2. go to Settings -> Icons and set "Adwaita"
  3. start Krusader
  4. I can see adwaita icons, missing icons, bookmarks are rendered correctly from "breeze", but "chronometer" is rendered as a fallback icon

If I set my icon theme back to breeze (in settings), all the icons are rendered from breeze along with "chronometer".

Also based on my debug messages calling QIcon::hasThemeIcon("chronometer") will yield false when system theme is not "breeze". Even if one line before we call QIcon::setThemeName("breeze"). Like I said, this was also tested with static QMutex to ensure no races.

Does anybody have any idea what can be wrong on my side?

I've tested also in Ubuntu 17.10, where this problem did not happen and "chronometer" was rendered from breeze correctly. So maybe there is a bug in Qt 5.10+, I try to investigate...

Anyway I agree with this direction and hopefully this is a bug in toolkit.

nmel added a comment.Apr 7 2018, 10:47 PM

Thanks Toni and Martin for testing!

I'm a bit worried if this will work when there is theoretically more then one thread taking care of painting icons across the application. Since QIcon::setThemeName and QIcon::fromTheme are static, there could be races. So maybe we can add a few QMutex::lock() / QMutex::unlock() later if issues arise.

Don't worry, Qt runs UI code in a single thread (1, 2, 3) called GUI thread. In Krusader we use additional (worker) threads only in synchronizer and packer. It doesn't conflict with this code.

And I actually have one issue. Although I have breeze installed, I don't get "chronometer" icon with this patch. It is always the fallback one. The "bookmarks" icon is fine though. This is most weird.

  1. I have Plasma (git master version) and Krusader master with this patch
  2. go to Settings -> Icons and set "Adwaita"
  3. start Krusader
  4. I can see adwaita icons, missing icons, bookmarks are rendered correctly from "breeze", but "chronometer" is rendered as a fallback icon

    If I set my icon theme back to breeze (in settings), all the icons are rendered from breeze along with "chronometer".

Thanks for reporting. I confirm this on Qt 5.9. I agree it's very weird. I will also try to investigate.

nmel updated this revision to Diff 31650.Apr 8 2018, 5:16 AM
  • Added workaround for Qt icon invalid caching issues
  • Added oxygen to the fallback list
nmel added a comment.Apr 8 2018, 5:25 AM

This is definitely a Qt bug. I need to file it to Qt tracker later — please remind me if I forget.
I found a workaround after a few hours of debugging and reading Qt code. Please help to test. It passes my tests now (both in VM and in host env).
I wonder what issues we'll discover once I start converting other icons to Icon instances... :/

Fine work, Nikita, now it works! Thanks for the investigation:). I've tested it in Plasma (on Arch linux) as well as in Ubuntu 17.10 VM.

The links about threads are interesting, thanks for pointing it out.

nmel added a comment.Apr 8 2018, 6:17 PM

Thanks for testing!

To everyone: Let's discuss the fallback icon. Do you like "emblem-unreadable" or you have another preference?

I'm fine with "emblem-unreadable" although my preference would rather be "image-missing" :).

nmel added a comment.Apr 8 2018, 7:48 PM

I'm fine with "emblem-unreadable" although my preference would rather be "image-missing" :).

I started with "image-missing" initially and it looked different from what expected to see. I think it sometimes falls back to "image" due to freedesktop spec. What does it look like on your end?

Maybe we need to use something neutral like circle or diamond... With these missing/unreadable/broken icons the app looks like a broken one while it's not...

Also, I guess we better save a particular image we pick into the resource file, so we can load it for sure. Are you fine with it?

Maybe we need to use something neutral like circle or diamond... With these missing/unreadable/broken icons the app looks like a broken one while it's not...
Also, I guess we better save a particular image we pick into the resource file, so we can load it for sure. Are you fine with it?

Yes, packing it inside Krusader would probably be ideal with e.g. "kr_missingicon" name. In that case I'm for "emblem-question" from Breeze or "dialog-question-symbolic" from Adwaita. It's just a suggestion of course :). It is true that the app will maybe look "broken" with the questionmarks but it will tell the true icon state -> missing/unknown.

asensi added a comment.Apr 9 2018, 8:23 PM

I'm fine with "emblem-unreadable" although my preference would rather be "image-missing" :).

Yes, a lot of help buttons have a question mark inside, so when I see an icon with a question mark inside I think that it's a button to get help.

Instead, at least using internet browsers, when I see an "image-missing" icon, that shows that this error happened.

nmel updated this revision to Diff 32037.Apr 13 2018, 7:26 AM
  • Replaced QIcon::fromTheme with Icon instances
  • Added loading fallback icon from resource file
  • Fixed icons in User Action Examples
  • Implemented icon cache and system theme change handling
  • + Konfigurator option to specify a fallback icon theme
  • Switched app icon to Icon
  • Replaced QIcon::hasThemeIcon calls with Icon::exists; refactored icon search
  • Fixed icons on Synchronizer buttons
  • Switched krLoader->loadIcon to Icon in case it's used as QIcon
  • Switched krLoader->loadIcon to Icon in case it's used as QPixmap
  • Switched from KIconLoader to Icon in ActionMan
  • Fixed misuse of FL_LOADICON
  • Replaced FL_LOADICON with FileListIcon::pixmap
  • Replaced SmallIcon with Icon
  • Used KIconLoader in icon search to improve mime icons
  • Icon: improved comments and messages
  • Removed KrGlobal::iconLoader and cleaned up KIconLoader includes
  • Icon: don't apply fallback in case of empty icon name
  • Fixed icon for dragging multiple items between panels
nmel added a comment.Apr 13 2018, 7:42 AM

I implemented the rest and fixed all the issues I was aware of. As you may notice from the commit message headers, Icon class now unifies loading with QIcon, KIconLoader (aka krLoader that is gone now), ICON macro, FL_LOADICON, SmallIcon etc. Unfortunately, a small number of places were not converted due to the use of overlays. I plan to work on this a little later — I wanted to update the CR early so you can provide feedback.

Please test and let me know if you see any missing icons or the fallback icon where the icon should be found. Thanks!

NOTE: The file moves in diff were determined incorrectly, please ignore.
nmel updated this revision to Diff 32095.Apr 14 2018, 6:05 AM
  • Icon: Added overlay support and replaced KDE::icon in the project
nmel updated this revision to Diff 32096.Apr 14 2018, 6:06 AM
  • Fixed names of variables, functions, members, methods related to icons
nmel added a comment.Apr 14 2018, 6:16 AM

Alright, this is it. Please help to test.

Testing hint: I added a qWarning in case the fallback icon is used. After you browse you can check console output for this type of lines:

warning default IconEngine::pixmap@264 # Unable to find icon "view_text" of size QSize(16, 16) in any configured theme

This way you'll catch a problem even if you missed the broken icon visually.

NOTE: I updated user action examples but you may have a copy of old examples in your config. To refresh you need to erase/replace ~/.local/share/krusader/useractions.xml.
asensi added a comment.EditedApr 14 2018, 3:34 PM

Thanks, Nikita! A lot of work! I also wanted to say that, after applying the diff file, using Ubuntu 18.04 daily build, I saw that error message when building Krusader (because the "icon-missing.svgz" file was not found):

[ 84%] Built target krusader_autogen
make[2]: *** No hay ninguna regla para construir el objetivo '/home/user/krusader/krusader/icons/icon-missing.svgz', necesario para 'krusader/qrc_resources.cpp'.  Alto.
CMakeFiles/Makefile2:222: recipe for target 'krusader/CMakeFiles/krusader.dir/all' failed
make[1]: *** [krusader/CMakeFiles/krusader.dir/all] Error 2
Makefile:140: recipe for target 'all' failed
make: *** [all] Error 2
asensi added a comment.EditedApr 14 2018, 5:02 PM

With the proposed changes:

The appearance of Krusader is improved when using Ubuntu 18.04 daily build (without having Breeze nor Oxygen installed):

In the command line those lines were seen:

However, using Kubuntu 17.10 the icons are "greyed out":

In the command line those lines were seen:

$ krusader
18:23:16.244-warning default unknown@0 # QWidget::insertAction: Attempt to insert null action
18:23:16.253-warning default unknown@0 # QWidget::insertAction: Attempt to insert null action
18:23:16.352-warning default unknown@0 # Trying to convert empty KLocalizedString to QString.

Usually, under Kubuntu 17.10 the icons were the expected ones (not "greyed out"):

staniek removed a subscriber: staniek.Apr 14 2018, 7:30 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 15 2018, 4:02 AM
This revision was automatically updated to reflect the committed changes.
nmel reopened this revision.Apr 15 2018, 4:13 AM

Hi Toni, Thanks a lot for testing.

I pushed the branch to ease testing the change. You can use fix-missing-icons branch (currently, same as the diff here but you don't have to copy icon-missing.svgz manually).

I'm reopening revision - it's accidentally closed by some hook, although I didn't push "Differential revision:" comment. Phabricator is still a mystery to me sometimes.

nmel added a comment.Apr 15 2018, 4:23 AM

However, using Kubuntu 17.10 the icons are "greyed out":

Usually, under Kubuntu 17.10 the icons were the expected ones (not "greyed out"):

I can't repro the grayed out icons issue yet. I tried to switch my host system to Breeze Dark, the icons look white and identical to the master version:


Does Kubuntu come with the dark theme by default? Probably I have to setup this VM as well :(.

For completeness, I'm sharing screenshots from my Ubuntu-17.10 VM with breeze installed:


nmel updated this revision to Diff 32165.Apr 15 2018, 4:27 AM

Fixing revision diff.

Thanks Nikita for your big work! :) It works nicely. I have 2 observations:

  1. choosing dark theme not working in dark environment - I'm proposing a simple solution - please see my code comment
  2. icon "application-x-cmakecache" (e.g. for file CMakeCache.txt inside krusader/build folder) is now rendered as a fallback icon (even with breeze) although previously it was rendered as "unknown"
    • I'm not sure if this is even solvable, or if it is important; I was currently unable to come up with a solution

Regarding the greyed out icons in Krusader: I believe this is because "breeze" is set as system theme (not "breeze-dark") and fallback is also set as "breeze", so there is no "breeze-dark" to fallback to.

  • but previously it did not matter for some reason. Usually applications are automatically choosing breeze-dark in dark environment (e.g. in Dolphin and previously in Krusader as well)
  • so there is probably a bug in Qt icon handling in this regard
  • I can replicate this behaviour in Arch Linux as well
  • this behaviour is probably related to the second mentioned observation

I will study this behaviour further and try to come up with something more concrete.

krusader/icon.cpp
58

Here I'd choose dynamically between dark and light breeze theme:

const QColor currentColor = QPalette().brush(QPalette::Text).color();
const bool lightThemeEnabled = (currentColor.red() + currentColor.green() + currentColor.blue()) / 3 < 128;
const QString chosenBreezeTheme(lightThemeEnabled ? "breeze" : "breeze-dark");
themes << chosenBreezeTheme << "oxygen";

Regarding the greyed out icons in Krusader: I believe this is because "breeze" is set
as system theme (not "breeze-dark") and fallback is also set as "breeze", so
there is no "breeze-dark" to fallback to.

Thanks, Martin and Nikita! If it may be useful: "Breeze" was set as system theme (not "breeze-dark"); if "breeze-dark" was set as system theme then Krusader was seen as it was expected.

[More important information]

I will study this behaviour further and try to come up with something more concrete.

Great!

nmel added a comment.Apr 16 2018, 7:02 AM

Thanks Nikita for your big work! :) It works nicely. I have 2 observations:

  1. choosing dark theme not working in dark environment - I'm proposing a simple solution - please see my code comment

It works if user specifies breeze-dark as the fallback theme in settings. I agree it would be nice to do this automatically. Your patch should work but also we need to make default of the fallback theme in settings empty, because the user's fallback theme takes precedence. Would you like to push it under your name to the remote branch?

  1. icon "application-x-cmakecache" (e.g. for file CMakeCache.txt inside krusader/build folder) is now rendered as a fallback icon (even with breeze) although previously it was rendered as "unknown"
    • I'm not sure if this is even solvable, or if it is important; I was currently unable to come up with a solution

I can repro this. I'll try to check what happens.

Regarding the greyed out icons in Krusader: I believe this is because "breeze" is set as system theme (not "breeze-dark") and fallback is also set as "breeze", so there is no "breeze-dark" to fallback to.

  • but previously it did not matter for some reason. Usually applications are automatically choosing breeze-dark in dark environment (e.g. in Dolphin and previously in Krusader as well)

It still works fine in my system. I'm not sure why it doesn't work in other systems.

  • so there is probably a bug in Qt icon handling in this regard
  • I can replicate this behaviour in Arch Linux as well

It's good that you're able to repro. If you have time, can you bisect the branch to see what commit breaks this?

Thanks, Martin and Nikita! If it may be useful: "Breeze" was set as system theme (not "breeze-dark"); if "breeze-dark" was set as system theme then Krusader was seen as it was expected.

It may be useful, thanks for letting us know. I looked at your screenshots again. Bookmarks and chronometer are greyed out too. They were added through the QIcon::fromTheme call previously which is basically what Icon is doing now with the exception that we are setting theme from code (starting with the workaround in main.cpp and then in searchIcon) and painting icons by ourselves. I bet one of these differences is the reason and I think the first one is likely.

nmel updated this revision to Diff 32446.Apr 18 2018, 7:37 AM
  • Fixed missing icons in the file list (KrView)
  • Updated icon-missing icon to be similar in style to the Breeze icon theme
  • Added dynamic selection of light or dark Breeze fallback theme
  • Replaced KIconLoader::loadMimeTypeIcon with Icon
  • Added workaround for Breeze variant selection based on theme lightness
  • Icon: improved a method name and a comment
nmel marked an inline comment as done.Apr 18 2018, 7:46 AM

Resolved all outstanding issues, please check on your end.
Also updated 'icon-missing' icon — please let me know if you like it better or not. If you have a better icon, please attach a particular svg/svgz file you're interested in.

I updated the remote branch accordingly — just checkout and build it. The diff in CR is updated for the purpose of consistency of inline comments you may have.

asensi added a comment.EditedApr 19 2018, 8:05 AM

With the latest improvements made by Nikita, In an Ubuntu 18.04 daily build virtual machine, and in a Kubuntu 17.10 virtual machine, the icons were shown as it was expected:

Ubuntu 18.04 daily build, without having installed Breeze nor Oxygen icons:


Ubuntu 18.04 daily build, after installing the oxygen-icon-theme package:


Ubuntu 18.04 daily build, after installing Breeze (using the breeze-icon-theme package) and Oxygen icons:


Kubuntu 17.10 using the Breeze icons (the result was the same when using «Breeze-Dark»):


Kubuntu 17.10 using the Oxygen icons:


nmel added a comment.Apr 19 2018, 9:13 PM

Thanks for testing, Toni!
Martin, please let me know if you spot any problem.

Take your time, I understand this is a rather big change. If you can browse various dialogs and elements (like quicksearch) where you expect to see icons, it would be nice. If you have a dark theme != breeze to test automatic fallback to breeze-dark, it would be nice too.

asensi added a comment.EditedApr 20 2018, 8:17 PM

More screenshots, using included icons in Kubuntu 17.10:

Oxygen icons using Kubuntu 17.10 - bookmark icons

Breeze icons using Kubuntu 17.10 - bookmark icons

Breeze-Dark icons using Kubuntu 17.10 - bookmark icons

Humanity icons using Kubuntu 17.10

Humanity icons using Kubuntu 17.10 - bookmark icons

Humanity-dark icons using Kubuntu 17.10

Humanity-dark icons using Kubuntu 17.10 - bookmark icons

LoginIcons icons using Kubuntu 17.10

LoginIcons icons using Kubuntu 17.10 - bookmark icons

Ubuntu-mono-dark icons using Kubuntu 17.10

Ubuntu-mono-dark icons using Kubuntu 17.10 - bookmark icons

Ubuntu-mono-light icons using Kubuntu 17.10

Ubuntu-mono-light icons using Kubuntu 17.10 - bookmark icons

A special case: using Adwaita icons

Adwaita icons using Kubuntu 17.10 (the first icons are not seen)

Adwaita icons using Kubuntu 17.10 - bookmark icons

Adwaita icons using Kubuntu 17.10 and Dolphin (almost all icons are not seen)

Sorry for my late response. Thanks Nikita for fixing all the issues and Toni for useful testing screenshots.

To answer one previous questinon:

Would you like to push it under your name to the remote branch?

No, I'm glad you did this. It wasn't originally my idea anyway. I've copied this logic from somewhere.

Now to the testing:
I have spotted one minor issue. When window theme is dark, disabled state of icons is not rendered properly. To be exact it does not fade to lower opacity like it normally does when no "magic" is done with icon themes. Interesting points:

  1. It works fine with light window colours
  2. All tested icon themes are affected (Oxygen, nuoveXT, Adwaita, even breeze/breeze-dark)
  3. Disabled state is partially working though - e.g. colours are changged to b/w

So basically in dark KDE environment (Breeze Dark colours and Breeze Dark icon theme) we have this problem as well. And it is triggered by the first call of QIcon::setThemeName(QIcon::themeName()) in main.cpp. It looks like a Qt bug. I only fail to understand why this is not happening with "light" colours.

To reproduce this issue: set a dark colours theme and oxygen icons, close all tabs in one panel and observe disabled icon in Window menu's Close Current Tab action. Try this with master branch and with fix-missing-icons branch.

I think we can live with this. But there is also the possibility to whitelist breeze/breeze-dark and oxygen to not interfere with default icon theme and use the magic only for other themes.


Another minor issue. With some legacy icon themes like nuoveXT (LXDE theme) there are missing icons: "arrow-left", "arrow-right". They are used in synchronizer where we use different logic so they are not rendered. Please see my code comments.

krusader/Synchronizer/synchronizergui.cpp
294

We can use text fallback:
createButton(showOptions, "arrow-right", checked, Qt::CTRL + Qt::Key_L, description, ">");

312

We can use text fallback:
createButton(showOptions, "arrow-left", checked, Qt::CTRL + Qt::Key_R, description, "<");

To reproduce this issue: set a dark colours theme and oxygen icons, close all tabs in one panel and observe disabled icon in Window menu's Close Current Tab action. Try this with master
branch and with fix-missing-icons branch.

I can confirm that it also happens using Kubuntu 17.10, I attach two screenshots if anyone wants to see the minor issue:

1 - using the master branch: the «Close Current Tab» icon is shown as a disabled one:

2 - using the fix-missing-icons branch: the «Close Current Tab» icon is shown as an enabled one (although it shouldn't):

nmel removed a reviewer: Krusader.Apr 21 2018, 9:03 PM
nmel marked 2 inline comments as done.

Thanks Toni and Martin for extensive testing!

I have spotted one minor issue. When window theme is dark, disabled state of icons is not rendered properly. To be exact it does not fade to lower opacity like it normally does when no "magic" is done with icon themes. Interesting points:

  1. It works fine with light window colours
  2. All tested icon themes are affected (Oxygen, nuoveXT, Adwaita, even breeze/breeze-dark)
  3. Disabled state is partially working though - e.g. colours are changged to b/w

    So basically in dark KDE environment (Breeze Dark colours and Breeze Dark icon theme) we have this problem as well. And it is triggered by the first call of QIcon::setThemeName(QIcon::themeName()) in main.cpp. It looks like a Qt bug.

Wow, you have a good sight and attention! In Breeze-dark the difference is in a small shade of gray. I confirm this is happening just because of the QIcon::setThemeName(QIcon::themeName()) call (by checking out b387aa5b6c5e9f9c2736b9db5792cc91f0c6f8a1 commit). This is a Qt bug! Of course, simply removing this line from main.cpp won't help as we use setThemeName later.

I only fail to understand why this is not happening with "light" colours.

Because it's a crazy bug! ;)

I think we can live with this. But there is also the possibility to whitelist breeze/breeze-dark and oxygen to not interfere with default icon theme and use the magic only for other themes.

I think whitelisting is the wrong way to go, because

  1. It only helps only partially - incomplete dark themes are still suffering.
  2. We can't whitelist oxygen for now as distributions use older theme version which is not complete yet.

IMO, we should leave it as is. I will try my luck with filing a Qt bug report to resolve this problem.
BTW, we also need to report two other issues that we found workaround for:

  1. Qt icon invalid caching issues if setTheme is not called (QIcon::hasThemeIcon returns false for an existing icon).
  2. Breeze/Breeze-dark dynamic selection stopped working after setTheme call.

Another minor issue. With some legacy icon themes like nuoveXT (LXDE theme) there are missing icons: "arrow-left", "arrow-right". They are used in synchronizer where we use different logic so they are not rendered. Please see my code comments.

Pushed this to the branch. Thanks for the fix! We actually use the Icon class inside createButton now, so fallback themes should work. I think you either don't have any fallback theme installed or Icon::exists fails for some reason, although both Icon and Icon::exists use the same call to searchIcon.

This revision is now accepted and ready to land.Apr 21 2018, 9:03 PM

A special case: using Adwaita icons

Adwaita icons using Kubuntu 17.10 (the first icons are not seen)

Adwaita icons using Kubuntu 17.10 - bookmark icons

Adwaita icons using Kubuntu 17.10 and Dolphin (almost all icons are not seen)

Interesting case. As Dolphin screenshot shows, it seems like Adwaita is not properly installed on Kubuntu — I'm not sure why. On my host system (Gentoo) Adwaita works as expected even though my primary DE is KDE.

Toni, please accept this revision if you think it's good to merge with master.

asensi accepted this revision.Apr 22 2018, 4:42 PM

Toni, please accept this revision if you think it's good to merge with master.

Yes, I think so, while the report bugs (that were talked about) are processed. Thank you, Nikita and Martin!

nmel closed this revision.Apr 22 2018, 10:19 PM

Didn't close automatically, probably because it was reopened earlier.

Nikita, thanks for merging this big thing.

I understand we have 3 Qt bugs to report:

  1. Qt icon invalid caching issues if setTheme is not called (QIcon::hasThemeIcon returns false for an existing icon).
  2. Breeze/Breeze-dark dynamic selection stopped working after setTheme call.
  3. Calling QIcon::setThemeName(QIcon::themeName()) partially breaks disabled state of icons in dark environment.

Did you have the chance to report some of them? I can do it, if You like.

nmel added a comment.Apr 27 2018, 5:56 AM

Nikita, thanks for merging this big thing.

I understand we have 3 Qt bugs to report:

  1. Qt icon invalid caching issues if setTheme is not called (QIcon::hasThemeIcon returns false for an existing icon).
  2. Breeze/Breeze-dark dynamic selection stopped working after setTheme call.
  3. Calling QIcon::setThemeName(QIcon::themeName()) partially breaks disabled state of icons in dark environment.

That's right!

Did you have the chance to report some of them? I can do it, if You like.

No, I haven't reported yet as I was busy with the release preparation. It's in my todo list, however if you volunteer to report them, I appreciate! :)
Please post links here, I'd like to track these reports and may add info if needed. It would be nice to build a small example app showing the problems (if we are able to find the time for this) - from my experience this highly increases chances of an issue to be fixed soon.