Unify QIcon theme icon loading and cache invalidation

The logic for invalidating a theme QIcon when the platform theme
changed, or when the user set an explicit icon theme, was tied
to QIconLoaderEngine, so any platform theme implementing
QPlatformTheme::createIconEngine() with a custom icon engine
would not take part in this cache invalidation.

As we want users of QIcon::fromTheme to be agnostic to where the
icon is actually coming from, and have a consistent behavior for
the various QIcon APIs for setting explicit themes, the logic
for invalidating the themed icon has now been moved up one
layer, to a new QThemeIconEngine, that is responsible for
lazily creating the actual engine based on the name. The
engine proxies the actual icon loading through to the
real engine via a new QProxyIconEngine helper class.

Pick-to: 6.6
Change-Id: I474589981f751d7467e3073533cba542182f2d36
Reviewed-by: Friedemann Kleint <Friedemann.Kleint@qt.io>
Reviewed-by: Axel Spoerl <axel.spoerl@qt.io>

src/gui/CMakeLists.txt

@@ -62,7 +62,7 @@ qt_internal_add_module(Gui
         image/qbitmap.cpp image/qbitmap.h
         image/qbmphandler.cpp image/qbmphandler_p.h
         image/qicon.cpp image/qicon.h image/qicon_p.h
-        image/qiconengine.cpp image/qiconengine.h
+        image/qiconengine.cpp image/qiconengine.h image/qiconengine_p.h
         image/qiconengineplugin.cpp image/qiconengineplugin.h
         image/qiconloader.cpp image/qiconloader_p.h
         image/qimage.cpp image/qimage.h image/qimage_p.h

src/gui/image/qicon.cpp

@@ -1285,18 +1285,11 @@ QIcon QIcon::fromTheme(const QString &name)
     if (QIcon *cachedIcon = qtIconCache()->object(name))
         return *cachedIcon;
 
-    QIcon icon;
-    if (QDir::isAbsolutePath(name)) {
+    if (QDir::isAbsolutePath(name))
         return QIcon(name);
-    } else {
-        QPlatformTheme * const platformTheme = QGuiApplicationPrivate::platformTheme();
-        bool hasUserTheme = QIconLoader::instance()->hasUserTheme();
-        QIconEngine * const engine = (platformTheme && !hasUserTheme) ? platformTheme->createIconEngine(name)
-                                                   : new QIconLoaderEngine(name);
-        icon = QIcon(engine);
-        qtIconCache()->insert(name, new QIcon(icon));
-    }
 
+    QIcon icon(new QThemeIconEngine(name));
+    qtIconCache()->insert(name, new QIcon(icon));
     return icon;
 }
 
@@ -1431,8 +1424,8 @@ QDataStream &operator>>(QDataStream &s, QIcon &icon)
         if (key == "QPixmapIconEngine"_L1) {
             icon.d = new QIconPrivate(new QPixmapIconEngine);
             icon.d->engine->read(s);
-        } else if (key == "QIconLoaderEngine"_L1) {
-            icon.d = new QIconPrivate(new QIconLoaderEngine());
+        } else if (key == "QIconLoaderEngine"_L1 || key == "QThemeIconEngine"_L1) {
+            icon.d = new QIconPrivate(new QThemeIconEngine);
             icon.d->engine->read(s);
         } else {
             const int index = iceLoader()->indexOf(key);

src/gui/image/qiconengine.cpp

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: LicenseRef-Qt-Commercial OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only
 
 #include "qiconengine.h"
+#include "qiconengine_p.h"
 #include "qpainter.h"
 
 QT_BEGIN_NAMESPACE
@@ -307,4 +308,78 @@ QPixmap QIconEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIcon::St
     return arg.pixmap;
 }
 
+
+// ------- QProxyIconEngine -----
+
+void QProxyIconEngine::paint(QPainter *painter, const QRect &rect, QIcon::Mode mode, QIcon::State state)
+{
+    proxiedEngine()->paint(painter, rect, mode, state);
+}
+
+QSize QProxyIconEngine::actualSize(const QSize &size, QIcon::Mode mode, QIcon::State state)
+{
+    return proxiedEngine()->actualSize(size, mode, state);
+}
+
+QPixmap QProxyIconEngine::pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state)
+{
+    return proxiedEngine()->pixmap(size, mode, state);
+}
+
+void QProxyIconEngine::addPixmap(const QPixmap &pixmap, QIcon::Mode mode, QIcon::State state)
+{
+    proxiedEngine()->addPixmap(pixmap, mode, state);
+}
+
+void QProxyIconEngine::addFile(const QString &fileName, const QSize &size, QIcon::Mode mode, QIcon::State state)
+{
+    proxiedEngine()->addFile(fileName, size, mode, state);
+}
+
+QString QProxyIconEngine::key() const
+{
+    return proxiedEngine()->key();
+}
+
+QIconEngine *QProxyIconEngine::clone() const
+{
+    return proxiedEngine()->clone();
+}
+
+bool QProxyIconEngine::read(QDataStream &in)
+{
+    return proxiedEngine()->read(in);
+}
+
+bool QProxyIconEngine::write(QDataStream &out) const
+{
+    return proxiedEngine()->write(out);
+}
+
+QList<QSize> QProxyIconEngine::availableSizes(QIcon::Mode mode, QIcon::State state)
+{
+    return proxiedEngine()->availableSizes(mode, state);
+}
+
+QString QProxyIconEngine::iconName()
+{
+    return proxiedEngine()->iconName();
+}
+
+bool QProxyIconEngine::isNull()
+{
+    return proxiedEngine()->isNull();
+}
+
+QPixmap QProxyIconEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale)
+{
+    return proxiedEngine()->scaledPixmap(size, mode, state, scale);
+}
+
+void QProxyIconEngine::virtual_hook(int id, void *data)
+{
+    proxiedEngine()->virtual_hook(id, data);
+}
+
+
 QT_END_NAMESPACE

src/gui/image/qiconengine_p.h

@@ -0,0 +1,59 @@
+// Copyright (C) 2023 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only
+
+#ifndef QICONENGINE_P_H
+#define QICONENGINE_P_H
+
+#include <QtGui/private/qtguiglobal_p.h>
+
+#ifndef QT_NO_ICON
+//
+//  W A R N I N G
+//  -------------
+//
+// This file is not part of the Qt API.  It exists purely as an
+// implementation detail.  This header file may change from version to
+// version without notice, or even be removed.
+//
+// We mean it.
+//
+
+#include <QtGui/QIcon>
+#include <QtGui/QIconEngine>
+
+QT_BEGIN_NAMESPACE
+
+class QIconEngine;
+
+class QProxyIconEngine : public QIconEngine
+{
+public:
+    void paint(QPainter *painter, const QRect &rect, QIcon::Mode mode, QIcon::State state) override;
+    QSize actualSize(const QSize &size, QIcon::Mode mode, QIcon::State state) override;
+    QPixmap pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state) override;
+
+    void addPixmap(const QPixmap &pixmap, QIcon::Mode mode, QIcon::State state) override;
+    void addFile(const QString &fileName, const QSize &size, QIcon::Mode mode, QIcon::State state) override;
+
+    QString key() const override;
+    QIconEngine *clone() const override;
+    bool read(QDataStream &in) override;
+    bool write(QDataStream &out) const override;
+
+    QList<QSize> availableSizes(QIcon::Mode mode = QIcon::Normal,
+                                    QIcon::State state = QIcon::Off) override;
+
+    QString iconName() override;
+    bool isNull() override;
+    QPixmap scaledPixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale) override;
+
+    void virtual_hook(int id, void *data) override;
+protected:
+    virtual QIconEngine *proxiedEngine() const = 0;
+};
+
+QT_END_NAMESPACE
+
+#endif // QT_NO_ICON
+
+#endif // QICONENGINE_P_H

src/gui/image/qiconloader.cpp

@@ -585,54 +585,128 @@ QThemeIconInfo QIconLoader::loadIcon(const QString &name) const
     return iconInfo;
 }
 
+#ifndef QT_NO_DEBUG_STREAM
+QDebug operator<<(QDebug debug, QIconEngine *engine)
+{
+    QDebugStateSaver saver(debug);
+    debug.nospace();
+    if (engine) {
+        debug.noquote() << engine->key() << "(";
+        debug << static_cast<const void *>(engine);
+        if (!engine->isNull())
+            debug.quote() << ", " << engine->iconName();
+        else
+            debug << ", null";
+        debug << ")";
+    } else {
+        debug << "QIconEngine(nullptr)";
+    }
+    return debug;
+}
+#endif
 
-// -------- Icon Loader Engine -------- //
+QIconEngine *QIconLoader::iconEngine(const QString &iconName) const
+{
+    qCDebug(lcIconLoader) << "Resolving icon engine for icon" << iconName;
 
+    auto *platformTheme = QGuiApplicationPrivate::platformTheme();
+    auto *iconEngine = hasUserTheme() || !platformTheme ?
+        new QIconLoaderEngine(iconName) : platformTheme->createIconEngine(iconName);
 
-QIconLoaderEngine::QIconLoaderEngine(const QString& iconName)
-        : m_iconName(iconName), m_key(0)
+    qCDebug(lcIconLoader) << "Resulting engine" << iconEngine;
+    return iconEngine;
+}
+
+/*!
+    \class QThemeIconEngine
+
+    \brief A named-based icon engine for providing theme icons.
+
+    The engine supports invalidation of prior lookups, e.g. when
+    the platform theme changes or the user sets an explicit icon
+    theme.
+
+    The actual icon lookup is handed over to an engine provided
+    by QIconLoader::iconEngine().
+*/
+
+QThemeIconEngine::QThemeIconEngine(const QString& iconName)
+    : QProxyIconEngine()
+    , m_iconName(iconName)
 {
 }
 
-QIconLoaderEngine::~QIconLoaderEngine() = default;
-
-QIconLoaderEngine::QIconLoaderEngine(const QIconLoaderEngine &other)
-        : QIconEngine(other),
-        m_iconName(other.m_iconName),
-        m_key(0)
+QThemeIconEngine::QThemeIconEngine(const QThemeIconEngine &other)
+    : QProxyIconEngine()
+    , m_iconName(other.m_iconName)
 {
 }
 
-QIconEngine *QIconLoaderEngine::clone() const
+QString QThemeIconEngine::key() const
 {
-    return new QIconLoaderEngine(*this);
+    // Although we proxy the underlying engine, that's an implementation
+    // detail, so from the point of view of QIcon, and in terms of
+    // serialization, we are the one and only theme icon engine.
+    return u"QThemeIconEngine"_s;
 }
 
-bool QIconLoaderEngine::read(QDataStream &in) {
+QIconEngine *QThemeIconEngine::clone() const
+{
+    return new QThemeIconEngine(*this);
+}
+
+bool QThemeIconEngine::read(QDataStream &in) {
     in >> m_iconName;
     return true;
 }
 
-bool QIconLoaderEngine::write(QDataStream &out) const
+bool QThemeIconEngine::write(QDataStream &out) const
 {
     out << m_iconName;
     return true;
 }
 
+QIconEngine *QThemeIconEngine::proxiedEngine() const
+{
+    const auto *iconLoader = QIconLoader::instance();
+    auto mostRecentThemeKey = iconLoader->themeKey();
+    if (mostRecentThemeKey != m_themeKey) {
+        qCDebug(lcIconLoader) << "Theme key" << mostRecentThemeKey << "is different"
+            << "than cached key" << m_themeKey << "for icon" << m_iconName;
+        m_proxiedEngine.reset(iconLoader->iconEngine(m_iconName));
+        m_themeKey = mostRecentThemeKey;
+    }
+    return m_proxiedEngine.get();
+}
+
+/*!
+    \class QIconLoaderEngine
+
+    \brief An icon engine based on icon entries collected by QIconLoader.
+
+    The design and implementation of QIconLoader is based on
+    the XDG icon specification.
+*/
+
+QIconLoaderEngine::QIconLoaderEngine(const QString& iconName)
+    : m_iconName(iconName)
+    , m_info(QIconLoader::instance()->loadIcon(m_iconName))
+{
+}
+
+QIconLoaderEngine::~QIconLoaderEngine() = default;
+
+QIconEngine *QIconLoaderEngine::clone() const
+{
+    Q_UNREACHABLE();
+    return nullptr; // Cannot be cloned
+}
+
 bool QIconLoaderEngine::hasIcon() const
 {
     return !(m_info.entries.empty());
 }
 
-// Lazily load the icon
-void QIconLoaderEngine::ensureLoaded()
-{
-    if (QIconLoader::instance()->themeKey() != m_key) {
-        m_info = QIconLoader::instance()->loadIcon(m_iconName);
-        m_key = QIconLoader::instance()->themeKey();
-    }
-}
-
 void QIconLoaderEngine::paint(QPainter *painter, const QRect &rect,
                              QIcon::Mode mode, QIcon::State state)
 {
@@ -738,8 +812,6 @@ QSize QIconLoaderEngine::actualSize(const QSize &size, QIcon::Mode mode,
     Q_UNUSED(mode);
     Q_UNUSED(state);
 
-    ensureLoaded();
-
     QIconLoaderEngineEntry *entry = entryForSize(m_info, size);
     if (entry) {
         const QIconDirInfo &dir = entry->dir;
@@ -808,8 +880,6 @@ QPixmap ScalableEntry::pixmap(const QSize &size, QIcon::Mode mode, QIcon::State
 QPixmap QIconLoaderEngine::pixmap(const QSize &size, QIcon::Mode mode,
                                  QIcon::State state)
 {
-    ensureLoaded();
-
     QIconLoaderEngineEntry *entry = entryForSize(m_info, size);
     if (entry)
         return entry->pixmap(size, mode, state);
@@ -824,19 +894,16 @@ QString QIconLoaderEngine::key() const
 
 QString QIconLoaderEngine::iconName()
 {
-    ensureLoaded();
     return m_info.iconName;
 }
 
 bool QIconLoaderEngine::isNull()
 {
-    ensureLoaded();
     return m_info.entries.empty();
 }
 
 QPixmap QIconLoaderEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale)
 {
-    ensureLoaded();
     const int integerScale = qCeil(scale);
     QIconLoaderEngineEntry *entry = entryForSize(m_info, size / integerScale, integerScale);
     return entry ? entry->pixmap(size, mode, state) : QPixmap();
@@ -846,7 +913,7 @@ QList<QSize> QIconLoaderEngine::availableSizes(QIcon::Mode mode, QIcon::State st
 {
     Q_UNUSED(mode);
     Q_UNUSED(state);
-    ensureLoaded();
+
     const qsizetype N = qsizetype(m_info.entries.size());
     QList<QSize> sizes;
     sizes.reserve(N);

src/gui/image/qiconloader_p.h

@@ -22,6 +22,7 @@
 #include <QtGui/QIconEngine>
 #include <QtGui/QPixmapCache>
 #include <private/qicon_p.h>
+#include <private/qiconengine_p.h>
 #include <private/qfactoryloader_p.h>
 #include <QtCore/QHash>
 #include <QtCore/QList>
@@ -86,6 +87,27 @@ struct QThemeIconInfo
     QString iconName;
 };
 
+class QThemeIconEngine : public QProxyIconEngine
+{
+public:
+    QThemeIconEngine(const QString& iconName = QString());
+    QIconEngine *clone() const override;
+    bool read(QDataStream &in) override;
+    bool write(QDataStream &out) const override;
+
+protected:
+    QIconEngine *proxiedEngine() const override;
+
+private:
+    QThemeIconEngine(const QThemeIconEngine &other);
+    QString key() const override;
+
+    QString m_iconName;
+    mutable uint m_themeKey = 0;
+
+    mutable std::unique_ptr<QIconEngine> m_proxiedEngine;
+};
+
 class QIconLoaderEngine : public QIconEngine
 {
 public:
@@ -96,8 +118,6 @@ public:
     QPixmap pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state) override;
     QSize actualSize(const QSize &size, QIcon::Mode mode, QIcon::State state) override;
     QIconEngine *clone() const override;
-    bool read(QDataStream &in) override;
-    bool write(QDataStream &out) const override;
 
     QString iconName() override;
     bool isNull() override;
@@ -107,14 +127,13 @@ public:
     Q_GUI_EXPORT static QIconLoaderEngineEntry *entryForSize(const QThemeIconInfo &info, const QSize &size, int scale = 1);
 
 private:
+    Q_DISABLE_COPY(QIconLoaderEngine)
+
     QString key() const override;
     bool hasIcon() const;
-    void ensureLoaded();
 
-    QIconLoaderEngine(const QIconLoaderEngine &other);
-    QThemeIconInfo m_info;
     QString m_iconName;
-    uint m_key;
+    QThemeIconInfo m_info;
 
     friend class QIconLoader;
 };
@@ -162,6 +181,8 @@ public:
     void ensureInitialized();
     bool hasUserTheme() const { return !m_userTheme.isEmpty(); }
 
+    QIconEngine *iconEngine(const QString &iconName) const;
+
 private:
     QThemeIconInfo findIconHelper(const QString &themeName,
                                   const QString &iconName,