Merge lp:~laney/ubuntu-system-settings/show-more-icons into lp:ubuntu-system-settings

[Iain Lane]

[about] Look harder for the desktop file so we can display more icons in the list of apps

Try harder to get at the desktop file in order to pull its icon.

Still doesn't get 100% coverage, need to look at why g_desktop_app_info_new_from_filename() doesn't parse some files.

[Sebastien Bacher (seb128)]

Looks fine but I didn't review in details, one review comment though

[Iain Lane (laney)]

On Fri, Jun 27, 2014 at 05:22:20PM -0000, Sebastien Bacher wrote:
> Looks fine but I didn't review in details, one review comment though
>
> Diff comments:
> […]
> > + if (!appinfo) {
> > + g_warning ("Couldn't parse desktop file %s",
> > + desktopFileName);
> > + return;
>
> "desktopFileName" is leaked in that case

Oh yes, well done. I fixed that now.

Note that the code is mostly moved from one giant function which we had
before to slightly smaller ones. Not the easiest diff to review,
granted.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:721
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/905/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/1311
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/1146
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-amd64-ci/97
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-armhf-ci/97
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-armhf-ci/97/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-i386-ci/97
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/1626
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2214
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2214/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/8965
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/947
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1289
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1289/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/905/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:722
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/906/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/1325/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/1155
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-amd64-ci/98
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-armhf-ci/98
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-armhf-ci/98/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-i386-ci/98
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/1638/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2232
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2232/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/8980
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/954
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1298
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1298/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/906/rebuild

review: Needs Fixing (continuous-integration)

[lp:~laney/ubuntu-system-settings/show-more-icons]

722. By Iain Lane on 2014-07-01

Reindent, check theme icon

723. By Iain Lane on 2014-07-01

Merge trunk, goto out instead

724. By Iain Lane on 2014-07-01

Read the desktop file as a keyfile instead of with GDesktopAppInfo

Some click packages ship spec-violating desktop files which GDesktopAppInfo
won't load.

725. By Iain Lane on 2014-07-01

comments

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:725
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/912/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/1405/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/1215
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-amd64-ci/104
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-armhf-ci/104
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-armhf-ci/104/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-i386-ci/104
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/1704/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2355
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2355/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9093
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/1001
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1361
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1361/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/912/rebuild

review: Needs Fixing (continuous-integration)

[Sebastien Bacher (seb128)]

looks good, thanks

review: Approve

=== modified file 'plugins/about/click.cpp'
--- plugins/about/click.cpp	2013-10-30 01:22:51 +0000
+++ plugins/about/click.cpp	2014-07-01 12:20:23 +0000
@@ -25,7 +25,6 @@
 #include <libintl.h>
 
 #include <QDebug>
-#include <QDir>
 #include <QIcon>
 #include <QJsonArray>
 #include <QJsonDocument>
@@ -38,6 +37,135 @@
     m_clickPackages = buildClickList();
 }
 
+/* Look through `hooks' for a desktop file in `directory'
+ * and set the display name and icon from this.
+ *
+ * Will set with information from the first desktop file found and parsed.
+ */
+void ClickModel::populateFromDesktopFile (Click *newClick,
+                                          QVariantMap hooks,
+                                          QDir directory)
+{
+    QVariantMap appHooks;
+    GKeyFile *appinfo = g_key_file_new();
+    gchar *desktopFileName = NULL;
+
+    QVariantMap::ConstIterator begin(hooks.constBegin());
+    QVariantMap::ConstIterator end(hooks.constEnd());
+
+    // Look through the hooks for a 'desktop' key which points to a desktop
+    // file referring to this app.
+    while (begin != end) {
+        appHooks = (*begin++).toMap();
+        if (!appHooks.isEmpty() &&
+            appHooks.contains("desktop") &&
+            directory.exists()) {
+            QFile desktopFile(directory.absoluteFilePath(
+                        appHooks.value("desktop", "undefined").toString()));
+
+           desktopFileName =
+               g_strdup(desktopFile.fileName().toLocal8Bit().constData());
+
+            g_debug ("Desktop file: %s", desktopFileName);
+
+            if (!desktopFile.exists())
+                goto out;
+
+            gboolean loaded = g_key_file_load_from_file(appinfo,
+                                                        desktopFileName,
+                                                        G_KEY_FILE_NONE,
+                                                        NULL);
+
+            if (!loaded) {
+                g_warning ("Couldn't parse desktop file %s", desktopFileName);
+                goto out;
+            }
+
+            gchar * name = g_key_file_get_string (appinfo,
+                                                  G_KEY_FILE_DESKTOP_GROUP,
+                                                  G_KEY_FILE_DESKTOP_KEY_NAME,
+                                                  NULL);
+
+            if (name) {
+                g_debug ("Name is %s", name);
+                newClick->displayName = name;
+                g_free (name);
+                name = NULL;
+            }
+
+            // Overwrite the icon with the .desktop file's one if we have it.
+            // This is the one that the app scope displays so use that if we
+            // can.
+            gchar * icon = g_key_file_get_string (appinfo,
+                                                  G_KEY_FILE_DESKTOP_GROUP,
+                                                  G_KEY_FILE_DESKTOP_KEY_ICON,
+                                                  NULL);
+
+            if (icon) {
+                g_debug ("Icon is %s", icon);
+                QFile iconFile(icon);
+                if (iconFile.exists()) {
+                    newClick->icon = icon;
+                } else {
+                    QString qIcon(QString::fromLocal8Bit(icon));
+                    iconFile.setFileName(directory.absoluteFilePath(
+                                QDir::cleanPath(qIcon)));
+                    if (iconFile.exists())
+                        newClick->icon = iconFile.fileName();
+                    else if (QIcon::hasThemeIcon(qIcon)) // try the icon theme
+                        newClick->icon = qIcon;
+                }
+            }
+        }
+out:
+        g_free (desktopFileName);
+        g_key_file_free (appinfo);
+        return;
+    }
+}
+
+ClickModel::Click ClickModel::buildClick(QVariantMap manifest)
+{
+    Click newClick;
+    QDir directory;
+
+    newClick.name = manifest.value("title",
+                              gettext("Unknown title")).toString();
+
+    // This key is the base directory where the click package is installed to.
+    // We'll look for files relative to this.
+    if (manifest.contains("_directory")) {
+        directory = manifest.value("_directory", "/undefined").toString();
+        // Set the icon from the click package. Might be a path or a reference to a themed icon.
+        QString iconFile(manifest.value("icon", "undefined").toString());
+
+        if (directory.exists()) {
+            QFile icon(directory.absoluteFilePath(iconFile.simplified()));
+            if (!icon.exists() && QIcon::hasThemeIcon(iconFile)) // try the icon theme
+                newClick.icon = iconFile;
+            else
+                newClick.icon = icon.fileName();
+        }
+
+    }
+
+    // "hooks" → title → "desktop" / "icon"
+    QVariant hooks(manifest.value("hooks"));
+
+    if (hooks.isValid()) {
+        QVariantMap allHooks(hooks.toMap());
+        // The desktop file contains an icon and the display name
+        populateFromDesktopFile(&newClick, allHooks, directory);
+   }
+
+    newClick.installSize = manifest.value("installed-size",
+        "0").toString().toUInt()*1024;
+
+    m_totalClickSize += newClick.installSize;
+
+    return newClick;
+}
+
 QList<ClickModel::Click> ClickModel::buildClickList()
 {
     QFile clickBinary("/usr/bin/click");
@@ -80,86 +208,8 @@
 
     while (begin != end) {
         QVariantMap val = (*begin++).toObject().toVariantMap();
-        Click newClick;
-        QDir directory;
-
-        newClick.name = val.value("title",
-                                  gettext("Unknown title")).toString();
-
-        if (val.contains("_directory")) {
-            directory = val.value("_directory", "/undefined").toString();
-            // Set the icon from the click package
-            QString iconFile(val.value("icon", "undefined").toString());
-            if (directory.exists()) {
-                QString icon(directory.filePath(iconFile.simplified()));
-                newClick.icon = icon;
-            }
-        }
-
-        // "hooks" → title → "desktop" / "icon"
-        QVariant hooks(val.value("hooks"));
-
-        if (hooks.isValid()) {
-            QVariantMap allHooks(hooks.toMap());
-
-            if (allHooks.contains(newClick.name)) {
-                QVariantMap appHooks(allHooks.value(newClick.name).toMap());
-                if (!appHooks.isEmpty() &&
-                    appHooks.contains("desktop") &&
-                    directory.exists()) {
-                    QFile desktopFile(
-                                directory.absoluteFilePath(
-                                    appHooks.value("desktop",
-                                                   "undefined").toString()));
-                    const char * desktopFileName =
-                        desktopFile.fileName().toLocal8Bit().constData();
-                    g_debug ("Desktop file: %s", desktopFileName);
-                    if (desktopFile.exists()) {
-                        GDesktopAppInfo *appinfo =
-                                g_desktop_app_info_new_from_filename (
-                                    desktopFileName);
-
-                        if (!appinfo) {
-                            g_warning ("Couldn't parse desktop file %s",
-                                       desktopFileName);
-                        } else {
-                            const char * name = g_app_info_get_name (
-                                        (GAppInfo *) appinfo);
-
-                            if (name) {
-                                g_debug ("Name is %s", name);
-                                newClick.displayName = name;
-                            }
-
-                            // Overwrite the icon with the .desktop file's one
-                            // if we have it. This one is more reliable.
-                            const char * icon = g_desktop_app_info_get_string (
-                                        appinfo, "Icon");
-                            if (icon) {
-                                g_debug ("Icon is %s", icon);
-                                QFile iconFile(icon);
-                                if (iconFile.exists()) {
-                                    newClick.icon = icon;
-                                } else {
-                                    iconFile.setFileName(
-                                                directory.absoluteFilePath(
-                                                    QString::fromLocal8Bit(icon)));
-                                    if (iconFile.exists())
-                                        newClick.icon = iconFile.fileName();
-                                }
-                            }
-                        }
-                    }
-                }
-            }
-        }
-
-        newClick.installSize = val.value("installed-size",
-                                         "0").toString().toUInt()*1024;
-
-        m_totalClickSize += newClick.installSize;
-
-        clickPackages.append(newClick);
+
+        clickPackages.append(buildClick(val));
     }
 
     return clickPackages;

=== modified file 'plugins/about/click.h'
--- plugins/about/click.h	2013-10-29 22:43:40 +0000
+++ plugins/about/click.h	2014-07-01 12:20:23 +0000
@@ -21,9 +21,10 @@
 #define CLICK_H
 
 #include <QAbstractTableModel>
-#include <QSortFilterProxyModel>
+#include <QDir>
 #include <QObject>
 #include <QProcess>
+#include <QSortFilterProxyModel>
 
 class ClickModel : public QAbstractTableModel
 {
@@ -56,7 +57,12 @@
     quint64 getClickSize() const;
 
 private:
+    void populateFromDesktopFile(Click *newClick,
+                                 QVariantMap hooks,
+                                 QDir directory);
+    Click buildClick(QVariantMap manifest);
     QList<Click> buildClickList();
+
     QList<Click> m_clickPackages;
     int m_totalClickSize;