Merge lp:~alecu/unity-scope-click/non-click-icons into lp:unity-scope-click

[Alejandro J. Cura]

Add the icon://theme/ prefix when the icon does not specify a path

[dobey (dobey)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:136
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/284/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/182/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/182/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/284/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:136
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/287/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/188
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/185
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/185/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/287/rebuild

review: Approve (continuous-integration)

[Thomas Voß (thomas-voss)]

While the code looks fine in general, I'm concerned with click::Interface becoming a kitchen sink here. As far as I understand it, is_icon_identifier and add_theme_scheme could be free functions living in a namespace click::util.

What do you think?

review: Needs Information

[dobey (dobey)]

> While the code looks fine in general, I'm concerned with click::Interface
> becoming a kitchen sink here. As far as I understand it, is_icon_identifier
> and add_theme_scheme could be free functions living in a namespace
> click::util.
>
> What do you think?

Unless there is a significant performance improvement that would result from this, I don't think we should block the branch on this. It's not public API (we're not a shared library that others can/should use), and we can refactor it later. I don't foresee any other code in the scope needing these calls though, so I don't think there would be any benefit to utility or style by moving it, either.

[Thomas Voß (thomas-voss)]

> > While the code looks fine in general, I'm concerned with click::Interface
> > becoming a kitchen sink here. As far as I understand it, is_icon_identifier
> > and add_theme_scheme could be free functions living in a namespace
> > click::util.
> >
> > What do you think?
>
> Unless there is a significant performance improvement that would result from
> this, I don't think we should block the branch on this. It's not public API
> (we're not a shared library that others can/should use), and we can refactor
> it later. I don't foresee any other code in the scope needing these calls
> though, so I don't think there would be any benefit to utility or style by
> moving it, either.

I disagree and I was not referring to performance improvements here. However, for the sake of landing this, I abstain.

review: Abstain

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[Roberto Alsina (ralsina)]

review: Approve

[dobey (dobey)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:138
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/310/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/211
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/208
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/208/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/310/rebuild

review: Approve (continuous-integration)

[Thomas Voß (thomas-voss)]

l. 88/89: The signature of EXPECT_EQ reads EXPECT_EQ(expected, actual). Please change the parameter ordering accordingly.

review: Needs Fixing

[lp:~alecu/unity-scope-click/non-click-icons]

139. By Alejandro J. Cura on 2014-02-18

Fix EXPECTED_EQ order

[Alejandro J. Cura (alecu)]

> l. 88/89: The signature of EXPECT_EQ reads EXPECT_EQ(expected, actual). Please
> change the parameter ordering accordingly.

done

=== modified file 'scope/click/interface.cpp'
--- scope/click/interface.cpp	2014-02-18 07:06:13 +0000
+++ scope/click/interface.cpp	2014-02-18 07:06:13 +0000
@@ -108,8 +108,9 @@
                 QString app_url = "application:///" + QString::fromStdString(filename);
                 app.url = app_url.toUtf8().data();
                 app.title = name.toUtf8().data();
-                app.icon_url = keyFile.get_string(DESKTOP_FILE_GROUP,
-                                                  DESKTOP_FILE_KEY_ICON);
+                app.icon_url = Interface::add_theme_scheme(
+                            keyFile.get_string(DESKTOP_FILE_GROUP,
+                                               DESKTOP_FILE_KEY_ICON));
                 if (keyFile.has_key(DESKTOP_FILE_GROUP, DESKTOP_FILE_KEY_APP_ID)) {
                     QString app_id = QString::fromStdString(keyFile.get_string(
                                                             DESKTOP_FILE_GROUP,
@@ -152,6 +153,29 @@
     return click::nonClickDesktopFiles().count(filename.toUtf8().data()) > 0;
 }
 
+/*
+ * is_icon_identifier()
+ *
+ * Checks if @filename has no / in it
+ */
+bool Interface::is_icon_identifier(const std::string &icon_id)
+{
+    return icon_id.find("/") == std::string::npos;
+}
+
+/*
+ * add_theme_scheme()
+ *
+ * Adds the theme prefix if the filename is not an icon identifier
+ */
+std::string Interface::add_theme_scheme(const std::string& icon_id)
+{
+    if (is_icon_identifier(icon_id)) {
+        return "image://theme/" + icon_id;
+    }
+    return icon_id;
+}
+
 /* find_apps_in_dir()
  *
  * Finds all the apps in the specified @dir_path, which also match
@@ -181,8 +205,9 @@
                     QString app_url = "application:///" + filename;
                     app.url = app_url.toUtf8().data();
                     app.title = name.toUtf8().data();
-                    app.icon_url = keyfile.get_string(DESKTOP_FILE_GROUP,
-                                                      DESKTOP_FILE_KEY_ICON);
+                    app.icon_url = Interface::add_theme_scheme(
+                                keyfile.get_string(DESKTOP_FILE_GROUP,
+                                                   DESKTOP_FILE_KEY_ICON));
                     if (keyfile.has_key(DESKTOP_FILE_GROUP, DESKTOP_FILE_UBUNTU_TOUCH)) {
                         app.description = keyfile.get_string(DESKTOP_FILE_GROUP,
                                                              DESKTOP_FILE_COMMENT);

=== modified file 'scope/click/interface.h'
--- scope/click/interface.h	2014-02-11 13:54:39 +0000
+++ scope/click/interface.h	2014-02-18 07:06:13 +0000
@@ -59,6 +59,8 @@
                                  const QString& search_query,
                                  std::vector<Application>& result_list);
 
+    static bool is_icon_identifier(const std::string &icon_id);
+    static std::string add_theme_scheme(const std::string &filename);
 private:
     QSharedPointer<KeyFileLocator> keyFileLocator;
 };

=== modified file 'scope/tests/test_interface.cpp'
--- scope/tests/test_interface.cpp	2014-02-18 07:06:13 +0000
+++ scope/tests/test_interface.cpp	2014-02-18 07:06:13 +0000
@@ -111,6 +111,20 @@
     EXPECT_TRUE(results.empty());
 }
 
+TEST(ClickInterface, testIsIconIdentifier)
+{
+    EXPECT_TRUE(Interface::is_icon_identifier("contacts-app"));
+    EXPECT_FALSE(Interface::is_icon_identifier(
+                    "/usr/share/unity8/graphics/applicationIcons/[e-mail]"));
+}
+
+TEST(ClickInterface, testAddThemeScheme)
+{
+    EXPECT_EQ("image://theme/contacts-app", Interface::add_theme_scheme("contacts-app"));
+    EXPECT_EQ("/usr/share/unity8/graphics/applicationIcons/[e-mail]",
+              Interface::add_theme_scheme("/usr/share/unity8/graphics/applicationIcons/[e-mail]"));
+}
+
 // TODO: Get rid of file-based testing and instead make unity::util::IniParser mockable
 // Maintaining this list here will become tedious over time.
 TEST(ClickInterface, findInstalledAppsReturnsCorrectListOfResults)
@@ -127,7 +141,7 @@
         {"com.ubuntu.developer.webapps.webapp-gmail", "Gmail", "", "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.developer.webapps.webapp-gmail/./gmail.png", "application:///com.ubuntu.developer.webapps.webapp-gmail_webapp-gmail_1.0.8.desktop", "", ""},
         {"com.ubuntu.terminal", "Terminal", "", "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.terminal/./terminal64.png", "application:///com.ubuntu.terminal_terminal_0.5.29.desktop", "", ""},
         {"com.ubuntu.calendar", "Calendar", "", "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.calendar/./calendar64.png", "application:///com.ubuntu.calendar_calendar_0.4.182.desktop", "", ""},
-        {"com.ubuntu.notes", "Notes", "", "notepad", "application:///com.ubuntu.notes_notes_1.4.242.desktop", "", ""},
+        {"com.ubuntu.notes", "Notes", "", "image://theme/notepad", "application:///com.ubuntu.notes_notes_1.4.242.desktop", "", ""},
         {"com.ubuntu.developer.webapps.webapp-amazon", "Amazon", "", "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.developer.webapps.webapp-amazon/./amazon.png", "application:///com.ubuntu.developer.webapps.webapp-amazon_webapp-amazon_1.0.6.desktop", "", ""},
         {"com.ubuntu.shorts", "Shorts", "", "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.shorts/./rssreader64.png", "application:///com.ubuntu.shorts_shorts_0.2.162.desktop", "", ""},
         {"com.ubuntu.filemanager", "File Manager", "", "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.filemanager/./filemanager64.png", "application:///com.ubuntu.filemanager_filemanager_0.1.1.97.desktop", "", ""},
@@ -135,8 +149,8 @@
         {"com.ubuntu.sudoku", "Sudoku", "", "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.sudoku/SudokuGameIcon.png", "application:///com.ubuntu.sudoku_sudoku_1.0.142.desktop", "", ""},
         {"com.ubuntu.developer.webapps.webapp-ebay", "eBay", "", "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.developer.webapps.webapp-ebay/./ebay.png", "application:///com.ubuntu.developer.webapps.webapp-ebay_webapp-ebay_1.0.8.desktop", "", ""},
         {"com.ubuntu.developer.webapps.webapp-facebook", "Facebook", "", "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.developer.webapps.webapp-facebook/./facebook.png", "application:///com.ubuntu.developer.webapps.webapp-facebook_webapp-facebook_1.0.5.desktop", "", ""},
-        {"", "Messaging", "", "messages-app", "application:///messaging-app.desktop", "", ""},
-        {"", "Contacts", "", "contacts-app", "application:///address-book-app.desktop", "", ""}
+        {"", "Messaging", "", "image://theme/messages-app", "application:///messaging-app.desktop", "", ""},
+        {"", "Contacts", "", "image://theme/contacts-app", "application:///address-book-app.desktop", "", ""}
     };
 
     click::Application mustNotBePartOfResult