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

[Marco Trevisan (Treviño)]

Added XUbuntuCancel method call for search providers

[Didier Roche-Tolomelli (didrocks)]

Ok, this looks good to me, see some of my questions.

I saw that you made some changes on the MR after Florian's comment on your patches. Is there anything you can borrow and refresh here?

See as well my 2 questions/comments

review: Needs Information

[Marco Trevisan (Treviño) (3v1n0)]

> I saw that you made some changes on the MR after Florian's comment on your
> patches. Is there anything you can borrow and refresh here?

Yes, I wanted to refresh in case a review (like in this case) was coming in time, so nothing functional has been changed there, but I can refresh it on that.

As per the two questions, let me know if you want get rid of that variable or use a property bound to the cancellable instead.

[Didier Roche-Tolomelli (didrocks)]

For the variable, it's really up to you and what is the most readable in your eyes. I just wanted to trigger that question so that you can think about it, but I have no strong opinion :)

[Didier Roche-Tolomelli (didrocks)]

Just a nitpick on wrong alignement, but otherwise, looks good :)

review: Approve

diff --git a/debian/changelog b/debian/changelog
index 6fbc804..16b0afa 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,4 +1,4 @@
-gnome-shell (3.29.92-1ubuntu1) UNRELEASED; urgency=medium
+gnome-shell (3.29.92-1ubuntu1) cosmic; urgency=medium
 
   * Merge with debian, remaining changes:
     + Replace gnome-backgrounds dep with ubuntu-wallpapers and Suggests
@@ -63,8 +63,14 @@ gnome-shell (3.29.92-1ubuntu1) UNRELEASED; urgency=medium
     - Updated as per upstream review
   * d/p/js-ui-Choose-some-actors-to-cache-on-the-GPU.patch:
     - Removed (applied upstream)
-
- -- Marco Trevisan (Treviño) <[e-mail]>  Thu, 30 Aug 2018 09:08:20 -0500
+  * d/p/search-Cancel-search-provider-operations-on-clear.patch,
+    d/p/search-Ignore-search-provider-results-metas-if-search-is-.patch,
+    d/p/viewSelector-Cancel-search-on-overview-hidden.patch,
+    d/p/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch:
+    - Add support for cancelling remote search providers when the overlay
+      is closed (and actually stop searches when requested from UI, LP: #1756826)
+
+ -- Marco Trevisan (Treviño) <[e-mail]>  Thu, 30 Aug 2018 17:59:08 -0500
 
 gnome-shell (3.29.92-1) experimental; urgency=medium
 
diff --git a/debian/patches/search-Cancel-search-provider-operations-on-clear.patch b/debian/patches/search-Cancel-search-provider-operations-on-clear.patch
new file mode 100644
index 0000000..052ee8a
--- /dev/null
+++ b/debian/patches/search-Cancel-search-provider-operations-on-clear.patch
@@ -0,0 +1,29 @@
+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <[e-mail]>
+Date: Thu, 23 Aug 2018 18:14:38 +0200
+Subject: search: Cancel search provider operations on clear
+
+Ensure that the search provider operations (just getResultMetas requests in the
+current implementation) in progress are properly cancelled when we clear the UI,
+otherwise returned results might still be added when not needed.
+
+This is triggered for each provider by the SearchResults reset.
+
+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
+Forwarded: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
+---
+ js/ui/search.js | 1 +
+ 1 file changed, 1 insertion(+)
+
+diff --git a/js/ui/search.js b/js/ui/search.js
+index 1fb54b4..804be95 100644
+--- a/js/ui/search.js
++++ b/js/ui/search.js
+@@ -192,6 +192,7 @@ var SearchResultsBase = new Lang.Class({
+     },
+ 
+     clear() {
++        this._cancellable.cancel();
+         for (let resultId in this._resultDisplays)
+             this._resultDisplays[resultId].actor.destroy();
+         this._resultDisplays = {};
diff --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
new file mode 100644
index 0000000..27645a5
--- /dev/null
+++ b/debian/patches/search-Ignore-search-provider-results-metas-if-search-is-.patch
@@ -0,0 +1,30 @@
+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <[e-mail]>
+Date: Thu, 30 Aug 2018 07:11:24 +0200
+Subject: search: Ignore search provider results metas if search is cancelled
+
+Call updateSearch callback with no results when the search provider has been
+cancelled, without doing any logging.
+
+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
+Forwarded: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
+---
+ js/ui/search.js | 5 +++--
+ 1 file changed, 3 insertions(+), 2 deletions(-)
+
+diff --git a/js/ui/search.js b/js/ui/search.js
+index 804be95..dd4bfad 100644
+--- a/js/ui/search.js
++++ b/js/ui/search.js
+@@ -227,8 +227,9 @@ var SearchResultsBase = new Lang.Class({
+ 
+             this.provider.getResultMetas(metasNeeded, metas => {
+                 if (metas.length != metasNeeded.length) {
+-                    log('Wrong number of result metas returned by search provider ' + this.provider.id +
+-                        ': expected ' + metasNeeded.length + ' but got ' + metas.length);
++                    if (!this._cancellable.is_cancelled())
++                        log(`Wrong number of result metas returned by search provider ${this.provider.id}` +
++                            `: expected ${metasNeeded.length} but got ${metas.length}`);
+                     callback(false);
+                     return;
+                 }
diff --git a/debian/patches/series b/debian/patches/series
index c198fe6..6833a31 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -22,3 +22,7 @@ dnd-Nullify-_dragActor-after-we-ve-destroyed-it-and-avoid.patch
 messageList-stop-syncing-if-closeButton-has-been-destroye.patch
 automountManager-remove-allowAutorun-expire-timeout-on-vo.patch
 workaround_crasher_fractional_scaling.patch
+viewSelector-Cancel-search-on-overview-hidden.patch
+search-Cancel-search-provider-operations-on-clear.patch
+search-Ignore-search-provider-results-metas-if-search-is-.patch
+ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch
diff --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
new file mode 100644
index 0000000..f601fd2
--- /dev/null
+++ b/debian/patches/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch
@@ -0,0 +1,167 @@
+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <[e-mail]>
+Date: Thu, 23 Aug 2018 20:00:57 +0200
+Subject: search: call XUbuntuCancel method on providers when no data is
+ needed
+
+Add XUbuntuCancel method to search providers and call it when a search provider
+is still doing operations.
+Ignore the result when the method does not exist or is cancelled.
+
+This will allow to stop operations on providers.
+
+Fixes LP: #1756826
+
+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
+Forwarded: not-needed
+---
+ data/org.gnome.ShellSearchProvider.xml  |  6 ++++++
+ data/org.gnome.ShellSearchProvider2.xml |  6 ++++++
+ js/ui/remoteSearch.js                   | 15 +++++++++++++++
+ js/ui/search.js                         | 34 +++++++++++++++++++++++++++++++++
+ 4 files changed, 61 insertions(+)
+
+diff --git a/data/org.gnome.ShellSearchProvider.xml b/data/org.gnome.ShellSearchProvider.xml
+index 78ad305..393cb01 100644
+--- a/data/org.gnome.ShellSearchProvider.xml
++++ b/data/org.gnome.ShellSearchProvider.xml
+@@ -69,5 +69,11 @@
+     <method name="ActivateResult">
+       <arg type="s" name="identifier" direction="in" />
+     </method>
++
++    <!--
++        XUbuntuCancel:
++        Cancel the current search operation
++    -->
++    <method name="XUbuntuCancel" />
+   </interface>
+ </node>
+diff --git a/data/org.gnome.ShellSearchProvider2.xml b/data/org.gnome.ShellSearchProvider2.xml
+index 9502340..8141bc0 100644
+--- a/data/org.gnome.ShellSearchProvider2.xml
++++ b/data/org.gnome.ShellSearchProvider2.xml
+@@ -83,5 +83,11 @@
+       <arg type="as" name="terms" direction="in" />
+       <arg type="u" name="timestamp" direction="in" />
+     </method>
++
++    <!--
++        XUbuntuCancel:
++        Cancel the current search operation
++    -->
++    <method name="XUbuntuCancel" />
+   </interface>
+ </node>
+diff --git a/js/ui/remoteSearch.js b/js/ui/remoteSearch.js
+index 3b847af..9245745 100644
+--- a/js/ui/remoteSearch.js
++++ b/js/ui/remoteSearch.js
+@@ -31,6 +31,7 @@ const SearchProviderIface = `
+ <method name="ActivateResult">
+     <arg type="s" direction="in" />
+ </method>
++<method name="XUbuntuCancel" />
+ </interface>
+ </node>`;
+ 
+@@ -59,6 +60,7 @@ const SearchProvider2Iface = `
+     <arg type="as" direction="in" />
+     <arg type="u" direction="in" />
+ </method>
++<method name="XUbuntuCancel" />
+ </interface>
+ </node>`;
+ 
+@@ -312,6 +314,19 @@ var RemoteSearchProvider = new Lang.Class({
+                                         cancellable);
+     },
+ 
++    XUbuntuCancel(cancellable, callback) {
++        this.proxy.XUbuntuCancelRemote((results, error) => {
++                if (error &&
++                    !error.matches(Gio.DBusError, Gio.DBusError.UNKNOWN_METHOD) &&
++                    !error.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED)) {
++                    log('Received error from DBus search provider %s during XUbuntuCancel: %s'.format(this.id, String(error)));
++                } else if (callback && !error) {
++                    callback();
++                }
++            },
++            cancellable);
++    },
++
+     activateResult(id) {
+         this.proxy.ActivateResultRemote(id);
+     },
+diff --git a/js/ui/search.js b/js/ui/search.js
+index dd4bfad..629664e 100644
+--- a/js/ui/search.js
++++ b/js/ui/search.js
+@@ -225,7 +225,9 @@ var SearchResultsBase = new Lang.Class({
+             this._cancellable.cancel();
+             this._cancellable.reset();
+ 
++            this.provider.resultsMetasInProgress = true;
+             this.provider.getResultMetas(metasNeeded, metas => {
++                this.provider.resultsMetasInProgress = this._cancellable.is_cancelled();
+                 if (metas.length != metasNeeded.length) {
+                     if (!this._cancellable.is_cancelled())
+                         log(`Wrong number of result metas returned by search provider ${this.provider.id}` +
+@@ -450,6 +452,10 @@ var SearchResults = new Lang.Class({
+ 
+         this._searchTimeoutId = 0;
+         this._cancellable = new Gio.Cancellable();
++        this._searchCancelCancellable = new Gio.Cancellable();
++        this._cancellable.connect(() => {
++            this._cancelSearchProviderRequest();
++        });
+ 
+         this._registerProvider(new AppDisplay.AppSearchProvider());
+         this._reloadRemoteProviders();
+@@ -491,11 +497,32 @@ var SearchResults = new Lang.Class({
+         }
+     },
+ 
++    _cancelSearchProviderRequest() {
++        if (this._terms.length != 0 || this._searchCancelTimeoutId > 0)
++            return;
++
++        this._searchCancelTimeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, 100, () => {
++            this._providers.forEach(provider => {
++                if (provider.isRemoteProvider &&
++                    (provider.searchInProgress || provider.resultsMetasInProgress)) {
++                    provider.XUbuntuCancel(this._searchCancelCancellable, () => {
++                        provider.searchInProgress = false;
++                        provider.resultsMetasInProgress = false;
++                    });
++                }
++            });
++
++            delete this._searchCancelTimeoutId;
++            return GLib.SOURCE_REMOVE;
++        });
++    },
++
+     _reset() {
+         this._terms = [];
+         this._results = {};
+         this._clearDisplay();
+         this._clearSearchTimeout();
++        this._cancelSearchProviderRequest();
+         this._defaultResult = null;
+         this._startingSearch = false;
+ 
+@@ -562,6 +589,13 @@ var SearchResults = new Lang.Class({
+         if (this._terms.length > 0)
+             isSubSearch = searchString.indexOf(previousSearchString) == 0;
+ 
++        this._searchCancelCancellable.cancel();
++        this._searchCancelCancellable.reset();
++        if (this._searchCancelTimeoutId > 0) {
++            GLib.source_remove(this._searchCancelTimeoutId);
++            delete this._searchCancelTimeoutId;
++        }
++
+         this._terms = terms;
+         this._isSubSearch = isSubSearch;
+         this._updateSearchProgress();
diff --git a/debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch b/debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch
new file mode 100644
index 0000000..e69ae1e
--- /dev/null
+++ b/debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch
@@ -0,0 +1,36 @@
+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <[e-mail]>
+Date: Thu, 23 Aug 2018 16:11:10 +0200
+Subject: viewSelector: Cancel search on overview hidden
+
+Currently when the overview is hidden, any pending search is kept alive, not only
+at remote search provider level (as per issue #183), but even the shell providers
+proxies continue to get and process data. This happens even if this is not needed
+anymore, while the UI reset is performed only next time that the overview is
+shown (causing some more computation presentation time).
+
+In order to stop this to happen, when the overview is hidden, we have to unset
+the search entry to an empty value as this would make SearchResults to have empty
+terms list and that would make the proxies cancellable to be triggered (without
+causing any further search to start).
+
+https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
+
+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
+Forwarded: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
+---
+ js/ui/viewSelector.js | 1 +
+ 1 file changed, 1 insertion(+)
+
+diff --git a/js/ui/viewSelector.js b/js/ui/viewSelector.js
+index 2d5c33f..c650886 100644
+--- a/js/ui/viewSelector.js
++++ b/js/ui/viewSelector.js
+@@ -311,6 +311,7 @@ var ViewSelector = new Lang.Class({
+     },
+ 
+     hide() {
++        this.reset();
+         this._workspacesDisplay.hide();
+     },
+