Created attachment 242843 [details] [review]
GApplication: implement fd.o application spec

The freedesktop application specification is largely overlapping the
GLib application D-Bus interface but implementing it will allow for
applications to be launched directly from desktop files, which we want.

We keep the old Gtk interface for compatibility reasons and because it
has some functionality not in the freedesktop spec (Busy state,
CommandLine, etc.).

Created attachment 242844 [details] [review]
GDesktopAppInfo: support DBusActivatable

Support the sender-side of the freedesktop application specification for
cases that we find 'DBusActivatable=true' in the desktop file.

Link to spec?  I don't see any discussion of it in xdg-list.

(In reply to comment #3)
> Link to spec?  I don't see any discussion of it in xdg-list.

pending me getting commit access :)

Review of attachment 242844 [details] [review]:

::: gio/gdesktopappinfo.c
@@ +433,3 @@
+          else
+            g_free (basename);
+

You leak basename in case the if is not entered...I would just strdup it in the g_dbus_is_interface_name() condition and always free it after the if block.

@@ +1523,3 @@
+  g_dbus_connection_call (session_bus, info->app_id, object_path, "org.freedesktop.Application",
+                          uris ? "Open" : "Activate", g_variant_builder_end (&builder),
+  GVariantBuilder builder;

Shouldn't you check for errors and return an appropriate value accordingly here?

On the other hand that would force you to use g_dbus_connection_call_sync(), but I'm not sure I like silently ignoring errors better.

Calling _sync() is really not acceptable here because this is an application starting up....

It's already documented that there is no real way to know if starting the app failed... Basically, once the child process has forked off, it's gone... even failing to load shared libraries won't be detected.

(In reply to comment #5)
> Review of attachment 242844 [details] [review]:
> You leak basename in case the if is not entered...I would just strdup it in the
> g_dbus_is_interface_name() condition and always free it after the if block.

Ya... I should know better than this.  Fixed.

Meanwhile, I put a bug upstream:  https://bugs.freedesktop.org/show_bug.cgi?id=64066

> We keep the old Gtk interface for compatibility reasons and because it
> has some functionality not in the freedesktop spec (Busy state,
CommandLine, etc.).

So whats the story going forward, for adding new functionality ? Do we keep the fd.o interface as a minimal shared base and add new functionality to the org.gtk interface ?

it depends on if we believe it is useful to standardise the behaviour and if we are successful in doing so.

Created attachment 243700 [details] [review]
GApplication: set prgname to appid for services

Since services are based on D-Bus activation and desktop files are
supposed to be named like the busname for DBusActivatable applications
and since gnome-shell wants wmclass equal to the desktop file name, we
therefore want wmclass equal to the application ID in this case.

wmclass is determined from the prgname, which is otherwise pretty
pointless to set to some random thing in $(libexec) for a D-Bus service,
so set that to the appid.

This means that for D-Bus services, the following things are now all the
same:

 - application ID
 - prgname
 - wmclass property set on all windows
 - desktop file name
 - well-known bus name

There are not many applications running as D-Bus services at present so
this shouldn't impact anybody except for gnome-clocks (where this change
will be fixing a bug).

Review of attachment 242843 [details] [review]:

::: gio/gapplicationimpl-dbus.c
@@ +269,3 @@
+      const gchar *name;
+
+      /* Only on the freedesktop interface */

Is there any reason to not at least make org.gtk.Application a superset of org.freedesktop.Application ?

(In reply to comment #12)
> Is there any reason to not at least make org.gtk.Application a superset of
> org.freedesktop.Application ?

Because we also have an actiongroup interface.  The freedesktop interface has ActivateAction because actiongroups (in their own right) are not available at that level.

Review of attachment 242844 [details] [review]:

::: gio/gdesktopappinfo.c
@@ +1673,3 @@
+                                                  user_setup_data,
+                                                  pid_callback,
+                                                  pid_callback_data,

launch_uris_as_manager docs say:

In contrast to g_app_info_launch_uris(), all processes created will
always be run directly as children as if by the UNIX fork()/exec()
calls.

Looks like you break that guarantee here ?

Review of attachment 243700 [details] [review]:

Doesn't look like we document the prgname setting aspect of GApplication at all, currently.
It is probably worth mentioning this in the release notes, since we do change observable behaviour here.

(In reply to comment #14)
> Review of attachment 242844 [details] [review]:
> 
> ::: gio/gdesktopappinfo.c
> @@ +1673,3 @@
> +                                                  user_setup_data,
> +                                                  pid_callback,
> +                                                  pid_callback_data,
> 
> launch_uris_as_manager docs say:
> 
> In contrast to g_app_info_launch_uris(), all processes created will
> always be run directly as children as if by the UNIX fork()/exec()
> calls.
> 
> Looks like you break that guarantee here ?

(as per discussion on IRC) I will update the patch so that the docs say "if the process is launched via fork/exec then you will get a pid and user setup callback..."

This API was really meant to get information from processes that the shell could not otherwise track (due to inability to match based on wmclass, startup notification or other mechanisms).  GApplications launched via D-Bus don't suffer from these problems so the pid callback is not needed.  Old programs with these problems will continue to be fork/execed, so the callback will still happen.

(In reply to comment #13)
> (In reply to comment #12)
> > Is there any reason to not at least make org.gtk.Application a superset of
> > org.freedesktop.Application ?
> 
> Because we also have an actiongroup interface.  The freedesktop interface has
> ActivateAction because actiongroups (in their own right) are not available at
> that level.

Ah, I missed that. Makes sense then

Comment on attachment 243700 [details] [review]
GApplication: set prgname to appid for services

Attachment 243700 [details] pushed as 7baea0a - GApplication: set prgname to appid for services

Pushed with the suggested documentation changes (and a small logic tweak).

Comment on attachment 242843 [details] [review]
GApplication: implement fd.o application spec

Attachment 242843 [details] pushed as b4df86f - GApplication: implement fd.o application spec

Created attachment 246103 [details] [review]
GDesktopAppInfo: support DBusActivatable

Support the sender-side of the freedesktop application specification for
cases that we find 'DBusActivatable=true' in the desktop file.

The patch has been updated to fix the memory leak issue and the docs for _as_manager() have also been changed.

Review of attachment 246103 [details] [review]:

A few mechanical comments only...the idea seems fine but I haven't looked at the big picture in detail.

::: gio/gdesktopappinfo.c
@@ +294,3 @@
   char *try_exec;
   char *exec;
+  gboolean dbusable;

An odd word; how about bus_activatable?  It's not much more verbose.

@@ +436,3 @@
   info->keyfile = g_key_file_ref (key_file);
 
+  g_free (basename);

How does this compile?  Basename isn't defined in this scope.

@@ +1521,3 @@
+  g_variant_builder_close (&builder);
+
+  /* This is non-nlocking API.  Similar to launching via fork()/exec()

blocking

@@ +1567,3 @@
     }
 
+  return success;

Still needs g_clear_object (&session_bus) in theory; not that this really matters since pretty much no one unrefs it and we end up with a refcount on the bus of over 9000.

(In reply to comment #22)
> @@ +436,3 @@
>    info->keyfile = g_key_file_ref (key_file);
> 
> +  g_free (basename);
> 
> How does this compile?  Basename isn't defined in this scope.

lol.  'basename' is a public function of the C library...  oops =)

Created attachment 246277 [details] [review]
GDesktopAppInfo: support DBusActivatable

Support the sender-side of the freedesktop application specification for
cases that we find 'DBusActivatable=true' in the desktop file.

Review of attachment 246277 [details] [review]:

One super minor thing, feel free to ignore.

::: gio/gdesktopappinfo.c
@@ +425,3 @@
+      last_dot = strrchr (basename, '.');
+
+      if (g_str_equal (last_dot, ".desktop"))

I'd be a bit more comfortable if this was:

if (last_dot != NULL && g_str_equal (last_dot, ".desktop"))

Yeah I don't think we'd ever load a desktop file with no ., but might as well be safe.

Before we close out this bug - it would be great to have some tests exercising the org.freedesktop.Application dbus api, in the name of coverage.

Comment on attachment 246277 [details] [review]
GDesktopAppInfo: support DBusActivatable

Attachment 246277 [details] pushed as afc8b10 - GDesktopAppInfo: support DBusActivatable

Tests coming soon...

Created attachment 246338 [details] [review]
Add a testcase for DBusActivatable=true

Add a fairly realistic testcase that ensures that GDesktopAppInfo with
DBusActivatable=true can successfully talk to GApplication for a variety
of purposes.

testcase depends on patch in bug 664444.

Review of attachment 246338 [details] [review]:

Looks good, in general. We just need to land the desktop entry spec update soon, so this all becomes 'legal'.

::: gio/tests/org.gtk.test.dbusappinfo.desktop
@@ +5,3 @@
+
+[Desktop Action frob]
+Name=Frobnicate

After looking up if the desktop action thing ever made it into the spec, I find that this desktop file is not valid according to the current spec: the Exec key in the Desktop Action group is required. Does your spec change make that optional ?

Also, you have your simple actions here, but not send-message. I guess there's no plan to update the spec to make it possible to declare such actions ?

(In reply to comment #30)
> ::: gio/tests/org.gtk.test.dbusappinfo.desktop
> @@ +5,3 @@
> +
> +[Desktop Action frob]
> +Name=Frobnicate
> 
> After looking up if the desktop action thing ever made it into the spec, I find
> that this desktop file is not valid according to the current spec: the Exec key
> in the Desktop Action group is required. Does your spec change make that
> optional ?

Correct.

> Also, you have your simple actions here, but not send-message. I guess there's
> no plan to update the spec to make it possible to declare such actions ?

This is what I was talking about when I said (in the docs):

+ * If you know the application corresponding to @info to be a
+ * #GApplication with "DBusActivatable" set to "true" in its desktop
+ * file, then this function can be used to invoke arbitrary actions
+ * (with arbitrary @parameter value) on the application's primary
+ * ("app.") action group, even if they are not listed in the desktop
+ * file.  This may or may not work with non-#GApplications, and will
+ * almost definitely not work at all if "DBusActivatable" is not true.

Bug 697994 is going to affect this a little...

When doing g_app_info_launch() or g_desktop_app_info_activate_action() without a GAppLaunchContext, in the DBusActivatable=true case, we can do better than we did before.  Specifically, once we have the platform_data hooks, we can use this to fill in a reasonable value for the platform_data on the D-Bus calls, even if we are missing the launch context.  This will allow us to at least forward timestamp information...

Should we wrap this up ?

Attachment 246338 [details] pushed as 3b1b044 - Add a testcase for DBusActivatable=true