D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko added a comment.


  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! :)

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Albert Astals Cid]

aacid added a comment.


  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

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R263:899c0f7fa457: [KMainWindow] Invoke 
QIcon::setFallbackThemeName (later) (authored by poboiko).

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83215&id=83216

REVISION DETAIL
  https://phabricator.kde.org/D29826

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Albert Astals Cid]

aacid added a comment.


  oh i think  i'm too late :D

INLINE COMMENTS

> kmainwindow.cpp:253
> +// TODO: remove this once we depend on Qt 5.15.1, where this is fixed
> +#if QT_VERSION >= QT_VERSION_CHECK(5, 12, 0)
> +if (QIcon::fallbackThemeName().isEmpty()) {

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

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko updated this revision to Diff 83215.
poboiko added a comment.


  Right, 5.15.1.

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83212&id=83215

BRANCH
  icon-load (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29826

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Albert Astals Cid]

aacid added a comment.


  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

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko updated this revision to Diff 83212.
poboiko added a comment.


  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?

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83141&id=83212

BRANCH
  icon-load (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29826

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Albert Astals Cid]

aacid added a comment.


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

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko updated this revision to Diff 83141.
poboiko added a comment.


  Sure, it's a temporary workaround anyways :)
  
  Update comment: add TODO mark and link to @aacid Qt patch

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83129&id=83141

BRANCH
  icon-load (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29826

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Albert Astals Cid]

aacid added a comment.


  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 ^_^ 😄

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko added a comment.


  In D29826#673585 , @aacid wrote:
  
  > What about discover for example?
  
  
  Good point...

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko updated this revision to Diff 83129.
poboiko added a comment.


  Don't override if fallbackThemeName is already set

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83109&id=83129

BRANCH
  icon-load (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29826

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Albert Astals Cid]

aacid added a comment.


  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?

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko added a comment.


  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.

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Albert Astals Cid]

aacid added a comment.


  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

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Albert Astals Cid]

aacid added a comment.


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

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko added a comment.


  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=20
  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!

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D29826

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: aacid, mart, broulik.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION 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 :)

REPOSITORY
  R263 KXmlGui

BRANCH
  icon-load (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29826

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns