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
diff --git a/debian/changelog b/debian/changelog
index 921715f..9bafaab 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,26 @@
1gnome-calculator (1:3.29.91-1ubuntu3) UNRELEASED; urgency=medium
2
3 * debian/gbp.conf:
4 - Add back debian-tag=ubuntu/%(version)s setting
5
6 -- Marco Trevisan (Treviño) <marco@ubuntu.com> Mon, 03 Sep 2018 16:46:28 +0200
7
8gnome-calculator (1:3.29.91-1ubuntu2) cosmic; urgency=medium
9
10 * d/p/search-provider-Use-async-calls-cancel-search-on-inactivi.patch,
11 d/p/search-provider-renew-inactivity-timeout-at-each-calculat.patch,
12 d/p/search-provider-Use-lower-inactivity-timeout.patch,
13 d/p/search-provider-simplify-solve_subprocess.patch,
14 d/p/search-provider-cache-equations-avoiding-spawning-calcula.patch,
15 d/p/search-provider-cancel-the-current-process-on-new-calcula.patch,
16 d/p/search-provider-cache-only-a-limited-number-of-equations.patch,
17 d/p/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch:
18 - Make search provider async and support XUbuntuCancel method to
19 stop expensive operations that might lead to an unresponsive
20 process (LP: #1756826)
21
22 -- Marco Trevisan (Treviño) <marco@ubuntu.com> Thu, 30 Aug 2018 12:57:51 -0500
23
1gnome-calculator (1:3.29.91-1ubuntu1) cosmic; urgency=medium24gnome-calculator (1:3.29.91-1ubuntu1) cosmic; urgency=medium
225
3 * Sync with Debian (lp: #1785802). Remaining change:26 * Sync with Debian (lp: #1785802). Remaining change:
diff --git a/debian/control b/debian/control
index 68d049d..495681f 100644
--- a/debian/control
+++ b/debian/control
@@ -5,7 +5,8 @@
5Source: gnome-calculator5Source: gnome-calculator
6Section: math6Section: math
7Priority: optional7Priority: optional
8Maintainer: Debian GNOME Maintainers <pkg-gnome-maintainers@lists.alioth.debian.org>8Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
9XSBC-Original-Maintainer: Debian GNOME Maintainers <pkg-gnome-maintainers@lists.alioth.debian.org>
9Uploaders: Jeremy Bicha <jbicha@debian.org>, Laurent Bigonville <bigon@debian.org>, Michael Biebl <biebl@debian.org>10Uploaders: Jeremy Bicha <jbicha@debian.org>, Laurent Bigonville <bigon@debian.org>, Michael Biebl <biebl@debian.org>
10Build-Depends: appstream-util,11Build-Depends: appstream-util,
11 debhelper (>= 11.1.3),12 debhelper (>= 11.1.3),
diff --git a/debian/control.in b/debian/control.in
index deb4c72..8e9918e 100644
--- a/debian/control.in
+++ b/debian/control.in
@@ -1,7 +1,8 @@
1Source: gnome-calculator1Source: gnome-calculator
2Section: math2Section: math
3Priority: optional3Priority: optional
4Maintainer: Debian GNOME Maintainers <pkg-gnome-maintainers@lists.alioth.debian.org>4Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
5XSBC-Original-Maintainer: Debian GNOME Maintainers <pkg-gnome-maintainers@lists.alioth.debian.org>
5Uploaders: @GNOME_TEAM@6Uploaders: @GNOME_TEAM@
6Build-Depends: appstream-util,7Build-Depends: appstream-util,
7 debhelper (>= 11.1.3),8 debhelper (>= 11.1.3),
diff --git a/debian/gbp.conf b/debian/gbp.conf
index c40187f..f6567cb 100644
--- a/debian/gbp.conf
+++ b/debian/gbp.conf
@@ -1,5 +1,6 @@
1[DEFAULT]1[DEFAULT]
2pristine-tar = True2pristine-tar = True
3debian-branch=debian/experimental3debian-branch=debian/experimental
4debian-tag=ubuntu/%(version)s
4upstream-branch=upstream/latest5upstream-branch=upstream/latest
5upstream-vcs-tag = %(version)s6upstream-vcs-tag = %(version)s
diff --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
6new file mode 1006447new file mode 100644
index 0000000..324be46
--- /dev/null
+++ b/debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch
@@ -0,0 +1,232 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Fri, 24 Aug 2018 06:57:03 +0200
3Subject: search-provider: Use async calls, cancel search on inactivity
4
5Move to async methods everywhere and factorize the `solve` calls to a single
6method that only uses Subprocess and that can be cancelled.
7
8As per this, if the the search-provider is trying to compute some complex
9operation, the daemon won't hang and once the applications' inactivity
10timeout is hit, any running instance of gnome-calculator will be killed and
11the daemon will return accordingly.
12
13This reduces the impact of an issue that can cause gnome-calculator to keep
14running forever if a complex computation is required (10!!!) from the shell,
15and won't ever be killed (see GNOME/gnome-shell#183).
16
17This is also a prerequisite for supporting the search Cancellation that is
18going to be available on next version of the Search provider protocol
19(as per GNOME/gnome-shell#436)
20
21Bug-Ubuntu: https://launchpad.net/bugs/1756826
22Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
23Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
24---
25 search-provider/search-provider.vala | 126 +++++++++++++++++++++++++----------
26 1 file changed, 89 insertions(+), 37 deletions(-)
27
28diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
29index 26c72af..659f73d 100644
30--- a/search-provider/search-provider.vala
31+++ b/search-provider/search-provider.vala
32@@ -1,6 +1,7 @@
33 /* -*- Mode: vala; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
34 *
35 * Copyright (C) 2014 Michael Catanzaro
36+ * Copyright (C) 2018 Marco Trevisan
37 *
38 * This program is free software: you can redistribute it and/or modify it under
39 * the terms of the GNU General Public License as published by the Free Software
40@@ -12,17 +13,85 @@
41 [DBus (name = "org.gnome.Shell.SearchProvider2")]
42 public class SearchProvider : Object
43 {
44+ private Cancellable cancellable = new Cancellable ();
45+
46+ ~SearchProvider ()
47+ {
48+ cancel ();
49+ }
50+
51+ [DBus (visible = false)]
52+ public void cancel ()
53+ {
54+ if (cancellable != null)
55+ {
56+ cancellable.cancel ();
57+ }
58+ }
59+
60 private static string terms_to_equation (string[] terms)
61 {
62 return string.joinv (" ", terms);
63 }
64
65- private static bool can_parse (string[] terms)
66+ private async Subprocess solve_subprocess (string equation, bool return_solution = false, out string? solution_buf = null) throws Error
67 {
68+ Subprocess subprocess;
69+ string[] argv = {"gnome-calculator", "--solve"};
70+ argv += equation;
71+ argv += null;
72+
73+ debug (@"Trying to solve $(equation)");
74+
75 try
76 {
77- int exit_status;
78+ // Eat output so that it doesn't wind up in the journal. It's
79+ // expected that most searches are not valid calculator input.
80+ var flags = SubprocessFlags.STDERR_PIPE;
81+
82+ if (return_solution)
83+ {
84+ flags |= SubprocessFlags.STDOUT_PIPE;
85+ }
86+
87+ subprocess = new Subprocess.newv (argv, flags);
88+ }
89+ catch (Error e)
90+ {
91+ error ("Failed to spawn Calculator: %s", e.message);
92+ }
93+
94+ try
95+ {
96+ string stderr_buf;
97+
98+ cancellable = new Cancellable ();
99+ cancellable.cancelled.connect (() => {
100+ subprocess.force_exit ();
101+ cancellable = null;
102+ });
103+
104+ yield subprocess.communicate_utf8_async (null, cancellable, out solution_buf, out stderr_buf);
105+ }
106+ catch (Error e)
107+ {
108+ if (e is IOError.CANCELLED)
109+ {
110+ throw e;
111+ }
112+ else
113+ {
114+ error ("Failed reading result: %s", e.message);
115+ }
116+ }
117
118+ return subprocess;
119+ }
120+
121+ private async bool can_parse (string[] terms)
122+ {
123+ try
124+ {
125 var tsep_string = Posix.nl_langinfo (Posix.NLItem.THOUSEP);
126 if (tsep_string == null || tsep_string == "")
127 tsep_string = " ";
128@@ -39,14 +108,8 @@ public class SearchProvider : Object
129 return false;
130 }
131
132- // Eat output so that it doesn't wind up in the journal. It's
133- // expected that most searches are not valid calculator input.
134- string stdout_buf;
135- string stderr_buf;
136- Process.spawn_command_line_sync (
137- "gnome-calculator --solve " + Shell.quote (equation),
138- out stdout_buf, out stderr_buf, out exit_status);
139- Process.check_exit_status (exit_status);
140+ var subprocess = yield solve_subprocess (equation);
141+ yield subprocess.wait_check_async ();
142 }
143 catch (SpawnError e)
144 {
145@@ -60,54 +123,37 @@ public class SearchProvider : Object
146 return true;
147 }
148
149- private static string[] get_result_identifier (string[] terms)
150- ensures (result.length == 0 || result.length == 1)
151+ private async string[] get_result_identifier (string[] terms)
152 {
153 /* We have at most one result: the search terms as one string */
154- if (can_parse (terms))
155+ if (yield can_parse (terms))
156 return { terms_to_equation (terms) };
157 else
158 return new string[0];
159 }
160
161- public string[] get_initial_result_set (string[] terms)
162+ public async string[] get_initial_result_set (string[] terms)
163 {
164- return get_result_identifier (terms);
165+ return yield get_result_identifier (terms);
166 }
167
168- public string[] get_subsearch_result_set (string[] previous_results, string[] terms)
169+ public async string[] get_subsearch_result_set (string[] previous_results, string[] terms)
170 {
171- return get_result_identifier (terms);
172+ return yield get_result_identifier (terms);
173 }
174
175- public HashTable<string, Variant>[] get_result_metas (string[] results)
176+ public async HashTable<string, Variant>[] get_result_metas (string[] results, GLib.BusName sender)
177 requires (results.length == 1)
178- ensures (result.length == 1)
179 {
180- Subprocess subprocess;
181 string stdout_buf;
182- string stderr_buf;
183-
184- string[] argv = {"gnome-calculator", "--solve"};
185- argv += results[0];
186- argv += null;
187-
188- try
189- {
190- subprocess = new Subprocess.newv (argv, SubprocessFlags.STDOUT_PIPE | SubprocessFlags.STDERR_PIPE);
191- }
192- catch (Error e)
193- {
194- error ("Failed to spawn Calculator: %s", e.message);
195- }
196
197 try
198 {
199- subprocess.communicate_utf8 (null, null, out stdout_buf, out stderr_buf);
200+ yield solve_subprocess (results[0], true, out stdout_buf);
201 }
202 catch (Error e)
203 {
204- error ("Failed reading result: %s", e.message);
205+ return new HashTable<string, Variant>[0];
206 }
207
208 var metadata = new HashTable<string, Variant>[1];
209@@ -155,9 +201,11 @@ public class SearchProviderApp : Application
210
211 public override bool dbus_register (DBusConnection connection, string object_path)
212 {
213+ SearchProvider search_provider = new SearchProvider ();
214+
215 try
216 {
217- connection.register_object (object_path, new SearchProvider ());
218+ connection.register_object (object_path, search_provider);
219 }
220 catch (IOError error)
221 {
222@@ -165,6 +213,10 @@ public class SearchProviderApp : Application
223 quit ();
224 }
225
226+ shutdown.connect (() => {
227+ search_provider.cancel ();
228+ });
229+
230 return true;
231 }
232 }
diff --git a/debian/patches/search-provider-Use-lower-inactivity-timeout.patch b/debian/patches/search-provider-Use-lower-inactivity-timeout.patch
0new file mode 100644233new file mode 100644
index 0000000..b5b0dd6
--- /dev/null
+++ b/debian/patches/search-provider-Use-lower-inactivity-timeout.patch
@@ -0,0 +1,29 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Fri, 24 Aug 2018 07:12:12 +0200
3Subject: search-provider: Use lower inactivity timeout
4
5Since we're renewing it at every call involving a process call, we can just
6set it to a lower value than the default dbus proxy timeout, so that the provider
7will return invalid values before that the timeout has been hit.
8
9Bug-Ubuntu: https://launchpad.net/bugs/1756826
10Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
11Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
12---
13 search-provider/search-provider.vala | 3 ++-
14 1 file changed, 2 insertions(+), 1 deletion(-)
15
16diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
17index 7e54fa6..9035c58 100644
18--- a/search-provider/search-provider.vala
19+++ b/search-provider/search-provider.vala
20@@ -203,7 +203,8 @@ public class SearchProviderApp : Application
21 {
22 Object (application_id: "org.gnome.Calculator.SearchProvider",
23 flags: ApplicationFlags.IS_SERVICE,
24- inactivity_timeout: 60000);
25+ inactivity_timeout: 20000);
26+ }
27
28 public void renew_inactivity_timeout ()
29 {
diff --git a/debian/patches/search-provider-cache-equations-avoiding-spawning-calcula.patch b/debian/patches/search-provider-cache-equations-avoiding-spawning-calcula.patch
0new file mode 10064430new file mode 100644
index 0000000..b3903df
--- /dev/null
+++ b/debian/patches/search-provider-cache-equations-avoiding-spawning-calcula.patch
@@ -0,0 +1,170 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Mon, 27 Aug 2018 18:45:34 +0200
3Subject: search-provider: cache equations avoiding spawning calculator twice
4
5Currently we spawn the calculator two times for each operation, one to check
6if the search provider is valid for such syntax, the other time to actually
7present the results. But since these results are just available at first
8call, we can just keep them around and return them if the shell requires them.
9
10Since the search provider deamon is kept alive for just few moments, there's
11no real need to cleanup the cache using a queue.
12
13In case of multiple async calls, reuse cancellable instead so that we can
14just kill all the related processes one time at once.
15
16Bug-Ubuntu: https://launchpad.net/bugs/1756826
17Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
18Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
19---
20 search-provider/search-provider.vala | 79 ++++++++++++++++++++----------------
21 1 file changed, 45 insertions(+), 34 deletions(-)
22
23diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
24index a3c57f7..af2779e 100644
25--- a/search-provider/search-provider.vala
26+++ b/search-provider/search-provider.vala
27@@ -14,10 +14,13 @@
28 public class SearchProvider : Object
29 {
30 private unowned SearchProviderApp application;
31- private Cancellable cancellable = new Cancellable ();
32+ private Cancellable cancellable;
33+
34+ private HashTable<string, string> cached_equations;
35 public SearchProvider (SearchProviderApp app)
36 {
37 application = app;
38+ cached_equations = new HashTable<string, string> (str_hash, str_equal);
39 }
40
41 ~SearchProvider ()
42@@ -29,9 +32,7 @@ public class SearchProvider : Object
43 public void cancel ()
44 {
45 if (cancellable != null)
46- {
47 cancellable.cancel ();
48- }
49 }
50
51 private static string terms_to_equation (string[] terms)
52@@ -60,7 +61,9 @@ public class SearchProvider : Object
53 error ("Failed to spawn Calculator: %s", e.message);
54 }
55
56- cancellable = new Cancellable ();
57+ if (cancellable == null)
58+ cancellable = new Cancellable ();
59+
60 cancellable.cancelled.connect (() => {
61 subprocess.force_exit ();
62 cancellable = null;
63@@ -71,29 +74,36 @@ public class SearchProvider : Object
64 return subprocess;
65 }
66
67- private async bool can_parse (string[] terms)
68+ private async bool solve_equation (string equation)
69 {
70- try
71- {
72- var tsep_string = Posix.nl_langinfo (Posix.NLItem.THOUSEP);
73- if (tsep_string == null || tsep_string == "")
74- tsep_string = " ";
75+ string? result;
76
77- var decimal = Posix.nl_langinfo (Posix.NLItem.RADIXCHAR);
78- if (decimal == null)
79- decimal = "";
80+ cancel();
81
82- // "normalize" input to a format known to double.try_parse
83- var equation = terms_to_equation (terms).replace (tsep_string, "").replace (decimal, ".");
84+ var tsep_string = Posix.nl_langinfo (Posix.NLItem.THOUSEP);
85+ if (tsep_string == null || tsep_string == "")
86+ tsep_string = " ";
87
88- cancel();
89+ var decimal = Posix.nl_langinfo (Posix.NLItem.RADIXCHAR);
90+ if (decimal == null)
91+ decimal = "";
92
93- // if the search is a plain number, don't process it
94- if (double.try_parse (equation)) {
95- return false;
96- }
97+ // "normalize" input to a format known to double.try_parse
98+ var normalized_equation = equation.replace (tsep_string, "").replace (decimal, ".");
99
100- (yield solve_subprocess (equation)).wait_check (cancellable);
101+ // if the search is a plain number, don't process it
102+ if (double.try_parse (normalized_equation)) {
103+ return false;
104+ }
105+
106+ if (cached_equations.lookup (equation) != null)
107+ return true;
108+
109+ try
110+ {
111+ var subprocess = yield solve_subprocess (normalized_equation);
112+ yield subprocess.communicate_utf8_async (null, cancellable, out result, null);
113+ subprocess.wait_check (cancellable);
114 }
115 catch (SpawnError e)
116 {
117@@ -104,14 +114,17 @@ public class SearchProvider : Object
118 return false;
119 }
120
121+ cached_equations.insert (equation, result.strip ());
122+
123 return true;
124 }
125
126 private async string[] get_result_identifier (string[] terms)
127 {
128 /* We have at most one result: the search terms as one string */
129- if (yield can_parse (terms))
130- return { terms_to_equation (terms) };
131+ var equation = terms_to_equation (terms);
132+ if (yield solve_equation (equation))
133+ return { equation };
134 else
135 return new string[0];
136 }
137@@ -129,24 +142,22 @@ public class SearchProvider : Object
138 public async HashTable<string, Variant>[] get_result_metas (string[] results, GLib.BusName sender)
139 requires (results.length == 1)
140 {
141- string stdout_buf;
142- string stderr_buf;
143+ string equation;
144+ string result;
145
146- try
147- {
148- var subprocess = yield solve_subprocess (results[0]);
149- yield subprocess.communicate_utf8_async (null, cancellable, out stdout_buf, out stderr_buf);
150- }
151- catch (Error e)
152- {
153+ equation = terms_to_equation (results);
154+
155+ if (!yield solve_equation (equation))
156 return new HashTable<string, Variant>[0];
157- }
158+
159+ result = cached_equations.lookup (equation);
160+ assert (result != null);
161
162 var metadata = new HashTable<string, Variant>[1];
163 metadata[0] = new HashTable<string, Variant>(str_hash, str_equal);
164 metadata[0].insert ("id", results[0]);
165 metadata[0].insert ("name", results[0] );
166- metadata[0].insert ("description", " = " + stdout_buf.strip ());
167+ metadata[0].insert ("description", @" = $result");
168
169 return metadata;
170 }
diff --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
0new file mode 100644171new file mode 100644
index 0000000..ca1c342
--- /dev/null
+++ b/debian/patches/search-provider-cache-only-a-limited-number-of-equations.patch
@@ -0,0 +1,44 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Mon, 27 Aug 2018 18:56:09 +0200
3Subject: search-provider: cache only a limited number of equations
4
5Bug-Ubuntu: https://launchpad.net/bugs/1756826
6Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
7Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
8---
9 search-provider/search-provider.vala | 9 +++++++++
10 1 file changed, 9 insertions(+)
11
12diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
13index af2779e..fc72128 100644
14--- a/search-provider/search-provider.vala
15+++ b/search-provider/search-provider.vala
16@@ -16,10 +16,15 @@ public class SearchProvider : Object
17 private unowned SearchProviderApp application;
18 private Cancellable cancellable;
19
20+ private const int MAX_CACHED_OPERATIONS = 10;
21+ private Queue<string> queued_equations;
22 private HashTable<string, string> cached_equations;
23+
24 public SearchProvider (SearchProviderApp app)
25 {
26 application = app;
27+
28+ queued_equations = new Queue<string> ();
29 cached_equations = new HashTable<string, string> (str_hash, str_equal);
30 }
31
32@@ -114,8 +119,12 @@ public class SearchProvider : Object
33 return false;
34 }
35
36+ queued_equations.push_tail (equation);
37 cached_equations.insert (equation, result.strip ());
38
39+ if (queued_equations.length > MAX_CACHED_OPERATIONS)
40+ cached_equations.remove (queued_equations.pop_head ());
41+
42 return true;
43 }
44
diff --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
0new file mode 10064445new file mode 100644
index 0000000..801d844
--- /dev/null
+++ b/debian/patches/search-provider-cancel-the-current-process-on-new-calcula.patch
@@ -0,0 +1,29 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Thu, 30 Aug 2018 18:47:16 +0200
3Subject: search-provider: cancel the current process on new calculation
4 request
5
6As there's just one shell running at time, there's no point of supporting
7parallel calls, we can just safely refer to the last equation as the only one
8we need to compute.
9
10Bug-Ubuntu: https://launchpad.net/bugs/1756826
11Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
12Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
13---
14 search-provider/search-provider.vala | 2 ++
15 1 file changed, 2 insertions(+)
16
17diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
18index df37b86..a3c57f7 100644
19--- a/search-provider/search-provider.vala
20+++ b/search-provider/search-provider.vala
21@@ -86,6 +86,8 @@ public class SearchProvider : Object
22 // "normalize" input to a format known to double.try_parse
23 var equation = terms_to_equation (terms).replace (tsep_string, "").replace (decimal, ".");
24
25+ cancel();
26+
27 // if the search is a plain number, don't process it
28 if (double.try_parse (equation)) {
29 return false;
diff --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
0new file mode 10064430new file mode 100644
index 0000000..41458ad
--- /dev/null
+++ b/debian/patches/search-provider-renew-inactivity-timeout-at-each-calculat.patch
@@ -0,0 +1,60 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Fri, 24 Aug 2018 07:10:17 +0200
3Subject: search-provider: renew inactivity timeout at each calculator run
4
5As per the async rewrite, now the daemon inactivity timeout might happen
6when another call has just been done, while we don't want this to be the case.
7
8So, everytime we do a subprocess call, let's renew the application timeout.
9
10Bug-Ubuntu: https://launchpad.net/bugs/1756826
11Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
12Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/100
13---
14 search-provider/search-provider.vala | 14 +++++++++++++-
15 1 file changed, 13 insertions(+), 1 deletion(-)
16
17diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
18index 659f73d..7e54fa6 100644
19--- a/search-provider/search-provider.vala
20+++ b/search-provider/search-provider.vala
21@@ -13,7 +13,12 @@
22 [DBus (name = "org.gnome.Shell.SearchProvider2")]
23 public class SearchProvider : Object
24 {
25+ private unowned SearchProviderApp application;
26 private Cancellable cancellable = new Cancellable ();
27+ public SearchProvider (SearchProviderApp app)
28+ {
29+ application = app;
30+ }
31
32 ~SearchProvider ()
33 {
34@@ -71,6 +76,8 @@ public class SearchProvider : Object
35 cancellable = null;
36 });
37
38+ application.renew_inactivity_timeout ();
39+
40 yield subprocess.communicate_utf8_async (null, cancellable, out solution_buf, out stderr_buf);
41 }
42 catch (Error e)
43@@ -197,11 +204,16 @@ public class SearchProviderApp : Application
44 Object (application_id: "org.gnome.Calculator.SearchProvider",
45 flags: ApplicationFlags.IS_SERVICE,
46 inactivity_timeout: 60000);
47+
48+ public void renew_inactivity_timeout ()
49+ {
50+ this.hold ();
51+ this.release ();
52 }
53
54 public override bool dbus_register (DBusConnection connection, string object_path)
55 {
56- SearchProvider search_provider = new SearchProvider ();
57+ SearchProvider search_provider = new SearchProvider (this);
58
59 try
60 {
diff --git a/debian/patches/search-provider-simplify-solve_subprocess.patch b/debian/patches/search-provider-simplify-solve_subprocess.patch
0new file mode 10064461new file mode 100644
index 0000000..7dc1d78
--- /dev/null
+++ b/debian/patches/search-provider-simplify-solve_subprocess.patch
@@ -0,0 +1,113 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Fri, 24 Aug 2018 07:31:37 +0200
3Subject: search-provider: simplify solve_subprocess
4
5Only return a subprocess in solve_subprocess and make the callers deal with
6the actual operation to perform.
7
8Bug-Ubuntu: https://launchpad.net/bugs/1756826
9Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
10Forwarded: https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
11---
12 search-provider/search-provider.vala | 49 ++++++++++--------------------------
13 1 file changed, 13 insertions(+), 36 deletions(-)
14
15diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
16index 9035c58..df37b86 100644
17--- a/search-provider/search-provider.vala
18+++ b/search-provider/search-provider.vala
19@@ -39,7 +39,7 @@ public class SearchProvider : Object
20 return string.joinv (" ", terms);
21 }
22
23- private async Subprocess solve_subprocess (string equation, bool return_solution = false, out string? solution_buf = null) throws Error
24+ private async Subprocess solve_subprocess (string equation) throws Error
25 {
26 Subprocess subprocess;
27 string[] argv = {"gnome-calculator", "--solve"};
28@@ -52,13 +52,7 @@ public class SearchProvider : Object
29 {
30 // Eat output so that it doesn't wind up in the journal. It's
31 // expected that most searches are not valid calculator input.
32- var flags = SubprocessFlags.STDERR_PIPE;
33-
34- if (return_solution)
35- {
36- flags |= SubprocessFlags.STDOUT_PIPE;
37- }
38-
39+ var flags = SubprocessFlags.STDOUT_PIPE | SubprocessFlags.STDERR_PIPE;
40 subprocess = new Subprocess.newv (argv, flags);
41 }
42 catch (Error e)
43@@ -66,31 +60,13 @@ public class SearchProvider : Object
44 error ("Failed to spawn Calculator: %s", e.message);
45 }
46
47- try
48- {
49- string stderr_buf;
50-
51- cancellable = new Cancellable ();
52- cancellable.cancelled.connect (() => {
53- subprocess.force_exit ();
54- cancellable = null;
55- });
56-
57- application.renew_inactivity_timeout ();
58+ cancellable = new Cancellable ();
59+ cancellable.cancelled.connect (() => {
60+ subprocess.force_exit ();
61+ cancellable = null;
62+ });
63
64- yield subprocess.communicate_utf8_async (null, cancellable, out solution_buf, out stderr_buf);
65- }
66- catch (Error e)
67- {
68- if (e is IOError.CANCELLED)
69- {
70- throw e;
71- }
72- else
73- {
74- error ("Failed reading result: %s", e.message);
75- }
76- }
77+ application.renew_inactivity_timeout ();
78
79 return subprocess;
80 }
81@@ -115,8 +91,7 @@ public class SearchProvider : Object
82 return false;
83 }
84
85- var subprocess = yield solve_subprocess (equation);
86- yield subprocess.wait_check_async ();
87+ (yield solve_subprocess (equation)).wait_check (cancellable);
88 }
89 catch (SpawnError e)
90 {
91@@ -153,10 +128,12 @@ public class SearchProvider : Object
92 requires (results.length == 1)
93 {
94 string stdout_buf;
95+ string stderr_buf;
96
97 try
98 {
99- yield solve_subprocess (results[0], true, out stdout_buf);
100+ var subprocess = yield solve_subprocess (results[0]);
101+ yield subprocess.communicate_utf8_async (null, cancellable, out stdout_buf, out stderr_buf);
102 }
103 catch (Error e)
104 {
105@@ -167,7 +144,7 @@ public class SearchProvider : Object
106 metadata[0] = new HashTable<string, Variant>(str_hash, str_equal);
107 metadata[0].insert ("id", results[0]);
108 metadata[0].insert ("name", results[0] );
109- metadata[0].insert ("description", " = " + stdout_buf.strip() );
110+ metadata[0].insert ("description", " = " + stdout_buf.strip ());
111
112 return metadata;
113 }
diff --git a/debian/patches/series b/debian/patches/series
index e69de29..4fd8ad8 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -0,0 +1,8 @@
1search-provider-Use-async-calls-cancel-search-on-inactivi.patch
2search-provider-renew-inactivity-timeout-at-each-calculat.patch
3search-provider-Use-lower-inactivity-timeout.patch
4search-provider-simplify-solve_subprocess.patch
5search-provider-cancel-the-current-process-on-new-calcula.patch
6search-provider-cache-equations-avoiding-spawning-calcula.patch
7search-provider-cache-only-a-limited-number-of-equations.patch
8ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch
diff --git a/debian/patches/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch b/debian/patches/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch
0new file mode 1006449new file mode 100644
index 0000000..77939ec
--- /dev/null
+++ b/debian/patches/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch
@@ -0,0 +1,118 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Tue, 28 Aug 2018 04:12:20 +0200
3Subject: search-provider: Cancel operations on XUbuntuCancel
4
5Stop process and any computation on XUbuntuCancel method, if the caller
6was the same triggering the last operation.
7
8Fixes LP: #1756826
9
10Bug-Ubuntu: https://launchpad.net/bugs/1756826
11Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
12Forwarded: not-needed
13---
14 search-provider/search-provider.vala | 46 ++++++++++++++++++++++++++++++++----
15 1 file changed, 41 insertions(+), 5 deletions(-)
16
17diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
18index fc72128..b7c869d 100644
19--- a/search-provider/search-provider.vala
20+++ b/search-provider/search-provider.vala
21@@ -10,11 +10,33 @@
22 * license.
23 */
24
25+public class CallerTracker : Object
26+{
27+ public BusName? bus { get; set; }
28+}
29+
30+public class Caller
31+{
32+ private CallerTracker caller_tracker;
33+
34+ public Caller (CallerTracker c, BusName sender)
35+ {
36+ caller_tracker = c;
37+ caller_tracker.bus = sender;
38+ }
39+
40+ ~Caller ()
41+ {
42+ caller_tracker.bus = null;
43+ }
44+}
45+
46 [DBus (name = "org.gnome.Shell.SearchProvider2")]
47 public class SearchProvider : Object
48 {
49 private unowned SearchProviderApp application;
50 private Cancellable cancellable;
51+ private CallerTracker caller_tracker;
52
53 private const int MAX_CACHED_OPERATIONS = 10;
54 private Queue<string> queued_equations;
55@@ -24,6 +46,7 @@ public class SearchProvider : Object
56 {
57 application = app;
58
59+ caller_tracker = new CallerTracker ();
60 queued_equations = new Queue<string> ();
61 cached_equations = new HashTable<string, string> (str_hash, str_equal);
62 }
63@@ -128,8 +151,10 @@ public class SearchProvider : Object
64 return true;
65 }
66
67- private async string[] get_result_identifier (string[] terms)
68+ private async string[] get_result_identifier (string[] terms, GLib.BusName sender)
69 {
70+ var owner = new Caller (caller_tracker, sender); (void) owner;
71+
72 /* We have at most one result: the search terms as one string */
73 var equation = terms_to_equation (terms);
74 if (yield solve_equation (equation))
75@@ -138,14 +163,14 @@ public class SearchProvider : Object
76 return new string[0];
77 }
78
79- public async string[] get_initial_result_set (string[] terms)
80+ public async string[] get_initial_result_set (string[] terms, GLib.BusName sender)
81 {
82- return yield get_result_identifier (terms);
83+ return yield get_result_identifier (terms, sender);
84 }
85
86- public async string[] get_subsearch_result_set (string[] previous_results, string[] terms)
87+ public async string[] get_subsearch_result_set (string[] previous_results, string[] terms, GLib.BusName sender)
88 {
89- return yield get_result_identifier (terms);
90+ return yield get_result_identifier (terms, sender);
91 }
92
93 public async HashTable<string, Variant>[] get_result_metas (string[] results, GLib.BusName sender)
94@@ -154,6 +179,8 @@ public class SearchProvider : Object
95 string equation;
96 string result;
97
98+ var owner = new Caller (caller_tracker, sender); (void) owner;
99+
100 equation = terms_to_equation (results);
101
102 if (!yield solve_equation (equation))
103@@ -171,6 +198,15 @@ public class SearchProvider : Object
104 return metadata;
105 }
106
107+ public async void x_ubuntu_cancel (BusName sender)
108+ {
109+ if (caller_tracker.bus == sender)
110+ {
111+ debug ("Cancelling Request");
112+ cancel ();
113+ }
114+ }
115+
116 private static void spawn_and_display_equation (string[] terms)
117 {
118 try

Subscribers

People subscribed via source and target branches