[Merge] ~3v1n0/ubuntu/+source/nautilus:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/nautilus:ubuntu/master
Didier Roche
didrocks at ubuntu.com
Thu Aug 30 08:50:14 UTC 2018
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 0000000..ca55544
> --- /dev/null
> +++ b/debian/patches/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch
> @@ -0,0 +1,212 @@
> +From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail at 3v1n0.net>
> +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 @@
> + <arg type='as' name='Terms' direction='in' />
> + <arg type='u' name='Timestamp' direction='in' />
> + </method>
> ++ <method name = 'XUbuntuCancel' />
> + </interface>
> + </node>
> +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 (&data->handle, nautilus_file_list_cancel_call_when_ready);
> ++
> + g_clear_object (&data->self);
> + g_clear_object (&data->invocation);
> + g_strfreev (data->uris);
> +@@ -549,7 +567,7 @@ result_metas_return_from_cache (ResultMetasData *data)
> +
> + g_variant_builder_init (&builder, 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 (ResultMetasData *data)
> + g_variant_new ("(aa{sv})", &builder));
> + }
> +
> ++static void
> ++cancel_result_meta_requests (NautilusShellSearchProvider *self,
> ++ GDBusMethodInvocation *invocation)
> ++{
> ++ GList *l;
> ++ GList *to_remove = NULL;
> ++
> ++ g_debug ("*** Cancel Results Meta requests");
> ++
> ++ for (l = self->metas_requests; l; l = l->next)
> ++ {
> ++ ResultMetasData *data = l->data;
> ++
> ++ if (invocation == NULL ||
> ++ g_strcmp0 (g_dbus_method_invocation_get_sender (data->invocation),
> ++ g_dbus_method_invocation_get_sender (invocation)) == 0)
> ++ {
> ++ g_clear_pointer (&data->uris, g_strfreev);
> ++ result_metas_return_from_cache (data);
> ++ to_remove = g_list_prepend (to_remove, data);
> ++ }
> ++ }
> ++
> ++ for (l = to_remove; l; l = l->next)
> ++ {
> ++ self->metas_requests = g_list_remove (self->metas_requests, l->data);
> ++ }
> ++
> ++ g_list_free (to_remove);
> ++}
> ++
> + static void
> + result_list_attributes_ready_cb (GList *file_list,
> + gpointer user_data)
> +@@ -639,6 +688,9 @@ result_list_attributes_ready_cb (GList *file_list,
> + g_free (uri);
> + }
> +
> ++ data->handle = NULL;
> ++ data->self->metas_requests = g_list_remove (data->self->metas_requests, data);
> ++
> + result_metas_return_from_cache (data);
> + result_metas_data_free (data);
> + }
> +@@ -682,9 +734,10 @@ handle_get_result_metas (NautilusShellSearchProvider2 *skeleton,
> +
> + nautilus_file_list_call_when_ready (missing_files,
> + NAUTILUS_FILE_ATTRIBUTES_FOR_ICON,
> +- NULL,
> ++ &data->handle,
> + result_list_attributes_ready_cb,
> + data);
> ++ self->metas_requests = g_list_prepend (self->metas_requests, data);
> + nautilus_file_list_free (missing_files);
> + return TRUE;
> + }
> +@@ -743,14 +796,37 @@ handle_launch_search (NautilusShellSearchProvider2 *skeleton,
> + return TRUE;
> + }
> +
> +-static void
> +-search_provider_dispose (GObject *obj)
> ++static gboolean
> ++handle_xubuntu_cancel (NautilusShellSearchProvider2 *skeleton,
> ++ GDBusMethodInvocation *invocation,
> ++ gpointer user_data)
> + {
> ++ NautilusShellSearchProvider *self = user_data;
> ++ PendingSearch *search = self->current_search;
> ++
> ++ g_debug ("*** XUbuntuCancel called");
> ++
> ++ if (search != NULL &&
> ++ g_strcmp0 (g_dbus_method_invocation_get_sender (search->invocation),
> ++ g_dbus_method_invocation_get_sender (invocation)) == 0)
> ++ {
> ++ cancel_current_search (self, TRUE);
> ++ }
> ++
> ++ cancel_result_meta_requests (self, invocation);
> ++
> ++ nautilus_shell_search_provider2_complete_xubuntu_cancel (skeleton, invocation);
> ++
> ++ return TRUE;
> ++}
> ++
> ++ static void search_provider_dispose(GObject * obj) {
Maybe keep it in 2 lines like the original code (static code / search_provider…) to minimize the diff?
> + NautilusShellSearchProvider *self = NAUTILUS_SHELL_SEARCH_PROVIDER (obj);
> +
> + g_clear_object (&self->skeleton);
> + g_hash_table_destroy (self->metas_cache);
> +- cancel_current_search (self);
> ++ cancel_current_search (self, TRUE);
> ++ cancel_result_meta_requests (self, NULL);
> +
> + G_OBJECT_CLASS (nautilus_shell_search_provider_parent_class)->dispose (obj);
> + }
> +@@ -773,6 +849,8 @@ nautilus_shell_search_provider_init (NautilusShellSearchProvider *self)
> + G_CALLBACK (handle_activate_result), self);
> + g_signal_connect (self->skeleton, "handle-launch-search",
> + G_CALLBACK (handle_launch_search), self);
> ++ g_signal_connect (self->skeleton, "handle-xubuntu-cancel",
> ++ G_CALLBACK (handle_xubuntu_cancel), self);
> + }
> +
> + static void
--
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.
More information about the ubuntu-desktop
mailing list