After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775956 - Make Geary DBus-activatable
Make Geary DBus-activatable
Status: RESOLVED FIXED
Product: geary
Classification: Other
Component: client
unspecified
Other Linux
: Normal normal
: 0.14.0
Assigned To: Geary Maintainers
Geary Maintainers
: 775955 (view as bug list)
Depends on:
Blocks: 713734 766133
 
 
Reported: 2016-12-11 18:48 UTC by Niels De Graef
Modified: 2019-04-12 12:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch - Make Geary DBus-activatable (40.69 KB, patch)
2016-12-21 13:41 UTC, Niels De Graef
needs-work Details | Review

Description Niels De Graef 2016-12-11 18:48:57 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
Comment 1 Niels De Graef 2016-12-21 13:41:29 UTC
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.
Comment 2 Michael Gratton 2016-12-23 02:02:16 UTC
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. :)
Comment 3 Michael Gratton 2017-10-11 17:10:54 UTC
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?
Comment 4 Michael Gratton 2018-06-26 04:46:35 UTC
Bump tickets to 0.14 that aren't going to make 0.13.
Comment 5 Michael Gratton 2019-04-09 12:24:41 UTC
*** Bug 775955 has been marked as a duplicate of this bug. ***
Comment 6 Michael Gratton 2019-04-12 12:10:41 UTC
Patch with some extra cleanup is landing over in: https://gitlab.gnome.org/GNOME/geary/merge_requests/204

Thanks Neils! :)