QIcon: Prefer high-quality images of Windows .ico files.

Refactor the code QPixmapIconEngine::addFile() using a convenience
class for reading all images.
Special-case .ico-files: Read images into a list and replace by
higher-quality ones.

Task-number: QTBUG-39287
Change-Id: I32ab6c77a276dc5d4d9a8f7b216c81149b8772b8
Reviewed-by: aavit <eirik.aavitsland@digia.com>

2 files changed, 93 insertions, 36 deletions

diff --git a/src/gui/image/qicon.cpp b/src/gui/image/qicon.cpp
index 0a4b50bbea..8172ecbb48 100644
--- a/src/gui/image/qicon.cpp
+++ b/src/gui/image/qicon.cpp
@@ -363,44 +363,93 @@ void QPixmapIconEngine::addPixmap(const QPixmap &pixmap, QIcon::Mode mode, QIcon
     }
 }
 
-void QPixmapIconEngine::addFile(const QString &fileName, const QSize &_size, QIcon::Mode mode, QIcon::State state)
+// Read out original image depth as set by ICOReader
+static inline int origIcoDepth(const QImage &image)
 {
-    if (!fileName.isEmpty()) {
-        QString abs = fileName;
-        if (fileName.at(0) != QLatin1Char(':'))
-            abs = QFileInfo(fileName).absoluteFilePath();
-        QImageReader reader(abs);
-
-        do {
-            QSize size = _size;
-            QPixmap pixmap;
-
-            for (int i = 0; i < pixmaps.count(); ++i) {
-                if (pixmaps.at(i).mode == mode && pixmaps.at(i).state == state) {
-                    QPixmapIconEngineEntry *pe = &pixmaps[i];
-                    if (size == QSize()) {
-                        pixmap.convertFromImage(reader.read());
-                        size = pixmap.size();
-                    }
-                    if (pe->size == QSize() && pe->pixmap.isNull()) {
-                        pe->pixmap = QPixmap(pe->fileName);
-                        // Reset the devicePixelRatio. The pixmap may be loaded from a @2x file,
-                        // but be used as a 1x pixmap by QIcon.
-                        pe->pixmap.setDevicePixelRatio(1.0);
-                        pe->size = pe->pixmap.size();
-                    }
-                    if (pe->size == size) {
-                        pe->pixmap = pixmap;
-                        pe->fileName = abs;
-                        return;
-                    }
-                }
+    const QString s = image.text(QStringLiteral("_q_icoOrigDepth"));
+    return s.isEmpty() ? 32 : s.toInt();
+}
+
+static inline int findBySize(const QList<QImage> &images, const QSize &size)
+{
+    for (int i = 0; i < images.size(); ++i) {
+        if (images.at(i).size() == size)
+            return i;
+    }
+    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())
+        return;
+    const QString abs = fileName.startsWith(QLatin1Char(':')) ? fileName : QFileInfo(fileName).absoluteFilePath();
+    const bool ignoreSize = !size.isValid();
+    ImageReader imageReader(abs);
+    const QByteArray format = imageReader.format();
+    if (format.isEmpty()) // Device failed to open or unsupported format.
+        return;
+    QImage image;
+    if (format != "ico") {
+        if (ignoreSize) { // No size specified: Add all images.
+            while (imageReader.read(&image))
+                pixmaps += QPixmapIconEngineEntry(abs, image, mode, state);
+        } else {
+            // Try to match size. If that fails, add a placeholder with the filename and empty pixmap for the size.
+            while (imageReader.read(&image) && image.size() != size) {}
+            pixmaps += image.size() == size ?
+                QPixmapIconEngineEntry(abs, image, mode, state) : QPixmapIconEngineEntry(abs, size, mode, state);
+        }
+        return;
+    }
+    // Special case for reading Windows ".ico" files. Historically (QTBUG-39287),
+    // these files may contain low-resolution images. As this information is lost,
+    // ICOReader sets the original format as an image text key value. Read all matching
+    // images into a list trying to find the highest quality per size.
+    QList<QImage> icoImages;
+    while (imageReader.read(&image)) {
+        if (ignoreSize || image.size() == size) {
+            const int position = findBySize(icoImages, image.size());
+            if (position >= 0) { // Higher quality available? -> replace.
+                if (origIcoDepth(image) > origIcoDepth(icoImages.at(position)))
+                    icoImages[position] = image;
+            } else {
+                icoImages.append(image);
             }
-            QPixmapIconEngineEntry e(abs, size, mode, state);
-            e.pixmap = pixmap;
-            pixmaps += e;
-        } while (reader.jumpToNextImage());
+        }
     }
+    foreach (const QImage &i, icoImages)
+        pixmaps += QPixmapIconEngineEntry(abs, i, mode, state);
+    if (icoImages.isEmpty() && !ignoreSize) // Add placeholder with the filename and empty pixmap for the size.
+        pixmaps += QPixmapIconEngineEntry(abs, size, mode, state);
 }
 
 QString QPixmapIconEngine::key() const
diff --git a/src/gui/image/qicon_p.h b/src/gui/image/qicon_p.h
index a46cc310ad..68cd4a5452 100644
--- a/src/gui/image/qicon_p.h
+++ b/src/gui/image/qicon_p.h
@@ -89,6 +89,7 @@ struct QPixmapIconEngineEntry
         :pixmap(pm), size(pm.size()), mode(m), state(s){}
     QPixmapIconEngineEntry(const QString &file, const QSize &sz = QSize(), QIcon::Mode m = QIcon::Normal, QIcon::State s = QIcon::Off)
         :fileName(file), size(sz), mode(m), state(s){}
+    QPixmapIconEngineEntry(const QString &file, const QImage &image, QIcon::Mode m = QIcon::Normal, QIcon::State s = QIcon::Off);
     QPixmap pixmap;
     QString fileName;
     QSize size;
@@ -97,7 +98,14 @@ struct QPixmapIconEngineEntry
     bool isNull() const {return (fileName.isEmpty() && pixmap.isNull()); }
 };
 
-
+inline QPixmapIconEngineEntry::QPixmapIconEngineEntry(const QString &file, const QImage &image, QIcon::Mode m, QIcon::State s)
+    : fileName(file), size(image.size()), mode(m), state(s)
+{
+    pixmap.convertFromImage(image);
+    // Reset the devicePixelRatio. The pixmap may be loaded from a @2x file,
+    // but be used as a 1x pixmap by QIcon.
+    pixmap.setDevicePixelRatio(1.0);
+}
 
 class QPixmapIconEngine : public QIconEngine {
 public: