Bug 434451 - SVG icons in custom Qt application stopped rendering
Summary: SVG icons in custom Qt application stopped rendering
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kiconthemes
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.80.0
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Christoph Cullmann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-15 17:35 UTC by krzmbrzl
Modified: 2021-09-12 00:59 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Test case for QIcon selected state in PyQt5 app (1.23 KB, text/x-python)
2021-03-19 17:10 UTC, Luke Horwell
Details

Note You need to log in before you can comment on or make changes to this bug.
Description krzmbrzl 2021-03-15 17:35:26 UTC
SUMMARY
Icons in the Mumble application (GUI built with Qt) stopped showing up after having upgraded to the most recent KDE frameworks version.
Note that this seems to only affect SVG icons. PNG icons are still rendered properly.

See https://github.com/mumble-voip/mumble/issues/4853 (there it was also mentioned that this apparently is related to kiconthemes).

Steps to reproduce would be to clone the source code from the linked repo, and build it as described in https://github.com/mumble-voip/mumble/blob/master/docs/dev/build-instructions/build_linux.md


Operating System: KDE neon 5.21
KDE Plasma Version: 5.21.2
KDE Frameworks Version: 5.80.0
Qt Version: 5.15.2
Kernel Version: 5.4.0-59-generic
OS Type: 64-bit
Graphics Platform: X11
Processors: 8 × Intel® Core™ i7-4770 CPU @ 3.40GHz
Memory: 15,6 GiB of RAM
Graphics Processor: GeForce GTX 660 Ti/PCIe/SSE2
Comment 1 Antonio Rojas 2021-03-15 17:41:50 UTC
We have reports of this also breaking icons in lxqt https://bugs.archlinux.org/task/69988 and deepin https://bbs.archlinux.org/viewtopic.php?id=264543
Comment 2 yan12125 2021-03-16 07:02:18 UTC
Got the same issue on LXQt, and reverting https://github.com/KDE/kiconthemes/commit/5666c0c46e913fecbe2b41157f241685f126ab30 fixes icons for me. Is there a reason that kiconthemes needs to be injected outside KDE?
Comment 3 Christoph Cullmann 2021-03-17 20:22:04 UTC
Ok, this is naturally unintended :/
Comment 4 Bug Janitor Service 2021-03-17 20:46:46 UTC
A possibly relevant merge request was started @ https://invent.kde.org/frameworks/kiconthemes/-/merge_requests/25
Comment 5 Bug Janitor Service 2021-03-18 09:18:53 UTC
A possibly relevant merge request was started @ https://invent.kde.org/frameworks/kiconthemes/-/merge_requests/26
Comment 6 Christoph Cullmann 2021-03-18 09:20:59 UTC
Ok,

it was not that trivial, it was not "always" broken, we tested this.

It didn't handle the case of QDir::setSearchPaths :/

https://invent.kde.org/frameworks/kiconthemes/-/merge_requests/26
Comment 7 Christoph Cullmann 2021-03-18 09:50:56 UTC
(In reply to yan12125 from comment #2)
> Got the same issue on LXQt, and reverting
> https://github.com/KDE/kiconthemes/commit/
> 5666c0c46e913fecbe2b41157f241685f126ab30 fixes icons for me. Is there a
> reason that kiconthemes needs to be injected outside KDE?

See https://kate-editor.org/post/2021/2021-03-07-cross-platform-light-dark-themes-and-icons/ for the reason.

But it should naturally not break applications, that was an error.

Could you try the patch

https://invent.kde.org/frameworks/kiconthemes/-/merge_requests/26

if it solves the issue for you, too?

Not that I miss something (again)...
Comment 8 Kishore Gopalakrishnan 2021-03-19 06:21:01 UTC
(In reply to Christoph Cullmann from comment #7)
> (In reply to yan12125 from comment #2)
> > Got the same issue on LXQt, and reverting
> > https://github.com/KDE/kiconthemes/commit/
> > 5666c0c46e913fecbe2b41157f241685f126ab30 fixes icons for me. Is there a
> > reason that kiconthemes needs to be injected outside KDE?
> 
> See
> https://kate-editor.org/post/2021/2021-03-07-cross-platform-light-dark-
> themes-and-icons/ for the reason.
> 
> But it should naturally not break applications, that was an error.
> 
> Could you try the patch
> 
> https://invent.kde.org/frameworks/kiconthemes/-/merge_requests/26
> 
> if it solves the issue for you, too?
> 
> Not that I miss something (again)...

Hi, I can confirm that rebuilding kiconthemes with your patch fixes the icons in git-cola (was also broken with kiconthemes 5.80).
Comment 9 Christoph Cullmann 2021-03-19 10:38:34 UTC
Thanks for testing and sorry for the issue :(
Comment 10 Luke Horwell 2021-03-19 17:10:14 UTC
Created attachment 136852 [details]
Test case for QIcon selected state in PyQt5 app

I tested the patch/branch and still found some issues, at least with PyQt5 apps.

I used git-cola for testing. While the icons have reappeared, the functionality to switch between dark/light themes from within the app does not work. The icons appear to be stuck in one theme. This works as expected under 5.79 or by checking out kiconthemes commit 6bada57e705190c20fd72c9e7b1ef1a25d6d44a3.

The other issue is that QIcon states don't seem to be working. I've attached a test case demonstrating the problem using PyQt5 which loads an SVG file for 2 buttons directly from /usr/share/icons/breeze/ (just as an example). The focused button should be a different icon. 

An application in practice using QIcon states on buttons and menus is polychromatic[-git] - a screenshot can be seen here: https://forum.endeavouros.com/t/12788
Comment 11 Christoph Cullmann 2021-03-19 18:53:20 UTC
Ok, I see, too many issues left, I will revert my old change to avoid having the stuff used outside of KDE(but keep my improvements).
Comment 12 Christoph Cullmann 2021-03-19 18:59:13 UTC
I amended the change now to avoid registering the engine per default.

Thanks for providing the test for the states.

For the dark/light themes, do you have a pointer how git-cola implements that?
Comment 13 Luke Horwell 2021-03-19 23:31:45 UTC
Thanks, I rebuilt the branch again and apps are back to normal.

Regarding git-cola, I have no code hints. After setting "Icon Theme" via the preferences window, the program just needs to be restarted to see the new icon scheme.

> See https://kate-editor.org/post/2021/2021-03-07-cross-platform-light-dark-themes-and-icons/ for the reason.

IMO, the "kiconthemes" package shouldn't risk inconsistencies or create unintended bugs for non-KDE Qt-based apps. I think it would be a better practice for QIconEngine not to be overridden by KIconEngine when this package is installed. If it can specifically be confined to KDE applications or be overridden by some other condition, like an environment variable, that would be better.
Comment 14 Christoph Cullmann 2021-03-20 10:52:20 UTC
Git commit 49bdb6310194cd899641b7d9cf8463d4fba6baea by Christoph Cullmann.
Committed on 19/03/2021 at 15:59.
Pushed by cullmann into branch 'master'.

ensure qrc + QDir::searchPaths work for icons

before we had some optimization to check which paths
are absolute, but that one is wrong, applications
might rely on set search paths

M  +4    -15   src/kiconloader.cpp

https://invent.kde.org/frameworks/kiconthemes/commit/49bdb6310194cd899641b7d9cf8463d4fba6baea
Comment 15 Christoph Cullmann 2021-03-20 10:55:05 UTC
(In reply to Luke Horwell from comment #13)
> Thanks, I rebuilt the branch again and apps are back to normal.
> 
> Regarding git-cola, I have no code hints. After setting "Icon Theme" via the
> preferences window, the program just needs to be restarted to see the new
> icon scheme.

Ok, thanks anyways for pointing out the extra issues.

> 
> > See https://kate-editor.org/post/2021/2021-03-07-cross-platform-light-dark-themes-and-icons/ for the reason.
> 
> IMO, the "kiconthemes" package shouldn't risk inconsistencies or create
> unintended bugs for non-KDE Qt-based apps. I think it would be a better
> practice for QIconEngine not to be overridden by KIconEngine when this
> package is installed. If it can specifically be confined to KDE applications
> or be overridden by some other condition, like an environment variable, that
> would be better.

The problem is, there is no good interface for this (at least nobody found one yet) that allows to have for KDE stuff the auto-coloring but not touching such global things.

Inside a KDE Plasma session, another plugin will ensure the KIconEngine is used (that will affect non-KDE applications there, too), but that won't help outside of a KDE Plasma session.

In any case, you are totally right, if the change is not 1:1 transparent for applications not using the "advanced" features, it is not acceptable, either our replacement works 1:1 like the QSvgIconEngine for other Qt stuff, or we shall not do this.

I am sorry that you had these issues, it was not clear from testing that our icon loader behaves differently from the Qt one (beside additional features Qt applications won't be able to use anyways).
Comment 16 krzmbrzl 2021-04-07 14:26:06 UTC
Is there an ETA for when this fix will be available on KDE Neon?