Merge ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/bionic-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/bionic

Proposed by Marco Trevisan (Treviño) on 2018-11-08
Status: Needs review
Proposed branch: ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/bionic-xubuntu-cancel-search
Merge into: ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/bionic
Diff against target: 317 lines (+277/-0)
6 files modified
debian/changelog (+11/-0)
debian/patches/search-Cancel-search-provider-operations-on-clear.patch (+29/-0)
debian/patches/search-Ignore-search-provider-results-metas-if-search-is-.patch (+30/-0)
debian/patches/series (+4/-0)
debian/patches/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch (+167/-0)
debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch (+36/-0)
Reviewer Review Type Date Requested Status
Iain Lane 2018-11-08 Pending
Review via email: mp+358477@code.launchpad.net

Description of the change

Reproposing after the revert...

To post a comment you must log in.
Iain Lane (laney) wrote :

I'm currently dealing with some horrible rebases due to Ubuntu patches in nautilus, e.g. <https://bugzilla.gnome.org/show_bug.cgi?id=758833>.

How can we be sure that this XUbuntuCancel stuff isn't going to end up in the same state?

Marco Trevisan (Treviño) (3v1n0) wrote :

I totally hope the same.

I just wish the upstream issue I opened to move with a proper fix will move, because I can't do much other than propose stuff, and ask for some discussion.

Unmerged commits

b923e63... by Marco Trevisan (Treviño) on 2018-08-27

debian/patches: add support fo XUbuntuCancel on remote search provider

Fixes LP: #1756826

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 2d2d1df..526692c 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,14 @@
6+gnome-shell (3.28.3-0ubuntu0.18.04.4) UNRELEASED; urgency=medium
7+
8+ * d/p/search-Cancel-search-provider-operations-on-clear.patch,
9+ d/p/search-Ignore-search-provider-results-metas-if-search-is-.patch,
10+ d/p/viewSelector-Cancel-search-on-overview-hidden.patch,
11+ d/p/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch:
12+ - Add support for cancelling remote search providers when the overlay
13+ is closed (and actually stop searches when requested from UI, LP: #1756826)
14+
15+ -- Marco Trevisan (Treviño) <marco@ubuntu.com> Thu, 08 Nov 2018 01:15:02 -0500
16+
17 gnome-shell (3.28.3-0ubuntu0.18.04.3) bionic; urgency=medium
18
19 * debian/ubuntu.css:
20diff --git a/debian/patches/search-Cancel-search-provider-operations-on-clear.patch b/debian/patches/search-Cancel-search-provider-operations-on-clear.patch
21new file mode 100644
22index 0000000..052ee8a
23--- /dev/null
24+++ b/debian/patches/search-Cancel-search-provider-operations-on-clear.patch
25@@ -0,0 +1,29 @@
26+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
27+Date: Thu, 23 Aug 2018 18:14:38 +0200
28+Subject: search: Cancel search provider operations on clear
29+
30+Ensure that the search provider operations (just getResultMetas requests in the
31+current implementation) in progress are properly cancelled when we clear the UI,
32+otherwise returned results might still be added when not needed.
33+
34+This is triggered for each provider by the SearchResults reset.
35+
36+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
37+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
38+Forwarded: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
39+---
40+ js/ui/search.js | 1 +
41+ 1 file changed, 1 insertion(+)
42+
43+diff --git a/js/ui/search.js b/js/ui/search.js
44+index 1fb54b4..804be95 100644
45+--- a/js/ui/search.js
46++++ b/js/ui/search.js
47+@@ -192,6 +192,7 @@ var SearchResultsBase = new Lang.Class({
48+ },
49+
50+ clear() {
51++ this._cancellable.cancel();
52+ for (let resultId in this._resultDisplays)
53+ this._resultDisplays[resultId].actor.destroy();
54+ this._resultDisplays = {};
55diff --git a/debian/patches/search-Ignore-search-provider-results-metas-if-search-is-.patch b/debian/patches/search-Ignore-search-provider-results-metas-if-search-is-.patch
56new file mode 100644
57index 0000000..27645a5
58--- /dev/null
59+++ b/debian/patches/search-Ignore-search-provider-results-metas-if-search-is-.patch
60@@ -0,0 +1,30 @@
61+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
62+Date: Thu, 30 Aug 2018 07:11:24 +0200
63+Subject: search: Ignore search provider results metas if search is cancelled
64+
65+Call updateSearch callback with no results when the search provider has been
66+cancelled, without doing any logging.
67+
68+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
69+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
70+Forwarded: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
71+---
72+ js/ui/search.js | 5 +++--
73+ 1 file changed, 3 insertions(+), 2 deletions(-)
74+
75+diff --git a/js/ui/search.js b/js/ui/search.js
76+index 804be95..dd4bfad 100644
77+--- a/js/ui/search.js
78++++ b/js/ui/search.js
79+@@ -227,8 +227,9 @@ var SearchResultsBase = new Lang.Class({
80+
81+ this.provider.getResultMetas(metasNeeded, metas => {
82+ if (metas.length != metasNeeded.length) {
83+- log('Wrong number of result metas returned by search provider ' + this.provider.id +
84+- ': expected ' + metasNeeded.length + ' but got ' + metas.length);
85++ if (!this._cancellable.is_cancelled())
86++ log(`Wrong number of result metas returned by search provider ${this.provider.id}` +
87++ `: expected ${metasNeeded.length} but got ${metas.length}`);
88+ callback(false);
89+ return;
90+ }
91diff --git a/debian/patches/series b/debian/patches/series
92index 6a12345..02a323d 100644
93--- a/debian/patches/series
94+++ b/debian/patches/series
95@@ -25,3 +25,7 @@ popupMenu-Don-t-handle-key-presses-directly-if-there-are-.patch
96 st-button-Ignore-pointer-emulated-touch-events.patch
97 st-scroll-view-Handle-the-case-where-scrollbars-are-NULL.patch
98 st-scroll-view-Remove-scrollbars-references-on-dispose.patch
99+viewSelector-Cancel-search-on-overview-hidden.patch
100+search-Cancel-search-provider-operations-on-clear.patch
101+search-Ignore-search-provider-results-metas-if-search-is-.patch
102+ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch
103diff --git a/debian/patches/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch b/debian/patches/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch
104new file mode 100644
105index 0000000..94a1d42
106--- /dev/null
107+++ b/debian/patches/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch
108@@ -0,0 +1,167 @@
109+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
110+Date: Thu, 23 Aug 2018 20:00:57 +0200
111+Subject: search: call XUbuntuCancel method on providers when no data is
112+ needed
113+
114+Add XUbuntuCancel method to search providers and call it when a search provider
115+is still doing operations.
116+Ignore the result when the method does not exist or is cancelled.
117+
118+This will allow to stop operations on providers.
119+
120+Fixes LP: #1756826
121+
122+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
123+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
124+Forwarded: not-needed
125+---
126+ data/org.gnome.ShellSearchProvider.xml | 6 ++++++
127+ data/org.gnome.ShellSearchProvider2.xml | 6 ++++++
128+ js/ui/remoteSearch.js | 15 +++++++++++++++
129+ js/ui/search.js | 34 +++++++++++++++++++++++++++++++++
130+ 4 files changed, 61 insertions(+)
131+
132+diff --git a/data/org.gnome.ShellSearchProvider.xml b/data/org.gnome.ShellSearchProvider.xml
133+index 78ad305..393cb01 100644
134+--- a/data/org.gnome.ShellSearchProvider.xml
135++++ b/data/org.gnome.ShellSearchProvider.xml
136+@@ -69,5 +69,11 @@
137+ <method name="ActivateResult">
138+ <arg type="s" name="identifier" direction="in" />
139+ </method>
140++
141++ <!--
142++ XUbuntuCancel:
143++ Cancel the current search operation
144++ -->
145++ <method name="XUbuntuCancel" />
146+ </interface>
147+ </node>
148+diff --git a/data/org.gnome.ShellSearchProvider2.xml b/data/org.gnome.ShellSearchProvider2.xml
149+index 9502340..8141bc0 100644
150+--- a/data/org.gnome.ShellSearchProvider2.xml
151++++ b/data/org.gnome.ShellSearchProvider2.xml
152+@@ -83,5 +83,11 @@
153+ <arg type="as" name="terms" direction="in" />
154+ <arg type="u" name="timestamp" direction="in" />
155+ </method>
156++
157++ <!--
158++ XUbuntuCancel:
159++ Cancel the current search operation
160++ -->
161++ <method name="XUbuntuCancel" />
162+ </interface>
163+ </node>
164+diff --git a/js/ui/remoteSearch.js b/js/ui/remoteSearch.js
165+index c6c5a29..8de6148 100644
166+--- a/js/ui/remoteSearch.js
167++++ b/js/ui/remoteSearch.js
168+@@ -30,6 +30,7 @@ const SearchProviderIface = '<node> \
169+ <method name="ActivateResult"> \
170+ <arg type="s" direction="in" /> \
171+ </method> \
172++<method name="XUbuntuCancel" /> \
173+ </interface> \
174+ </node>';
175+
176+@@ -57,6 +58,7 @@ const SearchProvider2Iface = '<node> \
177+ <arg type="as" direction="in" /> \
178+ <arg type="u" direction="in" /> \
179+ </method> \
180++<method name="XUbuntuCancel" /> \
181+ </interface> \
182+ </node>';
183+
184+@@ -310,6 +312,19 @@ var RemoteSearchProvider = new Lang.Class({
185+ cancellable);
186+ },
187+
188++ XUbuntuCancel(cancellable, callback) {
189++ this.proxy.XUbuntuCancelRemote((results, error) => {
190++ if (error &&
191++ !error.matches(Gio.DBusError, Gio.DBusError.UNKNOWN_METHOD) &&
192++ !error.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED)) {
193++ log('Received error from DBus search provider %s during XUbuntuCancel: %s'.format(this.id, String(error)));
194++ } else if (callback && !error) {
195++ callback();
196++ }
197++ },
198++ cancellable);
199++ },
200++
201+ activateResult(id) {
202+ this.proxy.ActivateResultRemote(id);
203+ },
204+diff --git a/js/ui/search.js b/js/ui/search.js
205+index dd4bfad..629664e 100644
206+--- a/js/ui/search.js
207++++ b/js/ui/search.js
208+@@ -225,7 +225,9 @@ var SearchResultsBase = new Lang.Class({
209+ this._cancellable.cancel();
210+ this._cancellable.reset();
211+
212++ this.provider.resultsMetasInProgress = true;
213+ this.provider.getResultMetas(metasNeeded, metas => {
214++ this.provider.resultsMetasInProgress = this._cancellable.is_cancelled();
215+ if (metas.length != metasNeeded.length) {
216+ if (!this._cancellable.is_cancelled())
217+ log(`Wrong number of result metas returned by search provider ${this.provider.id}` +
218+@@ -450,6 +452,10 @@ var SearchResults = new Lang.Class({
219+
220+ this._searchTimeoutId = 0;
221+ this._cancellable = new Gio.Cancellable();
222++ this._searchCancelCancellable = new Gio.Cancellable();
223++ this._cancellable.connect(() => {
224++ this._cancelSearchProviderRequest();
225++ });
226+
227+ this._registerProvider(new AppDisplay.AppSearchProvider());
228+ this._reloadRemoteProviders();
229+@@ -491,11 +497,32 @@ var SearchResults = new Lang.Class({
230+ }
231+ },
232+
233++ _cancelSearchProviderRequest() {
234++ if (this._terms.length != 0 || this._searchCancelTimeoutId > 0)
235++ return;
236++
237++ this._searchCancelTimeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, 100, () => {
238++ this._providers.forEach(provider => {
239++ if (provider.isRemoteProvider &&
240++ (provider.searchInProgress || provider.resultsMetasInProgress)) {
241++ provider.XUbuntuCancel(this._searchCancelCancellable, () => {
242++ provider.searchInProgress = false;
243++ provider.resultsMetasInProgress = false;
244++ });
245++ }
246++ });
247++
248++ delete this._searchCancelTimeoutId;
249++ return GLib.SOURCE_REMOVE;
250++ });
251++ },
252++
253+ _reset() {
254+ this._terms = [];
255+ this._results = {};
256+ this._clearDisplay();
257+ this._clearSearchTimeout();
258++ this._cancelSearchProviderRequest();
259+ this._defaultResult = null;
260+ this._startingSearch = false;
261+
262+@@ -562,6 +589,13 @@ var SearchResults = new Lang.Class({
263+ if (this._terms.length > 0)
264+ isSubSearch = searchString.indexOf(previousSearchString) == 0;
265+
266++ this._searchCancelCancellable.cancel();
267++ this._searchCancelCancellable.reset();
268++ if (this._searchCancelTimeoutId > 0) {
269++ GLib.source_remove(this._searchCancelTimeoutId);
270++ delete this._searchCancelTimeoutId;
271++ }
272++
273+ this._terms = terms;
274+ this._isSubSearch = isSubSearch;
275+ this._updateSearchProgress();
276diff --git a/debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch b/debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch
277new file mode 100644
278index 0000000..88ae73e
279--- /dev/null
280+++ b/debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch
281@@ -0,0 +1,36 @@
282+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
283+Date: Thu, 23 Aug 2018 16:11:10 +0200
284+Subject: viewSelector: Cancel search on overview hidden
285+
286+Currently when the overview is hidden, any pending search is kept alive, not only
287+at remote search provider level (as per issue #183), but even the shell providers
288+proxies continue to get and process data. This happens even if this is not needed
289+anymore, while the UI reset is performed only next time that the overview is
290+shown (causing some more computation presentation time).
291+
292+In order to stop this to happen, when the overview is hidden, we have to unset
293+the search entry to an empty value as this would make SearchResults to have empty
294+terms list and that would make the proxies cancellable to be triggered (without
295+causing any further search to start).
296+
297+https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
298+
299+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
300+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
301+Forwarded: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
302+---
303+ js/ui/viewSelector.js | 1 +
304+ 1 file changed, 1 insertion(+)
305+
306+diff --git a/js/ui/viewSelector.js b/js/ui/viewSelector.js
307+index 91bc222..c46afc5 100644
308+--- a/js/ui/viewSelector.js
309++++ b/js/ui/viewSelector.js
310+@@ -311,6 +311,7 @@ var ViewSelector = new Lang.Class({
311+ },
312+
313+ hide() {
314++ this.reset();
315+ this._workspacesDisplay.hide();
316+ },
317+

Subscribers

People subscribed via source and target branches