[Merge] ~3v1n0/ubuntu/+source/gnome-calculator:ubuntu/master-xubuntu-cancel-search into ~ubuntu-desktop/ubuntu/+source/gnome-calculator:ubuntu/master
Didier Roche
didrocks at ubuntu.com
Thu Aug 30 08:49:24 UTC 2018
Review: Needs Fixing
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 ;)
Diff comments:
> diff --git a/debian/changelog b/debian/changelog
> index 921715f..20249df 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,33 +1,24 @@
> -gnome-calculator (1:3.29.91-1ubuntu1) cosmic; urgency=medium
> +gnome-calculator (1:3.28.1-1ubuntu2) UNRELEASED; urgency=medium
We want this for cosmic, correct and this isn't the SRU version (as we need the fix for cosmic first and it seems the "proper" solution won't be in 3.30 but 3.32). So, shouldn't the changelog reflects the cosmic version + your patch?
All the rest (VCS branch targeted and debian-branch=ubuntu/master in gbp.conf) seems to look like it's for cosmic…
>
> - * Sync with Debian (lp: #1785802). Remaining change:
> - - Add epoch
> -
> - -- Sebastien Bacher <seb128 at ubuntu.com> Tue, 14 Aug 2018 17:03:08 +0200
> -
> -gnome-calculator (3.29.91-1) experimental; urgency=medium
> + * d/p/search-provider-Use-async-calls-cancel-search-on-inactivi.patch,
> + d/p/search-provider-renew-inactivity-timeout-at-each-calculat.patch,
> + d/p/search-provider-Use-lower-inactivity-timeout.patch,
> + d/p/search-provider-simplify-solve_subprocess.patch,
> + d/p/search-provider-cache-equations-avoiding-spawning-calcula.patch,
> + d/p/search-provider-cache-only-a-limited-number-of-equations.patch,
> + d/p/ubuntu/search-provider-Cancel-operations-on-XUbuntuCancel.patch:
> + - Make search provider async and support XUbuntuCancel method to
> + stop expensive operations that might lead to an unresponsive
> + process (LP: #1756826)
>
> - * New upstream version
> + -- Marco Trevisan (Treviño) <marco at ubuntu.com> Tue, 28 Aug 2018 04:17:51 +0200
>
> - -- Sebastien Bacher <seb128 at ubuntu.com> Tue, 14 Aug 2018 16:42:48 +0200
> -
> -gnome-calculator (3.29.90-1) experimental; urgency=medium
> -
> - * New upstream release
> - * debian/control*, gbp.conf: Update for experimental (3.29 series)
> +gnome-calculator (1:3.28.1-1ubuntu1) bionic; urgency=medium
>
> - -- Andrea Azzarone <andrea.azzarone at canonical.com> Tue, 07 Aug 2018 13:17:23 +0300
> -
> -gnome-calculator (3.28.2-1) unstable; urgency=medium
> -
> - [ Andrea Azzarone ]
> - * New upstream release
> -
> - [ Iain Lane ]
> - * Set Rules-Requires-Root to "no" - we don't need to be root to build.
> - * Set Standards-Version to 4.1.5
> + * Sync with Debian. Remaining change:
> + - Add epoch
>
> - -- Andrea Azzarone <andrea.azzarone at canonical.com> Fri, 27 Jul 2018 22:13:29 +0200
> + -- Jeremy Bicha <jbicha at ubuntu.com> Tue, 10 Apr 2018 14:33:47 -0400
>
> gnome-calculator (3.28.1-1) unstable; urgency=medium
>
> diff --git a/debian/control.in b/debian/control.in
> index deb4c72..522dc74 100644
> --- a/debian/control.in
> +++ b/debian/control.in
> @@ -16,11 +16,12 @@ Build-Depends: appstream-util,
> libsoup2.4-dev (>= 2.42),
> libmpc-dev,
> libmpfr-dev
> -Standards-Version: 4.1.5
> -Vcs-Browser: https://salsa.debian.org/gnome-team/gnome-calculator
> -Vcs-Git: https://salsa.debian.org/gnome-team/gnome-calculator.git -b debian/experimental
> +Standards-Version: 4.1.4
Downgrading Standards-Version?
> +XS-Debian-Vcs-Browser: https://salsa.debian.org/gnome-team/gnome-calculator
> +XS-Debian-Vcs-Git: https://salsa.debian.org/gnome-team/gnome-calculator.git
> +Vcs-Browser: https://git.launchpad.net/~ubuntu-desktop/ubuntu/+source/gnome-calculator
> +Vcs-Git: https://git.launchpad.net/~ubuntu-desktop/ubuntu/+source/gnome-calculator
> Homepage: https://wiki.gnome.org/Apps/Calculator
> -Rules-Requires-Root: no
>
> Package: gnome-calculator
> Architecture: any
> 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
> new file mode 100644
> index 0000000..a371dc8
> --- /dev/null
> +++ b/debian/patches/search-provider-Use-async-calls-cancel-search-on-inactivi.patch
> @@ -0,0 +1,232 @@
> +From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail at 3v1n0.net>
> +Date: Fri, 24 Aug 2018 06:57:03 +0200
> +Subject: search-provider: Use async calls, cancel search on inactivity
> +
> +Move to async methods everywhere and factorize the `solve` calls to a single
> +method that only uses Subprocess and that can be cancelled.
> +
> +As per this, if the the search-provider is trying to compute some complex
> +operation, the daemon won't hang and once the applications' inactivity
> +timeout is hit, any running instance of gnome-calculator will be killed and
> +the daemon will return accordingly.
> +
> +This reduces the impact of an issue that can cause gnome-calculator to keep
> +running forever if a complex computation is required (10!!!) from the shell,
> +and won't ever be killed (see GNOME/gnome-shell#183).
> +
> +This is also a prerequisite for supporting the search Cancellation that is
> +going to be available on next version of the Search provider protocol
> +(as per GNOME/gnome-shell#436)
> +
> +Bug-Ubuntu: https://launchpad.net/bugs/1756826
> +Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
> +Forwarded: yes, https://gitlab.gnome.org/GNOME/gnome-calculator/merge_requests/10
Nitpick: no need for the "yes, " here ;)
> +---
> + search-provider/search-provider.vala | 126 +++++++++++++++++++++++++----------
> + 1 file changed, 89 insertions(+), 37 deletions(-)
> +
> +diff --git a/search-provider/search-provider.vala b/search-provider/search-provider.vala
> +index 26c72af..659f73d 100644
> +--- a/search-provider/search-provider.vala
> ++++ b/search-provider/search-provider.vala
> +@@ -1,6 +1,7 @@
> + /* -*- Mode: vala; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + *
> + * Copyright (C) 2014 Michael Catanzaro
> ++ * Copyright (C) 2018 Marco Trevisan
> + *
> + * This program is free software: you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License as published by the Free Software
> +@@ -12,17 +13,85 @@
> + [DBus (name = "org.gnome.Shell.SearchProvider2")]
> + public class SearchProvider : Object
> + {
> ++ private Cancellable cancellable = new Cancellable ();
> ++
> ++ ~SearchProvider ()
> ++ {
> ++ cancel ();
> ++ }
> ++
> ++ [DBus (visible = false)]
> ++ public void cancel ()
> ++ {
> ++ if (cancellable != null)
> ++ {
> ++ cancellable.cancel ();
> ++ }
> ++ }
> ++
> + private static string terms_to_equation (string[] terms)
> + {
> + return string.joinv (" ", terms);
> + }
> +
> +- private static bool can_parse (string[] terms)
> ++ private async Subprocess solve_subprocess (string equation, bool return_solution = false, out string? solution_buf = null) throws Error
> + {
> ++ Subprocess subprocess;
> ++ string[] argv = {"gnome-calculator", "--solve"};
> ++ argv += equation;
> ++ argv += null;
> ++
> ++ debug (@"Trying to solve $(equation)");
> ++
> + try
> + {
> +- int exit_status;
> ++ // Eat output so that it doesn't wind up in the journal. It's
> ++ // expected that most searches are not valid calculator input.
> ++ var flags = SubprocessFlags.STDERR_PIPE;
> ++
> ++ if (return_solution)
> ++ {
> ++ flags |= SubprocessFlags.STDOUT_PIPE;
> ++ }
> ++
> ++ subprocess = new Subprocess.newv (argv, flags);
> ++ }
> ++ catch (Error e)
> ++ {
> ++ error ("Failed to spawn Calculator: %s", e.message);
> ++ }
> ++
> ++ try
> ++ {
> ++ string stderr_buf;
> ++
> ++ cancellable = new Cancellable ();
> ++ cancellable.cancelled.connect (() => {
> ++ subprocess.force_exit ();
> ++ cancellable = null;
> ++ });
> ++
> ++ yield subprocess.communicate_utf8_async (null, cancellable, out solution_buf, out stderr_buf);
> ++ }
> ++ catch (Error e)
> ++ {
> ++ if (e is IOError.CANCELLED)
> ++ {
> ++ throw e;
> ++ }
> ++ else
> ++ {
> ++ error ("Failed reading result: %s", e.message);
> ++ }
> ++ }
> +
> ++ return subprocess;
> ++ }
> ++
> ++ private async bool can_parse (string[] terms)
> ++ {
> ++ try
> ++ {
> + var tsep_string = Posix.nl_langinfo (Posix.NLItem.THOUSEP);
> + if (tsep_string == null || tsep_string == "")
> + tsep_string = " ";
> +@@ -39,14 +108,8 @@ public class SearchProvider : Object
> + return false;
> + }
> +
> +- // Eat output so that it doesn't wind up in the journal. It's
> +- // expected that most searches are not valid calculator input.
> +- string stdout_buf;
> +- string stderr_buf;
> +- Process.spawn_command_line_sync (
> +- "gnome-calculator --solve " + Shell.quote (equation),
> +- out stdout_buf, out stderr_buf, out exit_status);
> +- Process.check_exit_status (exit_status);
> ++ var subprocess = yield solve_subprocess (equation);
> ++ yield subprocess.wait_check_async ();
Do we really need that (not really familiar with vala), but as we yield on the previous call, this one can be just sync as it should be a quick check?
> + }
> + catch (SpawnError e)
> + {
> +@@ -60,54 +123,37 @@ public class SearchProvider : Object
> + return true;
> + }
> +
> +- private static string[] get_result_identifier (string[] terms)
> +- ensures (result.length == 0 || result.length == 1)
> ++ private async string[] get_result_identifier (string[] terms)
> + {
> + /* We have at most one result: the search terms as one string */
> +- if (can_parse (terms))
> ++ if (yield can_parse (terms))
> + return { terms_to_equation (terms) };
> + else
> + return new string[0];
> + }
> +
> +- public string[] get_initial_result_set (string[] terms)
> ++ public async string[] get_initial_result_set (string[] terms)
> + {
> +- return get_result_identifier (terms);
> ++ return yield get_result_identifier (terms);
> + }
> +
> +- public string[] get_subsearch_result_set (string[] previous_results, string[] terms)
> ++ public async string[] get_subsearch_result_set (string[] previous_results, string[] terms)
> + {
> +- return get_result_identifier (terms);
> ++ return yield get_result_identifier (terms);
> + }
> +
> +- public HashTable<string, Variant>[] get_result_metas (string[] results)
> ++ public async HashTable<string, Variant>[] get_result_metas (string[] results, GLib.BusName sender)
> + requires (results.length == 1)
> +- ensures (result.length == 1)
> + {
> +- Subprocess subprocess;
> + string stdout_buf;
> +- string stderr_buf;
> +-
> +- string[] argv = {"gnome-calculator", "--solve"};
> +- argv += results[0];
> +- argv += null;
> +-
> +- try
> +- {
> +- subprocess = new Subprocess.newv (argv, SubprocessFlags.STDOUT_PIPE | SubprocessFlags.STDERR_PIPE);
> +- }
> +- catch (Error e)
> +- {
> +- error ("Failed to spawn Calculator: %s", e.message);
> +- }
> +
> + try
> + {
> +- subprocess.communicate_utf8 (null, null, out stdout_buf, out stderr_buf);
> ++ yield solve_subprocess (results[0], true, out stdout_buf);
> + }
> + catch (Error e)
> + {
> +- error ("Failed reading result: %s", e.message);
> ++ return new HashTable<string, Variant>[0];
> + }
> +
> + var metadata = new HashTable<string, Variant>[1];
> +@@ -155,9 +201,11 @@ public class SearchProviderApp : Application
> +
> + public override bool dbus_register (DBusConnection connection, string object_path)
> + {
> ++ SearchProvider search_provider = new SearchProvider ();
> ++
> + try
> + {
> +- connection.register_object (object_path, new SearchProvider ());
> ++ connection.register_object (object_path, search_provider);
> + }
> + catch (IOError error)
> + {
> +@@ -165,6 +213,10 @@ public class SearchProviderApp : Application
> + quit ();
> + }
> +
> ++ shutdown.connect (() => {
> ++ search_provider.cancel ();
> ++ });
> ++
> + return true;
> + }
> + }
--
https://code.launchpad.net/~3v1n0/ubuntu/+source/gnome-calculator/+git/gnome-calculator/+merge/353828
Your team Ubuntu Desktop is subscribed to branch ~ubuntu-desktop/ubuntu/+source/gnome-calculator:ubuntu/master.
More information about the ubuntu-desktop
mailing list