QIcon/QIconLoader: misc cleanup

Cleanup QIcon:
 - factor out actualSize() calculation
 - factor out deleting invalid pixmap entry
 - don't overflow on int*int calculation
 - use quint8 for state and mode in cacheKey calculation
 - sync cacheKey calculation
 - make QIconPrivate::pixmapDevicePixelRatio() static

Change-Id: I7716b6f69687b6e5c910a0cb180b32b2de9a015d
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

src/gui/image/qicon.cpp

@@ -32,6 +32,33 @@
 QT_BEGIN_NAMESPACE
 
 using namespace Qt::StringLiterals;
+// Convenience class providing a bool read() function.
+namespace {
+class ImageReader
+{
+public:
+    ImageReader(const QString &fileName) : m_reader(fileName), m_atEnd(false) { }
+
+    QByteArray format() const { return m_reader.format(); }
+
+    bool read(QImage *image)
+    {
+        if (m_atEnd)
+            return false;
+        *image = m_reader.read();
+        if (!image->size().isValid()) {
+            m_atEnd = true;
+            return false;
+        }
+        m_atEnd = !m_reader.jumpToNextImage();
+        return true;
+    }
+
+private:
+    QImageReader m_reader;
+    bool m_atEnd;
+};
+} // namespace
 
 /*!
     \enum QIcon::Mode
@@ -150,7 +177,7 @@ void QPixmapIconEngine::paint(QPainter *painter, const QRect &rect, QIcon::Mode
     painter->drawPixmap(rect, px);
 }
 
-static inline int area(const QSize &s) { return s.width() * s.height(); }
+static inline qint64 area(const QSize &s) { return qint64(s.width()) * s.height(); }
 
 // Returns the smallest of the two that is still larger than or equal to size.
 // Pixmaps at the correct scale are preferred, pixmaps at lower scale are
@@ -178,18 +205,18 @@ static QPixmapIconEngineEntry *bestSizeScaleMatch(const QSize &size, qreal scale
         return (qAbs(ascore) < qAbs(bscore)) ? pa : pb;
     }
 
-    int s = area(size);
+    qint64 s = area(size);
     if (pa->size == QSize() && pa->pixmap.isNull()) {
         pa->pixmap = QPixmap(pa->fileName);
         pa->size = pa->pixmap.size();
     }
-    int a = area(pa->size);
+    qint64 a = area(pa->size);
     if (pb->size == QSize() && pb->pixmap.isNull()) {
         pb->pixmap = QPixmap(pb->fileName);
         pb->size = pb->pixmap.size();
     }
-    int b = area(pb->size);
+    qint64 b = area(pb->size);
-    int res = a;
+    qint64 res = a;
     if (qMin(a,b) >= s)
         res = qMin(a,b);
     else
@@ -273,41 +300,30 @@ QPixmap QPixmapIconEngine::pixmap(const QSize &size, QIcon::Mode mode, QIcon::St
 
 QPixmap QPixmapIconEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale)
 {
-
     QPixmap pm;
     QPixmapIconEngineEntry *pe = bestMatch(size, scale, mode, state, false);
     if (pe)
         pm = pe->pixmap;
 
     if (pm.isNull()) {
-        auto idx = pixmaps.size();
+        removePixmapEntry(pe);
-        while (--idx >= 0) {
-            if (pe == &pixmaps.at(idx)) {
-                pixmaps.remove(idx);
-                break;
-            }
-        }
         if (pixmaps.isEmpty())
             return pm;
-        else
+        return scaledPixmap(size, mode, state, scale);
-            return pixmap(size, mode, state);
     }
 
-    QSize actualSize = pm.size();
+    const auto actualSize = adjustSize(size, pm.size());
-    if (!actualSize.isNull() && (actualSize.width() > size.width() || actualSize.height() > size.height()))
-        actualSize.scale(size, Qt::KeepAspectRatio);
-
     QString key = "qt_"_L1
                   % HexString<quint64>(pm.cacheKey())
-                  % HexString<uint>(pe ? pe->mode : QIcon::Normal)
+                  % HexString<quint8>(pe ? pe->mode : QIcon::Normal)
                   % HexString<quint64>(QGuiApplication::palette().cacheKey())
                   % HexString<uint>(actualSize.width())
                   % HexString<uint>(actualSize.height());
 
     if (mode == QIcon::Active) {
-        if (QPixmapCache::find(key % HexString<uint>(mode), &pm))
+        if (QPixmapCache::find(key % HexString<quint8>(mode), &pm))
             return pm; // horray
-        if (QPixmapCache::find(key % HexString<uint>(QIcon::Normal), &pm)) {
+        if (QPixmapCache::find(key % HexString<quint8>(QIcon::Normal), &pm)) {
             QPixmap active = pm;
             if (QGuiApplication *guiApp = qobject_cast<QGuiApplication *>(qApp))
                 active = static_cast<QGuiApplicationPrivate*>(QObjectPrivate::get(guiApp))->applyQIconStyleHelper(QIcon::Active, pm);
@@ -316,7 +332,7 @@ QPixmap QPixmapIconEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIc
         }
     }
 
-    if (!QPixmapCache::find(key % HexString<uint>(mode), &pm)) {
+    if (!QPixmapCache::find(key % HexString<quint8>(mode), &pm)) {
         if (pm.size() != actualSize)
             pm = pm.scaled(actualSize, Qt::IgnoreAspectRatio, Qt::SmoothTransformation);
         if (pe->mode != mode && mode != QIcon::Normal) {
@@ -326,7 +342,7 @@ QPixmap QPixmapIconEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIc
             if (!generated.isNull())
                 pm = generated;
         }
-        QPixmapCache::insert(key % HexString<uint>(mode), pm);
+        QPixmapCache::insert(key % HexString<quint8>(mode), pm);
     }
     return pm;
 }
@@ -343,12 +359,7 @@ QSize QPixmapIconEngine::actualSize(const QSize &size, QIcon::Mode mode, QIcon::
     if (QPixmapIconEngineEntry *pe = bestMatch(size, scale, mode, state, true))
         actualSize = pe->size;
 
-    if (actualSize.isNull())
+    return adjustSize(size, actualSize);
-        return actualSize;
-
-    if (!actualSize.isNull() && (actualSize.width() > size.width() || actualSize.height() > size.height()))
-        actualSize.scale(size, Qt::KeepAspectRatio);
-    return actualSize;
 }
 
 QList<QSize> QPixmapIconEngine::availableSizes(QIcon::Mode mode, QIcon::State state)
@@ -396,34 +407,6 @@ static inline int findBySize(const QList<QImage> &images, const QSize &size)
     return -1;
 }
 
-// Convenience class providing a bool read() function.
-namespace {
-class ImageReader
-{
-public:
-    ImageReader(const QString &fileName) : m_reader(fileName), m_atEnd(false) {}
-
-    QByteArray format() const { return m_reader.format(); }
-
-    bool read(QImage *image)
-    {
-        if (m_atEnd)
-            return false;
-        *image = m_reader.read();
-        if (!image->size().isValid()) {
-            m_atEnd = true;
-            return false;
-        }
-        m_atEnd = !m_reader.jumpToNextImage();
-        return true;
-    }
-
-private:
-    QImageReader m_reader;
-    bool m_atEnd;
-};
-} // namespace
-
 void QPixmapIconEngine::addFile(const QString &fileName, const QSize &size, QIcon::Mode mode, QIcon::State state)
 {
     if (fileName.isEmpty())

src/gui/image/qicon_p.h

@@ -34,7 +34,7 @@ public:
         delete engine;
     }
 
-    qreal pixmapDevicePixelRatio(qreal displayDevicePixelRatio, const QSize &requestedSize, const QSize &actualSize);
+    static qreal pixmapDevicePixelRatio(qreal displayDevicePixelRatio, const QSize &requestedSize, const QSize &actualSize);
 
     QIconEngine *engine;
 
@@ -89,7 +89,24 @@ public:
     bool read(QDataStream &in) override;
     bool write(QDataStream &out) const override;
 
+    static inline QSize adjustSize(const QSize &expectedSize, QSize size)
+    {
+        if (!size.isNull() && (size.width() > expectedSize.width() || size.height() > expectedSize.height()))
+            size.scale(expectedSize, Qt::KeepAspectRatio);
+        return size;
+    }
+
 private:
+    void removePixmapEntry(QPixmapIconEngineEntry *pe)
+    {
+        auto idx = pixmaps.size();
+        while (--idx >= 0) {
+            if (pe == &pixmaps.at(idx)) {
+                pixmaps.remove(idx);
+                return;
+            }
+        }
+    }
     QPixmapIconEngineEntry *tryMatch(const QSize &size, qreal scale, QIcon::Mode mode, QIcon::State state);
     QList<QPixmapIconEngineEntry> pixmaps;
 

src/gui/image/qiconloader.cpp

@@ -894,18 +894,15 @@ QPixmap PixmapEntry::pixmap(const QSize &size, QIcon::Mode mode, QIcon::State st
     if (basePixmap.isNull())
         basePixmap.load(filename);
 
-    QSize actualSize = basePixmap.size();
     // If the size of the best match we have (basePixmap) is larger than the
     // requested size, we downscale it to match.
-    if (!actualSize.isNull() && (actualSize.width() > size.width() || actualSize.height() > size.height()))
+    const auto actualSize = QPixmapIconEngine::adjustSize(size, basePixmap.size());
-        actualSize.scale(size, Qt::KeepAspectRatio);
-
     QString key = "$qt_theme_"_L1
-                  % HexString<qint64>(basePixmap.cacheKey())
+                  % HexString<quint64>(basePixmap.cacheKey())
-                  % HexString<int>(mode)
+                  % HexString<quint8>(mode)
-                  % HexString<qint64>(QGuiApplication::palette().cacheKey())
+                  % HexString<quint64>(QGuiApplication::palette().cacheKey())
-                  % HexString<int>(actualSize.width())
+                  % HexString<uint>(actualSize.width())
-                  % HexString<int>(actualSize.height());
+                  % HexString<uint>(actualSize.height());
 
     QPixmap cachedPixmap;
     if (QPixmapCache::find(key, &cachedPixmap)) {