| Frameworks | |
| Plasma |
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
BUG: 402172
icons load correctly in material,
and the fallback to breeze works with gnome themes
| No Linters Available |
| No Unit Test Coverage |
| Buildable 14007 | |
| Build 14025: arc lint + arc unit |
| src/kicontheme.cpp | ||
|---|---|---|
| 264 | I guess you can just do a static s_fallbackThemeInitialized = false;
if (!s_fallbackThemeInitialized) {
...
} | |
I don't think this is going to work.
Who is going to create a KIconTheme when running Okular on gnome shell?
From what i can see we do create one in okular/part.cpp:386 because we happen to call KParts::Part::iconLoader() there, but that's too late, there has been already 4 calls to QIcon::fromTheme at that stage.
Wouldn't it make more sense to fix in Qt?
Something like
diff --git a/src/gui/image/qicon.cpp b/src/gui/image/qicon.cpp index 41fe649fc5..f86ad3c760 100644 --- a/src/gui/image/qicon.cpp +++ b/src/gui/image/qicon.cpp @@ -1260,7 +1260,7 @@ QString QIcon::fallbackThemeName() */ void QIcon::setFallbackThemeName(const QString &name) { - QIconLoader::instance()->setFallbackThemeName(name); + QIconLoader::instance(NoInializeNeeded)->setFallbackThemeName(name); } /*! diff --git a/src/gui/image/qiconloader.cpp b/src/gui/image/qiconloader.cpp index 15ab1b3cd9..96dfd9163d 100644 --- a/src/gui/image/qiconloader.cpp +++ b/src/gui/image/qiconloader.cpp @@ -125,10 +125,12 @@ void QIconLoader::ensureInitialized() } } -QIconLoader *QIconLoader::instance() +QIconLoader *QIconLoader::instance(InstanceOptions o) { - iconLoaderInstance()->ensureInitialized(); - return iconLoaderInstance(); + if (o == EnsureInitialized) { + iconLoaderInstance()->ensureInitialized(); + } + return iconLoaderInstance(); } // Queries the system theme and invalidates existing diff --git a/src/gui/image/qiconloader_p.h b/src/gui/image/qiconloader_p.h index fac18b5d79..a69da4a5f7 100644 --- a/src/gui/image/qiconloader_p.h +++ b/src/gui/image/qiconloader_p.h @@ -185,7 +185,11 @@ public: void setFallbackSearchPaths(const QStringList &searchPaths); QStringList fallbackSearchPaths() const; QIconDirInfo dirInfo(int dirindex); - static QIconLoader *instance(); + enum InstanceOptions { + NoInializeNeeded = 0, + EnsureInitialized = 1 + } + static QIconLoader *instance(InstanceOptions o = EnsureInitialized); void updateSystemTheme(); void invalidateKey() { m_themeKey++; } void ensureInitialized();
I think this should be relatively easy to push though in Qt explaining that small ordering issue there is
Not sure it compiles, for some reason i can't compile Qt 5.15 branch right now
Now i understand this is causing relatively several issues, sorry for dropping the ball, so we should probably fix for users before Qt 5.15.1
What about something like
diff --git a/src/kicontheme.cpp b/src/kicontheme.cpp index 4f5d9d5..62fc9d7 100644 --- a/src/kicontheme.cpp +++ b/src/kicontheme.cpp @@ -70,6 +70,19 @@ void initRCCIconTheme() } Q_COREAPP_STARTUP_FUNCTION(initRCCIconTheme) +class SetBreezeFallbackHelper : public QObject +{ + bool event(QEvent *e) override + { + if (e->type() == QEvent::User) { + QIcon::setFallbackThemeName(QStringLiteral("breeze")); + deleteLater(); + return true; + } + return QObject::event(e); + } +}; + // Set the icon theme fallback to breeze // Most of our apps use "lots" of icons that most of the times // are only available with breeze, we still honour the user icon @@ -77,7 +90,8 @@ Q_COREAPP_STARTUP_FUNCTION(initRCCIconTheme) // since it's almost sure it'll be there static void setBreezeFallback() { - QIcon::setFallbackThemeName(QStringLiteral("breeze")); + SetBreezeFallbackHelper *helper = new SetBreezeFallbackHelper(); + QCoreApplication::instance()->postEvent(helper, new QEvent(QEvent::User), Qt::HighEventPriority); } Q_COREAPP_STARTUP_FUNCTION(setBreezeFallback)
It's still totally bad since it only sets the fallback after QGuiApplication *starts* running (which is mega late since most of the constructors will have already be created), but at least doesn't rely on the app creating a KIconTheme.
It would make more sense.
I thought as a temporary solution it would be nice to fix it in KF as well, since KF releases go out more often.
Well, if it serves the purpose - why not? Have you tested it?
(I just only have Plasma QPA platform installed :( )
UPD: seems like it doesn't. I've added couple qDebug lines, to QIcon::fromTheme() method and near QIcon::setFallbackThemeName() with your patch, and then ran okular and dolphin.
By the time setFallbackThemeName() is called, almost all icons are already loaded :(
Yes, that's exactly what i said with my comment about it being bad :D
Ok, maybe we can go extra crazy and [assuming my patch makes it to Qt 5.15.1] have this block and your block from KMainWindow (maybe adding a if QIcon::fallbackThemeName().isEmpty() first, we don't want to overwrite if people manually chose something else)