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

Proposed by Antti Kaijanmäki
Status: Rejected
Rejected by: dobey
Proposed branch: lp:~unity-api-team/unity-scope-click/blacklist-apps
Merge into: lp:unity-scope-click
Diff against target: 377 lines (+143/-32)
10 files modified
data/com.canonical.unity.clickscope.gschema.xml (+6/-1)
libclickscope/click/application.h (+22/-0)
libclickscope/click/configuration.cpp (+15/-1)
libclickscope/click/configuration.h (+2/-0)
libclickscope/click/interface.cpp (+8/-2)
libclickscope/click/interface.h (+2/-1)
libclickscope/tests/test_configuration.cpp (+54/-0)
libclickscope/tests/test_interface.cpp (+32/-2)
scope/clickapps/apps-query.cpp (+2/-24)
scope/clickapps/apps-query.h (+0/-1)
To merge this branch: bzr merge lp:~unity-api-team/unity-scope-click/blacklist-apps
Reviewer Review Type Date Requested Status
dobey (community) Needs Fixing
PS Jenkins bot continuous-integration Needs Fixing
Review via email: mp+286518@code.launchpad.net

Commit message

* 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()

Description of the change

.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
dobey (dobey) wrote :

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

blacklist applications

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/com.canonical.unity.clickscope.gschema.xml'
2--- data/com.canonical.unity.clickscope.gschema.xml 2014-07-04 13:10:05 +0000
3+++ data/com.canonical.unity.clickscope.gschema.xml 2016-02-18 14:56:35 +0000
4@@ -4,7 +4,12 @@
5 <key type="as" name="core-apps">
6 <default>[]</default>
7 <summary>Applications to display in the top category of the Apps scope</summary>
8- <description>List of application IDs that will be displayed in the upper area of the Applications scope for quick access.</description>
9+ <description>List of application names that will be displayed in the upper area of the Applications scope for quick access.</description>
10+ </key>
11+ <key type="as" name="apps-blacklist">
12+ <default>[]</default>
13+ <summary>Applications to be blacklisted from the Apps scope</summary>
14+ <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>
15 </key>
16 </schema>
17 </schemalist>
18
19=== modified file 'libclickscope/click/application.h'
20--- libclickscope/click/application.h 2014-08-08 08:20:38 +0000
21+++ libclickscope/click/application.h 2016-02-18 14:56:35 +0000
22@@ -62,6 +62,28 @@
23 std::string default_department;
24 std::string real_department;
25 time_t installed_time;
26+
27+ //
28+ // Return an application name used to match applications against core-apps dconf key;
29+ // For click apps, it just returns application name (e.g. com.canonical.calculator).
30+ // For non-click apps, it return the desktop file name (without extension), taken from app uri.
31+ std::string get_app_name() const
32+ {
33+ static const std::string app_prefix("application:///");
34+ if (!name.empty())
35+ {
36+ return name;
37+ }
38+ if (url.size() > app_prefix.size())
39+ {
40+ auto i = url.rfind('.');
41+ if (i != std::string::npos)
42+ {
43+ return url.substr(app_prefix.size(), i - app_prefix.size());
44+ }
45+ }
46+ throw std::runtime_error("Cannot determine application identifier for" + url);
47+ }
48 };
49
50 std::ostream& operator<<(std::ostream& out, const Application& app);
51
52=== modified file 'libclickscope/click/configuration.cpp'
53--- libclickscope/click/configuration.cpp 2015-11-24 19:14:11 +0000
54+++ libclickscope/click/configuration.cpp 2016-02-18 14:56:35 +0000
55@@ -27,6 +27,7 @@
56 * files in the program, then also delete it here.
57 */
58
59+#include <algorithm>
60 #include <string>
61 #include <vector>
62
63@@ -232,7 +233,20 @@
64 if (apps.empty()) {
65 apps = get_default_core_apps();
66 }
67- return apps;
68+
69+ auto blacklist = get_apps_blacklist();
70+ auto end = std::remove_if(apps.begin(),
71+ apps.end(),
72+ [blacklist](std::string i) {
73+ return std::count(blacklist.begin(), blacklist.end(), i) != 0;
74+ });
75+ return std::vector<std::string>(apps.begin(), end);
76+}
77+
78+ const std::vector<std::string> Configuration::get_apps_blacklist() const
79+{
80+ auto blacklist = get_dconf_strings(Configuration::COREAPPS_SCHEMA, Configuration::BLACKLIST_KEY);
81+ return blacklist;
82 }
83
84 } // namespace click
85
86=== modified file 'libclickscope/click/configuration.h'
87--- libclickscope/click/configuration.h 2015-11-24 19:14:11 +0000
88+++ libclickscope/click/configuration.h 2016-02-18 14:56:35 +0000
89@@ -66,8 +66,10 @@
90
91 constexpr static const char* COREAPPS_SCHEMA {"com.canonical.Unity.ClickScope"};
92 constexpr static const char* COREAPPS_KEY {"coreApps"};
93+ constexpr static const char* BLACKLIST_KEY {"appsBlacklist"};
94
95 virtual const std::vector<std::string> get_core_apps() const;
96+ virtual const std::vector<std::string> get_apps_blacklist() const;
97 virtual ~Configuration() {}
98 protected:
99 virtual std::vector<std::string> list_folder(const std::string &folder, const std::string &pattern);
100
101=== modified file 'libclickscope/click/interface.cpp'
102--- libclickscope/click/interface.cpp 2016-01-04 20:56:17 +0000
103+++ libclickscope/click/interface.cpp 2016-02-18 14:56:35 +0000
104@@ -33,6 +33,7 @@
105 #include <QStandardPaths>
106 #include <QTimer>
107
108+#include <algorithm>
109 #include <cstdio>
110 #include <list>
111 #include <sys/stat.h>
112@@ -240,7 +241,8 @@
113 */
114 std::vector<click::Application> Interface::find_installed_apps(const std::string& search_query,
115 const std::string& current_department,
116- const std::shared_ptr<click::DepartmentsDb>& depts_db)
117+ const std::shared_ptr<click::DepartmentsDb>& depts_db,
118+ const std::vector<std::string>& blacklist)
119 {
120 //
121 // only apply department filtering if not in root of all departments.
122@@ -265,7 +267,7 @@
123
124 bool include_desktop_results = show_desktop_apps();
125
126- auto enumerator = [&result, this, search_query, current_department, packages_in_department, apply_department_filter, include_desktop_results, depts_db]
127+ auto enumerator = [&result, this, search_query, current_department, packages_in_department, apply_department_filter, include_desktop_results, depts_db, blacklist]
128 (const unity::util::IniParser& keyFile, const std::string& filename)
129 {
130 if (keyFile.has_group(DESKTOP_FILE_GROUP) == false) {
131@@ -340,6 +342,10 @@
132 }
133 }
134
135+ if (std::count(blacklist.begin(), blacklist.end(), app.get_app_name()) != 0) {
136+ return;
137+ }
138+
139 if (search_query.empty()) {
140 result.push_back(app);
141 } else {
142
143=== modified file 'libclickscope/click/interface.h'
144--- libclickscope/click/interface.h 2014-07-18 11:18:27 +0000
145+++ libclickscope/click/interface.h 2016-02-18 14:56:35 +0000
146@@ -94,7 +94,8 @@
147 static std::vector<Application> sort_apps(const std::vector<Application>& apps);
148 virtual std::vector<Application> find_installed_apps(const std::string& search_query,
149 const std::string& current_department = "",
150- const std::shared_ptr<click::DepartmentsDb>& depts_db = nullptr);
151+ const std::shared_ptr<click::DepartmentsDb>& depts_db = nullptr,
152+ const std::vector<std::string>& blacklist = {});
153
154 static bool is_non_click_app(const QString& filename);
155
156
157=== modified file 'libclickscope/tests/test_configuration.cpp'
158--- libclickscope/tests/test_configuration.cpp 2015-11-24 19:14:11 +0000
159+++ libclickscope/tests/test_configuration.cpp 2016-02-18 14:56:35 +0000
160@@ -27,6 +27,7 @@
161 * files in the program, then also delete it here.
162 */
163
164+#include <algorithm>
165 #include <QStringList>
166
167 #include <gmock/gmock.h>
168@@ -47,6 +48,7 @@
169 const std::string& folder, const std::string& pattern));
170 MOCK_CONST_METHOD2(get_dconf_strings, const std::vector<std::string>(const std::string& schema, const std::string& key));
171 using Configuration::get_default_core_apps;
172+ using Configuration::get_apps_blacklist;
173 };
174
175 }
176@@ -56,6 +58,9 @@
177 using namespace ::testing;
178 FakeConfiguration c;
179 EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
180+ Configuration::BLACKLIST_KEY))
181+ .WillOnce(Return(std::vector<std::string>{}));
182+ EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
183 Configuration::COREAPPS_KEY))
184 .WillOnce(Return(std::vector<std::string>{"package1", "package2"}));
185 auto found_apps = c.get_core_apps();
186@@ -68,6 +73,9 @@
187 using namespace ::testing;
188 FakeConfiguration c;
189 EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
190+ Configuration::BLACKLIST_KEY))
191+ .WillOnce(Return(std::vector<std::string>{}));
192+ EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
193 Configuration::COREAPPS_KEY))
194 .WillOnce(Return(std::vector<std::string>{}));
195 auto found_apps = c.get_core_apps();
196@@ -75,6 +83,52 @@
197 ASSERT_EQ(found_apps, expected_apps);
198 }
199
200+TEST(Configuration, getAppsBlacklist)
201+{
202+ using namespace ::testing;
203+ FakeConfiguration c;
204+ EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
205+ Configuration::BLACKLIST_KEY))
206+ .WillOnce(Return(std::vector<std::string>{"package1", "package2"}));
207+ auto blacklist = c.get_apps_blacklist();
208+ auto expected = std::vector<std::string>{"package1", "package2"};
209+ ASSERT_EQ(blacklist, expected);
210+}
211+
212+TEST(Configuration, getCoreAppsFoundBlacklisted)
213+{
214+ using namespace ::testing;
215+ FakeConfiguration c;
216+ EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
217+ Configuration::BLACKLIST_KEY))
218+ .WillOnce(Return(std::vector<std::string>{"package1"}));
219+ EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
220+ Configuration::COREAPPS_KEY))
221+ .WillOnce(Return(std::vector<std::string>{"package1", "package2"}));
222+ auto found_apps = c.get_core_apps();
223+ auto expected_apps = std::vector<std::string>{"package2"};
224+ ASSERT_EQ(found_apps, expected_apps);
225+}
226+
227+TEST(Configuration, getCoreAppsEmptyBlacklisted)
228+{
229+ using namespace ::testing;
230+ FakeConfiguration c;
231+ EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
232+ Configuration::BLACKLIST_KEY))
233+ .WillOnce(Return(std::vector<std::string>{"dialer-app", "messaging-app"}));
234+ EXPECT_CALL(c, get_dconf_strings(Configuration::COREAPPS_SCHEMA,
235+ Configuration::COREAPPS_KEY))
236+ .WillOnce(Return(std::vector<std::string>{}));
237+ auto found_apps = c.get_core_apps();
238+
239+ auto tmp = c.get_default_core_apps();
240+ auto end = std::remove_if(tmp.begin(), tmp.end(), [&c](std::string i){ return i == "dialer-app" || i == "messaging-app"; });
241+ std::vector<std::string> expected_apps(tmp.begin(), end);
242+
243+ ASSERT_EQ(found_apps, expected_apps);
244+}
245+
246 TEST(Configuration, getAvailableFrameworksUsesRightFolder)
247 {
248 using namespace ::testing;
249
250=== modified file 'libclickscope/tests/test_interface.cpp'
251--- libclickscope/tests/test_interface.cpp 2016-01-04 20:56:17 +0000
252+++ libclickscope/tests/test_interface.cpp 2016-02-18 14:56:35 +0000
253@@ -35,6 +35,7 @@
254 #include <QDir>
255 #include <QTimer>
256
257+#include <algorithm>
258 #include <cstdlib>
259
260 #include <gmock/gmock.h>
261@@ -379,7 +380,7 @@
262 Interface::add_theme_scheme("/usr/share/unity8/graphics/applicationIcons/contacts-app@18.png"));
263 }
264
265-std::vector<click::Application> find_installed_apps(const std::string& query, bool include_desktop_results)
266+std::vector<click::Application> find_installed_apps(const std::string& query, bool include_desktop_results, const std::vector<std::string>& blacklist={})
267 {
268 using namespace ::testing;
269 QSharedPointer<click::KeyFileLocator> keyFileLocator(
270@@ -392,7 +393,7 @@
271 .Times(1)
272 .WillOnce(Return(include_desktop_results));
273
274- return iface.find_installed_apps(query);
275+ return iface.find_installed_apps(query, "", nullptr, blacklist);
276 }
277
278 TEST(ClickInterface, testFindInstalledAppsOnPhone)
279@@ -410,6 +411,35 @@
280 EXPECT_EQ(result.end(), std::find(result.begin(), result.end(), desktop_application));
281 }
282
283+TEST(ClickInterface, testFindInstalledAppsOnPhoneBlacklist)
284+{
285+ auto result = find_installed_apps(emptyQuery, false, {"com.ubuntu.terminal", "messaging-app"});
286+
287+ EXPECT_TRUE(result.size() > 0);
288+
289+ std::vector<Application> tmp;
290+ std::copy_if(non_desktop_applications.begin(),
291+ non_desktop_applications.end(),
292+ std::back_inserter(tmp),
293+ [](const Application &app) {
294+ if (app.get_app_name() == "com.ubuntu.terminal") {
295+ return false;
296+ }
297+ if (app.get_app_name() == "messaging-app") {
298+ return false;
299+ }
300+ return true;
301+ });
302+
303+ for (const auto& app : tmp)
304+ {
305+ qDebug() << "comparing" << QString::fromStdString(app.title);
306+ EXPECT_NE(result.end(), std::find(result.begin(), result.end(), app));
307+ }
308+
309+ EXPECT_EQ(result.end(), std::find(result.begin(), result.end(), desktop_application));
310+}
311+
312 TEST(ClickInterface, testFindInstalledAppsOnDesktop)
313 {
314 auto result = find_installed_apps(emptyQuery, true);
315
316=== modified file 'scope/clickapps/apps-query.cpp'
317--- scope/clickapps/apps-query.cpp 2016-01-06 18:08:33 +0000
318+++ scope/clickapps/apps-query.cpp 2016-02-18 14:56:35 +0000
319@@ -130,28 +130,6 @@
320 replyProxy->push(res);
321 }
322
323-//
324-// Return an application identifier used to match applications against core-apps dconf key;
325-// For click apps, it just returns application name (e.g. com.canonical.calculator).
326-// For non-click apps, it return the desktop file name (without extension), taken from app uri.
327-std::string click::apps::ResultPusher::get_app_identifier(const click::Application& app)
328-{
329- static const std::string app_prefix("application:///");
330- if (!app.name.empty())
331- {
332- return app.name;
333- }
334- if (app.url.size() > app_prefix.size())
335- {
336- auto i = app.url.rfind('.');
337- if (i != std::string::npos)
338- {
339- return app.url.substr(app_prefix.size(), i - app_prefix.size());
340- }
341- }
342- throw std::runtime_error("Cannot determine application identifier for" + app.url);
343-}
344-
345 void click::apps::ResultPusher::push_local_results(
346 const std::vector<click::Application> &apps,
347 const std::string &categoryTemplate,
348@@ -164,7 +142,7 @@
349 {
350 try
351 {
352- if (top_apps_lookup.size() == 0 || top_apps_lookup.find(get_app_identifier(a)) == top_apps_lookup.end())
353+ if (top_apps_lookup.size() == 0 || top_apps_lookup.find(a.get_app_name()) == top_apps_lookup.end())
354 {
355 push_result(cat, a);
356 }
357@@ -190,7 +168,7 @@
358 {
359 try
360 {
361- const auto id = get_app_identifier(a);
362+ const auto id = a.get_app_name();
363 if (top_apps_lookup.find(id) != top_apps_lookup.end())
364 {
365 top_apps_to_push[id] = a;
366
367=== modified file 'scope/clickapps/apps-query.h'
368--- scope/clickapps/apps-query.h 2014-08-19 18:21:14 +0000
369+++ scope/clickapps/apps-query.h 2016-02-18 14:56:35 +0000
370@@ -102,7 +102,6 @@
371 const std::string& categoryTemplate);
372 protected:
373 virtual void push_result(scopes::Category::SCPtr& cat, const click::Application& a);
374- static std::string get_app_identifier(const click::Application& app);
375 };
376 } // namespace apps
377 } // namespace query

Subscribers

People subscribed via source and target branches

to all changes: