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

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 :
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
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 :
review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'libclickscope/click/preview.cpp'
--- libclickscope/click/preview.cpp 2016-04-01 17:16:05 +0000
+++ libclickscope/click/preview.cpp 2016-05-10 14:58:44 +0000
@@ -158,8 +158,12 @@
158// Preview base class158// Preview base class
159159
160Preview::Preview(const unity::scopes::Result& result,160Preview::Preview(const unity::scopes::Result& result,
161 const unity::scopes::ActionMetadata& metadata)161 const unity::scopes::ActionMetadata& metadata,
162 : PreviewQueryBase(result, metadata), result(result), metadata(metadata)162 std::shared_future<void> const& qt_ready)
163 : PreviewQueryBase(result, metadata),
164 result(result),
165 metadata(metadata),
166 qt_ready_(qt_ready)
163{167{
164}168}
165169
@@ -259,6 +263,9 @@
259263
260void Preview::run(const unity::scopes::PreviewReplyProxy &reply)264void Preview::run(const unity::scopes::PreviewReplyProxy &reply)
261{265{
266 if (qt_ready_.valid())
267 qt_ready_.wait();
268
262 strategy->run(reply);269 strategy->run(reply);
263}270}
264271
265272
=== modified file 'libclickscope/click/preview.h'
--- libclickscope/click/preview.h 2016-03-07 14:10:27 +0000
+++ libclickscope/click/preview.h 2016-05-10 14:58:44 +0000
@@ -102,6 +102,7 @@
102 std::unique_ptr<PreviewStrategy> strategy;102 std::unique_ptr<PreviewStrategy> strategy;
103 const unity::scopes::Result& result;103 const unity::scopes::Result& result;
104 const unity::scopes::ActionMetadata& metadata;104 const unity::scopes::ActionMetadata& metadata;
105 std::shared_future<void> qt_ready_;
105 PreviewStrategy* build_strategy(const unity::scopes::Result& result,106 PreviewStrategy* build_strategy(const unity::scopes::Result& result,
106 const unity::scopes::ActionMetadata& metadata,107 const unity::scopes::ActionMetadata& metadata,
107 const QSharedPointer<web::Client> &client,108 const QSharedPointer<web::Client> &client,
@@ -142,7 +143,8 @@
142143
143 Preview(const unity::scopes::Result& result);144 Preview(const unity::scopes::Result& result);
144 Preview(const unity::scopes::Result& result,145 Preview(const unity::scopes::Result& result,
145 const unity::scopes::ActionMetadata& metadata);146 const unity::scopes::ActionMetadata& metadata,
147 std::shared_future<void> const& qt_ready = std::future<void>());
146 virtual ~Preview();148 virtual ~Preview();
147 void choose_strategy(const QSharedPointer<web::Client> &client,149 void choose_strategy(const QSharedPointer<web::Client> &client,
148 const QSharedPointer<pay::Package>& ppackage,150 const QSharedPointer<pay::Package>& ppackage,
149151
=== modified file 'scope/clickapps/apps-query.cpp'
--- scope/clickapps/apps-query.cpp 2016-03-09 11:01:19 +0000
+++ scope/clickapps/apps-query.cpp 2016-05-10 14:58:44 +0000
@@ -222,21 +222,27 @@
222222
223struct click::apps::Query::Private223struct click::apps::Query::Private
224{224{
225 Private(std::shared_ptr<click::DepartmentsDb> depts_db, const scopes::SearchMetadata& metadata)225 Private(std::shared_ptr<click::DepartmentsDb> depts_db,
226 const scopes::SearchMetadata& metadata,
227 std::shared_future<void> const& qt_ready)
226 : depts_db(depts_db),228 : depts_db(depts_db),
227 meta(metadata)229 meta(metadata),
230 qt_ready_(qt_ready)
228 {231 {
229 }232 }
230233
231 std::shared_ptr<click::DepartmentsDb> depts_db;234 std::shared_ptr<click::DepartmentsDb> depts_db;
232 scopes::SearchMetadata meta;235 scopes::SearchMetadata meta;
233 click::Configuration configuration;236 click::Configuration configuration;
237 std::shared_future<void> qt_ready_;
234};238};
235239
236click::apps::Query::Query(unity::scopes::CannedQuery const& query, std::shared_ptr<DepartmentsDb> depts_db,240click::apps::Query::Query(unity::scopes::CannedQuery const& query,
237 scopes::SearchMetadata const& metadata)241 std::shared_ptr<DepartmentsDb> depts_db,
242 scopes::SearchMetadata const& metadata,
243 std::shared_future<void> const& qt_ready)
238 : unity::scopes::SearchQueryBase(query, metadata),244 : unity::scopes::SearchQueryBase(query, metadata),
239 impl(new Private(depts_db, metadata))245 impl(new Private(depts_db, metadata, qt_ready))
240{246{
241}247}
242248
@@ -403,6 +409,9 @@
403409
404void click::apps::Query::run(scopes::SearchReplyProxy const& searchReply)410void click::apps::Query::run(scopes::SearchReplyProxy const& searchReply)
405{411{
412 if (impl->qt_ready_.valid())
413 impl->qt_ready_.wait();
414
406 const std::string categoryTemplate = CATEGORY_APPS_DISPLAY;415 const std::string categoryTemplate = CATEGORY_APPS_DISPLAY;
407 auto const current_dept = query().department_id();416 auto const current_dept = query().department_id();
408 auto const querystr = query().query_string();417 auto const querystr = query().query_string();
409418
=== 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-05-10 14:58:44 +0000
@@ -40,6 +40,8 @@
40#include <unordered_set>40#include <unordered_set>
41#include <click/interface.h>41#include <click/interface.h>
4242
43#include <future>
44
43namespace click45namespace click
44{46{
4547
@@ -64,7 +66,10 @@
64 constexpr static const char* VERSION{"version"};66 constexpr static const char* VERSION{"version"};
65 };67 };
6668
67 Query(unity::scopes::CannedQuery const& query, std::shared_ptr<DepartmentsDb> depts_db, scopes::SearchMetadata const& metadata);69 Query(unity::scopes::CannedQuery const& query,
70 std::shared_ptr<DepartmentsDb> depts_db,
71 scopes::SearchMetadata const& metadata,
72 std::shared_future<void> const& qt_ready = std::future<void>());
68 virtual ~Query();73 virtual ~Query();
6974
70 virtual void cancelled() override;75 virtual void cancelled() override;
7176
=== modified file 'scope/clickapps/apps-scope.cpp'
--- scope/clickapps/apps-scope.cpp 2016-04-15 09:24:18 +0000
+++ scope/clickapps/apps-scope.cpp 2016-05-10 14:58:44 +0000
@@ -49,7 +49,8 @@
4949
50click::Scope::Scope()50click::Scope::Scope()
51{51{
52 qt_ready_f = qt_ready_p.get_future();52 qt_ready_for_search_f = qt_ready_for_search_p.get_future();
53 qt_ready_for_preview_f = qt_ready_for_preview_p.get_future();
53 nam.reset(new click::network::AccessManager());54 nam.reset(new click::network::AccessManager());
54 client.reset(new click::web::Client(nam));55 client.reset(new click::web::Client(nam));
55 index.reset(new click::Index(client));56 index.reset(new click::Index(client));
@@ -82,10 +83,12 @@
82 static const int zero = 0;83 static const int zero = 0;
83 auto emptyCb = [this]()84 auto emptyCb = [this]()
84 {85 {
86 dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
87 qt_ready_for_search_p.set_value();
88
85 sso.reset(new click::CredentialsService());89 sso.reset(new click::CredentialsService());
86 client->setCredentialsService(sso);90 client->setCredentialsService(sso);
87 dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());91 qt_ready_for_preview_p.set_value();
88 qt_ready_p.set_value();
89 };92 };
9093
91 qt::core::world::build_and_run(zero, nullptr, emptyCb);94 qt::core::world::build_and_run(zero, nullptr, emptyCb);
@@ -98,16 +101,14 @@
98101
99scopes::SearchQueryBase::UPtr click::Scope::search(unity::scopes::CannedQuery const& q, scopes::SearchMetadata const& metadata)102scopes::SearchQueryBase::UPtr click::Scope::search(unity::scopes::CannedQuery const& q, scopes::SearchMetadata const& metadata)
100{103{
101 qt_ready_f.wait();104 return scopes::SearchQueryBase::UPtr(new click::apps::Query(q, depts_db, metadata, qt_ready_for_search_f.share()));
102 return scopes::SearchQueryBase::UPtr(new click::apps::Query(q, depts_db, metadata));
103}105}
104106
105107
106unity::scopes::PreviewQueryBase::UPtr click::Scope::preview(const unity::scopes::Result& result,108unity::scopes::PreviewQueryBase::UPtr click::Scope::preview(const unity::scopes::Result& result,
107 const unity::scopes::ActionMetadata& metadata) {109 const unity::scopes::ActionMetadata& metadata) {
108 qt_ready_f.wait();
109 qDebug() << "Scope::preview() called.";110 qDebug() << "Scope::preview() called.";
110 auto preview = new click::Preview(result, metadata);111 auto preview = new click::Preview(result, metadata, qt_ready_for_preview_f.share());
111 preview->choose_strategy(client, pay_package, dm, depts_db);112 preview->choose_strategy(client, pay_package, dm, depts_db);
112 return unity::scopes::PreviewQueryBase::UPtr{preview};113 return unity::scopes::PreviewQueryBase::UPtr{preview};
113}114}
114115
=== modified file 'scope/clickapps/apps-scope.h'
--- scope/clickapps/apps-scope.h 2016-04-15 09:24:18 +0000
+++ scope/clickapps/apps-scope.h 2016-05-10 14:58:44 +0000
@@ -30,8 +30,6 @@
30#ifndef APPS_SCOPE_H30#ifndef APPS_SCOPE_H
31#define APPS_SCOPE_H31#define APPS_SCOPE_H
3232
33#include <future>
34
35#include <click/index.h>33#include <click/index.h>
36#include <click/network_access_manager.h>34#include <click/network_access_manager.h>
37#include <click/pay.h>35#include <click/pay.h>
@@ -42,6 +40,7 @@
42#include <unity/scopes/QueryBase.h>40#include <unity/scopes/QueryBase.h>
43#include <unity/scopes/ActivationQueryBase.h>41#include <unity/scopes/ActivationQueryBase.h>
4442
43#include <future>
4544
46namespace scopes = unity::scopes;45namespace scopes = unity::scopes;
4746
@@ -68,8 +67,10 @@
68 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;67 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;
6968
70private:69private:
71 std::promise<void> qt_ready_p;70 std::promise<void> qt_ready_for_search_p;
72 std::future<void> qt_ready_f;71 std::future<void> qt_ready_for_search_f;
72 std::promise<void> qt_ready_for_preview_p;
73 std::future<void> qt_ready_for_preview_f;
73 QSharedPointer<click::network::AccessManager> nam;74 QSharedPointer<click::network::AccessManager> nam;
74 QSharedPointer<click::web::Client> client;75 QSharedPointer<click::web::Client> client;
75 QSharedPointer<click::Index> index;76 QSharedPointer<click::Index> index;
7677
=== modified file 'scope/clickstore/store-query.cpp'
--- scope/clickstore/store-query.cpp 2016-01-12 10:23:19 +0000
+++ scope/clickstore/store-query.cpp 2016-05-10 14:58:44 +0000
@@ -152,13 +152,15 @@
152 std::shared_ptr<click::DepartmentsDb> depts_db,152 std::shared_ptr<click::DepartmentsDb> depts_db,
153 click::HighlightList& highlights,153 click::HighlightList& highlights,
154 const scopes::SearchMetadata& metadata,154 const scopes::SearchMetadata& metadata,
155 pay::Package& in_package)155 pay::Package& in_package,
156 std::shared_future<void> const& qt_ready)
156 : index(index),157 : index(index),
157 department_lookup(depts),158 department_lookup(depts),
158 depts_db(depts_db),159 depts_db(depts_db),
159 highlights(highlights),160 highlights(highlights),
160 meta(metadata),161 meta(metadata),
161 pay_package(in_package)162 pay_package(in_package),
163 qt_ready_(qt_ready)
162 {164 {
163 }165 }
164 click::Index& index;166 click::Index& index;
@@ -169,6 +171,7 @@
169 click::web::Cancellable search_operation;171 click::web::Cancellable search_operation;
170 click::web::Cancellable purchases_operation;172 click::web::Cancellable purchases_operation;
171 pay::Package& pay_package;173 pay::Package& pay_package;
174 std::shared_future<void> qt_ready_;
172};175};
173176
174click::Query::Query(unity::scopes::CannedQuery const& query,177click::Query::Query(unity::scopes::CannedQuery const& query,
@@ -177,9 +180,10 @@
177 std::shared_ptr<click::DepartmentsDb> depts_db,180 std::shared_ptr<click::DepartmentsDb> depts_db,
178 click::HighlightList& highlights,181 click::HighlightList& highlights,
179 scopes::SearchMetadata const& metadata,182 scopes::SearchMetadata const& metadata,
180 pay::Package& in_package)183 pay::Package& in_package,
184 std::shared_future<void> const& qt_ready)
181 : unity::scopes::SearchQueryBase(query, metadata),185 : unity::scopes::SearchQueryBase(query, metadata),
182 impl(new Private(index, depts, depts_db, highlights, metadata, in_package))186 impl(new Private(index, depts, depts_db, highlights, metadata, in_package, qt_ready))
183{187{
184}188}
185189
@@ -614,6 +618,9 @@
614618
615void click::Query::run(scopes::SearchReplyProxy const& searchReply)619void click::Query::run(scopes::SearchReplyProxy const& searchReply)
616{620{
621 if (impl->qt_ready_.valid())
622 impl->qt_ready_.wait();
623
617 auto q = query().query_string();624 auto q = query().query_string();
618 std::string categoryTemplate = CATEGORY_APPS_SEARCH;625 std::string categoryTemplate = CATEGORY_APPS_SEARCH;
619 if (q.empty()) {626 if (q.empty()) {
620627
=== modified file 'scope/clickstore/store-query.h'
--- scope/clickstore/store-query.h 2015-12-01 15:39:41 +0000
+++ scope/clickstore/store-query.h 2016-05-10 14:58:44 +0000
@@ -44,6 +44,8 @@
44#include <click/highlights.h>44#include <click/highlights.h>
45#include <click/interface.h>45#include <click/interface.h>
4646
47#include <future>
48
47namespace click49namespace click
48{50{
4951
@@ -74,7 +76,8 @@
74 std::shared_ptr<click::DepartmentsDb> depts_db,76 std::shared_ptr<click::DepartmentsDb> depts_db,
75 click::HighlightList& highlights,77 click::HighlightList& highlights,
76 scopes::SearchMetadata const& metadata,78 scopes::SearchMetadata const& metadata,
77 pay::Package& in_package);79 pay::Package& in_package,
80 std::shared_future<void> const& qt_ready = std::future<void>());
78 virtual ~Query();81 virtual ~Query();
7982
80 virtual void cancelled() override;83 virtual void cancelled() override;
8184
=== modified file 'scope/clickstore/store-scope.cpp'
--- scope/clickstore/store-scope.cpp 2016-04-15 09:24:18 +0000
+++ scope/clickstore/store-scope.cpp 2016-05-10 14:58:44 +0000
@@ -47,7 +47,8 @@
4747
48click::Scope::Scope()48click::Scope::Scope()
49{49{
50 qt_ready_f = qt_ready_p.get_future();50 qt_ready_for_search_f = qt_ready_for_search_p.get_future();
51 qt_ready_for_preview_f = qt_ready_for_preview_p.get_future();
51 nam.reset(new click::network::AccessManager());52 nam.reset(new click::network::AccessManager());
52 client.reset(new click::web::Client(nam));53 client.reset(new click::web::Client(nam));
53 index.reset(new click::Index(client));54 index.reset(new click::Index(client));
@@ -84,10 +85,12 @@
84 static const int zero = 0;85 static const int zero = 0;
85 auto emptyCb = [this]()86 auto emptyCb = [this]()
86 {87 {
88 dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
89 qt_ready_for_search_p.set_value();
90
87 sso.reset(new click::CredentialsService());91 sso.reset(new click::CredentialsService());
88 client->setCredentialsService(sso);92 client->setCredentialsService(sso);
89 dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());93 qt_ready_for_preview_p.set_value();
90 qt_ready_p.set_value();
91 };94 };
9295
93 qt::core::world::build_and_run(zero, nullptr, emptyCb);96 qt::core::world::build_and_run(zero, nullptr, emptyCb);
@@ -100,16 +103,15 @@
100103
101scopes::SearchQueryBase::UPtr click::Scope::search(unity::scopes::CannedQuery const& q, scopes::SearchMetadata const& metadata)104scopes::SearchQueryBase::UPtr click::Scope::search(unity::scopes::CannedQuery const& q, scopes::SearchMetadata const& metadata)
102{105{
103 qt_ready_f.wait();106 return scopes::SearchQueryBase::UPtr(new click::Query(q, *index, *depts, depts_db, *highlights, metadata,
104 return scopes::SearchQueryBase::UPtr(new click::Query(q, *index, *depts, depts_db, *highlights, metadata, *pay_package));107 *pay_package, qt_ready_for_search_f.share()));
105}108}
106109
107110
108unity::scopes::PreviewQueryBase::UPtr click::Scope::preview(const unity::scopes::Result& result,111unity::scopes::PreviewQueryBase::UPtr click::Scope::preview(const unity::scopes::Result& result,
109 const unity::scopes::ActionMetadata& metadata) {112 const unity::scopes::ActionMetadata& metadata) {
110 qt_ready_f.wait();
111 qDebug() << "Scope::preview() called.";113 qDebug() << "Scope::preview() called.";
112 auto preview = new click::Preview(result, metadata);114 auto preview = new click::Preview(result, metadata, qt_ready_for_preview_f.share());
113 preview->choose_strategy(client, pay_package, dm, depts_db);115 preview->choose_strategy(client, pay_package, dm, depts_db);
114 return unity::scopes::PreviewQueryBase::UPtr{preview};116 return unity::scopes::PreviewQueryBase::UPtr{preview};
115}117}
116118
=== modified file 'scope/clickstore/store-scope.h'
--- scope/clickstore/store-scope.h 2016-04-15 09:24:18 +0000
+++ scope/clickstore/store-scope.h 2016-05-10 14:58:44 +0000
@@ -71,8 +71,10 @@
71 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;71 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;
7272
73private:73private:
74 std::promise<void> qt_ready_p;74 std::promise<void> qt_ready_for_search_p;
75 std::future<void> qt_ready_f;75 std::future<void> qt_ready_for_search_f;
76 std::promise<void> qt_ready_for_preview_p;
77 std::future<void> qt_ready_for_preview_f;
76 QSharedPointer<click::network::AccessManager> nam;78 QSharedPointer<click::network::AccessManager> nam;
77 QSharedPointer<click::web::Client> client;79 QSharedPointer<click::web::Client> client;
78 QSharedPointer<click::Index> index;80 QSharedPointer<click::Index> index;

Subscribers

People subscribed via source and target branches