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

Proposed by Marco Trevisan (Treviño)
Status: Merged
Merged at revision: 1c45339f350ca659afe1ecb0c6827a3eac9777aa
Proposed branch: ~3v1n0/ubuntu/+source/gnome-calculator:ubuntu/master-xubuntu-cancel-search
Merge into: ~ubuntu-desktop/ubuntu/+source/gnome-calculator:ubuntu/master
Prerequisite: ~3v1n0/ubuntu/+source/gnome-calculator:ubuntu/master-3.29.91
Diff against target: 926 lines (+831/-2)
13 files modified
debian/changelog (+23/-0)
debian/control (+2/-1)
debian/control.in (+2/-1)
debian/gbp.conf (+1/-0)
debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch (+232/-0)
debian/patches/search-provider-Use-lower-inactivity-timeout.patch (+29/-0)
debian/patches/search-provider-cache-equations-avoiding-spawning-calcula.patch (+170/-0)
debian/patches/search-provider-cache-only-a-limited-number-of-equations.patch (+44/-0)
debian/patches/search-provider-cancel-the-current-process-on-new-calcula.patch (+29/-0)
debian/patches/search-provider-renew-inactivity-timeout-at-each-calculat.patch (+60/-0)
debian/patches/search-provider-simplify-solve_subprocess.patch (+113/-0)
debian/patches/series (+8/-0)
debian/patches/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch (+118/-0)
Reviewer Review Type Date Requested Status
Didier Roche-Tolomelli Approve
Review via email: mp+353828@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

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 ;)

review: Needs Fixing
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

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.

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

I didn't see any repush? or any MP fixing those?

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

From IRC:
Trevinho: https://code.launchpad.net/~3v1n0/ubuntu/+source/gnome-calculator/+git/gnome-calculator/+merge/353828/comments/921762 as well
Trevinho: have you just repushed another commit with the initial date staying the same? (hard to know with launchpad MP UI)
which sounds like from the diff
ah interesting, you can switch the diff and you see the the new diff was generated on the 30 while the commit is still on the 28… (but no evidence that you pushed a new
commit)
quite confusing, used to gitlab/github when you see something else was pushed and overrides

looks good to approve now then!

review: Approve

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 921715f..9bafaab 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,26 @@
6+gnome-calculator (1:3.29.91-1ubuntu3) UNRELEASED; urgency=medium
7+
8+ * debian/gbp.conf:
9+ - Add back debian-tag=ubuntu/%(version)s setting
10+
11+ -- Marco Trevisan (Treviño) <marco@ubuntu.com> Mon, 03 Sep 2018 16:46:28 +0200
12+
13+gnome-calculator (1:3.29.91-1ubuntu2) cosmic; urgency=medium
14+
15+ * d/p/search-provider-Use-async-calls-cancel-search-on-inactivi.patch,
16+ d/p/search-provider-renew-inactivity-timeout-at-each-calculat.patch,
17+ d/p/search-provider-Use-lower-inactivity-timeout.patch,
18+ d/p/search-provider-simplify-solve_subprocess.patch,
19+ d/p/search-provider-cache-equations-avoiding-spawning-calcula.patch,
20+ d/p/search-provider-cancel-the-current-process-on-new-calcula.patch,
21+ d/p/search-provider-cache-only-a-limited-number-of-equations.patch,
22+ d/p/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch:
23+ - Make search provider async and support XUbuntuCancel method to
24+ stop expensive operations that might lead to an unresponsive
25+ process (LP: #1756826)
26+
27+ -- Marco Trevisan (Treviño) <marco@ubuntu.com> Thu, 30 Aug 2018 12:57:51 -0500
28+
29 gnome-calculator (1:3.29.91-1ubuntu1) cosmic; urgency=medium
30
31 * Sync with Debian (lp: #1785802). Remaining change:
32diff --git a/debian/control b/debian/control
33index 68d049d..495681f 100644
34--- a/debian/control
35+++ b/debian/control
36@@ -5,7 +5,8 @@
37 Source: gnome-calculator
38 Section: math
39 Priority: optional
40-Maintainer: Debian GNOME Maintainers <pkg-gnome-maintainers@lists.alioth.debian.org>
41+Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
42+XSBC-Original-Maintainer: Debian GNOME Maintainers <pkg-gnome-maintainers@lists.alioth.debian.org>
43 Uploaders: Jeremy Bicha <jbicha@debian.org>, Laurent Bigonville <bigon@debian.org>, Michael Biebl <biebl@debian.org>
44 Build-Depends: appstream-util,
45 debhelper (>= 11.1.3),
46diff --git a/debian/control.in b/debian/control.in
47index deb4c72..8e9918e 100644
48--- a/debian/control.in
49+++ b/debian/control.in
50@@ -1,7 +1,8 @@
51 Source: gnome-calculator
52 Section: math
53 Priority: optional
54-Maintainer: Debian GNOME Maintainers <pkg-gnome-maintainers@lists.alioth.debian.org>
55+Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
56+XSBC-Original-Maintainer: Debian GNOME Maintainers <pkg-gnome-maintainers@lists.alioth.debian.org>
57 Uploaders: @GNOME_TEAM@
58 Build-Depends: appstream-util,
59 debhelper (>= 11.1.3),
60diff --git a/debian/gbp.conf b/debian/gbp.conf
61index c40187f..f6567cb 100644
62--- a/debian/gbp.conf
63+++ b/debian/gbp.conf
64@@ -1,5 +1,6 @@
65 [DEFAULT]
66 pristine-tar = True
67 debian-branch=debian/experimental
68+debian-tag=ubuntu/%(version)s
69 upstream-branch=upstream/latest
70 upstream-vcs-tag = %(version)s
71diff --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
72new file mode 100644
73index 0000000..324be46
74--- /dev/null
75+++ b/debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch
76@@ -0,0 +1,232 @@
77+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
78+Date: Fri, 24 Aug 2018 06:57:03 +0200
79+Subject: search-provider: Use async calls, cancel search on inactivity
80+
81+Move to async methods everywhere and factorize the `solve` calls to a single
82+method that only uses Subprocess and that can be cancelled.
83+
84+As per this, if the the search-provider is trying to compute some complex
85+operation, the daemon won't hang and once the applications' inactivity
86+timeout is hit, any running instance of gnome-calculator will be killed and
87+the daemon will return accordingly.
88+
89+This reduces the impact of an issue that can cause gnome-calculator to keep
90+running forever if a complex computation is required (10!!!) from the shell,
91+and won't ever be killed (see GNOME/gnome-shell#183).
92+
93+This is also a prerequisite for supporting the search Cancellation that is
94+going to be available on next version of the Search provider protocol
95+(as per GNOME/gnome-shell#436)
96+
97+Bug-Ubuntu: https://launchpad.net/bugs/1756826
98+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
99+Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
100+---
101+ search-provider/search-provider.vala | 126 +++++++++++++++++++++++++----------
102+ 1 file changed, 89 insertions(+), 37 deletions(-)
103+
104+diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
105+index 26c72af..659f73d 100644
106+--- a/search-provider/search-provider.vala
107++++ b/search-provider/search-provider.vala
108+@@ -1,6 +1,7 @@
109+ /* -*- Mode: vala; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
110+ *
111+ * Copyright (C) 2014 Michael Catanzaro
112++ * Copyright (C) 2018 Marco Trevisan
113+ *
114+ * This program is free software: you can redistribute it and/or modify it under
115+ * the terms of the GNU General Public License as published by the Free Software
116+@@ -12,17 +13,85 @@
117+ [DBus (name = "org.gnome.Shell.SearchProvider2")]
118+ public class SearchProvider : Object
119+ {
120++ private Cancellable cancellable = new Cancellable ();
121++
122++ ~SearchProvider ()
123++ {
124++ cancel ();
125++ }
126++
127++ [DBus (visible = false)]
128++ public void cancel ()
129++ {
130++ if (cancellable != null)
131++ {
132++ cancellable.cancel ();
133++ }
134++ }
135++
136+ private static string terms_to_equation (string[] terms)
137+ {
138+ return string.joinv (" ", terms);
139+ }
140+
141+- private static bool can_parse (string[] terms)
142++ private async Subprocess solve_subprocess (string equation, bool return_solution = false, out string? solution_buf = null) throws Error
143+ {
144++ Subprocess subprocess;
145++ string[] argv = {"gnome-calculator", "--solve"};
146++ argv += equation;
147++ argv += null;
148++
149++ debug (@"Trying to solve $(equation)");
150++
151+ try
152+ {
153+- int exit_status;
154++ // Eat output so that it doesn't wind up in the journal. It's
155++ // expected that most searches are not valid calculator input.
156++ var flags = SubprocessFlags.STDERR_PIPE;
157++
158++ if (return_solution)
159++ {
160++ flags |= SubprocessFlags.STDOUT_PIPE;
161++ }
162++
163++ subprocess = new Subprocess.newv (argv, flags);
164++ }
165++ catch (Error e)
166++ {
167++ error ("Failed to spawn Calculator: %s", e.message);
168++ }
169++
170++ try
171++ {
172++ string stderr_buf;
173++
174++ cancellable = new Cancellable ();
175++ cancellable.cancelled.connect (() => {
176++ subprocess.force_exit ();
177++ cancellable = null;
178++ });
179++
180++ yield subprocess.communicate_utf8_async (null, cancellable, out solution_buf, out stderr_buf);
181++ }
182++ catch (Error e)
183++ {
184++ if (e is IOError.CANCELLED)
185++ {
186++ throw e;
187++ }
188++ else
189++ {
190++ error ("Failed reading result: %s", e.message);
191++ }
192++ }
193+
194++ return subprocess;
195++ }
196++
197++ private async bool can_parse (string[] terms)
198++ {
199++ try
200++ {
201+ var tsep_string = Posix.nl_langinfo (Posix.NLItem.THOUSEP);
202+ if (tsep_string == null || tsep_string == "")
203+ tsep_string = " ";
204+@@ -39,14 +108,8 @@ public class SearchProvider : Object
205+ return false;
206+ }
207+
208+- // Eat output so that it doesn't wind up in the journal. It's
209+- // expected that most searches are not valid calculator input.
210+- string stdout_buf;
211+- string stderr_buf;
212+- Process.spawn_command_line_sync (
213+- "gnome-calculator --solve " + Shell.quote (equation),
214+- out stdout_buf, out stderr_buf, out exit_status);
215+- Process.check_exit_status (exit_status);
216++ var subprocess = yield solve_subprocess (equation);
217++ yield subprocess.wait_check_async ();
218+ }
219+ catch (SpawnError e)
220+ {
221+@@ -60,54 +123,37 @@ public class SearchProvider : Object
222+ return true;
223+ }
224+
225+- private static string[] get_result_identifier (string[] terms)
226+- ensures (result.length == 0 || result.length == 1)
227++ private async string[] get_result_identifier (string[] terms)
228+ {
229+ /* We have at most one result: the search terms as one string */
230+- if (can_parse (terms))
231++ if (yield can_parse (terms))
232+ return { terms_to_equation (terms) };
233+ else
234+ return new string[0];
235+ }
236+
237+- public string[] get_initial_result_set (string[] terms)
238++ public async string[] get_initial_result_set (string[] terms)
239+ {
240+- return get_result_identifier (terms);
241++ return yield get_result_identifier (terms);
242+ }
243+
244+- public string[] get_subsearch_result_set (string[] previous_results, string[] terms)
245++ public async string[] get_subsearch_result_set (string[] previous_results, string[] terms)
246+ {
247+- return get_result_identifier (terms);
248++ return yield get_result_identifier (terms);
249+ }
250+
251+- public HashTable<string, Variant>[] get_result_metas (string[] results)
252++ public async HashTable<string, Variant>[] get_result_metas (string[] results, GLib.BusName sender)
253+ requires (results.length == 1)
254+- ensures (result.length == 1)
255+ {
256+- Subprocess subprocess;
257+ string stdout_buf;
258+- string stderr_buf;
259+-
260+- string[] argv = {"gnome-calculator", "--solve"};
261+- argv += results[0];
262+- argv += null;
263+-
264+- try
265+- {
266+- subprocess = new Subprocess.newv (argv, SubprocessFlags.STDOUT_PIPE | SubprocessFlags.STDERR_PIPE);
267+- }
268+- catch (Error e)
269+- {
270+- error ("Failed to spawn Calculator: %s", e.message);
271+- }
272+
273+ try
274+ {
275+- subprocess.communicate_utf8 (null, null, out stdout_buf, out stderr_buf);
276++ yield solve_subprocess (results[0], true, out stdout_buf);
277+ }
278+ catch (Error e)
279+ {
280+- error ("Failed reading result: %s", e.message);
281++ return new HashTable<string, Variant>[0];
282+ }
283+
284+ var metadata = new HashTable<string, Variant>[1];
285+@@ -155,9 +201,11 @@ public class SearchProviderApp : Application
286+
287+ public override bool dbus_register (DBusConnection connection, string object_path)
288+ {
289++ SearchProvider search_provider = new SearchProvider ();
290++
291+ try
292+ {
293+- connection.register_object (object_path, new SearchProvider ());
294++ connection.register_object (object_path, search_provider);
295+ }
296+ catch (IOError error)
297+ {
298+@@ -165,6 +213,10 @@ public class SearchProviderApp : Application
299+ quit ();
300+ }
301+
302++ shutdown.connect (() => {
303++ search_provider.cancel ();
304++ });
305++
306+ return true;
307+ }
308+ }
309diff --git a/debian/patches/search-provider-Use-lower-inactivity-timeout.patch b/debian/patches/search-provider-Use-lower-inactivity-timeout.patch
310new file mode 100644
311index 0000000..b5b0dd6
312--- /dev/null
313+++ b/debian/patches/search-provider-Use-lower-inactivity-timeout.patch
314@@ -0,0 +1,29 @@
315+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
316+Date: Fri, 24 Aug 2018 07:12:12 +0200
317+Subject: search-provider: Use lower inactivity timeout
318+
319+Since we're renewing it at every call involving a process call, we can just
320+set it to a lower value than the default dbus proxy timeout, so that the provider
321+will return invalid values before that the timeout has been hit.
322+
323+Bug-Ubuntu: https://launchpad.net/bugs/1756826
324+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
325+Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
326+---
327+ search-provider/search-provider.vala | 3 ++-
328+ 1 file changed, 2 insertions(+), 1 deletion(-)
329+
330+diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
331+index 7e54fa6..9035c58 100644
332+--- a/search-provider/search-provider.vala
333++++ b/search-provider/search-provider.vala
334+@@ -203,7 +203,8 @@ public class SearchProviderApp : Application
335+ {
336+ Object (application_id: "org.gnome.Calculator.SearchProvider",
337+ flags: ApplicationFlags.IS_SERVICE,
338+- inactivity_timeout: 60000);
339++ inactivity_timeout: 20000);
340++ }
341+
342+ public void renew_inactivity_timeout ()
343+ {
344diff --git a/debian/patches/search-provider-cache-equations-avoiding-spawning-calcula.patch b/debian/patches/search-provider-cache-equations-avoiding-spawning-calcula.patch
345new file mode 100644
346index 0000000..b3903df
347--- /dev/null
348+++ b/debian/patches/search-provider-cache-equations-avoiding-spawning-calcula.patch
349@@ -0,0 +1,170 @@
350+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
351+Date: Mon, 27 Aug 2018 18:45:34 +0200
352+Subject: search-provider: cache equations avoiding spawning calculator twice
353+
354+Currently we spawn the calculator two times for each operation, one to check
355+if the search provider is valid for such syntax, the other time to actually
356+present the results. But since these results are just available at first
357+call, we can just keep them around and return them if the shell requires them.
358+
359+Since the search provider deamon is kept alive for just few moments, there's
360+no real need to cleanup the cache using a queue.
361+
362+In case of multiple async calls, reuse cancellable instead so that we can
363+just kill all the related processes one time at once.
364+
365+Bug-Ubuntu: https://launchpad.net/bugs/1756826
366+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
367+Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
368+---
369+ search-provider/search-provider.vala | 79 ++++++++++++++++++++----------------
370+ 1 file changed, 45 insertions(+), 34 deletions(-)
371+
372+diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
373+index a3c57f7..af2779e 100644
374+--- a/search-provider/search-provider.vala
375++++ b/search-provider/search-provider.vala
376+@@ -14,10 +14,13 @@
377+ public class SearchProvider : Object
378+ {
379+ private unowned SearchProviderApp application;
380+- private Cancellable cancellable = new Cancellable ();
381++ private Cancellable cancellable;
382++
383++ private HashTable<string, string> cached_equations;
384+ public SearchProvider (SearchProviderApp app)
385+ {
386+ application = app;
387++ cached_equations = new HashTable<string, string> (str_hash, str_equal);
388+ }
389+
390+ ~SearchProvider ()
391+@@ -29,9 +32,7 @@ public class SearchProvider : Object
392+ public void cancel ()
393+ {
394+ if (cancellable != null)
395+- {
396+ cancellable.cancel ();
397+- }
398+ }
399+
400+ private static string terms_to_equation (string[] terms)
401+@@ -60,7 +61,9 @@ public class SearchProvider : Object
402+ error ("Failed to spawn Calculator: %s", e.message);
403+ }
404+
405+- cancellable = new Cancellable ();
406++ if (cancellable == null)
407++ cancellable = new Cancellable ();
408++
409+ cancellable.cancelled.connect (() => {
410+ subprocess.force_exit ();
411+ cancellable = null;
412+@@ -71,29 +74,36 @@ public class SearchProvider : Object
413+ return subprocess;
414+ }
415+
416+- private async bool can_parse (string[] terms)
417++ private async bool solve_equation (string equation)
418+ {
419+- try
420+- {
421+- var tsep_string = Posix.nl_langinfo (Posix.NLItem.THOUSEP);
422+- if (tsep_string == null || tsep_string == "")
423+- tsep_string = " ";
424++ string? result;
425+
426+- var decimal = Posix.nl_langinfo (Posix.NLItem.RADIXCHAR);
427+- if (decimal == null)
428+- decimal = "";
429++ cancel();
430+
431+- // "normalize" input to a format known to double.try_parse
432+- var equation = terms_to_equation (terms).replace (tsep_string, "").replace (decimal, ".");
433++ var tsep_string = Posix.nl_langinfo (Posix.NLItem.THOUSEP);
434++ if (tsep_string == null || tsep_string == "")
435++ tsep_string = " ";
436+
437+- cancel();
438++ var decimal = Posix.nl_langinfo (Posix.NLItem.RADIXCHAR);
439++ if (decimal == null)
440++ decimal = "";
441+
442+- // if the search is a plain number, don't process it
443+- if (double.try_parse (equation)) {
444+- return false;
445+- }
446++ // "normalize" input to a format known to double.try_parse
447++ var normalized_equation = equation.replace (tsep_string, "").replace (decimal, ".");
448+
449+- (yield solve_subprocess (equation)).wait_check (cancellable);
450++ // if the search is a plain number, don't process it
451++ if (double.try_parse (normalized_equation)) {
452++ return false;
453++ }
454++
455++ if (cached_equations.lookup (equation) != null)
456++ return true;
457++
458++ try
459++ {
460++ var subprocess = yield solve_subprocess (normalized_equation);
461++ yield subprocess.communicate_utf8_async (null, cancellable, out result, null);
462++ subprocess.wait_check (cancellable);
463+ }
464+ catch (SpawnError e)
465+ {
466+@@ -104,14 +114,17 @@ public class SearchProvider : Object
467+ return false;
468+ }
469+
470++ cached_equations.insert (equation, result.strip ());
471++
472+ return true;
473+ }
474+
475+ private async string[] get_result_identifier (string[] terms)
476+ {
477+ /* We have at most one result: the search terms as one string */
478+- if (yield can_parse (terms))
479+- return { terms_to_equation (terms) };
480++ var equation = terms_to_equation (terms);
481++ if (yield solve_equation (equation))
482++ return { equation };
483+ else
484+ return new string[0];
485+ }
486+@@ -129,24 +142,22 @@ public class SearchProvider : Object
487+ public async HashTable<string, Variant>[] get_result_metas (string[] results, GLib.BusName sender)
488+ requires (results.length == 1)
489+ {
490+- string stdout_buf;
491+- string stderr_buf;
492++ string equation;
493++ string result;
494+
495+- try
496+- {
497+- var subprocess = yield solve_subprocess (results[0]);
498+- yield subprocess.communicate_utf8_async (null, cancellable, out stdout_buf, out stderr_buf);
499+- }
500+- catch (Error e)
501+- {
502++ equation = terms_to_equation (results);
503++
504++ if (!yield solve_equation (equation))
505+ return new HashTable<string, Variant>[0];
506+- }
507++
508++ result = cached_equations.lookup (equation);
509++ assert (result != null);
510+
511+ var metadata = new HashTable<string, Variant>[1];
512+ metadata[0] = new HashTable<string, Variant>(str_hash, str_equal);
513+ metadata[0].insert ("id", results[0]);
514+ metadata[0].insert ("name", results[0] );
515+- metadata[0].insert ("description", " = " + stdout_buf.strip ());
516++ metadata[0].insert ("description", @" = $result");
517+
518+ return metadata;
519+ }
520diff --git a/debian/patches/search-provider-cache-only-a-limited-number-of-equations.patch b/debian/patches/search-provider-cache-only-a-limited-number-of-equations.patch
521new file mode 100644
522index 0000000..ca1c342
523--- /dev/null
524+++ b/debian/patches/search-provider-cache-only-a-limited-number-of-equations.patch
525@@ -0,0 +1,44 @@
526+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
527+Date: Mon, 27 Aug 2018 18:56:09 +0200
528+Subject: search-provider: cache only a limited number of equations
529+
530+Bug-Ubuntu: https://launchpad.net/bugs/1756826
531+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
532+Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
533+---
534+ search-provider/search-provider.vala | 9 +++++++++
535+ 1 file changed, 9 insertions(+)
536+
537+diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
538+index af2779e..fc72128 100644
539+--- a/search-provider/search-provider.vala
540++++ b/search-provider/search-provider.vala
541+@@ -16,10 +16,15 @@ public class SearchProvider : Object
542+ private unowned SearchProviderApp application;
543+ private Cancellable cancellable;
544+
545++ private const int MAX_CACHED_OPERATIONS = 10;
546++ private Queue<string> queued_equations;
547+ private HashTable<string, string> cached_equations;
548++
549+ public SearchProvider (SearchProviderApp app)
550+ {
551+ application = app;
552++
553++ queued_equations = new Queue<string> ();
554+ cached_equations = new HashTable<string, string> (str_hash, str_equal);
555+ }
556+
557+@@ -114,8 +119,12 @@ public class SearchProvider : Object
558+ return false;
559+ }
560+
561++ queued_equations.push_tail (equation);
562+ cached_equations.insert (equation, result.strip ());
563+
564++ if (queued_equations.length > MAX_CACHED_OPERATIONS)
565++ cached_equations.remove (queued_equations.pop_head ());
566++
567+ return true;
568+ }
569+
570diff --git a/debian/patches/search-provider-cancel-the-current-process-on-new-calcula.patch b/debian/patches/search-provider-cancel-the-current-process-on-new-calcula.patch
571new file mode 100644
572index 0000000..801d844
573--- /dev/null
574+++ b/debian/patches/search-provider-cancel-the-current-process-on-new-calcula.patch
575@@ -0,0 +1,29 @@
576+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
577+Date: Thu, 30 Aug 2018 18:47:16 +0200
578+Subject: search-provider: cancel the current process on new calculation
579+ request
580+
581+As there's just one shell running at time, there's no point of supporting
582+parallel calls, we can just safely refer to the last equation as the only one
583+we need to compute.
584+
585+Bug-Ubuntu: https://launchpad.net/bugs/1756826
586+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
587+Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
588+---
589+ search-provider/search-provider.vala | 2 ++
590+ 1 file changed, 2 insertions(+)
591+
592+diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
593+index df37b86..a3c57f7 100644
594+--- a/search-provider/search-provider.vala
595++++ b/search-provider/search-provider.vala
596+@@ -86,6 +86,8 @@ public class SearchProvider : Object
597+ // "normalize" input to a format known to double.try_parse
598+ var equation = terms_to_equation (terms).replace (tsep_string, "").replace (decimal, ".");
599+
600++ cancel();
601++
602+ // if the search is a plain number, don't process it
603+ if (double.try_parse (equation)) {
604+ return false;
605diff --git a/debian/patches/search-provider-renew-inactivity-timeout-at-each-calculat.patch b/debian/patches/search-provider-renew-inactivity-timeout-at-each-calculat.patch
606new file mode 100644
607index 0000000..41458ad
608--- /dev/null
609+++ b/debian/patches/search-provider-renew-inactivity-timeout-at-each-calculat.patch
610@@ -0,0 +1,60 @@
611+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
612+Date: Fri, 24 Aug 2018 07:10:17 +0200
613+Subject: search-provider: renew inactivity timeout at each calculator run
614+
615+As per the async rewrite, now the daemon inactivity timeout might happen
616+when another call has just been done, while we don't want this to be the case.
617+
618+So, everytime we do a subprocess call, let's renew the application timeout.
619+
620+Bug-Ubuntu: https://launchpad.net/bugs/1756826
621+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
622+Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/100
623+---
624+ search-provider/search-provider.vala | 14 +++++++++++++-
625+ 1 file changed, 13 insertions(+), 1 deletion(-)
626+
627+diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
628+index 659f73d..7e54fa6 100644
629+--- a/search-provider/search-provider.vala
630++++ b/search-provider/search-provider.vala
631+@@ -13,7 +13,12 @@
632+ [DBus (name = "org.gnome.Shell.SearchProvider2")]
633+ public class SearchProvider : Object
634+ {
635++ private unowned SearchProviderApp application;
636+ private Cancellable cancellable = new Cancellable ();
637++ public SearchProvider (SearchProviderApp app)
638++ {
639++ application = app;
640++ }
641+
642+ ~SearchProvider ()
643+ {
644+@@ -71,6 +76,8 @@ public class SearchProvider : Object
645+ cancellable = null;
646+ });
647+
648++ application.renew_inactivity_timeout ();
649++
650+ yield subprocess.communicate_utf8_async (null, cancellable, out solution_buf, out stderr_buf);
651+ }
652+ catch (Error e)
653+@@ -197,11 +204,16 @@ public class SearchProviderApp : Application
654+ Object (application_id: "org.gnome.Calculator.SearchProvider",
655+ flags: ApplicationFlags.IS_SERVICE,
656+ inactivity_timeout: 60000);
657++
658++ public void renew_inactivity_timeout ()
659++ {
660++ this.hold ();
661++ this.release ();
662+ }
663+
664+ public override bool dbus_register (DBusConnection connection, string object_path)
665+ {
666+- SearchProvider search_provider = new SearchProvider ();
667++ SearchProvider search_provider = new SearchProvider (this);
668+
669+ try
670+ {
671diff --git a/debian/patches/search-provider-simplify-solve_subprocess.patch b/debian/patches/search-provider-simplify-solve_subprocess.patch
672new file mode 100644
673index 0000000..7dc1d78
674--- /dev/null
675+++ b/debian/patches/search-provider-simplify-solve_subprocess.patch
676@@ -0,0 +1,113 @@
677+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
678+Date: Fri, 24 Aug 2018 07:31:37 +0200
679+Subject: search-provider: simplify solve_subprocess
680+
681+Only return a subprocess in solve_subprocess and make the callers deal with
682+the actual operation to perform.
683+
684+Bug-Ubuntu: https://launchpad.net/bugs/1756826
685+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
686+Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
687+---
688+ search-provider/search-provider.vala | 49 ++++++++++--------------------------
689+ 1 file changed, 13 insertions(+), 36 deletions(-)
690+
691+diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
692+index 9035c58..df37b86 100644
693+--- a/search-provider/search-provider.vala
694++++ b/search-provider/search-provider.vala
695+@@ -39,7 +39,7 @@ public class SearchProvider : Object
696+ return string.joinv (" ", terms);
697+ }
698+
699+- private async Subprocess solve_subprocess (string equation, bool return_solution = false, out string? solution_buf = null) throws Error
700++ private async Subprocess solve_subprocess (string equation) throws Error
701+ {
702+ Subprocess subprocess;
703+ string[] argv = {"gnome-calculator", "--solve"};
704+@@ -52,13 +52,7 @@ public class SearchProvider : Object
705+ {
706+ // Eat output so that it doesn't wind up in the journal. It's
707+ // expected that most searches are not valid calculator input.
708+- var flags = SubprocessFlags.STDERR_PIPE;
709+-
710+- if (return_solution)
711+- {
712+- flags |= SubprocessFlags.STDOUT_PIPE;
713+- }
714+-
715++ var flags = SubprocessFlags.STDOUT_PIPE | SubprocessFlags.STDERR_PIPE;
716+ subprocess = new Subprocess.newv (argv, flags);
717+ }
718+ catch (Error e)
719+@@ -66,31 +60,13 @@ public class SearchProvider : Object
720+ error ("Failed to spawn Calculator: %s", e.message);
721+ }
722+
723+- try
724+- {
725+- string stderr_buf;
726+-
727+- cancellable = new Cancellable ();
728+- cancellable.cancelled.connect (() => {
729+- subprocess.force_exit ();
730+- cancellable = null;
731+- });
732+-
733+- application.renew_inactivity_timeout ();
734++ cancellable = new Cancellable ();
735++ cancellable.cancelled.connect (() => {
736++ subprocess.force_exit ();
737++ cancellable = null;
738++ });
739+
740+- yield subprocess.communicate_utf8_async (null, cancellable, out solution_buf, out stderr_buf);
741+- }
742+- catch (Error e)
743+- {
744+- if (e is IOError.CANCELLED)
745+- {
746+- throw e;
747+- }
748+- else
749+- {
750+- error ("Failed reading result: %s", e.message);
751+- }
752+- }
753++ application.renew_inactivity_timeout ();
754+
755+ return subprocess;
756+ }
757+@@ -115,8 +91,7 @@ public class SearchProvider : Object
758+ return false;
759+ }
760+
761+- var subprocess = yield solve_subprocess (equation);
762+- yield subprocess.wait_check_async ();
763++ (yield solve_subprocess (equation)).wait_check (cancellable);
764+ }
765+ catch (SpawnError e)
766+ {
767+@@ -153,10 +128,12 @@ public class SearchProvider : Object
768+ requires (results.length == 1)
769+ {
770+ string stdout_buf;
771++ string stderr_buf;
772+
773+ try
774+ {
775+- yield solve_subprocess (results[0], true, out stdout_buf);
776++ var subprocess = yield solve_subprocess (results[0]);
777++ yield subprocess.communicate_utf8_async (null, cancellable, out stdout_buf, out stderr_buf);
778+ }
779+ catch (Error e)
780+ {
781+@@ -167,7 +144,7 @@ public class SearchProvider : Object
782+ metadata[0] = new HashTable<string, Variant>(str_hash, str_equal);
783+ metadata[0].insert ("id", results[0]);
784+ metadata[0].insert ("name", results[0] );
785+- metadata[0].insert ("description", " = " + stdout_buf.strip() );
786++ metadata[0].insert ("description", " = " + stdout_buf.strip ());
787+
788+ return metadata;
789+ }
790diff --git a/debian/patches/series b/debian/patches/series
791index e69de29..4fd8ad8 100644
792--- a/debian/patches/series
793+++ b/debian/patches/series
794@@ -0,0 +1,8 @@
795+search-provider-Use-async-calls-cancel-search-on-inactivi.patch
796+search-provider-renew-inactivity-timeout-at-each-calculat.patch
797+search-provider-Use-lower-inactivity-timeout.patch
798+search-provider-simplify-solve_subprocess.patch
799+search-provider-cancel-the-current-process-on-new-calcula.patch
800+search-provider-cache-equations-avoiding-spawning-calcula.patch
801+search-provider-cache-only-a-limited-number-of-equations.patch
802+ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch
803diff --git a/debian/patches/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch b/debian/patches/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch
804new file mode 100644
805index 0000000..77939ec
806--- /dev/null
807+++ b/debian/patches/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch
808@@ -0,0 +1,118 @@
809+From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
810+Date: Tue, 28 Aug 2018 04:12:20 +0200
811+Subject: search-provider: Cancel operations on XUbuntuCancel
812+
813+Stop process and any computation on XUbuntuCancel method, if the caller
814+was the same triggering the last operation.
815+
816+Fixes LP: #1756826
817+
818+Bug-Ubuntu: https://launchpad.net/bugs/1756826
819+Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
820+Forwarded: not-needed
821+---
822+ search-provider/search-provider.vala | 46 ++++++++++++++++++++++++++++++++----
823+ 1 file changed, 41 insertions(+), 5 deletions(-)
824+
825+diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
826+index fc72128..b7c869d 100644
827+--- a/search-provider/search-provider.vala
828++++ b/search-provider/search-provider.vala
829+@@ -10,11 +10,33 @@
830+ * license.
831+ */
832+
833++public class CallerTracker : Object
834++{
835++ public BusName? bus { get; set; }
836++}
837++
838++public class Caller
839++{
840++ private CallerTracker caller_tracker;
841++
842++ public Caller (CallerTracker c, BusName sender)
843++ {
844++ caller_tracker = c;
845++ caller_tracker.bus = sender;
846++ }
847++
848++ ~Caller ()
849++ {
850++ caller_tracker.bus = null;
851++ }
852++}
853++
854+ [DBus (name = "org.gnome.Shell.SearchProvider2")]
855+ public class SearchProvider : Object
856+ {
857+ private unowned SearchProviderApp application;
858+ private Cancellable cancellable;
859++ private CallerTracker caller_tracker;
860+
861+ private const int MAX_CACHED_OPERATIONS = 10;
862+ private Queue<string> queued_equations;
863+@@ -24,6 +46,7 @@ public class SearchProvider : Object
864+ {
865+ application = app;
866+
867++ caller_tracker = new CallerTracker ();
868+ queued_equations = new Queue<string> ();
869+ cached_equations = new HashTable<string, string> (str_hash, str_equal);
870+ }
871+@@ -128,8 +151,10 @@ public class SearchProvider : Object
872+ return true;
873+ }
874+
875+- private async string[] get_result_identifier (string[] terms)
876++ private async string[] get_result_identifier (string[] terms, GLib.BusName sender)
877+ {
878++ var owner = new Caller (caller_tracker, sender); (void) owner;
879++
880+ /* We have at most one result: the search terms as one string */
881+ var equation = terms_to_equation (terms);
882+ if (yield solve_equation (equation))
883+@@ -138,14 +163,14 @@ public class SearchProvider : Object
884+ return new string[0];
885+ }
886+
887+- public async string[] get_initial_result_set (string[] terms)
888++ public async string[] get_initial_result_set (string[] terms, GLib.BusName sender)
889+ {
890+- return yield get_result_identifier (terms);
891++ return yield get_result_identifier (terms, sender);
892+ }
893+
894+- public async string[] get_subsearch_result_set (string[] previous_results, string[] terms)
895++ public async string[] get_subsearch_result_set (string[] previous_results, string[] terms, GLib.BusName sender)
896+ {
897+- return yield get_result_identifier (terms);
898++ return yield get_result_identifier (terms, sender);
899+ }
900+
901+ public async HashTable<string, Variant>[] get_result_metas (string[] results, GLib.BusName sender)
902+@@ -154,6 +179,8 @@ public class SearchProvider : Object
903+ string equation;
904+ string result;
905+
906++ var owner = new Caller (caller_tracker, sender); (void) owner;
907++
908+ equation = terms_to_equation (results);
909+
910+ if (!yield solve_equation (equation))
911+@@ -171,6 +198,15 @@ public class SearchProvider : Object
912+ return metadata;
913+ }
914+
915++ public async void x_ubuntu_cancel (BusName sender)
916++ {
917++ if (caller_tracker.bus == sender)
918++ {
919++ debug ("Cancelling Request");
920++ cancel ();
921++ }
922++ }
923++
924+ private static void spawn_and_display_equation (string[] terms)
925+ {
926+ try

Subscribers

People subscribed via source and target branches