[KMainWindow] Invoke QIcon::setFallbackThemeName (later)
Needs ReviewPublic

Authored by poboiko on Fri, May 22, 9:37 AM.

Details

Reviewers
aacid
mart
broulik
Summary

This is alternative approach to D22488: invoke QIcon::setFallbackThemeName a bit later 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

CCBUG: 402172

Test Plan

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

Diff Detail

Repository
R263 KXmlGui
Branch
icon-load (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27069
Build 27087: arc lint + arc unit
poboiko created this revision.Fri, May 22, 9:37 AM
Restricted Application added a project: Frameworks. Β· View Herald TranscriptFri, May 22, 9:37 AM
poboiko requested review of this revision.Fri, May 22, 9:37 AM
poboiko edited the summary of this revision. (Show Details)Fri, May 22, 2:09 PM

My main motivation here is following: https://gerrit.libreoffice.org/c/core/+/94691.
If it gets accepted, it would fix long-standing issue that people aren't able to use Libreoffice with KF5 integration plugin together with dark color scheme (and "breeze-dark" icon theme) out-of-the-box.

See also:
https://bugs.documentfoundation.org/show_bug.cgi?id=116683
https://bugs.documentfoundation.org/show_bug.cgi?id=127138
https://forum.manjaro.org/t/change-libreoffice-look-and-feel/48357
https://bbs.archlinux.org/viewtopic.php?id=201111
https://www.reddit.com/r/kde/comments/4qqpqr/kde_dark_themes_and_libreoffice/
https://askubuntu.com/questions/1026103/libre-office-display-more-visible-icons-on-dark-breeze-kde-theme

In short, people suggest using gtk3 VCL plugin, because it hooks with kde-gtk-config which exports KDE icon theme to GTK. Instead of native KF5 VCL plugin. What a shame!

I don't think moving this code from KIconThemes to kmainwindow makes sense, what about all the apps that use KIconThemes but no KMainWindow?

Not sure if you're subscribed to https://phabricator.kde.org/D22488 see my last comment there i think it's the way to go

Thanks for looking into it! :)

I don't think moving this code from KIconThemes to kmainwindow makes sense, what about all the apps that use KIconThemes but no KMainWindow?

I'm just not aware of any. I've looked at some of the most used (by me :]), noted that all of them use KMainWindow and thought it might be a good option.

Thanks for looking into it! :)

I don't think moving this code from KIconThemes to kmainwindow makes sense, what about all the apps that use KIconThemes but no KMainWindow?

I'm just not aware of any. I've looked at some of the most used (by me :]), noted that all of them use KMainWindow and thought it might be a good option.

What about discover for example?

poboiko updated this revision to Diff 83129.Sat, May 23, 12:51 PM

Don't override if fallbackThemeName is already set

What about discover for example?

Good point...

aacid added a comment.Sat, May 23, 2:08 PM

Do you think we should mention something like

// TODO Remove once we can depend on a Qt version that has https://codereview.qt-project.org/c/qt/qtbase/+/301588

Or maybe i'm being too optimistic in getting that landed ^_^ πŸ˜„

poboiko updated this revision to Diff 83141.Sun, May 24, 1:36 PM

Sure, it's a temporary workaround anyways :)

Update comment: add TODO mark and link to @aacid Qt patch

aacid added a comment.Sun, May 24, 4:38 PM

Personally I'm happy enough with this, i guess wait a few days and if noone disagrees, land it