[Merge] ~3v1n0/ubuntu/+source/nautilus:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/nautilus:ubuntu/master
The proposal to merge ~3v1n0/ubuntu/+source/nautilus:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/nautilus:ubuntu/master has been updated. Status: Needs review => Approved For more details, see: https://code.launchpad.net/~3v1n0/ubuntu/+source/nautilus/+git/nautilus/+merge/353826 -- Your team Ubuntu Desktop is subscribed to branch ~ubuntu-desktop/ubuntu/+source/nautilus:ubuntu/master. -- ubuntu-desktop mailing list ubuntu-desktop@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-desktop
[Merge] ~3v1n0/ubuntu/+source/nautilus:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/nautilus:ubuntu/master
The proposal to merge ~3v1n0/ubuntu/+source/nautilus:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/nautilus:ubuntu/master has been updated. Status: Approved => Merged For more details, see: https://code.launchpad.net/~3v1n0/ubuntu/+source/nautilus/+git/nautilus/+merge/353826 -- Your team Ubuntu Desktop is subscribed to branch ~ubuntu-desktop/ubuntu/+source/nautilus: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
FYI I've submitted also part of this upstream (as it will be needed anyway for future developments): https://gitlab.gnome.org/GNOME/nautilus/merge_requests/303 -- https://code.launchpad.net/~3v1n0/ubuntu/+source/nautilus/+git/nautilus/+merge/353826 Your team Ubuntu Desktop is subscribed to branch ~ubuntu-desktop/ubuntu/+source/nautilus: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: Approve LGTM, thanks for addressing the issues and answering my questions :) -- https://code.launchpad.net/~3v1n0/ubuntu/+source/nautilus/+git/nautilus/+merge/353826 Your team Ubuntu Desktop is subscribed to branch ~ubuntu-desktop/ubuntu/+source/nautilus: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)
> +
>
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
