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

[Albert Astals Cid]

Improve icon file searching

Leave searching in hicolor to the end
Do not search on icon themes we've already searched

hicolor is the last theme, don't search it before others, for example without this patch the "search" icon is searched at suru, Humanity, Adwaita, hicolor and finally ubuntu-mobile where it is found. If hicolor had a search icon we would be incorrectly returning it.

Also themes can have parents that are recursive, i.e. suru has Humanity and ubuntu-mobile as parents and ubuntu-mobile also has Humanity as parent, so when we get to ubuntu-mobile it doesn't make sense searching Humanity again.

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

1900. By Albert Astals Cid on 2016-03-16

Add note explaining why hicolor is searched last

[Tim Peeters (tpeeters)]

Can we add a test to tst_iconprovider.cpp that checks the new behavior?

review: Needs Fixing

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

1901. By Albert Astals Cid on 2016-03-16

Tests that hicolor is last in search

[Albert Astals Cid (aacid)]

> Can we add a test to tst_iconprovider.cpp that checks the new behavior?

Added.

[Tim Peeters (tpeeters)]

Looks good, thanks! Happroving.

review: Approve

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1901
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/534/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1952/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/534/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1901
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/314/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1953/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/314/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1901
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/545/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1954/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/545/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1901
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/288/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1957/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/288/rebuild

review: Approve (continuous-integration)

=== modified file 'src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp'
--- src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp	2015-02-27 07:55:51 +0000
+++ src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp	2016-03-16 15:03:14 +0000
@@ -48,8 +48,14 @@
     // 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)
+    QPixmap findBestIcon(const QStringList &names, const QSize &size, QSet<QString> *alreadySearchedThemes)
     {
+        if (alreadySearchedThemes) {
+            if (alreadySearchedThemes->contains(name))
+                return QPixmap();
+            alreadySearchedThemes->insert(name);
+        }
+
         Q_FOREACH(const QString &name, names) {
             QPixmap pixmap = lookupIcon(name, size);
             if (!pixmap.isNull())
@@ -57,7 +63,7 @@
         }
 
         Q_FOREACH(IconThemePointer theme, parents) {
-            QPixmap pixmap = theme->findBestIcon(names, size);
+            QPixmap pixmap = theme->findBestIcon(names, size, alreadySearchedThemes);
             if (!pixmap.isNull())
                 return pixmap;
         }
@@ -100,8 +106,11 @@
                     directories.append(dir);
                 }
 
-                Q_FOREACH(const QString &name, settings.value("Icon Theme/Inherits").toStringList())
-                    parents.append(IconTheme::get(name));
+                Q_FOREACH(const QString &name, settings.value("Icon Theme/Inherits").toStringList()) {
+                    if (name != "hicolor") {
+                        parents.append(IconTheme::get(name));
+                    }
+                }
 
                 // there can only be one index.theme
                 break;
@@ -256,7 +265,17 @@
 
 QPixmap UnityThemeIconProvider::requestPixmap(const QString &id, QSize *size, const QSize &requestedSize)
 {
-    QPixmap pixmap = theme->findBestIcon(id.split(",", QString::SkipEmptyParts), 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(",", QString::SkipEmptyParts);
+    QPixmap pixmap = theme->findBestIcon(names, requestedSize, &alreadySearchedThemes);
+
+    if (pixmap.isNull()) {
+        IconTheme::IconThemePointer theme = IconTheme::get("hicolor");
+        return theme->findBestIcon(names, requestedSize, nullptr);
+    }
+
     *size = pixmap.size();
     return pixmap;
 }

=== added directory 'tests/unit_x11/tst_iconprovider/icons/hicolor'
=== added directory 'tests/unit_x11/tst_iconprovider/icons/hicolor/apps'
=== added directory 'tests/unit_x11/tst_iconprovider/icons/hicolor/apps/512'
=== added file 'tests/unit_x11/tst_iconprovider/icons/hicolor/apps/512/myapp.png'
Binary files tests/unit_x11/tst_iconprovider/icons/hicolor/apps/512/myapp.png	1970-01-01 00:00:00 +0000 and tests/unit_x11/tst_iconprovider/icons/hicolor/apps/512/myapp.png	2016-03-16 15:03:14 +0000 differ
=== added file 'tests/unit_x11/tst_iconprovider/icons/hicolor/apps/512/myapp2.png'
Binary files tests/unit_x11/tst_iconprovider/icons/hicolor/apps/512/myapp2.png	1970-01-01 00:00:00 +0000 and tests/unit_x11/tst_iconprovider/icons/hicolor/apps/512/myapp2.png	2016-03-16 15:03:14 +0000 differ
=== added file 'tests/unit_x11/tst_iconprovider/icons/hicolor/index.theme'
--- tests/unit_x11/tst_iconprovider/icons/hicolor/index.theme	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_iconprovider/icons/hicolor/index.theme	2016-03-16 15:03:14 +0000
@@ -0,0 +1,10 @@
+[Icon Theme]
+Name=MockTheme3
+
+Directories=apps/512
+
+[apps/512]
+Size=512
+Context=Applications
+Type=Fixed
+

=== modified file 'tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme'
--- tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme	2015-02-18 12:19:02 +0000
+++ tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme	2016-03-16 15:03:14 +0000
@@ -1,5 +1,6 @@
 [Icon Theme]
 Name=MockTheme
+Inherits=mockTheme2,mockTheme3
 
 Directories=actions/scalable,apps/512
 

=== added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme2'
=== added file 'tests/unit_x11/tst_iconprovider/icons/mockTheme2/index.theme'
--- tests/unit_x11/tst_iconprovider/icons/mockTheme2/index.theme	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_iconprovider/icons/mockTheme2/index.theme	2016-03-16 15:03:14 +0000
@@ -0,0 +1,3 @@
+[Icon Theme]
+Name=MockTheme2
+Inherits=hicolor

=== added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme3'
=== added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme3/apps'
=== added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme3/apps/512'
=== added file 'tests/unit_x11/tst_iconprovider/icons/mockTheme3/apps/512/myapp.png'
Binary files tests/unit_x11/tst_iconprovider/icons/mockTheme3/apps/512/myapp.png	1970-01-01 00:00:00 +0000 and tests/unit_x11/tst_iconprovider/icons/mockTheme3/apps/512/myapp.png	2016-03-16 15:03:14 +0000 differ
=== added file 'tests/unit_x11/tst_iconprovider/icons/mockTheme3/index.theme'
--- tests/unit_x11/tst_iconprovider/icons/mockTheme3/index.theme	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_iconprovider/icons/mockTheme3/index.theme	2016-03-16 15:03:14 +0000
@@ -0,0 +1,10 @@
+[Icon Theme]
+Name=MockTheme3
+
+Directories=apps/512
+
+[apps/512]
+Size=512
+Context=Applications
+Type=Fixed
+

=== modified file 'tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp'
--- tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp	2015-02-27 07:55:51 +0000
+++ tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp	2016-03-16 15:03:14 +0000
@@ -65,9 +65,31 @@
 
         UnityThemeIconProvider provider("mockTheme");
         QSize returnedSize;
-        const QPixmap p = provider.requestPixmap(icon, &returnedSize, requestSize);
-        QCOMPARE(p.size(), resultSize);
-        QCOMPARE(returnedSize, resultSize);
+        QPixmap p = provider.requestPixmap(icon, &returnedSize, requestSize);
+        QCOMPARE(p.size(), resultSize);
+        QCOMPARE(returnedSize, resultSize);
+
+        // Search again to make sure subsequent searches still work
+        p = provider.requestPixmap(icon, &returnedSize, requestSize);
+        QCOMPARE(p.size(), resultSize);
+        QCOMPARE(returnedSize, resultSize);
+    }
+
+    void test_hicolorLast()
+    {
+        QSize returnedSize;
+        UnityThemeIconProvider provider("mockTheme");
+
+        // 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));
+
+        // 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));
     }
 };