Differential D29826

[KMainWindow] Invoke QIcon::setFallbackThemeName (later)
ClosedPublic
Actions

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

Details

Reviewers
aacid
mart
broulik
Commits
R263:899c0f7fa457: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
poboiko created this revision.May 22 2020, 9:37 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 22 2020, 9:37 AM
poboiko requested review of this revision.May 22 2020, 9:37 AM
poboiko edited the summary of this revision. (Show Details)May 22 2020, 2:09 PM
poboiko added a comment.May 22 2020, 2:18 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!

aacid added a comment.May 22 2020, 11:17 PM

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

aacid added a comment.May 22 2020, 11:19 PM

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

poboiko added a comment.May 23 2020, 9:00 AM

Thanks for looking into it! :)

In D29826#673543, @aacid wrote:

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.

aacid added a comment.May 23 2020, 10:19 AM
In D29826#673572, @poboiko wrote:

Thanks for looking into it! :)

In D29826#673543, @aacid wrote:

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.May 23 2020, 12:51 PM

Don't override if fallbackThemeName is already set

poboiko added a comment.May 23 2020, 1:05 PM
In D29826#673585, @aacid wrote:

What about discover for example?

Good point...

aacid added a comment.May 23 2020, 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.May 24 2020, 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.May 24 2020, 4:38 PM

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

poboiko updated this revision to Diff 83212.Jun 4 2020, 1:24 PM

Seems like the Qt fix was landed

... another one (https://codereview.qt-project.org/c/qt/qtbase/+/302341), but whatever.
So I've removed link to the old one and added note to remove this after we depend on Qt 5.15.

Any objections, should I land this one?

aacid added a comment.Jun 4 2020, 5:36 PM

It didn't really land on Qt 5.15 yet, so we may need to update the comment to say 5.15.1 or something

https://codereview.qt-project.org/c/qt/qtbase/+/302581

But as said i think this is reasonable for now

poboiko updated this revision to Diff 83215.Jun 4 2020, 10:00 PM

Right, 5.15.1.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 4 2020, 10:03 PM
Closed by commit R263:899c0f7fa457: [KMainWindow] Invoke QIcon::setFallbackThemeName (later) (authored by poboiko). · Explain Why
This revision was automatically updated to reflect the committed changes.
aacid added a comment.Jun 4 2020, 10:04 PM

oh i think i'm too late :D

src/kmainwindow.cpp
253

should this be
#if QT_VERSION >= QT_VERSION_CHECK(5, 12, 0) && #if QT_VERSION < QT_VERSION_CHECK(5, 15, 1)
?

aacid added a comment.Jun 4 2020, 10:04 PM
In D29826#674634, @aacid wrote:

oh i think i'm too late :D

Let's wait for the fix to actually land and then i'll propose that change

poboiko added a comment.Jun 4 2020, 10:17 PM
In D29826#674634, @aacid wrote:

oh i think i'm too late :D

Whoops, sorry :S

Actually, I thought that if this piece of code is executed with Qt >= 5.15.1 (or whenever the proper fix is landed), it shouldn't hurt anyone anyways.
The TODO is more about code maintainability and reducing amount of various hacks in the code, and in that case another #if won't help much :)

Let's wait for the fix to actually land and then i'll propose that change

OK! :)

Revision Contents
Changeset List

PathPackages
Msrc/kmainwindow.cpp (17 lines)

Diff 83216

View Options

src/kmainwindow.cpp

Loading...