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

Proposed by Marco Trevisan (Treviño) on 2018-08-28
Status: Merged
Approved by: Didier Roche on 2018-09-03
Approved revision: e4f04914def636229990bc1860e97613485129e1
Merged at revision: 979a68b437973554094ad1499e0141658c07864b
Proposed branch: ~3v1n0/ubuntu/+source/nautilus:ubuntu/master-xubuntu-cancel-search
Merge into: ~ubuntu-desktop/ubuntu/+source/nautilus:ubuntu/master
Diff against target: 259 lines (+237/-0)
3 files modified
debian/changelog (+8/-0)
debian/patches/series (+1/-0)
debian/patches/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch (+228/-0)
Reviewer Review Type Date Requested Status
Didier Roche 2018-08-28 Approve on 2018-09-03
Review via email: mp+353826@code.launchpad.net

Description of the change

To post a comment you must log in.
Didier Roche (didrocks) wrote :

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?

review: Needs Information
Marco Trevisan (Treviño) (3v1n0) wrote :

> * 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.

Didier Roche (didrocks) wrote :

LGTM, thanks for addressing the issues and answering my questions :)

review: Approve
Marco Trevisan (Treviño) (3v1n0) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 3e72451..4ceff74 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,11 @@
6+nautilus (1:3.26.4-0ubuntu3) cosmic; urgency=medium
7+
8+ * d/p/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch:
9+ shell-search-provider: implement XUbuntuCancel to request pending
10+ search cancellation from gnome-shell (LP: #1756826)
11+
12+ -- Marco Trevisan (Treviño) <marco@ubuntu.com> Thu, 30 Aug 2018 18:15:24 -0500
13+
14 nautilus (1:3.26.4-0ubuntu2) cosmic; urgency=medium
15
16 * debian/patches/nautilusgtkplacesview-show-error-if-volume-is-not-mo.patch:
17diff --git a/debian/patches/series b/debian/patches/series
18index 41a35d1..61d3d3a 100644
19--- a/debian/patches/series
20+++ b/debian/patches/series
21@@ -24,3 +24,4 @@ query-add-recursive-flags-and-use-it-in-search-engines.patch
22 appstream-compulsory.patch
23 nautilusgtkplacesview-show-error-if-volume-is-not-mo.patch
24 file-view-Always-unset-pending_selection-after-freeing-it.patch
25+ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch
26diff --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
27new file mode 100644
28index 0000000..910d08f
29--- /dev/null
30+++ b/debian/patches/ubuntu/shell-search-provider-implement-XUbuntuCancel-to-request-.patch
31@@ -0,0 +1,228 @@
32+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
33+Date: Tue, 28 Aug 2018 01:44:49 +0200
34+Subject: shell-search-provider: implement XUbuntuCancel to request search
35+ cancellation
36+
37+Stop search and Metadata fetching on XUbuntuCancel dbus method call.
38+Only allow this if the caller is the same who triggered the actual event.
39+
40+To implement this redefine `cancel_current_search` with `ignore_partial_results`
41+parameter so that when this is TRUE we won't return any value to the caller.
42+This is the case on disposal and when the XUbuntuCancel has been requested,
43+to avoid some uneeded traffic.
44+
45+Keep track of all the pending metadata requests in `metas_requests` and
46+use it to cancel the requests if requested.
47+
48+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
49+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/nautilus/+bug/1756826
50+Forwarded: not-needed
51+---
52+ data/shell-search-provider-dbus-interfaces.xml | 1 +
53+ src/nautilus-shell-search-provider.c | 101 +++++++++++++++++++++++--
54+ 2 files changed, 95 insertions(+), 7 deletions(-)
55+
56+diff --git a/data/shell-search-provider-dbus-interfaces.xml b/data/shell-search-provider-dbus-interfaces.xml
57+index f6840e2..4529c1e 100644
58+--- a/data/shell-search-provider-dbus-interfaces.xml
59++++ b/data/shell-search-provider-dbus-interfaces.xml
60+@@ -40,5 +40,6 @@
61+ <arg type='as' name='Terms' direction='in' />
62+ <arg type='u' name='Timestamp' direction='in' />
63+ </method>
64++ <method name = 'XUbuntuCancel' />
65+ </interface>
66+ </node>
67+diff --git a/src/nautilus-shell-search-provider.c b/src/nautilus-shell-search-provider.c
68+index ffc2b7f..535436c 100644
69+--- a/src/nautilus-shell-search-provider.c
70++++ b/src/nautilus-shell-search-provider.c
71+@@ -60,6 +60,7 @@ struct _NautilusShellSearchProvider
72+
73+ PendingSearch *current_search;
74+
75++ GList *metas_requests;
76+ GHashTable *metas_cache;
77+ };
78+
79+@@ -143,11 +144,25 @@ pending_search_finish (PendingSearch *search,
80+ }
81+
82+ static void
83+-cancel_current_search (NautilusShellSearchProvider *self)
84++cancel_current_search (NautilusShellSearchProvider *self,
85++ gboolean ignore_partial_results)
86+ {
87+- if (self->current_search != NULL)
88++ PendingSearch *search = self->current_search;
89++
90++ if (search != NULL)
91+ {
92+- nautilus_search_provider_stop (NAUTILUS_SEARCH_PROVIDER (self->current_search->engine));
93++ g_debug ("*** Cancel current search");
94++
95++ nautilus_search_provider_stop (NAUTILUS_SEARCH_PROVIDER (search->engine));
96++
97++ if (ignore_partial_results)
98++ {
99++ g_signal_handlers_disconnect_by_data (G_OBJECT (search->engine),
100++ search);
101++
102++ pending_search_finish (search, search->invocation,
103++ g_variant_new ("(as)", NULL));
104++ }
105+ }
106+ }
107+
108+@@ -451,7 +466,7 @@ execute_search (NautilusShellSearchProvider *self,
109+ NautilusQuery *query;
110+ PendingSearch *pending_search;
111+
112+- cancel_current_search (self);
113++ cancel_current_search (self, FALSE);
114+
115+ /* don't attempt searches for a single character */
116+ if (g_strv_length (terms) == 1 &&
117+@@ -524,6 +539,7 @@ typedef struct
118+ NautilusShellSearchProvider *self;
119+
120+ gint64 start_time;
121++ NautilusFileListHandle *handle;
122+ GDBusMethodInvocation *invocation;
123+
124+ gchar **uris;
125+@@ -532,6 +548,7 @@ typedef struct
126+ static void
127+ result_metas_data_free (ResultMetasData *data)
128+ {
129++ g_clear_pointer (&data->handle, nautilus_file_list_cancel_call_when_ready);
130+ g_clear_object (&data->self);
131+ g_clear_object (&data->invocation);
132+ g_strfreev (data->uris);
133+@@ -549,7 +566,7 @@ result_metas_return_from_cache (ResultMetasData *data)
134+
135+ g_variant_builder_init (&builder, G_VARIANT_TYPE ("aa{sv}"));
136+
137+- for (idx = 0; data->uris[idx] != NULL; idx++)
138++ for (idx = 0; data->uris && data->uris[idx] != NULL; idx++)
139+ {
140+ meta = g_hash_table_lookup (data->self->metas_cache,
141+ data->uris[idx]);
142+@@ -564,6 +581,45 @@ result_metas_return_from_cache (ResultMetasData *data)
143+ g_variant_new ("(aa{sv})", &builder));
144+ }
145+
146++static void
147++result_metas_return_empty (ResultMetasData *data)
148++{
149++ g_clear_pointer (&data->uris, g_strfreev);
150++ result_metas_return_from_cache (data);
151++ result_metas_data_free (data);
152++}
153++
154++static void
155++cancel_result_meta_requests (NautilusShellSearchProvider *self,
156++ GDBusMethodInvocation *invocation)
157++{
158++ g_debug ("*** Cancel Results Meta requests");
159++
160++ if (invocation == NULL)
161++ {
162++ g_list_free_full (self->metas_requests,
163++ (GDestroyNotify) result_metas_return_empty);
164++ self->metas_requests = NULL;
165++ }
166++ else
167++ {
168++ GList *l, *next = NULL;
169++
170++ for (l = self->metas_requests; l != NULL; l = next)
171++ {
172++ next = l->next;
173++ ResultMetasData *data = l->data;
174++
175++ if (g_strcmp0 (g_dbus_method_invocation_get_sender (data->invocation),
176++ g_dbus_method_invocation_get_sender (invocation)) == 0)
177++ {
178++ result_metas_return_empty (data);
179++ self->metas_requests = g_list_delete_link (self->metas_requests, l);
180++ }
181++ }
182++ }
183++}
184++
185+ static void
186+ result_list_attributes_ready_cb (GList *file_list,
187+ gpointer user_data)
188+@@ -639,6 +695,9 @@ result_list_attributes_ready_cb (GList *file_list,
189+ g_free (uri);
190+ }
191+
192++ data->handle = NULL;
193++ data->self->metas_requests = g_list_remove (data->self->metas_requests, data);
194++
195+ result_metas_return_from_cache (data);
196+ result_metas_data_free (data);
197+ }
198+@@ -682,9 +741,10 @@ handle_get_result_metas (NautilusShellSearchProvider2 *skeleton,
199+
200+ nautilus_file_list_call_when_ready (missing_files,
201+ NAUTILUS_FILE_ATTRIBUTES_FOR_ICON,
202+- NULL,
203++ &data->handle,
204+ result_list_attributes_ready_cb,
205+ data);
206++ self->metas_requests = g_list_prepend (self->metas_requests, data);
207+ nautilus_file_list_free (missing_files);
208+ return TRUE;
209+ }
210+@@ -743,6 +803,30 @@ handle_launch_search (NautilusShellSearchProvider2 *skeleton,
211+ return TRUE;
212+ }
213+
214++static gboolean
215++handle_xubuntu_cancel (NautilusShellSearchProvider2 *skeleton,
216++ GDBusMethodInvocation *invocation,
217++ gpointer user_data)
218++{
219++ NautilusShellSearchProvider *self = user_data;
220++ PendingSearch *search = self->current_search;
221++
222++ g_debug ("*** XUbuntuCancel called");
223++
224++ if (search != NULL &&
225++ g_strcmp0 (g_dbus_method_invocation_get_sender (search->invocation),
226++ g_dbus_method_invocation_get_sender (invocation)) == 0)
227++ {
228++ cancel_current_search (self, TRUE);
229++ }
230++
231++ cancel_result_meta_requests (self, invocation);
232++
233++ nautilus_shell_search_provider2_complete_xubuntu_cancel (skeleton, invocation);
234++
235++ return TRUE;
236++}
237++
238+ static void
239+ search_provider_dispose (GObject *obj)
240+ {
241+@@ -750,7 +834,8 @@ search_provider_dispose (GObject *obj)
242+
243+ g_clear_object (&self->skeleton);
244+ g_hash_table_destroy (self->metas_cache);
245+- cancel_current_search (self);
246++ cancel_current_search (self, TRUE);
247++ cancel_result_meta_requests (self, NULL);
248+
249+ G_OBJECT_CLASS (nautilus_shell_search_provider_parent_class)->dispose (obj);
250+ }
251+@@ -773,6 +858,8 @@ nautilus_shell_search_provider_init (NautilusShellSearchProvider *self)
252+ G_CALLBACK (handle_activate_result), self);
253+ g_signal_connect (self->skeleton, "handle-launch-search",
254+ G_CALLBACK (handle_launch_search), self);
255++ g_signal_connect (self->skeleton, "handle-xubuntu-cancel",
256++ G_CALLBACK (handle_xubuntu_cancel), self);
257+ }
258+
259+ static void

Subscribers

People subscribed via source and target branches