Merge lp:~unity-api-team/unity-scope-click/blacklist-apps into lp:unity-scope-click

[Antti Kaijanmäki]

* Add "apps-blacklist" key to com.canonical.unity.clickscope.gschema.xml
  - also change the description of "core-apps" keys
* Move click::apps::ResultPusher::get_app_identifier() to
  click::Application::get_app_name()
* Add application blacklisting support to click::Configuration
  - add get_apps_blacklist()
  - filter get_core_apps() through the blacklist
* Add blacklisting support for the click::Interface::get_installed_apps()

.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:419
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/707/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/111/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/107/console

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

review: Needs Fixing (continuous-integration)

[dobey (dobey)]

First thing I see is that the bug is not linked with commit --fixes on the commit.
Beyond that, the more I've looked at this MP, the less I like it. The code itself isn't terribly incorrect or anything, but the term "blacklist" just disturbs me, and the implementation here has some inconsistency with the existing implementation of the core-apps gsettings key. I'm also not fond of moving the one function into the struct in application.h. I also noticed that you modified the find_installed_apps() API call, but did not actually adjust the call to it within apps-query.cpp's run() method. You seem to have only implemented a call to it within Configuration::get_core_apps() by removing items from that list only. This would not completely resolve the bug, as it would only remove the items from the top set in the scope. The way the tests work here also seems a bit complicated.

I've made an MP at https://code.launchpad.net/~dobey/unity-scope-click/ignored-setting/+merge/286597 which I think is more consistent and complete. Please feel free to review it and comment, and we can discuss further tomorrow.

review: Needs Fixing

Unmerged revisions

419. By Antti Kaijanmäki on 2016-02-18

blacklist applications

=== modified file 'data/com.canonical.unity.clickscope.gschema.xml'
--- data/com.canonical.unity.clickscope.gschema.xml	2014-07-04 13:10:05 +0000
+++ data/com.canonical.unity.clickscope.gschema.xml	2016-02-18 14:56:35 +0000
@@ -4,7 +4,12 @@
     <key type="as" name="core-apps">
       <default>[]</default>
       <summary>Applications to display in the top category of the Apps scope</summary>
-      <description>List of application IDs that will be displayed in the upper area of the Applications scope for quick access.</description>
+      <description>List of application names that will be displayed in the upper area of the Applications scope for quick access.</description>
+    </key>
+    <key type="as" name="apps-blacklist">
+      <default>[]</default>
+      <summary>Applications to be blacklisted from the Apps scope</summary>
+      <description>List of application names that will not be displayed in the Apps scope. Blacklisting is applied to search results as well as core-apps.</description>
     </key>
   </schema>
 </schemalist>
 
=== modified file 'libclickscope/click/application.h'
--- libclickscope/click/application.h	2014-08-08 08:20:38 +0000
+++ libclickscope/click/application.h	2016-02-18 14:56:35 +0000
@@ -62,6 +62,28 @@
     std::string default_department;
     std::string real_department;
     time_t installed_time;
+
+    //
+    // Return an application name used to match applications against core-apps dconf key;
+    // For click apps, it just returns application name (e.g. com.canonical.calculator).
+    // For non-click apps, it return the desktop file name (without extension), taken from app uri.
+    std::string get_app_name() const
+    {
+        static const std::string app_prefix("application:///");
+        if (!name.empty())
+        {
+            return name;
+        }
+        if (url.size() > app_prefix.size())
+        {
+            auto i = url.rfind('.');
+            if (i != std::string::npos)
+            {
+                return url.substr(app_prefix.size(), i - app_prefix.size());
+            }
+        }
+        throw std::runtime_error("Cannot determine application identifier for" + url);
+    }
 };
 
 std::ostream& operator<<(std::ostream& out, const Application& app);
 
=== modified file 'libclickscope/click/configuration.cpp'
--- libclickscope/click/configuration.cpp	2015-11-24 19:14:11 +0000
+++ libclickscope/click/configuration.cpp	2016-02-18 14:56:35 +0000
@@ -27,6 +27,7 @@
  * files in the program, then also delete it here.
  */
 
+#include <algorithm>
 #include <string>
 #include <vector>
 
@@ -232,7 +233,20 @@
     if (apps.empty()) {
         apps = get_default_core_apps();
     }
-    return apps;
+
+    auto blacklist = get_apps_blacklist();    
+    auto end = std::remove_if(apps.begin(),
+                              apps.end(),
+                              [blacklist](std::string i) {
+            return std::count(blacklist.begin(), blacklist.end(), i) != 0;
+        });
+    return std::vector<std::string>(apps.begin(), end);
+}
+
+  const std::vector<std::string> Configuration::get_apps_blacklist() const
+{
+    auto blacklist = get_dconf_strings(Configuration::COREAPPS_SCHEMA, Configuration::BLACKLIST_KEY);
+    return blacklist;
 }
 
 } // namespace click
 
=== modified file 'libclickscope/click/configuration.h'
--- libclickscope/click/configuration.h	2015-11-24 19:14:11 +0000
+++ libclickscope/click/configuration.h	2016-02-18 14:56:35 +0000
@@ -66,8 +66,10 @@
 
     constexpr static const char* COREAPPS_SCHEMA {"com.canonical.Unity.ClickScope"};
     constexpr static const char* COREAPPS_KEY {"coreApps"};
+    constexpr static const char* BLACKLIST_KEY {"appsBlacklist"};
 
     virtual const std::vector<std::string> get_core_apps() const;
+    virtual const std::vector<std::string> get_apps_blacklist() const;
     virtual ~Configuration() {}
 protected:
     virtual std::vector<std::string> list_folder(const std::string &folder, const std::string &pattern);
 
=== modified file 'libclickscope/click/interface.cpp'
--- libclickscope/click/interface.cpp	2016-01-04 20:56:17 +0000
+++ libclickscope/click/interface.cpp	2016-02-18 14:56:35 +0000
@@ -33,6 +33,7 @@
 #include <QStandardPaths>
 #include <QTimer>
 
+#include <algorithm>
 #include <cstdio>
 #include <list>
 #include <sys/stat.h>
@@ -240,7 +241,8 @@
  */
 std::vector<click::Application> Interface::find_installed_apps(const std::string& search_query,
         const std::string& current_department,
-        const std::shared_ptr<click::DepartmentsDb>& depts_db)
+        const std::shared_ptr<click::DepartmentsDb>& depts_db,
+        const std::vector<std::string>& blacklist)
 {
     //
     // only apply department filtering if not in root of all departments.
@@ -265,7 +267,7 @@
 
     bool include_desktop_results = show_desktop_apps();
 
-    auto enumerator = [&result, this, search_query, current_department, packages_in_department, apply_department_filter, include_desktop_results, depts_db]
+    auto enumerator = [&result, this, search_query, current_department, packages_in_department, apply_department_filter, include_desktop_results, depts_db, blacklist]
             (const unity::util::IniParser& keyFile, const std::string& filename)
     {
         if (keyFile.has_group(DESKTOP_FILE_GROUP) == false) {
@@ -340,6 +342,10 @@
                 }
             }
 
+            if (std::count(blacklist.begin(), blacklist.end(), app.get_app_name()) != 0) {
+                return;
+            }
+
             if (search_query.empty()) {
                 result.push_back(app);
             } else {
 
=== modified file 'libclickscope/click/interface.h'
--- libclickscope/click/interface.h	2014-07-18 11:18:27 +0000
+++ libclickscope/click/interface.h	2016-02-18 14:56:35 +0000
@@ -94,7 +94,8 @@
     static std::vector<Application> sort_apps(const std::vector<Application>& apps);
     virtual std::vector<Application> find_installed_apps(const std::string& search_query,
             const std::string& current_department = "",
-            const std::shared_ptr<click::DepartmentsDb>& depts_db = nullptr);
+            const std::shared_ptr<click::DepartmentsDb>& depts_db = nullptr,
+            const std::vector<std::string>& blacklist = {});
 
     static bool is_non_click_app(const QString& filename);
 
 
=== modified file 'libclickscope/tests/test_configuration.cpp'
--- libclickscope/tests/test_configuration.cpp	2015-11-24 19:14:11 +0000
+++ libclickscope/tests/test_configuration.cpp	2016-02-18 14:56:35 +0000
@@ -27,6 +27,7 @@
  * files in the program, then also delete it here.
  */
 
+#include <algorithm>
 #include <QStringList>
 
 #include <gmock/gmock.h>
@@ -47,6 +48,7 @@
                      const std::string& folder, const std::string& pattern));
     MOCK_CONST_METHOD2(get_dconf_strings, const std::vector<std::string>(const std::string& schema, const std::string& key));
     using Configuration::get_default_core_apps;
+    using Configuration::get_apps_blacklist;
 };
 
 }
@@ -56,6 +58,9 @@
     using namespace ::testing;
     FakeConfiguration c;
     EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
+                                     Configuration::BLACKLIST_KEY))
+            .WillOnce(Return(std::vector<std::string>{}));
+    EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
                                      Configuration::COREAPPS_KEY))
             .WillOnce(Return(std::vector<std::string>{"package1", "package2"}));
     auto found_apps = c.get_core_apps();
@@ -68,6 +73,9 @@
     using namespace ::testing;
     FakeConfiguration c;
     EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
+                                     Configuration::BLACKLIST_KEY))
+            .WillOnce(Return(std::vector<std::string>{}));
+    EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
                                      Configuration::COREAPPS_KEY))
             .WillOnce(Return(std::vector<std::string>{}));
     auto found_apps = c.get_core_apps();
@@ -75,6 +83,52 @@
     ASSERT_EQ(found_apps, expected_apps);
 }
 
+TEST(Configuration, getAppsBlacklist)
+{
+    using namespace ::testing;
+    FakeConfiguration c;
+    EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
+                                     Configuration::BLACKLIST_KEY))
+            .WillOnce(Return(std::vector<std::string>{"package1", "package2"}));
+    auto blacklist = c.get_apps_blacklist();
+    auto expected = std::vector<std::string>{"package1", "package2"};
+    ASSERT_EQ(blacklist, expected);
+}
+
+TEST(Configuration, getCoreAppsFoundBlacklisted)
+{
+    using namespace ::testing;
+    FakeConfiguration c;
+    EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
+                                     Configuration::BLACKLIST_KEY))
+            .WillOnce(Return(std::vector<std::string>{"package1"}));
+    EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
+                                     Configuration::COREAPPS_KEY))
+            .WillOnce(Return(std::vector<std::string>{"package1", "package2"}));
+    auto found_apps = c.get_core_apps();
+    auto expected_apps = std::vector<std::string>{"package2"};
+    ASSERT_EQ(found_apps, expected_apps);
+}
+
+TEST(Configuration, getCoreAppsEmptyBlacklisted)
+{
+    using namespace ::testing;
+    FakeConfiguration c;
+    EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
+                                     Configuration::BLACKLIST_KEY))
+            .WillOnce(Return(std::vector<std::string>{"dialer-app", "messaging-app"}));
+    EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
+                                     Configuration::COREAPPS_KEY))
+            .WillOnce(Return(std::vector<std::string>{}));
+    auto found_apps = c.get_core_apps();
+
+    auto tmp = c.get_default_core_apps();
+    auto end = std::remove_if(tmp.begin(), tmp.end(), [&c](std::string i){ return i == "dialer-app" || i == "messaging-app"; });
+    std::vector<std::string> expected_apps(tmp.begin(), end);
+
+    ASSERT_EQ(found_apps, expected_apps);
+}
+
 TEST(Configuration, getAvailableFrameworksUsesRightFolder)
 {
     using namespace ::testing;
 
=== modified file 'libclickscope/tests/test_interface.cpp'
--- libclickscope/tests/test_interface.cpp	2016-01-04 20:56:17 +0000
+++ libclickscope/tests/test_interface.cpp	2016-02-18 14:56:35 +0000
@@ -35,6 +35,7 @@
 #include <QDir>
 #include <QTimer>
 
+#include <algorithm>
 #include <cstdlib>
 
 #include <gmock/gmock.h>
@@ -379,7 +380,7 @@
               Interface::add_theme_scheme("/usr/share/unity8/graphics/applicationIcons/[e-mail]"));
 }
 
-std::vector<click::Application> find_installed_apps(const std::string& query, bool include_desktop_results)
+std::vector<click::Application> find_installed_apps(const std::string& query, bool include_desktop_results, const std::vector<std::string>& blacklist={})
 {
     using namespace ::testing;
     QSharedPointer<click::KeyFileLocator> keyFileLocator(
@@ -392,7 +393,7 @@
             .Times(1)
             .WillOnce(Return(include_desktop_results));
 
-    return iface.find_installed_apps(query);
+    return iface.find_installed_apps(query, "", nullptr, blacklist);
 }
 
 TEST(ClickInterface, testFindInstalledAppsOnPhone)
@@ -410,6 +411,35 @@
     EXPECT_EQ(result.end(), std::find(result.begin(), result.end(), desktop_application));
 }
 
+TEST(ClickInterface, testFindInstalledAppsOnPhoneBlacklist)
+{
+    auto result = find_installed_apps(emptyQuery, false, {"com.ubuntu.terminal", "messaging-app"});
+
+    EXPECT_TRUE(result.size() > 0);
+
+    std::vector<Application> tmp;
+    std::copy_if(non_desktop_applications.begin(),
+                 non_desktop_applications.end(),
+                 std::back_inserter(tmp),
+                 [](const Application &app) {
+        if (app.get_app_name() == "com.ubuntu.terminal") {
+            return false;
+        }
+        if (app.get_app_name() == "messaging-app") {
+            return false;
+        }
+        return true;
+    });
+
+    for (const auto& app : tmp)
+    {
+        qDebug() << "comparing" << QString::fromStdString(app.title);
+        EXPECT_NE(result.end(), std::find(result.begin(), result.end(), app));
+    }
+
+    EXPECT_EQ(result.end(), std::find(result.begin(), result.end(), desktop_application));
+}
+
 TEST(ClickInterface, testFindInstalledAppsOnDesktop)
 {
     auto result = find_installed_apps(emptyQuery, true);
 
=== modified file 'scope/clickapps/apps-query.cpp'
--- scope/clickapps/apps-query.cpp	2016-01-06 18:08:33 +0000
+++ scope/clickapps/apps-query.cpp	2016-02-18 14:56:35 +0000
@@ -130,28 +130,6 @@
     replyProxy->push(res);
 }
 
-//
-// Return an application identifier used to match applications against core-apps dconf key;
-// For click apps, it just returns application name (e.g. com.canonical.calculator).
-// For non-click apps, it return the desktop file name (without extension), taken from app uri.
-std::string click::apps::ResultPusher::get_app_identifier(const click::Application& app)
-{
-    static const std::string app_prefix("application:///");
-    if (!app.name.empty())
-    {
-        return app.name;
-    }
-    if (app.url.size() > app_prefix.size())
-    {
-        auto i = app.url.rfind('.');
-        if (i != std::string::npos)
-        {
-            return app.url.substr(app_prefix.size(), i - app_prefix.size());
-        }
-    }
-    throw std::runtime_error("Cannot determine application identifier for" + app.url);
-}
-
 void click::apps::ResultPusher::push_local_results(
                                       const std::vector<click::Application> &apps,
                                       const std::string &categoryTemplate,
@@ -164,7 +142,7 @@
     {
         try
         {
-            if (top_apps_lookup.size() == 0 || top_apps_lookup.find(get_app_identifier(a)) == top_apps_lookup.end())
+            if (top_apps_lookup.size() == 0 || top_apps_lookup.find(a.get_app_name()) == top_apps_lookup.end())
             {
                 push_result(cat, a);
             }
@@ -190,7 +168,7 @@
     {
         try
         {
-            const auto id = get_app_identifier(a);
+            const auto id = a.get_app_name();
             if (top_apps_lookup.find(id) != top_apps_lookup.end())
             {
                 top_apps_to_push[id] = a;
 
=== modified file 'scope/clickapps/apps-query.h'
--- scope/clickapps/apps-query.h	2014-08-19 18:21:14 +0000
+++ scope/clickapps/apps-query.h	2016-02-18 14:56:35 +0000
@@ -102,7 +102,6 @@
             const std::string& categoryTemplate);
 protected:
     virtual void push_result(scopes::Category::SCPtr& cat, const click::Application& a);
-    static std::string get_app_identifier(const click::Application& app);
 };
 } // namespace apps
 } // namespace query