Merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk-fix-icon-svg-image-loading into lp:ubuntu-ui-toolkit

[Gerry Boland]

Adjust the UnityThemeIconProvider to duplicate Qt's own image loading behaviour, fixes some image scaling issues

[Gerry Boland (gerboland)]

The Pixmap->Image thing might be unnecessary, but
1) I wanted to duplicate exactly Qt's behaviour and
2) Qt converts the QPixmap to a QImage internally before uploading to texture

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1281
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/2307/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-vivid-amd64-ci/130/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-vivid-armhf-ci/131/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-ui-toolkit-ci/2307/rebuild

review: Needs Fixing (continuous-integration)

[lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk-fix-icon-svg-image-loading]

1282. By Michał Sawicz on 2016-04-20

Merge trunk

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1282
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/2310/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-vivid-amd64-ci/133/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-vivid-armhf-ci/134/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-ui-toolkit-ci/2310/rebuild

review: Needs Fixing (continuous-integration)

[lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk-fix-icon-svg-image-loading]

1283. By Michał Sawicz on 2016-04-21

Make tst_IconProvider talk in QImage

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1283
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/2315/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-vivid-amd64-ci/138/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-vivid-armhf-ci/139/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-ui-toolkit-ci/2315/rebuild

review: Needs Fixing (continuous-integration)

[Michał Sawicz (saviq)]

Notes to self:
* the second argument to request{Pixmap,Image} is the implicit image size
* it's a bit trickier with icons because we should respect the icon theme properties like size and maxSize

[lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk-fix-icon-svg-image-loading]

1284. By Lukáš Tinkl on 2016-04-22

fix loading & scaling SVGs

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1284
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/2319/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-vivid-amd64-ci/142/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-vivid-armhf-ci/143/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-ui-toolkit-ci/2319/rebuild

review: Needs Fixing (continuous-integration)

[lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk-fix-icon-svg-image-loading]

1285. By Lukáš Tinkl on 2016-04-25

add a test with wrong aspect ratio that returns a correct one

1286. By Lukáš Tinkl on 2016-04-27

add a test that passes here and fails with trunk

[Gerry Boland (gerboland)]

Presumably we need to propose this same change against the uitk staging branch. Please make a second MP for this (keep this one too, we'll land this one)

[lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk-fix-icon-svg-image-loading]

1287. By Lukáš Tinkl on 2016-05-02

sync with staging

[Lukáš Tinkl (lukas-kde)]

Sync with staging version (https://code.launchpad.net/~lukas-kde/ubuntu-ui-toolkit/staging-fix-icon-svg-image-loading/+merge/293281)

[Tim Peeters (tpeeters)]

Same as the staging MR.

review: Approve

=== modified file 'src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp'
--- src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp	2016-03-16 14:24:15 +0000
+++ src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp	2016-05-02 13:23:29 +0000
@@ -21,8 +21,7 @@
 #include <QDir>
 #include <QFileInfo>
 #include <QSettings>
-#include <QSvgRenderer>
-#include <QPainter>
+#include <QImageReader>
 #include <QStandardPaths>
 #include <QtDebug>
 
@@ -48,27 +47,27 @@
     // Does a breadth-first search for an icon with any name in @names. Parent
     // themes are only looked at if the current theme doesn't contain any icon
     // in @names.
-    QPixmap findBestIcon(const QStringList &names, const QSize &size, QSet<QString> *alreadySearchedThemes)
+    QImage findBestIcon(const QStringList &names, QSize *impsize, const QSize &size, QSet<QString> *alreadySearchedThemes)
     {
         if (alreadySearchedThemes) {
             if (alreadySearchedThemes->contains(name))
-                return QPixmap();
+                return QImage();
             alreadySearchedThemes->insert(name);
         }
 
         Q_FOREACH(const QString &name, names) {
-            QPixmap pixmap = lookupIcon(name, size);
-            if (!pixmap.isNull())
-                return pixmap;
+            QImage image = lookupIcon(name, impsize, size);
+            if (!image.isNull())
+                return image;
         }
 
         Q_FOREACH(IconThemePointer theme, parents) {
-            QPixmap pixmap = theme->findBestIcon(names, size, alreadySearchedThemes);
-            if (!pixmap.isNull())
-                return pixmap;
+            QImage image = theme->findBestIcon(names, impsize, size, alreadySearchedThemes);
+            if (!image.isNull())
+                return image;
         }
 
-        return QPixmap();
+        return QImage();
     }
 
 private:
@@ -82,7 +81,7 @@
 
     IconTheme(const QString &name): name(name)
     {
-        QStringList paths = QStandardPaths::standardLocations(QStandardPaths::GenericDataLocation);
+        const QStringList paths = QStandardPaths::standardLocations(QStandardPaths::GenericDataLocation);
 
         Q_FOREACH(const QString &path, paths) {
             QDir dir(path + "/icons/" + name);
@@ -130,30 +129,42 @@
         return Fixed;
     }
 
-    static QPixmap loadIcon(const QString &filename, const QSize &size)
+    static QImage loadIcon(const QString &filename, QSize *impsize, const QSize &requestSize)
     {
-        QPixmap pixmap;
-
-        const bool anyZero = size.width() <= 0 || size.height() <= 0;
-        const Qt::AspectRatioMode scaleMode = anyZero ? Qt::KeepAspectRatioByExpanding : Qt::KeepAspectRatio;
-
-        if (filename.endsWith(QLatin1String(".png"))) {
-            pixmap = QPixmap(filename);
-            if (!pixmap.isNull() && !size.isNull() && (pixmap.width() != size.width() || pixmap.height() != size.height())) {
-                const QSize newSize = pixmap.size().scaled(size.width(), size.height(), scaleMode);
-                pixmap = pixmap.scaled(newSize);
-            }
-        }
-        else if (filename.endsWith(QLatin1String(".svg"))) {
-            QSvgRenderer renderer(filename);
-            pixmap = QPixmap(renderer.defaultSize().scaled(size.width(), size.height(), scaleMode));
-            pixmap.fill(Qt::transparent);
-            QPainter painter(&pixmap);
-            renderer.render(&painter);
-            painter.end();
-        }
-
-        return pixmap;
+        QImage image;
+        QImageReader imgio(filename);
+
+        const bool force_scale = imgio.format() == QStringLiteral("svg")
+            || imgio.format() == QStringLiteral("svgz");
+
+        if (requestSize.width() > 0 || requestSize.height() > 0) {
+            QSize s = imgio.size();
+            qreal ratio = 0.0;
+            if (requestSize.width() > 0 && (force_scale || requestSize.width() < s.width())) {
+                ratio = qreal(requestSize.width())/s.width();
+            }
+            if (requestSize.height() > 0 && (force_scale || requestSize.height() < s.height())) {
+                qreal hr = qreal(requestSize.height())/s.height();
+                if (ratio == 0.0 || hr < ratio)
+                    ratio = hr;
+            }
+            if (ratio > 0.0) {
+                s.setHeight(qRound(s.height() * ratio));
+                s.setWidth(qRound(s.width() * ratio));
+                imgio.setScaledSize(s);
+            }
+        }
+
+        if (impsize)
+            *impsize = imgio.scaledSize();
+
+        if (imgio.read(&image)) {
+            if (impsize)
+                *impsize = image.size();
+            return image;
+        } else {
+            return QImage();
+        }
     }
 
     QString lookupIconFile(const QString &dir, const QString &name)
@@ -174,16 +185,16 @@
         return QString();
     }
 
-    QPixmap lookupIcon(const QString &iconName, const QSize &size)
+    QImage lookupIcon(const QString &iconName, QSize *impsize, const QSize &size)
     {
         const int iconSize = qMax(size.width(), size.height());
         if (iconSize > 0)
-            return lookupBestMatchingIcon(iconName, size);
+            return lookupBestMatchingIcon(iconName, impsize, size);
         else
-            return lookupLargestIcon(iconName);
+            return lookupLargestIcon(iconName, impsize);
     }
 
-    QPixmap lookupBestMatchingIcon(const QString &iconName, const QSize &size)
+    QImage lookupBestMatchingIcon(const QString &iconName, QSize *impsize, const QSize &size)
     {
         int minDistance = 10000;
         QString bestFilename;
@@ -205,12 +216,12 @@
         }
 
         if (!bestFilename.isNull())
-            return loadIcon(bestFilename, size);
+            return loadIcon(bestFilename, impsize, size);
 
-        return QPixmap();
+        return QImage();
     }
 
-    QPixmap lookupLargestIcon(const QString &iconName)
+    QImage lookupLargestIcon(const QString &iconName, QSize *impsize)
     {
         int maxSize = 0;
         QString bestFilename;
@@ -228,9 +239,9 @@
         }
 
         if (!bestFilename.isNull())
-            return loadIcon(bestFilename, QSize(maxSize, maxSize));
+            return loadIcon(bestFilename, impsize, QSize(maxSize, maxSize));
 
-        return QPixmap();
+        return QImage();
     }
 
     int directorySizeDistance(const Directory &dir, const QSize &iconSize)
@@ -258,24 +269,23 @@
 };
 
 UnityThemeIconProvider::UnityThemeIconProvider(const QString &themeName):
-  QQuickImageProvider(QQuickImageProvider::Pixmap)
+  QQuickImageProvider(QQuickImageProvider::Image)
 {
     theme = IconTheme::get(themeName);
 }
 
-QPixmap UnityThemeIconProvider::requestPixmap(const QString &id, QSize *size, const QSize &requestedSize)
+QImage UnityThemeIconProvider::requestImage(const QString &id, QSize *size, const QSize &requestedSize)
 {
     // The hicolor theme will be searched last as per
     // https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html
     QSet<QString> alreadySearchedThemes;
     const QStringList names = id.split(QLatin1Char(','), QString::SkipEmptyParts);
-    QPixmap pixmap = theme->findBestIcon(names, requestedSize, &alreadySearchedThemes);
+    QImage image = theme->findBestIcon(names, size, requestedSize, &alreadySearchedThemes);
 
-    if (pixmap.isNull()) {
+    if (image.isNull()) {
         IconTheme::IconThemePointer theme = IconTheme::get(QStringLiteral("hicolor"));
-        return theme->findBestIcon(names, requestedSize, nullptr);
+        return theme->findBestIcon(names, size, requestedSize, nullptr);
     }
 
-    *size = pixmap.size();
-    return pixmap;
+    return image;
 }

=== modified file 'src/Ubuntu/Components/plugin/unitythemeiconprovider.h'
--- src/Ubuntu/Components/plugin/unitythemeiconprovider.h	2016-03-16 14:24:15 +0000
+++ src/Ubuntu/Components/plugin/unitythemeiconprovider.h	2016-05-02 13:23:29 +0000
@@ -25,7 +25,7 @@
 {
 public:
     UnityThemeIconProvider(const QString &themeName = QStringLiteral("suru"));
-    QPixmap requestPixmap(const QString &id, QSize *size, const QSize &requestedSize);
+    QImage requestImage(const QString &id, QSize *size, const QSize &requestedSize);
 
 private:
     QSharedPointer<class IconTheme> theme;

=== modified file 'tests/unit_x11/tst_components/tst_icon.qml'
--- tests/unit_x11/tst_components/tst_icon.qml	2016-02-02 10:19:17 +0000
+++ tests/unit_x11/tst_components/tst_icon.qml	2016-05-02 13:23:29 +0000
@@ -58,6 +58,19 @@
             height: width
             source: Qt.resolvedUrl("tst_icon-select.png")
         }
+
+        // Regression test for bug #1421293
+        Icon {
+            id: icon4
+            height: units.gu(2)
+            source: Qt.resolvedUrl("logo.png")
+        }
+        Icon {
+            id: icon5
+            height: units.gu(2)
+            width: implicitWidth > 0 && implicitHeight > 0 ? (implicitWidth / implicitHeight * height) : implicitWidth
+            source: Qt.resolvedUrl("logo.png")
+        }
     }
 
     UbuntuTestCase {
@@ -131,5 +144,11 @@
             icon.color = Qt.rgba(0.0, 0.0, 0.0, 0.0);
             compare(shader.visible, false);
         }
+
+        function test_iconScaling_bug1421293() {
+            // make sure both icons end up the same size
+            compare(icon4.width, icon5.width);
+            compare(icon4.height, icon5.height);
+        }
     }
 }

=== modified file 'tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp'
--- tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp	2016-03-16 15:02:35 +0000
+++ tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp	2016-05-02 13:23:29 +0000
@@ -39,14 +39,15 @@
         QTest::addColumn<QSize>("requestSize");
         QTest::addColumn<QSize>("resultSize");
 
-        QTest::newRow("battery0") << "battery-100-charging" << QSize(-1, -1) << QSize(256, 165);
-        QTest::newRow("battery1") << "battery-100-charging" << QSize(-1, 16) << QSize(24, 16);
+        QTest::newRow("battery0") << "battery-100-charging" << QSize(-1, -1) << QSize(256, 166);
+        QTest::newRow("battery1") << "battery-100-charging" << QSize(-1, 16) << QSize(25, 16);
         QTest::newRow("battery2") << "battery-100-charging" << QSize(16, -1) << QSize(16, 10);
-        QTest::newRow("battery3") << "battery-100-charging" << QSize(0, 16) << QSize(24, 16);
+        QTest::newRow("battery3") << "battery-100-charging" << QSize(0, 16) << QSize(25, 16);
         QTest::newRow("battery4") << "battery-100-charging" << QSize(16, 0) << QSize(16, 10);
         QTest::newRow("battery5") << "battery-100-charging" << QSize(24, 16) << QSize(24, 16);
-        QTest::newRow("battery6") << "battery-100-charging" << QSize(24, 24) << QSize(24, 15);
+        QTest::newRow("battery6") << "battery-100-charging" << QSize(24, 24) << QSize(24, 16);
         QTest::newRow("battery7") << "battery-100-charging" << QSize(37, 24) << QSize(37, 24);
+        QTest::newRow("battery8") << "battery-100-charging" << QSize(15, 512) << QSize(15, 10);
 
         QTest::newRow("gallery0") << "gallery-app" << QSize(-1, -1) << QSize(512, 512);
         QTest::newRow("gallery1") << "gallery-app" << QSize(-1, 16) << QSize(16, 16);
@@ -65,13 +66,13 @@
 
         UnityThemeIconProvider provider("mockTheme");
         QSize returnedSize;
-        QPixmap p = provider.requestPixmap(icon, &returnedSize, requestSize);
-        QCOMPARE(p.size(), resultSize);
+        QImage i = provider.requestImage(icon, &returnedSize, requestSize);
+        QCOMPARE(i.size(), resultSize);
         QCOMPARE(returnedSize, resultSize);
 
         // Search again to make sure subsequent searches still work
-        p = provider.requestPixmap(icon, &returnedSize, requestSize);
-        QCOMPARE(p.size(), resultSize);
+        i = provider.requestImage(icon, &returnedSize, requestSize);
+        QCOMPARE(i.size(), resultSize);
         QCOMPARE(returnedSize, resultSize);
     }
 
@@ -82,14 +83,14 @@
 
         // myapp is in MockTheme3 (white) and hicolor (black)
         // MockTheme3 one is returned since hicolor is last per spec
-        QPixmap p = provider.requestPixmap("myapp", &returnedSize, QSize(-1, -1));
-        QVERIFY(!p.isNull());
-        QCOMPARE(QColor(p.toImage().pixel(0,0)), QColor(Qt::white));
+        QImage i = provider.requestImage("myapp", &returnedSize, QSize(-1, -1));
+        QVERIFY(!i.isNull());
+        QCOMPARE(QColor(i.pixel(0,0)), QColor(Qt::white));
 
         // myapp2 is only in hicolor (black) so that's the one returned
-        p = provider.requestPixmap("myapp2", &returnedSize, QSize(-1, -1));
-        QVERIFY(!p.isNull());
-        QCOMPARE(QColor(p.toImage().pixel(0,0)), QColor(Qt::black));
+        i = provider.requestImage("myapp2", &returnedSize, QSize(-1, -1));
+        QVERIFY(!i.isNull());
+        QCOMPARE(QColor(i.pixel(0,0)), QColor(Qt::black));
     }
 };