GNOME Bugzilla – Bug 775956
Make Geary DBus-activatable
Last modified: 2019-04-12 12:10:41 UTC
Now we have a conventional app ID, we can make Geary DBus-activatable, which gives us a few advantages. The biggest resource for us would be [1]. We should also look at how this might change our autostart-desktop file since we would then already have service that can be launched at startup. [1] https://wiki.gnome.org/HowDoI/DBusApplicationLaunching
Created attachment 342317 [details] [review] Proposed patch - Make Geary DBus-activatable I recommend reading through the list in the commit message for remarks about what might've changed. Some other remarks about the patch itself: * After installing Geary, it is now possible to use `gapplication launch org.gnome.Geary`, as the DBus service file automatically launches. * Command line arguments are now passed on to config, rather than using the global Args.<args>. I've edited the files where these were used, but with the instance property of GearyApplication. Using dependency injection in the constructor (which would be the final goal) made the commit a bit too noisy. * The --quit argument still works, but calling this will give an error in the calling application, as it cannot send a reply before exiting (due to the way GApplication.run() works). * This also solves bug 775955.
Review of attachment 342317 [details] [review]: Looks good! Have you tested it under Debian stable? Some comments & questions below. ::: desktop/geary-autostart.desktop.in @@ +7,3 @@ Icon=geary TryExec=geary +Exec=geary --gapplication-service Are users that already have this file installed getting this new version migrated? ::: src/client/application/geary-application.vala @@ +106,3 @@ */ public bool is_background_service { + get { return (this.flags & ApplicationFlags.IS_SERVICE) != 0; } So in the case where the startup_notifications pref is set, but Geary hasn't been started using --gapplication-service arg, this change means despite the pref, Geary will just quit when the main window is closed, right? Is that a good thing? @@ +447,3 @@ private void on_activate_mailto(SimpleAction action, Variant? param) { if (this.controller != null && param != null) { + this.controller.compose(param.get_string()); These aren't equivalent, are they? The MAILTO action is used when invoked from the command line with a "mailto:[address]" arg, whereas COMPOSE is used from the desktop shell when the user selects the Compose action in the app's skip lists. I feel like we might want to keep the different behaviour here, but I can't remember why at the moment. ::: src/client/application/geary-config.vala @@ +41,3 @@ + // Can be set as an arguments + public bool revoke_certs { get; set; default = false; } + This lets us get rid of all of the global Args vars, right? Excellent stuff! ::: src/client/application/geary-controller.vala @@ +291,3 @@ Geary.Engine.instance.untrusted_host.disconnect(on_untrusted_host); + if (this.main_window != null) { So here there's a whole bunch of null checks needed to avoid segfaults when calling close_async if open_async hasn't been called first. Wouldn't it be better to add a "opened" property to the controller, set it try at the end of open_async, and just not call close_async from the application if that is not true? That'd avoid a lot of null checks, which makes the method a lot less verbose, which would be a good thing considering how complicated the controller is already. ::: src/client/composer/composer-widget.vala @@ +542,3 @@ s.enable_java_applet = false; s.enable_plugins = false; + s.enable_developer_extras = GearyApplication.instance.config.enable_inspector; I'd like to to migrate away from using these global instance vars - they make having multiple main windows and unit testing hard. Can you get the config object some other way? @@ +2121,3 @@ }); + if (GearyApplication.instance.config.enable_inspector) { Ditto. ::: src/client/conversation-viewer/conversation-message.vala @@ +317,3 @@ web_view.copy_clipboard(); }); + add_action(ACTION_OPEN_INSPECTOR, GearyApplication.instance.config.enable_inspector).activate.connect(() => { Ditto about the instance global var. @@ +349,3 @@ context_menu_main = (MenuModel) builder.get_object("context_menu_main"); context_menu_contact = (MenuModel) builder.get_object("context_menu_contact"); + if (GearyApplication.instance.config.enable_inspector) { And ditto again. ::: src/client/conversation-viewer/conversation-web-view.vala @@ +54,3 @@ config.enable_java_applet = false; config.enable_plugins = false; + config.enable_developer_extras = GearyApplication.instance.config.enable_inspector; Ditto. :)
Hey Niels, I'm feeling like we should just take the plunge with this, since I'd like to get moving with Bug 766133 as well and it's a requirement for persistent notifications. The patch will probs need some rebasing, do you have the time or shall I have a go at it?
Bump tickets to 0.14 that aren't going to make 0.13.
*** Bug 775955 has been marked as a duplicate of this bug. ***
Patch with some extra cleanup is landing over in: https://gitlab.gnome.org/GNOME/geary/merge_requests/204 Thanks Neils! :)