unity-scope-click

Merge lp:~marcustomlinson/unity-scope-click/lp-1578283-2 into lp:unity-scope-click

  1. lp-1578283-2
  2. Merge into trunk
Proposed by Marcus Tomlinson
Status: Merged
Approved by: Paweł Stołowski
Approved revision: 447
Merged at revision: 444
Proposed branch: lp:~marcustomlinson/unity-scope-click/lp-1578283-2
Merge into: lp:unity-scope-click
Diff against target: 346 lines (+73/-34)
10 files modified
libclickscope/click/preview.cpp (+9/-2)
libclickscope/click/preview.h (+3/-1)
scope/clickapps/apps-query.cpp (+14/-5)
scope/clickapps/apps-query.h (+6/-1)
scope/clickapps/apps-scope.cpp (+8/-7)
scope/clickapps/apps-scope.h (+5/-4)
scope/clickstore/store-query.cpp (+11/-4)
scope/clickstore/store-query.h (+4/-1)
scope/clickstore/store-scope.cpp (+9/-7)
scope/clickstore/store-scope.h (+4/-2)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scope-click/lp-1578283-2
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Needs Fixing
Paweł Stołowski (community) Approve
Review via email: mp+294246@code.launchpad.net

Commit message

Don't block search() and preview() calls while the Qt main loop spins up

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

FAILED: Continuous integration, rev:446
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/743/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/147/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/143/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Paweł Stołowski (stolowski) wrote :

Great job finding out about SSO slowness! As discussed on IRC however, preview() is still affected, because click scope may get killed after displaying search results and then when preview is requested, we hit the same issue you found with search(). I think the only fix is to wait for initialization to complete to run() methods of search and preview query classes.

review: Needs Fixing
lp:~marcustomlinson/unity-scope-click/lp-1578283-2 updated
447. By Marcus Tomlinson

Wait for qt mainloop initialisation in run() methods rather than search() and preview()

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> Great job finding out about SSO slowness! As discussed on IRC however,
> preview() is still affected, because click scope may get killed after
> displaying search results and then when preview is requested, we hit the same
> issue you found with search(). I think the only fix is to wait for
> initialization to complete to run() methods of search and preview query
> classes.

What a mess the diff ended being. Hopefully this does it

Revision history for this message
Paweł Stołowski (stolowski) wrote :

Looks good! +1

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:447
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/744/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/148/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/144/console

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

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libclickscope/click/preview.cpp'
2--- libclickscope/click/preview.cpp 2016-04-01 17:16:05 +0000
3+++ libclickscope/click/preview.cpp 2016-05-10 14:58:44 +0000
4@@ -158,8 +158,12 @@
5 // Preview base class
6
7 Preview::Preview(const unity::scopes::Result& result,
8- const unity::scopes::ActionMetadata& metadata)
9- : PreviewQueryBase(result, metadata), result(result), metadata(metadata)
10+ const unity::scopes::ActionMetadata& metadata,
11+ std::shared_future<void> const& qt_ready)
12+ : PreviewQueryBase(result, metadata),
13+ result(result),
14+ metadata(metadata),
15+ qt_ready_(qt_ready)
16 {
17 }
18
19@@ -259,6 +263,9 @@
20
21 void Preview::run(const unity::scopes::PreviewReplyProxy &reply)
22 {
23+ if (qt_ready_.valid())
24+ qt_ready_.wait();
25+
26 strategy->run(reply);
27 }
28
29
30=== modified file 'libclickscope/click/preview.h'
31--- libclickscope/click/preview.h 2016-03-07 14:10:27 +0000
32+++ libclickscope/click/preview.h 2016-05-10 14:58:44 +0000
33@@ -102,6 +102,7 @@
34 std::unique_ptr<PreviewStrategy> strategy;
35 const unity::scopes::Result& result;
36 const unity::scopes::ActionMetadata& metadata;
37+ std::shared_future<void> qt_ready_;
38 PreviewStrategy* build_strategy(const unity::scopes::Result& result,
39 const unity::scopes::ActionMetadata& metadata,
40 const QSharedPointer<web::Client> &client,
41@@ -142,7 +143,8 @@
42
43 Preview(const unity::scopes::Result& result);
44 Preview(const unity::scopes::Result& result,
45- const unity::scopes::ActionMetadata& metadata);
46+ const unity::scopes::ActionMetadata& metadata,
47+ std::shared_future<void> const& qt_ready = std::future<void>());
48 virtual ~Preview();
49 void choose_strategy(const QSharedPointer<web::Client> &client,
50 const QSharedPointer<pay::Package>& ppackage,
51
52=== modified file 'scope/clickapps/apps-query.cpp'
53--- scope/clickapps/apps-query.cpp 2016-03-09 11:01:19 +0000
54+++ scope/clickapps/apps-query.cpp 2016-05-10 14:58:44 +0000
55@@ -222,21 +222,27 @@
56
57 struct click::apps::Query::Private
58 {
59- Private(std::shared_ptr<click::DepartmentsDb> depts_db, const scopes::SearchMetadata& metadata)
60+ Private(std::shared_ptr<click::DepartmentsDb> depts_db,
61+ const scopes::SearchMetadata& metadata,
62+ std::shared_future<void> const& qt_ready)
63 : depts_db(depts_db),
64- meta(metadata)
65+ meta(metadata),
66+ qt_ready_(qt_ready)
67 {
68 }
69
70 std::shared_ptr<click::DepartmentsDb> depts_db;
71 scopes::SearchMetadata meta;
72 click::Configuration configuration;
73+ std::shared_future<void> qt_ready_;
74 };
75
76-click::apps::Query::Query(unity::scopes::CannedQuery const& query, std::shared_ptr<DepartmentsDb> depts_db,
77- scopes::SearchMetadata const& metadata)
78+click::apps::Query::Query(unity::scopes::CannedQuery const& query,
79+ std::shared_ptr<DepartmentsDb> depts_db,
80+ scopes::SearchMetadata const& metadata,
81+ std::shared_future<void> const& qt_ready)
82 : unity::scopes::SearchQueryBase(query, metadata),
83- impl(new Private(depts_db, metadata))
84+ impl(new Private(depts_db, metadata, qt_ready))
85 {
86 }
87
88@@ -403,6 +409,9 @@
89
90 void click::apps::Query::run(scopes::SearchReplyProxy const& searchReply)
91 {
92+ if (impl->qt_ready_.valid())
93+ impl->qt_ready_.wait();
94+
95 const std::string categoryTemplate = CATEGORY_APPS_DISPLAY;
96 auto const current_dept = query().department_id();
97 auto const querystr = query().query_string();
98
99=== modified file 'scope/clickapps/apps-query.h'
100--- scope/clickapps/apps-query.h 2014-08-19 18:21:14 +0000
101+++ scope/clickapps/apps-query.h 2016-05-10 14:58:44 +0000
102@@ -40,6 +40,8 @@
103 #include <unordered_set>
104 #include <click/interface.h>
105
106+#include <future>
107+
108 namespace click
109 {
110
111@@ -64,7 +66,10 @@
112 constexpr static const char* VERSION{"version"};
113 };
114
115- Query(unity::scopes::CannedQuery const& query, std::shared_ptr<DepartmentsDb> depts_db, scopes::SearchMetadata const& metadata);
116+ Query(unity::scopes::CannedQuery const& query,
117+ std::shared_ptr<DepartmentsDb> depts_db,
118+ scopes::SearchMetadata const& metadata,
119+ std::shared_future<void> const& qt_ready = std::future<void>());
120 virtual ~Query();
121
122 virtual void cancelled() override;
123
124=== modified file 'scope/clickapps/apps-scope.cpp'
125--- scope/clickapps/apps-scope.cpp 2016-04-15 09:24:18 +0000
126+++ scope/clickapps/apps-scope.cpp 2016-05-10 14:58:44 +0000
127@@ -49,7 +49,8 @@
128
129 click::Scope::Scope()
130 {
131- qt_ready_f = qt_ready_p.get_future();
132+ qt_ready_for_search_f = qt_ready_for_search_p.get_future();
133+ qt_ready_for_preview_f = qt_ready_for_preview_p.get_future();
134 nam.reset(new click::network::AccessManager());
135 client.reset(new click::web::Client(nam));
136 index.reset(new click::Index(client));
137@@ -82,10 +83,12 @@
138 static const int zero = 0;
139 auto emptyCb = [this]()
140 {
141+ dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
142+ qt_ready_for_search_p.set_value();
143+
144 sso.reset(new click::CredentialsService());
145 client->setCredentialsService(sso);
146- dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
147- qt_ready_p.set_value();
148+ qt_ready_for_preview_p.set_value();
149 };
150
151 qt::core::world::build_and_run(zero, nullptr, emptyCb);
152@@ -98,16 +101,14 @@
153
154 scopes::SearchQueryBase::UPtr click::Scope::search(unity::scopes::CannedQuery const& q, scopes::SearchMetadata const& metadata)
155 {
156- qt_ready_f.wait();
157- return scopes::SearchQueryBase::UPtr(new click::apps::Query(q, depts_db, metadata));
158+ return scopes::SearchQueryBase::UPtr(new click::apps::Query(q, depts_db, metadata, qt_ready_for_search_f.share()));
159 }
160
161
162 unity::scopes::PreviewQueryBase::UPtr click::Scope::preview(const unity::scopes::Result& result,
163 const unity::scopes::ActionMetadata& metadata) {
164- qt_ready_f.wait();
165 qDebug() << "Scope::preview() called.";
166- auto preview = new click::Preview(result, metadata);
167+ auto preview = new click::Preview(result, metadata, qt_ready_for_preview_f.share());
168 preview->choose_strategy(client, pay_package, dm, depts_db);
169 return unity::scopes::PreviewQueryBase::UPtr{preview};
170 }
171
172=== modified file 'scope/clickapps/apps-scope.h'
173--- scope/clickapps/apps-scope.h 2016-04-15 09:24:18 +0000
174+++ scope/clickapps/apps-scope.h 2016-05-10 14:58:44 +0000
175@@ -30,8 +30,6 @@
176 #ifndef APPS_SCOPE_H
177 #define APPS_SCOPE_H
178
179-#include <future>
180-
181 #include <click/index.h>
182 #include <click/network_access_manager.h>
183 #include <click/pay.h>
184@@ -42,6 +40,7 @@
185 #include <unity/scopes/QueryBase.h>
186 #include <unity/scopes/ActivationQueryBase.h>
187
188+#include <future>
189
190 namespace scopes = unity::scopes;
191
192@@ -68,8 +67,10 @@
193 virtual unity::scopes::ActivationQueryBase::UPtr perform_action(unity::scopes::Result const& result, unity::scopes::ActionMetadata const& metadata, std::string const& widget_id, std::string const& action_id) override;
194
195 private:
196- std::promise<void> qt_ready_p;
197- std::future<void> qt_ready_f;
198+ std::promise<void> qt_ready_for_search_p;
199+ std::future<void> qt_ready_for_search_f;
200+ std::promise<void> qt_ready_for_preview_p;
201+ std::future<void> qt_ready_for_preview_f;
202 QSharedPointer<click::network::AccessManager> nam;
203 QSharedPointer<click::web::Client> client;
204 QSharedPointer<click::Index> index;
205
206=== modified file 'scope/clickstore/store-query.cpp'
207--- scope/clickstore/store-query.cpp 2016-01-12 10:23:19 +0000
208+++ scope/clickstore/store-query.cpp 2016-05-10 14:58:44 +0000
209@@ -152,13 +152,15 @@
210 std::shared_ptr<click::DepartmentsDb> depts_db,
211 click::HighlightList& highlights,
212 const scopes::SearchMetadata& metadata,
213- pay::Package& in_package)
214+ pay::Package& in_package,
215+ std::shared_future<void> const& qt_ready)
216 : index(index),
217 department_lookup(depts),
218 depts_db(depts_db),
219 highlights(highlights),
220 meta(metadata),
221- pay_package(in_package)
222+ pay_package(in_package),
223+ qt_ready_(qt_ready)
224 {
225 }
226 click::Index& index;
227@@ -169,6 +171,7 @@
228 click::web::Cancellable search_operation;
229 click::web::Cancellable purchases_operation;
230 pay::Package& pay_package;
231+ std::shared_future<void> qt_ready_;
232 };
233
234 click::Query::Query(unity::scopes::CannedQuery const& query,
235@@ -177,9 +180,10 @@
236 std::shared_ptr<click::DepartmentsDb> depts_db,
237 click::HighlightList& highlights,
238 scopes::SearchMetadata const& metadata,
239- pay::Package& in_package)
240+ pay::Package& in_package,
241+ std::shared_future<void> const& qt_ready)
242 : unity::scopes::SearchQueryBase(query, metadata),
243- impl(new Private(index, depts, depts_db, highlights, metadata, in_package))
244+ impl(new Private(index, depts, depts_db, highlights, metadata, in_package, qt_ready))
245 {
246 }
247
248@@ -614,6 +618,9 @@
249
250 void click::Query::run(scopes::SearchReplyProxy const& searchReply)
251 {
252+ if (impl->qt_ready_.valid())
253+ impl->qt_ready_.wait();
254+
255 auto q = query().query_string();
256 std::string categoryTemplate = CATEGORY_APPS_SEARCH;
257 if (q.empty()) {
258
259=== modified file 'scope/clickstore/store-query.h'
260--- scope/clickstore/store-query.h 2015-12-01 15:39:41 +0000
261+++ scope/clickstore/store-query.h 2016-05-10 14:58:44 +0000
262@@ -44,6 +44,8 @@
263 #include <click/highlights.h>
264 #include <click/interface.h>
265
266+#include <future>
267+
268 namespace click
269 {
270
271@@ -74,7 +76,8 @@
272 std::shared_ptr<click::DepartmentsDb> depts_db,
273 click::HighlightList& highlights,
274 scopes::SearchMetadata const& metadata,
275- pay::Package& in_package);
276+ pay::Package& in_package,
277+ std::shared_future<void> const& qt_ready = std::future<void>());
278 virtual ~Query();
279
280 virtual void cancelled() override;
281
282=== modified file 'scope/clickstore/store-scope.cpp'
283--- scope/clickstore/store-scope.cpp 2016-04-15 09:24:18 +0000
284+++ scope/clickstore/store-scope.cpp 2016-05-10 14:58:44 +0000
285@@ -47,7 +47,8 @@
286
287 click::Scope::Scope()
288 {
289- qt_ready_f = qt_ready_p.get_future();
290+ qt_ready_for_search_f = qt_ready_for_search_p.get_future();
291+ qt_ready_for_preview_f = qt_ready_for_preview_p.get_future();
292 nam.reset(new click::network::AccessManager());
293 client.reset(new click::web::Client(nam));
294 index.reset(new click::Index(client));
295@@ -84,10 +85,12 @@
296 static const int zero = 0;
297 auto emptyCb = [this]()
298 {
299+ dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
300+ qt_ready_for_search_p.set_value();
301+
302 sso.reset(new click::CredentialsService());
303 client->setCredentialsService(sso);
304- dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
305- qt_ready_p.set_value();
306+ qt_ready_for_preview_p.set_value();
307 };
308
309 qt::core::world::build_and_run(zero, nullptr, emptyCb);
310@@ -100,16 +103,15 @@
311
312 scopes::SearchQueryBase::UPtr click::Scope::search(unity::scopes::CannedQuery const& q, scopes::SearchMetadata const& metadata)
313 {
314- qt_ready_f.wait();
315- return scopes::SearchQueryBase::UPtr(new click::Query(q, *index, *depts, depts_db, *highlights, metadata, *pay_package));
316+ return scopes::SearchQueryBase::UPtr(new click::Query(q, *index, *depts, depts_db, *highlights, metadata,
317+ *pay_package, qt_ready_for_search_f.share()));
318 }
319
320
321 unity::scopes::PreviewQueryBase::UPtr click::Scope::preview(const unity::scopes::Result& result,
322 const unity::scopes::ActionMetadata& metadata) {
323- qt_ready_f.wait();
324 qDebug() << "Scope::preview() called.";
325- auto preview = new click::Preview(result, metadata);
326+ auto preview = new click::Preview(result, metadata, qt_ready_for_preview_f.share());
327 preview->choose_strategy(client, pay_package, dm, depts_db);
328 return unity::scopes::PreviewQueryBase::UPtr{preview};
329 }
330
331=== modified file 'scope/clickstore/store-scope.h'
332--- scope/clickstore/store-scope.h 2016-04-15 09:24:18 +0000
333+++ scope/clickstore/store-scope.h 2016-05-10 14:58:44 +0000
334@@ -71,8 +71,10 @@
335 virtual unity::scopes::ActivationQueryBase::UPtr perform_action(unity::scopes::Result const& result, unity::scopes::ActionMetadata const& metadata, std::string const& widget_id, std::string const& action_id) override;
336
337 private:
338- std::promise<void> qt_ready_p;
339- std::future<void> qt_ready_f;
340+ std::promise<void> qt_ready_for_search_p;
341+ std::future<void> qt_ready_for_search_f;
342+ std::promise<void> qt_ready_for_preview_p;
343+ std::future<void> qt_ready_for_preview_f;
344 QSharedPointer<click::network::AccessManager> nam;
345 QSharedPointer<click::web::Client> client;
346 QSharedPointer<click::Index> index;

Subscribers

People subscribed via source and target branches