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

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

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

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

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

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

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

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

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

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=83215 BRANCH icon-load (branched from master) REVISION DETAIL https://phabricator.kde.org/D29826

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

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

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

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.

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

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,

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

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=83141 BRANCH

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

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

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

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,

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

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=83129 BRANCH icon-load (branched from master) REVISION DETAIL

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

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

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

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?

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

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,

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

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,

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

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"

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

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)

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