Merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/unity-theme-icon-provider into lp:ubuntu-ui-toolkit

[Antti Kaijanmäki]

Add UnityThemeIconProvider - A QQuickImageProvider that loads icons from the current icon theme, following the xdg icon spec.

Add UnityThemeIconProvider

A QQuickImageProvider that loads icons from the current icon theme, following the xdg icon spec. It can be used like this:

  image://theme/icon-name,fallback-icon-name,another-fallback-icon-name

This is supposed to replace the gicon provider. I left that one in for now because it's still used by some components.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:680
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/480/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/1989
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/337
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/337
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/337/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/1994
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/1994/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/1698

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

review: Approve (continuous-integration)

[Antti Kaijanmäki (kaijanmaki)]

Looks valid! I need this for the Launcher. Somebody, please, top approve.

review: Approve

[Zsombor Egri (zsombi)]

Unit tests testing API and component functionality is missing. The MR cannot be approved without that.
Code wise looks good.

review: Needs Fixing

[Nick Dedekind (nick-dedekind)]

Getting fuzzy icons.
Please use available sizes if not provided.

QIcon themedIcon = QIcon::fromTheme(id);
if (requestedSize.isValid()) {
    return themedIcon.pixmap(requestedSize);
} else {
    return themedIcon.pixmap(themedIcon.availableSizes().last());
}

review: Needs Fixing

[Nick Dedekind (nick-dedekind)]

Actually a more robust solution:
I've proposed these changes to qmenumodel where this provider currently resides.

    if (!icon.isNull()) {
        if (requestedSize.isValid()) {
            return themedIcon.pixmap(requestedSize);
        } else {
            QList<QSize> sizes = icon.availableSizes();
            if (sizes.count() > 0 && sizes.last().isValid()) {
                return icon.pixmap(sizes.last());
            } else {
                return icon.pixmap(QSize(32,32));
            }
        }
    }
    return QPixmap();

[Antti Kaijanmäki (kaijanmaki)]

superseding this one with a public branch MR.

[Antti Kaijanmäki (kaijanmaki)]

Seems I hit a couple of bugs:

notify-osd and notify-osd-icons packages are not providing index.theme :/
QIcon::themedIcon is not looking up all the base directories. It just takes the first one with matching theme directory and gives up if the requested icon is not found. Thus notify-osd icons are not found (after adding the index.theme).

Both needs fixing.

[Antti Kaijanmäki (kaijanmaki)]

FYI, the reason for dealing with notify-osd is to provide the functionality proposed in this MR for gicon:
https://code.launchpad.net/~macslow/ubuntu-ui-toolkit/add-search-path-to-giconprovider/+merge/164197

[Antti Kaijanmäki (kaijanmaki)]

Another QIcon::fromTheme() bug:

QIcon is not loading images from /usr/share/pixmaps correctly as required by the icon-theme-spec.

Just try to load "firefox".

[Antti Kaijanmäki (kaijanmaki)]

based on the IRC discussion this provider should also be able to load images with absolute and relative paths. The current code does not support this. simply try to load /usr/share/pixmaps/firefox.png

[Michał Sawicz (saviq)]

Actually the above is incorrect.

I believe the "name" property should only be left as convenience when used directly:

Icon {
    name: "some-known-icon-name"
}

Whenever the icon url is provided by a backend - it should be a "fully-qualified" url, i.e. full image://theme/name path, not just "name", and Icon should have a "source" property to deal with those. No one should need to guess whether it's a themed icon or a full path. IMO Icon should be renamed to "ColourizedIcon" or similar, to denote its real purpose, but Image { } should be enough to use, letting the image providers do their thing.

[Nick Dedekind (nick-dedekind)]

Now using maximum available icon size if requested size not set.

[Antti Kaijanmäki (kaijanmaki)]

> Now using maximum available icon size if requested size not set.

Thanks! How does this work with SVG images? QIcon probably SHOULD honour the MaxSize specified in index.theme, but for some reason I've lost all hope regarding QImage::fromTheme() ;)

Obviously this has to be tested so we don't get oversized images.

[Antti Kaijanmäki (kaijanmaki)]

> Actually the above is incorrect.
>
> I believe the "name" property should only be left as convenience when used
> directly:
>
> Icon {
> name: "some-known-icon-name"
> }
>
> Whenever the icon url is provided by a backend - it should be a "fully-
> qualified" url, i.e. full image://theme/name path, not just "name", and Icon
> should have a "source" property to deal with those. No one should need to
> guess whether it's a themed icon or a full path. IMO Icon should be renamed to
> "ColourizedIcon" or similar, to denote its real purpose, but Image { } should
> be enough to use, letting the image providers do their thing.

OK, so conclusions from IRC discussions were that theme image provider should only load themed icons. If the developer wants to load absolute or relative icons he should use "file://"

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:685
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/528/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/2345
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/385
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/385
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/385/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2351
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2351/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/1993

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:686
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/530/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/2353/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/387/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/387/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2359/console

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

review: Needs Fixing (continuous-integration)

[Michal Hruby (mhr3)]

135 + QIcon::setThemeSearchPaths(paths);

Unnecessary.

69 + engine->addImageProvider(QLatin1String("theme"), new UnityThemeIconProvider);

The same provider is registered in libqmenumodel, is that one going to be removed?

Would be nice if someone could fix the conflict too.

review: Needs Information

[Lars Karlitski (larsu)]

> 69 + engine->addImageProvider(QLatin1String("theme"), new
> UnityThemeIconProvider);
>
> The same provider is registered in libqmenumodel, is that one going to be
> removed?
>
> Would be nice if someone could fix the conflict too.

I asked about this in #ubuntu-unity the other day and didn't get a clear answer. I think it's easiest if we just make libqmenumodel depend on ubuntu-ui-toolkit and remove its copy of the "theme" provider. I'll propose a merge for that later today.

[Michal Hruby (mhr3)]

review: Approve

[Michał Sawicz (saviq)]

Yup, this is good as a first step towards "no guessing" around icons.

review: Approve (conceptual)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:688
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/562/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/2555
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/419
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/419
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/419/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2561
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2561/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/2157

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

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

The changes on top of my branch look good to me, thanks.

review: Approve

[lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/unity-theme-icon-provider]

689. By Antti Kaijanmäki on 2013-08-28

Test for unity-theme-icon-provider

690. By Antti Kaijanmäki on 2013-08-28

merge from upstream

691. By Antti Kaijanmäki on 2013-08-28

remove notifyosd icons from gallery.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:691
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/575/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/2731/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/432/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/432/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2737/console

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

review: Needs Fixing (continuous-integration)

[Antti Kaijanmäki (kaijanmaki)]

> Seems I hit a couple of bugs:
>
> notify-osd and notify-osd-icons packages are not providing index.theme :/

This is actually not a bug IF the directory is simply extending another theme directory which contains the index.theme. QIconLoader just can't handle themes expanding over multiple base dirs at the moment, so using this loader for notify-osd INSIDE the shell is not possible at the moment.

I removed the remaining notify-osd stuff for now. NotifyOSD has to use it's custom loader.

> QIcon::themedIcon is not looking up all the base directories. It just takes
> the first one with matching theme directory and gives up if the requested icon
> is not found. Thus notify-osd icons are not found (after adding the
> index.theme).
>
> Both needs fixing.

So, this is an upstream bug and not trivial to fix. The whole QIconLoader would need a redesign.

[Antti Kaijanmäki (kaijanmaki)]

> Another QIcon::fromTheme() bug:
>
> QIcon is not loading images from /usr/share/pixmaps correctly as required by
> the icon-theme-spec.
>
> Just try to load "firefox".

When these changes get merged to Qt dev we can distro patch them in.

https://codereview.qt-project.org/63987
https://codereview.qt-project.org/63989

[Antti Kaijanmäki (kaijanmaki)]

OK, I have a test now, BUT.

The unit tests are run with -platform minimal and this breaks the QIcon::fromTheme().
The only option I see is to create a .qrc and embed an icon theme there and test with that (as :/ is always added to the search paths. this is actually not documented..)

I'm starting to wonder if it's really worth it to write these tests..

[lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/unity-theme-icon-provider]

692. By Antti Kaijanmäki on 2013-08-28

remove test case

[Antti Kaijanmäki (kaijanmaki)]

OK, I give up.

There seems to be no way of reliably test the theme icon provider under CI right now.

As all the tests are run with -platform minimal this prevents picking up an actual system theme (which probably is not set properly under Jenkins anyway) and thus preventing loading icons from a system theme (tried to pull in humanity-icon-theme as build-dep for this test).

I also tried to embed a theme as qrc file the same way upstream Qt tests the QIcon::fromTheme() but seems that quick test lib does not load app .qrc files properly so this was a dead end also.

I would either accept this MR as it is or somebody should instruct me how to test this. I'm out of ideas.

Anyone who is interested the test case is available in commit 689. It can be run manually on a standard desktop session and it passes, but as I said I'm unable to get it working with Jenkins environment.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:692
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/578/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/2762
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/435
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/435
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/435/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2768
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2768/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/2342

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'modules/Ubuntu/Components/Icon.qml'
--- modules/Ubuntu/Components/Icon.qml	2013-08-27 09:28:49 +0000
+++ modules/Ubuntu/Components/Icon.qml	2013-08-28 20:35:44 +0000
@@ -82,7 +82,7 @@
         Component.onCompleted: ready = true
 
         anchors.fill: parent
-        source: ready && width > 0 && height > 0 && icon.name ? "image://gicon/%1".arg(icon.name) : ""
+        source: ready && width > 0 && height > 0 && icon.name ? "image://theme/%1".arg(icon.name) : ""
         sourceSize {
             width: width
             height: height

=== modified file 'modules/Ubuntu/Components/plugin/plugin.cpp'
--- modules/Ubuntu/Components/plugin/plugin.cpp	2013-08-16 10:16:48 +0000
+++ modules/Ubuntu/Components/plugin/plugin.cpp	2013-08-28 20:35:44 +0000
@@ -44,6 +44,7 @@
 #include "ucargument.h"
 #include "ucalarm.h"
 #include "ucalarmmodel.h"
+#include "unitythemeiconprovider.h"
 
 #include <sys/types.h>
 #include <unistd.h>
@@ -184,8 +185,9 @@
 
     engine->addImageProvider(QLatin1String("scaling"), new UCScalingImageProvider);
 
-    // register gicon provider
+    // register icon providers
     engine->addImageProvider(QLatin1String("gicon"), new GIconProvider);
+    engine->addImageProvider(QLatin1String("theme"), new UnityThemeIconProvider);
 
     // Necessary for Screen.orientation (from import QtQuick.Window 2.0) to work
     QGuiApplication::primaryScreen()->setOrientationUpdateMask(

=== modified file 'modules/Ubuntu/Components/plugin/plugin.pro'
--- modules/Ubuntu/Components/plugin/plugin.pro	2013-08-23 11:01:37 +0000
+++ modules/Ubuntu/Components/plugin/plugin.pro	2013-08-28 20:35:44 +0000
@@ -47,6 +47,7 @@
     alarmmanager_p_p.h \
     alarmmanager_p.h \
     ucalarmmodel.h \
+    unitythemeiconprovider.h \
     alarmrequest_p.h \
     alarmrequest_p_p.h
 
@@ -72,6 +73,7 @@
     ucalarm.cpp \
     alarmmanager_p.cpp \
     ucalarmmodel.cpp \
+    unitythemeiconprovider.cpp \
     alarmrequest_p.cpp
 
 # adapters

=== added file 'modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp'
--- modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp	1970-01-01 00:00:00 +0000
+++ modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp	2013-08-28 20:35:44 +0000
@@ -0,0 +1,51 @@
+/*
+ * Copyright 2013 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/>.
+ *
+ * Authors: Lars Uebernickel <[e-mail]>
+ */
+
+#include "unitythemeiconprovider.h"
+
+#include <QIcon>
+
+UnityThemeIconProvider::UnityThemeIconProvider():
+  QQuickImageProvider(QQuickImageProvider::Pixmap)
+{
+}
+
+QPixmap UnityThemeIconProvider::requestPixmap(const QString &id, QSize *realSize, const QSize &requestedSize)
+{
+    QIcon icon;
+
+    Q_FOREACH (QString name, id.split(",", QString::SkipEmptyParts)) {
+        icon = QIcon::fromTheme(name);
+        if (!icon.isNull()) {
+            if (requestedSize.isValid()) {
+                *realSize =requestedSize;
+                return icon.pixmap(requestedSize);
+            } else {
+                QList<QSize> sizes = icon.availableSizes();
+                if (sizes.count() > 0 && sizes.last().isValid()) {
+                    *realSize = sizes.last();
+                } else {
+                    *realSize = QSize(32,32);
+                }
+                return icon.pixmap(*realSize);
+            }
+            break;
+        }
+    }
+    return QPixmap();
+}

=== added file 'modules/Ubuntu/Components/plugin/unitythemeiconprovider.h'
--- modules/Ubuntu/Components/plugin/unitythemeiconprovider.h	1970-01-01 00:00:00 +0000
+++ modules/Ubuntu/Components/plugin/unitythemeiconprovider.h	2013-08-28 20:35:44 +0000
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2013 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/>.
+ *
+ * Authors: Lars Uebernickel <[e-mail]>
+ */
+
+#ifndef UNITY_THEME_ICON_PROVIDER_H
+#define UNITY_THEME_ICON_PROVIDER_H
+
+#include <QQuickImageProvider>
+
+class UnityThemeIconProvider: public QQuickImageProvider
+{
+public:
+    UnityThemeIconProvider();
+    QPixmap requestPixmap(const QString &id, QSize *size, const QSize &requestedSize);
+};
+
+#endif