Bug 402172 - Compiling against Qt 5.12 breaks QIcon::themeName with Plasma platform plugin
Summary: Compiling against Qt 5.12 breaks QIcon::themeName with Plasma platform plugin
Status: RESOLVED UPSTREAM
Alias: None
Product: frameworks-kiconthemes
Classification: Unclassified
Component: general (show other bugs)
Version: 5.53.0
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Christoph Feck
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-15 22:21 UTC by Antonio Rojas
Modified: 2020-06-04 22:03 UTC (History)
11 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Test code (200 bytes, text/x-c++src)
2018-12-15 22:21 UTC, Antonio Rojas
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Rojas 2018-12-15 22:21:56 UTC
Created attachment 116944 [details]
Test code

SUMMARY
When kiconthemes is compiled against Qt 5.12, QIcon::themeName returns an empty string when using Plasma platform plugin

STEPS TO REPRODUCE
1. Compile kiconthemes with Qt 5.11
2. Run the attached program

OBSERVED RESULT
"breeze"

STEPS TO REPRODUCE
3. Compile kiconthemes with Qt 5.12
4. Run the attached program

OBSERVED RESULT
""

EXPECTED RESULI
"breeze"

SOFTWARE/OS VERSIONS
KDE Plasma Version: 5.14.4
KDE Frameworks Version: 5.53
Qt Version: 5.12
Comment 1 Antonio Rojas 2018-12-15 22:37:40 UTC
Removing the QIcon::setFallbackThemeName("breeze"); line in setBreezeFallback() fixes the problem
Comment 2 Fabian Vogt 2018-12-15 23:14:10 UTC
I debugged this a bit.

The reason this fails is because the call to QIcon::setFallbackThemeName causes QIconLoader::ensureInitialized to be called, which sets up the internal configuration. At this point, however, the platform theme is not yet loaded to m_system_theme is empty.

By commenting out the call, the QIconLoader is only initialized after the platform theme got loaded and it works as expected.
Comment 3 Bobby Wibowo 2019-09-21 03:00:11 UTC
Hi there, I'd like to bump this report, confirming that I'm also getting the issue.

Albert launcher (https://github.com/albertlauncher/albert) seems to be using QIcon::themeName to get user's preferred icon theme, and due to it returning empty string, Albert ends up failing in displaying the icons using user's preferred icon theme.

A relevant bug report in their Git repo can be found here: https://github.com/albertlauncher/albert/issues/778, along with some other people confirming they also encountered the issue.
Comment 4 Johannes Jordan 2019-11-10 23:40:08 UTC
In my application, I have a simple test for empty QIcon::themeName(), in which case I set a fallback theme name. The reason is that on other platforms (Windows etc.), using theme icons with fallback files in qrc *only* works when a theme name is set.[1]

Now with current KDE, Qt this leads to my application to set the false theme name and ignore desktop icons. This means, there is quite a few cross-platform applications out there with this problem.

[1] https://bugreports.qt.io/browse/QTBUG-16697
Comment 5 Kevin Kofler 2020-01-06 15:27:38 UTC
I can confirm this. I have just wasted something like 3 hours with GDB to come to the same conclusion already found in this bug report.

This completely breaks icon theming in Trojitá, because Trojitá has a custom icon loader wrapper that does this:
https://cgit.kde.org/trojita.git/tree/src/UiUtils/IconLoader.cpp#n50

Due to QIcon::themeName() being empty, ":/icons/%1/%2.svg" expands to ":/icons//iconname.svg", which happens to be the fallback location ":/icons/iconname.svg", and so it thinks the fallback icon is the override icon and wrongly prefers it to the theme.
Comment 6 Kevin Kofler 2020-01-06 16:33:40 UTC
Should the Plasma platform plugin call QIconLoader::updateSystemTheme()?
Comment 7 Kevin Kofler 2020-01-06 17:16:08 UTC
It looks like the same bug was reported to https://bugreports.qt.io/browse/QTBUG-74252 – I have cross-linked the 2 reports.
Comment 8 Kevin Kofler 2020-01-20 22:30:22 UTC
Is there anybody here who can answer my question:
Should the Plasma platform plugin call QIconLoader::updateSystemTheme()? Or should this maybe even be done somewhere within Qt itself (after loading a platform plugin?), as hinted at in https://bugreports.qt.io/browse/QTBUG-74252 ?

We need SOME way to fix this, sooner rather than later (this completely breaks theming in some applications and it has been open for over a year!), and the obvious fix (reverting the KIconThemes addition) does not look like the correct fix to me.
Comment 9 Kevin Kofler 2020-01-20 22:34:27 UTC
I would be willing to test and submit a fix, if people tell me (here or at Qt upstream) where the fix should go.
Comment 10 Christoph Feck 2020-02-13 13:55:57 UTC
Kevin, can you confirm Albert's comment in QTBUG-74252 that it works correctly with Qt 5.14?
Comment 11 Kevin Kofler 2020-02-13 14:04:02 UTC
I can neither confirm nor disprove that at this time, because we do not have Qt 5.14 packaged in Fedora yet.
Comment 12 Kevin Kofler 2020-02-13 14:08:43 UTC
At most, I could try Qt 5.13.2 with some extra effort (either upgrading my notebook from Fedora 30 to 31 or rebuilding Qt 5.13.2 for Fedora 30, unless it already sits in a Copr somewhere), but 5.14 is not even in Rawhide yet.
Comment 13 Kevin Kofler 2020-02-13 14:11:15 UTC
(Even if I test fixes, the way I generally go at it is that I develop them against the version that is packaged, test them by adding them as patches to the SRPM, then (before upstreaming them) forward-port them to whatever branch upstream wants patches submitted against.)
Comment 14 Antonio Rojas 2020-02-13 17:37:41 UTC
No, it doesn't work with Qt 5.14
Comment 15 Jonathan Chun 2020-04-03 01:53:39 UTC
albert -r
        Albert version: 0.16.1
            Build date: Mar 30 2020 02:58:04
            Qt version: 5.14.1
  QT_QPA_PLATFORMTHEME: 
       Binary location: /usr/local/bin/albert
                   PWD: /home/jchun/Documents/Projects
                 SHELL: /bin/bash
                  LANG: en_US.UTF-8
      XDG_SESSION_TYPE: x11
   XDG_CURRENT_DESKTOP: KDE
       DESKTOP_SESSION: /usr/share/xsessions/plasma
   XDG_SESSION_DESKTOP: KDE
                    OS: KDE neon User Edition 5.18
     OS (type/version): neon/18.04
             Build ABI: x86_64-little_endian-lp64
  Arch (build/current): x86_64/x86_64
 Kernel (type/version): linux/5.3.0-45-generic

Bumping this to confirm it doesn't work with 5.14
Comment 16 Kevin Kofler 2020-06-03 02:13:49 UTC
This is the Qt fix that should fix this:
https://codereview.qt-project.org/c/qt/qtbase/+/302581

5.12 LTS backport:
https://codereview.qt-project.org/c/qt/qtbase/+/302582
Comment 17 Antonio Rojas 2020-06-04 14:07:39 UTC
I confirm the Qt commit fixes the issue
Comment 18 Igor Poboiko 2020-06-04 22:03:55 UTC
Git commit 899c0f7fa457d77f4b53f8540e22290da862d57d by Igor Poboiko.
Committed on 04/06/2020 at 22:03.
Pushed by poboiko into branch 'master'.

[KMainWindow] Invoke QIcon::setFallbackThemeName (later)

Summary:
This is alternative approach to {D22488} and commit 4214045 to KIconThemes.
Okular (and most - if not all - KDE apps) inherit KMainWindow, so KDE apps
should have breeze icons). KMainWindow ctor should be early enough so no icons
are yet loaded, but late enough so QGuiApplication is already inited.

This should be followed by reverting commit 4214045 in KIconThemes.

Original problem description (by @mart):
invoking QIcon::setFallbackThemeName at QCoreApplication ctor
with Q_COREAPP_STARTUP_FUNCTION breaks the internal status of
QIconLoader as it instantiates it before the QPlatformTheme,
but QIconLoader depends from QPlatformTheme to be already instantiated
otherwise it won't load correctly, thus breaking icon loading
in QtQuickControls2 styles, such as Material and Fusion
see https://bugreports.qt.io/browse/QTBUG-74252

Test Plan:
Don't have GTK3 QPA plugin, so cannot test it yet.
I would appreciate if someone helped me with testing :)

Reviewers: aacid, mart, broulik

Subscribers: mart, kde-frameworks-devel

Tags: #frameworks

Differential Revision: https://phabricator.kde.org/D29826

M  +17   -0    src/kmainwindow.cpp

https://invent.kde.org/frameworks/kxmlgui/commit/899c0f7fa457d77f4b53f8540e22290da862d57d