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?= <m...@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. -- ubuntu-desktop mailing list ubuntu-desktop@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-desktop
