Merge lp:~aacid/ubuntu-ui-toolkit/nonsquareicons into lp:ubuntu-ui-toolkit/staging

[Albert Astals Cid]

Fix infinite icon size grow when asked for a non square icon

Without this patch we could end up in loops like
requested icon "battery-080-charging" QSize(24, 24)
returned QSize(37, 24)
requested icon "battery-080-charging" QSize(37, 24)
returned QSize(57, 37)
requested icon "battery-080-charging" QSize(57, 24)
returned QSize(88, 57)
requested icon "battery-080-charging" QSize(57, 37)
returned QSize(88, 57)

[Albert Astals Cid (aacid)]

I'm also working on a unit test, since i couldn't find any

[lp:~aacid/ubuntu-ui-toolkit/nonsquareicons]

1411. By Albert Astals Cid on 2015-02-18

Add tests for the icon provider

They are bit dependent on the suru icon theme so may need a bit of adjusting as time goes by

[Albert Astals Cid (aacid)]

The diff seems very big but that's just because i moved the declaration of the icon theme class to the header in r1411 :/

[lp:~aacid/ubuntu-ui-toolkit/nonsquareicons]

1412. By Albert Astals Cid on 2015-02-18

Update

1413. By Albert Astals Cid on 2015-02-18

Get my own mockTheme for more stable testin

1414. By Albert Astals Cid on 2015-02-18

Revert unneeded changes

1415. By Albert Astals Cid on 2015-02-18

We don't need this either

1416. By Albert Astals Cid on 2015-02-18

smaller diff against original code

1417. By Albert Astals Cid on 2015-02-18

my mock inherits noone

[Albert Astals Cid (aacid)]

Without the changes in the cpp code these are the tests that fail

tst_iconprovider: FAIL! : tst_IconProvider::test_loadIcon(battery2) Compared values are not the same
   tst_iconprovider: Actual (p.size()) : QSize(24x16)
   tst_iconprovider: Expected (resultSize): QSize(16x10)
   tst_iconprovider: Loc: [tst_iconprovider.cpp(69)]
i.e. asking for a 16x-1 icon returns a 24x16 icon

tst_iconprovider: FAIL! : tst_IconProvider::test_loadIcon(battery4) Compared values are not the same
   tst_iconprovider: Actual (p.size()) : QSize(24x16)
   tst_iconprovider: Expected (resultSize): QSize(16x10)
   tst_iconprovider: Loc: [tst_iconprovider.cpp(69)]
i.e. asking for a 16x0 icon returns a 24x16 icon

tst_iconprovider: FAIL! : tst_IconProvider::test_loadIcon(battery5) Compared values are not the same
   tst_iconprovider: Actual (p.size()) : QSize(37x24)
   tst_iconprovider: Expected (resultSize): QSize(24x16)
   tst_iconprovider: Loc: [tst_iconprovider.cpp(69)]
i.e. asking for a 24x16 icon returns a 37x24 icon

tst_iconprovider: FAIL! : tst_IconProvider::test_loadIcon(battery7) Compared values are not the same
   tst_iconprovider: Actual (p.size()) : QSize(57x37)
   tst_iconprovider: Expected (resultSize): QSize(37x24)
   tst_iconprovider: Loc: [tst_iconprovider.cpp(69)]
i.e. asking for a 37x24 icon returns a 57x27 icon

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1410
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1450/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1409
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/177/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/180
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/180/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/177/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1247
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1407
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1407/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18145

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1450/rebuild

review: Needs Fixing (continuous-integration)

[Sebastien Bacher (seb128)]

The changes work for me, my unity8 desktop session starts again with that update

review: Approve (runtime testing)

[Zsombor Egri (zsombi)]

Looks good, lets wait till we get the staging fixed with 5.4 (i.e. binding loops in Picker)

review: Approve

[Lars Karlitski (larsu)]

I can't reproduce the infinite loop. That isn't an issue in with the provider, though, as it sets its outgoing size correctly.

However, I agree that returning e.g. a pixmap sized (57, 37) when requesting (37, 24) is obviously bogus. Thanks for the patch!

Note that you still return values greater than requested in some cases. Not sure if that lead to the orginal problem.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1417
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1451/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1416
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/178/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/181
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/181/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/178/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1253
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1414
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1414/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18156

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1451/rebuild

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

Agreed, the Qt::KeepAspectRatioByExpanding makes it return an icon of QSize(37, 24) when asking one of QSize(24, 24) and i'm with you that's a questionable choice.

Zsombor, do you want me to explore fixing that too?

[lp:~aacid/ubuntu-ui-toolkit/nonsquareicons]

1418. By Albert Astals Cid on 2015-02-19

Merge staging

[Zsombor Egri (zsombi)]

> Agreed, the Qt::KeepAspectRatioByExpanding makes it return an icon of
> QSize(37, 24) when asking one of QSize(24, 24) and i'm with you that's a
> questionable choice.
>
> Zsombor, do you want me to explore fixing that too?

If you have time to check it, I wouldn't mind, more, I would be very thankful!

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1418
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1456/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1438
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/183
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/186
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/186/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/183
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1275
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1436
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1436/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18193

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1456/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp'
--- modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp	2014-09-02 09:39:09 +0000
+++ modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp	2015-02-19 08:47:10 +0000
@@ -48,7 +48,7 @@
     // 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, int size)
+    QPixmap findBestIcon(const QStringList &names, const QSize &size)
     {
         Q_FOREACH(const QString &name, names) {
             QPixmap pixmap = lookupIcon(name, size);
@@ -121,18 +121,20 @@
         return Fixed;
     }
 
-    static QPixmap loadIcon(const QString &filename, int size)
+    static QPixmap loadIcon(const QString &filename, const QSize &size)
     {
         QPixmap pixmap;
 
         if (filename.endsWith(".png")) {
             pixmap = QPixmap(filename);
-            if (!pixmap.isNull() && size > 0 && (pixmap.width() != size || pixmap.height() != size))
-                pixmap = pixmap.scaled(size, size, Qt::KeepAspectRatioByExpanding);
+            if (!pixmap.isNull() && !size.isNull() && (pixmap.width() != size.width() || pixmap.height() != size.height())) {
+                const QSize newSize = pixmap.size().scaled(size.width(), size.height(), Qt::KeepAspectRatioByExpanding);
+                pixmap = pixmap.scaled(newSize);
+            }
         }
         else if (filename.endsWith(".svg")) {
             QSvgRenderer renderer(filename);
-            pixmap = QPixmap(renderer.defaultSize().scaled(size, size, Qt::KeepAspectRatioByExpanding));
+            pixmap = QPixmap(renderer.defaultSize().scaled(size.width(), size.height(), Qt::KeepAspectRatioByExpanding));
             pixmap.fill(Qt::transparent);
             QPainter painter(&pixmap);
             renderer.render(&painter);
@@ -160,15 +162,16 @@
         return QString();
     }
 
-    QPixmap lookupIcon(const QString &iconName, int size)
+    QPixmap lookupIcon(const QString &iconName, const QSize &size)
     {
-        if (size > 0)
+        const int iconSize = qMax(size.width(), size.height());
+        if (iconSize > 0)
             return lookupBestMatchingIcon(iconName, size);
         else
             return lookupLargestIcon(iconName);
     }
 
-    QPixmap lookupBestMatchingIcon(const QString &iconName, int size)
+    QPixmap lookupBestMatchingIcon(const QString &iconName, const QSize &size)
     {
         int minDistance = 10000;
         QString bestFilename;
@@ -213,13 +216,14 @@
         }
 
         if (!bestFilename.isNull())
-            return loadIcon(bestFilename, maxSize);
+            return loadIcon(bestFilename, QSize(maxSize, maxSize));
 
         return QPixmap();
     }
 
-    int directorySizeDistance(const Directory &dir, int size)
+    int directorySizeDistance(const Directory &dir, const QSize &iconSize)
     {
+        const int size = qMax(iconSize.width(), iconSize.height());
         switch (dir.sizeType) {
             case Fixed:
                 return qAbs(size - dir.size);
@@ -241,16 +245,15 @@
     QList<IconThemePointer> parents;
 };
 
-UnityThemeIconProvider::UnityThemeIconProvider():
+UnityThemeIconProvider::UnityThemeIconProvider(const QString &themeName):
   QQuickImageProvider(QQuickImageProvider::Pixmap)
 {
-    theme = IconTheme::get("suru");
+    theme = IconTheme::get(themeName);
 }
 
 QPixmap UnityThemeIconProvider::requestPixmap(const QString &id, QSize *size, const QSize &requestedSize)
 {
-    int iconSize = qMax(requestedSize.width(), requestedSize.height());
-    QPixmap pixmap = theme->findBestIcon(id.split(",", QString::SkipEmptyParts), iconSize);
+    QPixmap pixmap = theme->findBestIcon(id.split(",", QString::SkipEmptyParts), requestedSize);
     *size = pixmap.size();
     return pixmap;
 }
 
=== modified file 'modules/Ubuntu/Components/plugin/unitythemeiconprovider.h'
--- modules/Ubuntu/Components/plugin/unitythemeiconprovider.h	2014-09-02 09:39:09 +0000
+++ modules/Ubuntu/Components/plugin/unitythemeiconprovider.h	2015-02-19 08:47:10 +0000
@@ -24,7 +24,7 @@
 class UnityThemeIconProvider: public QQuickImageProvider
 {
 public:
-    UnityThemeIconProvider();
+    UnityThemeIconProvider(const QString &themeName = "suru");
     QPixmap requestPixmap(const QString &id, QSize *size, const QSize &requestedSize);
 
 private:
 
=== added directory 'tests/unit_x11/tst_iconprovider'
=== added directory 'tests/unit_x11/tst_iconprovider/icons'
=== added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme'
=== added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme/actions'
=== added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme/actions/scalable'
=== added file 'tests/unit_x11/tst_iconprovider/icons/mockTheme/actions/scalable/battery-100-charging.svg'
--- tests/unit_x11/tst_iconprovider/icons/mockTheme/actions/scalable/battery-100-charging.svg	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_iconprovider/icons/mockTheme/actions/scalable/battery-100-charging.svg	2015-02-19 08:47:10 +0000
@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<!-- Created with Inkscape (http://www.inkscape.org/) -->
+<svg id="svg4966" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns="http://www.w3.org/2000/svg" height="90" viewBox="0 0 139.00489 90.000001" width="139" version="1.1" xmlns:cc="http://creativecommons.org/ns#" xmlns:dc="http://purl.org/dc/elements/1.1/">
+ <metadata id="metadata4971">
+  <rdf:RDF>
+   <cc:Work rdf:about="">
+    <dc:format>image/svg+xml</dc:format>
+    <dc:type rdf:resource="http://purl.org/dc/dcmitype/StillImage"/>
+    <dc:title/>
+   </cc:Work>
+  </rdf:RDF>
+ </metadata>
+ <g id="layer1" transform="translate(-149.78 -484.51)">
+  <g id="g5120" transform="translate(-433.79 125.71)">
+   <g id="g5124" transform="translate(409.57 223.43)">
+    <g id="g5126" style="fill:none" transform="matrix(2.1875 0 0 1.875 -456 -1657.8)">
+     <rect id="rect5128" style="opacity:.21171;fill:none" transform="translate(0 804.36)" height="48" width="48" y="152" x="288"/>
+    </g>
+    <path id="path5130" style="fill:#808080" transform="translate(174 135.36)" d="m86 15-75.156 0.012c-7.844-0.012-10.844 1.988-10.844 11.988v18 18c0 10 3 12 10.844 11.988l75.156 0.012c7.8438 0.01172 11-2.3633 11-12v-4.0117h8v-13.988-13.988h-8v-4.012c0-9.637-3.156-12.012-11-12zm-75 6c21.619 0.0096 48.956 0.0081 75 0 4 0 5 1 5 6v18 18c0 5-1 6-5 6-26.044-0.008-53.381-0.01-75 0-4 0-5-1-5-6v-18-18c0-5 1-6 5-6z"/>
+    <path id="path5132" style="fill:#38b44a" d="m1051 697c-3 0-4 0-4 5v26c0 5 1 5 4 5h65c3 0 4 0 4-5v-26c0-5-1-5-4-5z" transform="translate(-861 -534.64)"/>
+    <path id="path3847-0" style="fill:#808080" d="m299 150.36-13.996 35h14v25l14.005-35h-14z"/>
+   </g>
+  </g>
+ </g>
+</svg>
 
=== added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme/apps'
=== added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme/apps/512'
=== added file 'tests/unit_x11/tst_iconprovider/icons/mockTheme/apps/512/gallery-app.png'
 Binary files tests/unit_x11/tst_iconprovider/icons/mockTheme/apps/512/gallery-app.png	1970-01-01 00:00:00 +0000 and tests/unit_x11/tst_iconprovider/icons/mockTheme/apps/512/gallery-app.png	2015-02-19 08:47:10 +0000 differ
=== added file 'tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme'
--- tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme	2015-02-19 08:47:10 +0000
@@ -0,0 +1,17 @@
+[Icon Theme]
+Name=MockTheme
+
+Directories=actions/scalable,apps/512
+
+[actions/scalable]
+MinSize=9
+Size=90
+MaxSize=256
+Context=Actions
+Type=Scalable
+
+[apps/512]
+Size=512
+Context=Applications
+Type=Fixed
+
 
=== added file 'tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp'
--- tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp	2015-02-19 08:47:10 +0000
@@ -0,0 +1,76 @@
+/*
+ * Copyright 2015 Canonical Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <QtTest/QtTest>
+
+#define private public
+#include "unitythemeiconprovider.h"
+#undef private
+
+class tst_IconProvider : public QObject
+{
+    Q_OBJECT
+public:
+    tst_IconProvider() {}
+
+private Q_SLOTS:
+
+    void initTestCase()
+    {
+        qputenv("XDG_DATA_DIRS", SRCDIR);
+    }
+
+    void test_loadIcon_data()
+    {
+        QTest::addColumn<QString>("icon");
+        QTest::addColumn<QSize>("requestSize");
+        QTest::addColumn<QSize>("resultSize");
+
+        QTest::newRow("battery0") << "battery-100-charging" << QSize(-1, -1) << QSize(395, 256);
+        QTest::newRow("battery1") << "battery-100-charging" << QSize(-1, 16) << QSize(24, 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("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(37, 24);
+        QTest::newRow("battery7") << "battery-100-charging" << QSize(37, 24) << QSize(37, 24);
+
+        QTest::newRow("gallery0") << "gallery-app" << QSize(-1, -1) << QSize(512, 512);
+        QTest::newRow("gallery1") << "gallery-app" << QSize(-1, 16) << QSize(16, 16);
+        QTest::newRow("gallery2") << "gallery-app" << QSize(16, -1) << QSize(16, 16);
+        QTest::newRow("gallery3") << "gallery-app" << QSize(0, 16) << QSize(16, 16);
+        QTest::newRow("gallery4") << "gallery-app" << QSize(16, 0) << QSize(16, 16);
+        QTest::newRow("gallery5") << "gallery-app" << QSize(24, 16) << QSize(24, 24);
+        QTest::newRow("gallery6") << "gallery-app" << QSize(24, 24) << QSize(24, 24);
+    }
+
+    void test_loadIcon()
+    {
+        QFETCH(QString, icon);
+        QFETCH(QSize, requestSize);
+        QFETCH(QSize, resultSize);
+
+        UnityThemeIconProvider provider("mockTheme");
+        QSize returnedSize;
+        const QPixmap p = provider.requestPixmap(icon, &returnedSize, requestSize);
+        QCOMPARE(p.size(), resultSize);
+        QCOMPARE(returnedSize, resultSize);
+    }
+};
+
+QTEST_MAIN(tst_IconProvider)
+
+#include "tst_iconprovider.moc"
 
=== added file 'tests/unit_x11/tst_iconprovider/tst_iconprovider.pro'
--- tests/unit_x11/tst_iconprovider/tst_iconprovider.pro	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_iconprovider/tst_iconprovider.pro	2015-02-19 08:47:10 +0000
@@ -0,0 +1,5 @@
+include(../test-include.pri)
+DEFINES += SRCDIR=\\\"$$PWD/\\\"
+
+SOURCES += \
+    tst_iconprovider.cpp
 
=== modified file 'tests/unit_x11/unit_x11.pro'
--- tests/unit_x11/unit_x11.pro	2015-02-10 16:47:59 +0000
+++ tests/unit_x11/unit_x11.pro	2015-02-19 08:47:10 +0000
@@ -4,6 +4,7 @@
     tst_ubuntu_shape \
     tst_page \
     tst_test \
+    tst_iconprovider \
     tst_inversemousearea \
     tst_recreateview \
     tst_statesaver \