D22488: invoke QIcon::setFallbackThemeName a bit later

[Albert Astals Cid]

aacid added a comment.


  In D22488#673575 , @poboiko wrote:
  
  > 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)

REPOSITORY
  R302 KIconThemes

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

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

D22488: invoke QIcon::setFallbackThemeName a bit later

[Igor Poboiko]

poboiko added a comment.


  In D22488#673544 , @aacid wrote:
  
  > Wouldn't it make more sense to fix in Qt?
  
  
  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.
  
  In D22488#673547 , @aacid wrote:
  
  > What about something like
  >
  > [...]
  
  
  Well, if it serves the purpose - why not? Have you tested it?
  (I just only have Plasma QPA platform installed :( )

REPOSITORY
  R302 KIconThemes

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

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

D22488: invoke QIcon::setFallbackThemeName a bit later

[Albert Astals Cid]

aacid added a comment.


  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.

REPOSITORY
  R302 KIconThemes

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

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

D22488: invoke QIcon::setFallbackThemeName a bit later

[Albert Astals Cid]

aacid added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D22488: invoke QIcon::setFallbackThemeName a bit later

[Albert Astals Cid]

aacid added a comment.


  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 )
 {
-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 );
 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

REPOSITORY
  R302 KIconThemes

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

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

D22488: invoke QIcon::setFallbackThemeName a bit later

[Albert Astals Cid]

aacid added a comment.


  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.

REPOSITORY
  R302 KIconThemes

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

To: mart, #frameworks, #plasma
Cc: aacid, mlaurent, broulik, kde-frameworks-devel, LeGast00n, sbergeron, 
michaelh, ngraham, bruns

D22488: invoke QIcon::setFallbackThemeName a bit later

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R302 KIconThemes

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

To: mart, #frameworks, #plasma
Cc: mlaurent, broulik, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, 
ngraham, bruns

D22488: invoke QIcon::setFallbackThemeName a bit later

[Laurent Montel]

mlaurent added a comment.


  will it fix this bug "https://bugs.kde.org/show_bug.cgi?id=407600; ?

REPOSITORY
  R302 KIconThemes

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

To: mart, #frameworks, #plasma
Cc: mlaurent, broulik, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, 
ngraham, bruns

D22488: invoke QIcon::setFallbackThemeName a bit later

[Marco Martin]

mart updated this revision to Diff 61857.
mart added a comment.


  - nitialize

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22488?vs=61855=61857

BRANCH
  phab/qqcicons

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

AFFECTED FILES
  src/kicontheme.cpp

To: mart, #frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22488: invoke QIcon::setFallbackThemeName a bit later

[Marco Martin]

mart edited the summary of this revision.

REPOSITORY
  R302 KIconThemes

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

To: mart, #frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22488: invoke QIcon::setFallbackThemeName a bit later

[Marco Martin]

mart marked an inline comment as done.

REPOSITORY
  R302 KIconThemes

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

To: mart, #frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22488: invoke QIcon::setFallbackThemeName a bit later

[Marco Martin]

mart updated this revision to Diff 61855.
mart added a comment.


  - use a local static

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22488?vs=61854=61855

BRANCH
  phab/qqcicons

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

AFFECTED FILES
  src/kicontheme.cpp

To: mart, #frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22488: invoke QIcon::setFallbackThemeName a bit later

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kicontheme.cpp:267
> +#if QT_VERSION >= QT_VERSION_CHECK(5, 12, 0)
> +if (!d->fallbackThemeInitialized) {
> +QIcon::setFallbackThemeName(QStringLiteral("breeze"));

I guess you can just do a

  static s_fallbackThemeInitialized = false;
  if (!s_fallbackThemeInitialized) {
  ...
  }

REPOSITORY
  R302 KIconThemes

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

To: mart, #frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22488: invoke QIcon::setFallbackThemeName a bit later

[Marco Martin]

mart created this revision.
mart added reviewers: Frameworks, Plasma.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
mart requested review of this revision.

REVISION SUMMARY
  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

TEST PLAN
  icons load correctly in material,
  and the fallback to breeze works with gnome themes

REPOSITORY
  R302 KIconThemes

BRANCH
  phab/qqcicons

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

AFFECTED FILES
  src/kicontheme.cpp

To: mart, #frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns