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