[Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-3.29.91 into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
The proposal to merge ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-3.29.91 into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master has been updated. Status: Needs review => Superseded For more details, see: https://code.launchpad.net/~3v1n0/ubuntu/+source/gnome-shell/+git/gnome-shell/+merge/353823 -- Your team Ubuntu Desktop is subscribed to branch ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master. -- ubuntu-desktop mailing list ubuntu-desktop@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-desktop
Re: [Merge] ~3v1n0/ubuntu/+source/nautilus:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/nautilus:ubuntu/master
> * You didn't metnion at all about the ignore_partial_results. [...]
> I think that ought some mention in the patch description.
Done
> * I'm unsure to understand what the meta_requests are and why they are
> treated differently (and unconditionnally).
> Is it a queue before them becoming the current requests, and this is why you
> are cancelling if the invocation caller matches as well?
So, while each search stops the previous one, it might happen that the shell
(not really as per how it's implemented, but the API isn't clear here, and so
we can't make assumptions), that the shell fetches the metadata on the returned
results in different calls, thus nautilus allowed these calls to be performed
in parallel if requested.
Creating multiple ResultMetasData instances... However in order to be able to
stop these requests, if XUbuntuCancel is called, we need to keep track of these
data instances, so that we can use the nautilus handler to stop the metadata
fetching and the invocation to return to the dbus caller. Not to mention to
free the instance when that happens.
Diff comments:
> diff --git
> a/debian/patches/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch
>
> b/debian/patches/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch
> new file mode 100644
> index 000..ca55544
> --- /dev/null
> +++
> b/debian/patches/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch
> @@ -0,0 +1,212 @@
> +From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?=
> +Date: Tue, 28 Aug 2018 01:44:49 +0200
> +Subject: shell-search-provider: implement XUbuntuCancel to request search
> cancellation
> +
> +Stop search and Metadata fetching on XUbuntuCancel dbus method call.
> +Only allow this if the caller is the same who triggered the actual event.
> +
> +Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
> +Bug-Ubuntu:
> https://bugs.launchpad.net/ubuntu/bionic/+source/nautilus/+bug/1756826
> +Forwarded: not-needed
> +---
> + data/shell-search-provider-dbus-interfaces.xml | 1 +
> + src/nautilus-shell-search-provider.c | 96
> +++---
> + 2 files changed, 88 insertions(+), 9 deletions(-)
> +
> +diff --git a/data/shell-search-provider-dbus-interfaces.xml
> b/data/shell-search-provider-dbus-interfaces.xml
> +index f6840e2..4529c1e 100644
> +--- a/data/shell-search-provider-dbus-interfaces.xml
> b/data/shell-search-provider-dbus-interfaces.xml
> +@@ -40,5 +40,6 @@
> +
> +
> +
> ++
> +
> +
> +diff --git a/src/nautilus-shell-search-provider.c
> b/src/nautilus-shell-search-provider.c
> +index ffc2b7f..58864d6 100644
> +--- a/src/nautilus-shell-search-provider.c
> b/src/nautilus-shell-search-provider.c
> +@@ -60,6 +60,7 @@ struct _NautilusShellSearchProvider
> +
> + PendingSearch *current_search;
> +
> ++GList *metas_requests;
> + GHashTable *metas_cache;
> + };
> +
> +@@ -143,11 +144,25 @@ pending_search_finish (PendingSearch *search,
> + }
> +
> + static void
> +-cancel_current_search (NautilusShellSearchProvider *self)
> ++cancel_current_search (NautilusShellSearchProvider *self,
> ++ gboolean ignore_partial_results)
> + {
> +-if (self->current_search != NULL)
> ++PendingSearch *search = self->current_search;
> ++
> ++if (search != NULL)
> + {
> +-nautilus_search_provider_stop (NAUTILUS_SEARCH_PROVIDER
> (self->current_search->engine));
> ++g_debug ("*** Cancel current search");
> ++
> ++nautilus_search_provider_stop (NAUTILUS_SEARCH_PROVIDER
> (search->engine));
> ++
> ++if (ignore_partial_results)
> ++{
> ++g_signal_handlers_disconnect_by_data (G_OBJECT (search->engine),
> ++ search);
> ++
> ++pending_search_finish (search, search->invocation,
> ++ g_variant_new ("(as)", NULL));
> ++}
> + }
> + }
> +
> +@@ -451,7 +466,7 @@ execute_search (NautilusShellSearchProvider *self,
> + NautilusQuery *query;
> + PendingSearch *pending_search;
> +
> +-cancel_current_search (self);
> ++cancel_current_search (self, FALSE);
> +
> + /* don't attempt searches for a single character */
> + if (g_strv_length (terms) == 1 &&
> +@@ -524,6 +539,7 @@ typedef struct
> + NautilusShellSearchProvider *self;
> +
> + gint64 start_time;
> ++NautilusFileListHandle *handle;
> + GDBusMethodInvocation *invocation;
> +
> + gchar **uris;
> +@@ -532,6 +548,8 @@ typedef struct
> + static void
> + result_metas_data_free (ResultMetasData *data)
> + {
> ++g_clear_pointer (>handle,
> nautilus_file_list_cancel_call_when_ready);
> ++
> + g_clear_object (>self);
> + g_clear_object (>invocation);
> + g_strfreev (data->uris);
> +@@ -549,7 +567,7 @@ result_metas_return_from_cache (ResultMetasData *data)
> +
>
[Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
Marco Trevisan (Treviño) has proposed merging
~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-xubuntu-cancel-search into
~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master with
~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-3.29.92 as a prerequisite.
Requested reviews:
Didier Roche (didrocks)
Related bugs:
Bug #1756826 in nautilus (Ubuntu): "hangs when remote search provider
performs expensive operation"
https://bugs.launchpad.net/ubuntu/+source/nautilus/+bug/1756826
For more details, see:
https://code.launchpad.net/~3v1n0/ubuntu/+source/gnome-shell/+git/gnome-shell/+merge/354050
Added XUbuntuCancel method call for search providers
--
Your team Ubuntu Desktop is subscribed to branch
~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master.
diff --git a/debian/changelog b/debian/changelog
index 6fbc804..6e58525 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -63,6 +63,11 @@ 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)
+ * d/p/js-viewSelector-Cancel-search-on-overview-hiding.patch,
+d/p/search-Cancel-search-provider-operations-on-clear.patch,
+d/p/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.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) Thu, 30 Aug 2018 09:08:20 -0500
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 000..052ee8a
--- /dev/null
+++ b/debian/patches/search-Cancel-search-provider-operations-on-clear.patch
@@ -0,0 +1,29 @@
+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?=
+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 000..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?=
+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..cbca12c 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -22,3 +22,7 @@
[Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
The proposal to merge ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master has been updated. Status: Needs review => Superseded For more details, see: https://code.launchpad.net/~3v1n0/ubuntu/+source/gnome-shell/+git/gnome-shell/+merge/353825 -- Your team Ubuntu Desktop is subscribed to branch ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master. -- ubuntu-desktop mailing list ubuntu-desktop@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-desktop
Re: [Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
Review: Needs Information
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
Diff comments:
> diff --git
> a/debian/patches/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.patch
>
> b/debian/patches/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.patch
> new file mode 100644
> index 000..de98f8e
> --- /dev/null
> +++
> b/debian/patches/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.patch
> @@ -0,0 +1,160 @@
> +From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?=
> +Date: Thu, 23 Aug 2018 20:00:57 +0200
> +Subject: search: call Cancel 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.
> +
> +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 | 13 +
> + js/ui/search.js | 31 +++
> + 4 files changed, 56 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 @@
> +
> +
> +
> ++
> ++
> ++
> +
> +
> +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
Stupid question, but do we use the API XubuntuCancel() on the 2 search provider
interface version?
> +@@ -83,5 +83,11 @@
> +
> +
> +
> ++
> ++
> ++
> +
> +
> +diff --git a/js/ui/remoteSearch.js b/js/ui/remoteSearch.js
> +index c6c5a29..79daba1 100644
> +--- a/js/ui/remoteSearch.js
> b/js/ui/remoteSearch.js
> +@@ -30,6 +30,7 @@ const SearchProviderIface = ' \
> + \
> + \
> + \
> ++ \
> + \
> + ';
> +
> +@@ -57,6 +58,7 @@ const SearchProvider2Iface = ' \
> + \
> + \
> + \
> ++ \
> + \
> + ';
> +
> +@@ -310,6 +312,17 @@ var RemoteSearchProvider = new Lang.Class({
> + cancellable);
> + },
> +
> ++XUbuntuCancel(cancellable) {
> ++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)));
> ++}
> ++},
> ++cancellable);
> ++},
> ++
> + activateResult(id) {
> + this.proxy.ActivateResultRemote(id);
> + },
> +diff --git a/js/ui/search.js b/js/ui/search.js
> +index f6fa719..fb2a021 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;
Can't we rather use the cancellable status to know if one is in progress or not
rather than another variable?
> + this.provider.getResultMetas(metasNeeded, metas => {
> ++this.provider.resultsMetasInProgress = false;
> + if (this._cancellable.is_cancelled()) {
> + callback(false);
> + return;
> +@@ -453,6 +455,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();
> +@@ -494,11 +500,29 @@ 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 => {
> ++
Re: [Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
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 :) -- https://code.launchpad.net/~3v1n0/ubuntu/+source/gnome-shell/+git/gnome-shell/+merge/353825 Your team Ubuntu Desktop is subscribed to branch ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master. -- ubuntu-desktop mailing list ubuntu-desktop@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-desktop
Re: [Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
> 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.
Diff comments:
> diff --git
> a/debian/patches/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.patch
>
> b/debian/patches/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.patch
> new file mode 100644
> index 000..de98f8e
> --- /dev/null
> +++
> b/debian/patches/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.patch
> @@ -0,0 +1,160 @@
> +From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?=
> +Date: Thu, 23 Aug 2018 20:00:57 +0200
> +Subject: search: call Cancel 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.
> +
> +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 | 13 +
> + js/ui/search.js | 31 +++
> + 4 files changed, 56 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 @@
> +
> +
> +
> ++
> ++
> ++
> +
> +
> +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
That will be used on both versions... Actually the js implementation reuses the
same code for both, while add timestamps on the 2 version.
> +@@ -83,5 +83,11 @@
> +
> +
> +
> ++
> ++
> ++
> +
> +
> +diff --git a/js/ui/remoteSearch.js b/js/ui/remoteSearch.js
> +index c6c5a29..79daba1 100644
> +--- a/js/ui/remoteSearch.js
> b/js/ui/remoteSearch.js
> +@@ -30,6 +30,7 @@ const SearchProviderIface = ' \
> + \
> + \
> + \
> ++ \
> + \
> + ';
> +
> +@@ -57,6 +58,7 @@ const SearchProvider2Iface = ' \
> + \
> + \
> + \
> ++ \
> + \
> + ';
> +
> +@@ -310,6 +312,17 @@ var RemoteSearchProvider = new Lang.Class({
> + cancellable);
> + },
> +
> ++XUbuntuCancel(cancellable) {
> ++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)));
> ++}
> ++},
> ++cancellable);
> ++},
> ++
> + activateResult(id) {
> + this.proxy.ActivateResultRemote(id);
> + },
> +diff --git a/js/ui/search.js b/js/ui/search.js
> +index f6fa719..fb2a021 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;
Yes, I just added another more readable variable to be more consistent with
what was done already, but if you prefer I can go with that.
> + this.provider.getResultMetas(metasNeeded, metas => {
> ++this.provider.resultsMetasInProgress = false;
> + if (this._cancellable.is_cancelled()) {
> + callback(false);
> + return;
> +@@ -453,6 +455,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();
> +@@ -494,11 +500,29 @@ var SearchResults = new Lang.Class({
> + }
> + },
> +
> ++_cancelSearchProviderRequest() {
> ++if (this._terms.length
Re: [Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
Diff comments:
> 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 000..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?=
> +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 @@
> +
> +
> +
> ++
> ++
> ++
> +
> +
> +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 @@
> +
> +
> +
> ++
> ++
> ++
> +
> +
> +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 = `
> +
> +
> +
> ++
> +
> + `;
> +
> +@@ -59,6 +60,7 @@ const SearchProvider2Iface = `
> +
> +
> +
> ++
> +
> + `;
> +
> +@@ -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();
Eventually on your suggestion about using _cancellable, I had to bind this...
Otherwise we were not stopping a remote operation that was cancelled locally.
However, I can't just get rid of resultsMetasInProgress as I don't want to
reset the cancellable outside this scope, and I want also to catch the cases
where the operation is still happening and not cancelled yet.
> + 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
Re: [Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-3.29.91 into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
Review: Needs Fixing
See my comment for the dep, the rest looks good to me :)
Diff comments:
> diff --git a/debian/control b/debian/control
> index 9e7f7c0..1291b71 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -89,18 +86,22 @@ Depends: ${gir:Depends},
> gir1.2-soup-2.4 (>= 2.40.1),
> gir1.2-upowerglib-1.0 (>= 0.99),
> gjs (>= 1.47.90),
> + gnome-backgrounds (>= 3.13.90),
Changelog mentions:
++ Replace gnome-backgrounds dep with ubuntu-wallpapers and Suggests
+ gnome-themes-standard-data, gnome-backgrounds
It does seem you readded gnome-backgrounds dep here?
> gnome-settings-daemon (>= 3.16.0),
> gnome-shell-common (= ${source:Version}),
> ubuntu-wallpapers,
> gsettings-desktop-schemas (>= 3.27.90),
> - mutter (>= 3.29.90),
> - python3,
> libglib2.0-bin (>= 2.53.0),
> + mutter (>= 3.29.91),
> + python3,
> + ${gir:Depends},
> + ${misc:Depends},
> + ${shlibs:Depends}
> Recommends: bolt (>= 0.3),
> +gdm3 (>= 3.10.0.1-3~),
> gkbd-capplet,
> gnome-control-center (>= 1:3.25.2),
> gnome-user-docs,
> -gdm3 (>= 3.10.0.1-3~),
> iio-sensor-proxy,
> switcheroo-control,
> ubuntu-session | gnome-session,
--
https://code.launchpad.net/~3v1n0/ubuntu/+source/gnome-shell/+git/gnome-shell/+merge/353823
Your team Ubuntu Desktop is subscribed to branch
~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master.
--
ubuntu-desktop mailing list
ubuntu-desktop@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-desktop
Re: [Merge] ~3v1n0/ubuntu/+source/gnome-calculator:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-calculator:ubuntu/master
Review: Needs Fixing So I have some requested changes and questions, but the code itself seems mostly OK. Note that I'm not a vala expert and would appreciate, even if the upstream patches are going to land in 3.32 (as per upstream MR comment) that you request still a quick review in your last set of patch. Do you think this is doable? See my comments about cosmic/changelog version. and oh, you are making me reviewing some vala code, this is mean of you ;) Diff comments: > diff --git a/debian/changelog b/debian/changelog > index 921715f..20249df 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -1,33 +1,24 @@ > -gnome-calculator (1:3.29.91-1ubuntu1) cosmic; urgency=medium > +gnome-calculator (1:3.28.1-1ubuntu2) UNRELEASED; urgency=medium We want this for cosmic, correct and this isn't the SRU version (as we need the fix for cosmic first and it seems the "proper" solution won't be in 3.30 but 3.32). So, shouldn't the changelog reflects the cosmic version + your patch? All the rest (VCS branch targeted and debian-branch=ubuntu/master in gbp.conf) seems to look like it's for cosmic… > > - * Sync with Debian (lp: #1785802). Remaining change: > -- Add epoch > - > - -- Sebastien Bacher Tue, 14 Aug 2018 17:03:08 +0200 > - > -gnome-calculator (3.29.91-1) experimental; urgency=medium > + * d/p/search-provider-Use-async-calls-cancel-search-on-inactivi.patch, > +d/p/search-provider-renew-inactivity-timeout-at-each-calculat.patch, > +d/p/search-provider-Use-lower-inactivity-timeout.patch, > +d/p/search-provider-simplify-solve_subprocess.patch, > +d/p/search-provider-cache-equations-avoiding-spawning-calcula.patch, > +d/p/search-provider-cache-only-a-limited-number-of-equations.patch, > +d/p/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch: > +- Make search provider async and support XUbuntuCancel method to > + stop expensive operations that might lead to an unresponsive > + process (LP: #1756826) > > - * New upstream version > + -- Marco Trevisan (Treviño) Tue, 28 Aug 2018 04:17:51 > +0200 > > - -- Sebastien Bacher Tue, 14 Aug 2018 16:42:48 +0200 > - > -gnome-calculator (3.29.90-1) experimental; urgency=medium > - > - * New upstream release > - * debian/control*, gbp.conf: Update for experimental (3.29 series) > +gnome-calculator (1:3.28.1-1ubuntu1) bionic; urgency=medium > > - -- Andrea Azzarone Tue, 07 Aug 2018 > 13:17:23 +0300 > - > -gnome-calculator (3.28.2-1) unstable; urgency=medium > - > - [ Andrea Azzarone ] > - * New upstream release > - > - [ Iain Lane ] > - * Set Rules-Requires-Root to "no" - we don't need to be root to build. > - * Set Standards-Version to 4.1.5 > + * Sync with Debian. Remaining change: > +- Add epoch > > - -- Andrea Azzarone Fri, 27 Jul 2018 > 22:13:29 +0200 > + -- Jeremy Bicha Tue, 10 Apr 2018 14:33:47 -0400 > > gnome-calculator (3.28.1-1) unstable; urgency=medium > > diff --git a/debian/control.in b/debian/control.in > index deb4c72..522dc74 100644 > --- a/debian/control.in > +++ b/debian/control.in > @@ -16,11 +16,12 @@ Build-Depends: appstream-util, > libsoup2.4-dev (>= 2.42), > libmpc-dev, > libmpfr-dev > -Standards-Version: 4.1.5 > -Vcs-Browser: https://salsa.debian.org/gnome-team/gnome-calculator > -Vcs-Git: https://salsa.debian.org/gnome-team/gnome-calculator.git -b > debian/experimental > +Standards-Version: 4.1.4 Downgrading Standards-Version? > +XS-Debian-Vcs-Browser: https://salsa.debian.org/gnome-team/gnome-calculator > +XS-Debian-Vcs-Git: https://salsa.debian.org/gnome-team/gnome-calculator.git > +Vcs-Browser: > https://git.launchpad.net/~ubuntu-desktop/ubuntu/+source/gnome-calculator > +Vcs-Git: > https://git.launchpad.net/~ubuntu-desktop/ubuntu/+source/gnome-calculator > Homepage: https://wiki.gnome.org/Apps/Calculator > -Rules-Requires-Root: no > > Package: gnome-calculator > Architecture: any > diff --git > a/debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch > > b/debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch > new file mode 100644 > index 000..a371dc8 > --- /dev/null > +++ > b/debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch > @@ -0,0 +1,232 @@ > +From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= > +Date: Fri, 24 Aug 2018 06:57:03 +0200 > +Subject: search-provider: Use async calls, cancel search on inactivity > + > +Move to async methods everywhere and factorize the `solve` calls to a single > +method that only uses Subprocess and that can be cancelled. > + > +As per this, if the the search-provider is trying to compute some complex > +operation, the daemon won't hang and once the applications' inactivity > +timeout is hit, any running instance of gnome-calculator will be killed and > +the daemon will return accordingly. > + > +This reduces the impact of an issue that can cause
Re: [Merge] ~3v1n0/ubuntu/+source/gnome-calculator:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-calculator:ubuntu/master
It looks rebasing with ubuntu/master created some troubles here (as I fixed in
another branch and I didn't use it when creating the MP :)), anyway let me
repush it with some other changes too.
While I've asked upstream about a first quick view.
Diff comments:
> diff --git
> a/debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch
>
> b/debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch
> new file mode 100644
> index 000..a371dc8
> --- /dev/null
> +++
> b/debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch
> @@ -0,0 +1,232 @@
> +From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?=
> +Date: Fri, 24 Aug 2018 06:57:03 +0200
> +Subject: search-provider: Use async calls, cancel search on inactivity
> +
> +Move to async methods everywhere and factorize the `solve` calls to a single
> +method that only uses Subprocess and that can be cancelled.
> +
> +As per this, if the the search-provider is trying to compute some complex
> +operation, the daemon won't hang and once the applications' inactivity
> +timeout is hit, any running instance of gnome-calculator will be killed and
> +the daemon will return accordingly.
> +
> +This reduces the impact of an issue that can cause gnome-calculator to keep
> +running forever if a complex computation is required (10!!!) from the shell,
> +and won't ever be killed (see GNOME/gnome-shell#183).
> +
> +This is also a prerequisite for supporting the search Cancellation that is
> +going to be available on next version of the Search provider protocol
> +(as per GNOME/gnome-shell#436)
> +
> +Bug-Ubuntu: https://launchpad.net/bugs/1756826
> +Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
> +Forwarded: yes,
> https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
> +---
> + search-provider/search-provider.vala | 126
> +--
> + 1 file changed, 89 insertions(+), 37 deletions(-)
> +
> +diff --git a/search-provider/search-provider.vala
> b/search-provider/search-provider.vala
> +index 26c72af..659f73d 100644
> +--- a/search-provider/search-provider.vala
> b/search-provider/search-provider.vala
> +@@ -1,6 +1,7 @@
> + /* -*- Mode: vala; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4
> -*-
> + *
> + * Copyright (C) 2014 Michael Catanzaro
> ++ * Copyright (C) 2018 Marco Trevisan
> + *
> + * This program is free software: you can redistribute it and/or modify it
> under
> + * the terms of the GNU General Public License as published by the Free
> Software
> +@@ -12,17 +13,85 @@
> + [DBus (name = "org.gnome.Shell.SearchProvider2")]
> + public class SearchProvider : Object
> + {
> ++private Cancellable cancellable = new Cancellable ();
> ++
> ++~SearchProvider ()
> ++{
> ++cancel ();
> ++}
> ++
> ++[DBus (visible = false)]
> ++public void cancel ()
> ++{
> ++if (cancellable != null)
> ++{
> ++cancellable.cancel ();
> ++}
> ++}
> ++
> + private static string terms_to_equation (string[] terms)
> + {
> + return string.joinv (" ", terms);
> + }
> +
> +-private static bool can_parse (string[] terms)
> ++private async Subprocess solve_subprocess (string equation, bool
> return_solution = false, out string? solution_buf = null) throws Error
> + {
> ++Subprocess subprocess;
> ++string[] argv = {"gnome-calculator", "--solve"};
> ++argv += equation;
> ++argv += null;
> ++
> ++debug (@"Trying to solve $(equation)");
> ++
> + try
> + {
> +-int exit_status;
> ++// Eat output so that it doesn't wind up in the journal. It's
> ++// expected that most searches are not valid calculator input.
> ++var flags = SubprocessFlags.STDERR_PIPE;
> ++
> ++if (return_solution)
> ++{
> ++flags |= SubprocessFlags.STDOUT_PIPE;
> ++}
> ++
> ++subprocess = new Subprocess.newv (argv, flags);
> ++}
> ++catch (Error e)
> ++{
> ++error ("Failed to spawn Calculator: %s", e.message);
> ++}
> ++
> ++try
> ++{
> ++string stderr_buf;
> ++
> ++cancellable = new Cancellable ();
> ++cancellable.cancelled.connect (() => {
> ++subprocess.force_exit ();
> ++cancellable = null;
> ++});
> ++
> ++yield subprocess.communicate_utf8_async (null, cancellable, out
> solution_buf, out stderr_buf);
> ++}
> ++catch (Error e)
> ++{
> ++if (e is IOError.CANCELLED)
> ++{
> ++throw e;
> ++}
> ++else
> ++{
> ++error ("Failed reading result: %s", e.message);
> ++}
> ++}
> +
> ++return subprocess;
> ++}
> ++
> ++
Re: [Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
Diff comments:
> diff --git
> a/debian/patches/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.patch
>
> b/debian/patches/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.patch
> new file mode 100644
> index 000..de98f8e
> --- /dev/null
> +++
> b/debian/patches/ubuntu/search-call-Cancel-method-on-providers-when-no-data-is-ne.patch
> @@ -0,0 +1,160 @@
> +From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?=
> +Date: Thu, 23 Aug 2018 20:00:57 +0200
> +Subject: search: call Cancel 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.
> +
> +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 | 13 +
> + js/ui/search.js | 31 +++
> + 4 files changed, 56 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 @@
> +
> +
> +
> ++
> ++
> ++
> +
> +
> +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 @@
> +
> +
> +
> ++
> ++
> ++
> +
> +
> +diff --git a/js/ui/remoteSearch.js b/js/ui/remoteSearch.js
> +index c6c5a29..79daba1 100644
> +--- a/js/ui/remoteSearch.js
> b/js/ui/remoteSearch.js
> +@@ -30,6 +30,7 @@ const SearchProviderIface = ' \
> + \
> + \
> + \
> ++ \
> + \
> + ';
> +
> +@@ -57,6 +58,7 @@ const SearchProvider2Iface = ' \
> + \
> + \
> + \
> ++ \
> + \
> + ';
> +
> +@@ -310,6 +312,17 @@ var RemoteSearchProvider = new Lang.Class({
> + cancellable);
> + },
> +
> ++XUbuntuCancel(cancellable) {
> ++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)));
> ++}
> ++},
> ++cancellable);
> ++},
> ++
> + activateResult(id) {
> + this.proxy.ActivateResultRemote(id);
> + },
> +diff --git a/js/ui/search.js b/js/ui/search.js
> +index f6fa719..fb2a021 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;
Also... No, because the cancellable could not been cancellable when the
provider is initialized, thus causing a false positive.
> + this.provider.getResultMetas(metasNeeded, metas => {
> ++this.provider.resultsMetasInProgress = false;
> + if (this._cancellable.is_cancelled()) {
> + callback(false);
> + return;
> +@@ -453,6 +455,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();
> +@@ -494,11 +500,29 @@ 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);
> ++}
> ++});
> ++
> ++delete this._searchCancelTimeoutId;
> ++return
Re: [Merge] ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/master-3.29.91 into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master
> See my comment for the dep, the rest looks good to me :) Ack, good catch... Sorry, the merge made me loose track of it. Thanks (and fixed) -- https://code.launchpad.net/~3v1n0/ubuntu/+source/gnome-shell/+git/gnome-shell/+merge/353823 Your team Ubuntu Desktop is subscribed to branch ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/master. -- ubuntu-desktop mailing list ubuntu-desktop@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-desktop
Re: [Merge] ~binli/ubuntu/+source/gdm3:disable-wayland-for-huawei-hi1710 into ~ubuntu-desktop/ubuntu/+source/gdm3:ubuntu/master
LGTM -- https://code.launchpad.net/~binli/ubuntu/+source/gdm3/+git/gdm3/+merge/353981 Your team Ubuntu Desktop is requested to review the proposed merge of ~binli/ubuntu/+source/gdm3:disable-wayland-for-huawei-hi1710 into ~ubuntu-desktop/ubuntu/+source/gdm3:ubuntu/master. -- ubuntu-desktop mailing list ubuntu-desktop@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-desktop
Re: [Merge] ~3v1n0/ubuntu/+source/nautilus:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/nautilus:ubuntu/master
Review: Needs Information
This looks mostly good, I have some nitpicks and some questions (see below and
inline diff):
* You didn't metnion at all about the ignore_partial_results which leads to 2
different conditions (we want to ignore them or not). I think that ought some
mention in the patch description.
* I'm unsure to understand what the meta_requests are and why they are treated
differently (and unconditionnally). Is it a queue before them becoming the
current requests, and this is why you are cancelling if the invocation caller
matches as well?
Diff comments:
> diff --git
> a/debian/patches/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch
>
> b/debian/patches/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch
> new file mode 100644
> index 000..ca55544
> --- /dev/null
> +++
> b/debian/patches/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch
> @@ -0,0 +1,212 @@
> +From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?=
> +Date: Tue, 28 Aug 2018 01:44:49 +0200
> +Subject: shell-search-provider: implement XUbuntuCancel to request search
> cancellation
> +
> +Stop search and Metadata fetching on XUbuntuCancel dbus method call.
> +Only allow this if the caller is the same who triggered the actual event.
> +
> +Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
> +Bug-Ubuntu:
> https://bugs.launchpad.net/ubuntu/bionic/+source/nautilus/+bug/1756826
> +Forwarded: not-needed
> +---
> + data/shell-search-provider-dbus-interfaces.xml | 1 +
> + src/nautilus-shell-search-provider.c | 96
> +++---
> + 2 files changed, 88 insertions(+), 9 deletions(-)
> +
> +diff --git a/data/shell-search-provider-dbus-interfaces.xml
> b/data/shell-search-provider-dbus-interfaces.xml
> +index f6840e2..4529c1e 100644
> +--- a/data/shell-search-provider-dbus-interfaces.xml
> b/data/shell-search-provider-dbus-interfaces.xml
> +@@ -40,5 +40,6 @@
> +
> +
> +
> ++
> +
> +
> +diff --git a/src/nautilus-shell-search-provider.c
> b/src/nautilus-shell-search-provider.c
> +index ffc2b7f..58864d6 100644
> +--- a/src/nautilus-shell-search-provider.c
> b/src/nautilus-shell-search-provider.c
> +@@ -60,6 +60,7 @@ struct _NautilusShellSearchProvider
> +
> + PendingSearch *current_search;
> +
> ++GList *metas_requests;
> + GHashTable *metas_cache;
> + };
> +
> +@@ -143,11 +144,25 @@ pending_search_finish (PendingSearch *search,
> + }
> +
> + static void
> +-cancel_current_search (NautilusShellSearchProvider *self)
> ++cancel_current_search (NautilusShellSearchProvider *self,
> ++ gboolean ignore_partial_results)
> + {
> +-if (self->current_search != NULL)
> ++PendingSearch *search = self->current_search;
> ++
> ++if (search != NULL)
> + {
> +-nautilus_search_provider_stop (NAUTILUS_SEARCH_PROVIDER
> (self->current_search->engine));
> ++g_debug ("*** Cancel current search");
> ++
> ++nautilus_search_provider_stop (NAUTILUS_SEARCH_PROVIDER
> (search->engine));
> ++
> ++if (ignore_partial_results)
> ++{
> ++g_signal_handlers_disconnect_by_data (G_OBJECT (search->engine),
> ++ search);
> ++
> ++pending_search_finish (search, search->invocation,
> ++ g_variant_new ("(as)", NULL));
> ++}
> + }
> + }
> +
> +@@ -451,7 +466,7 @@ execute_search (NautilusShellSearchProvider *self,
> + NautilusQuery *query;
> + PendingSearch *pending_search;
> +
> +-cancel_current_search (self);
> ++cancel_current_search (self, FALSE);
> +
> + /* don't attempt searches for a single character */
> + if (g_strv_length (terms) == 1 &&
> +@@ -524,6 +539,7 @@ typedef struct
> + NautilusShellSearchProvider *self;
> +
> + gint64 start_time;
> ++NautilusFileListHandle *handle;
> + GDBusMethodInvocation *invocation;
> +
> + gchar **uris;
> +@@ -532,6 +548,8 @@ typedef struct
> + static void
> + result_metas_data_free (ResultMetasData *data)
> + {
> ++g_clear_pointer (>handle,
> nautilus_file_list_cancel_call_when_ready);
> ++
> + g_clear_object (>self);
> + g_clear_object (>invocation);
> + g_strfreev (data->uris);
> +@@ -549,7 +567,7 @@ result_metas_return_from_cache (ResultMetasData *data)
> +
> + g_variant_builder_init (, G_VARIANT_TYPE ("aa{sv}"));
> +
> +-for (idx = 0; data->uris[idx] != NULL; idx++)
> ++for (idx = 0; data->uris && data->uris[idx] != NULL; idx++)
Shoudn't we do the data->uris check before the loop, or is there any
concurrency race that could change it an nullify it while this loop is running?
> + {
> + meta = g_hash_table_lookup (data->self->metas_cache,
> + data->uris[idx]);
> +@@ -564,6 +582,37 @@ result_metas_return_from_cache
