Created attachment 201780 [details] [review]
gio: introduce desktop shorcuts

This patch introduces support in GDesktopAppInfo for so called "desktop
shortcuts". Those are extra names/exec pairs that can be associated
with an application, to create static launcher actions.
The format is the same as Unity, which first implemented this.

Created attachment 201819 [details] [review]
gio: introduce desktop shorcuts

This patch introduces support in GDesktopAppInfo for so called "desktop
shortcuts". Those are extra names/exec pairs that can be associated
with an application, to create static launcher actions.
The format is the same as Unity, which first implemented this.

Completed with docs and annotations.

Created attachment 201820 [details] [review]
gio: introduce desktop shorcuts

This patch introduces support in GDesktopAppInfo for so called "desktop
shortcuts". Those are extra names/exec pairs that can be associated
with an application, to create static launcher actions.
The format is the same as Unity, which first implemented this.

Before even considering this, I'd like the desktop entry syntax proposed and discussed.

(In reply to comment #4)
> Before even considering this, I'd like the desktop entry syntax proposed and
> discussed.

This uses Unity . If there are no particular concerns, I think we should stick to it, so that app writers and distributors don't need to deal with different, incompatible syntaxes.
Still, I was thinking of proposing this to Freedesktop if approved for glib, and then deprecating the X-Ayatana prefix with a proper extension to the Desktop Entry Specification.

The spec itself is pretty simple: one X-Ayatana-Desktop-Shortcuts key with a list of strings, each identifies a group of the form [<key> Shortcut Group. Each group has Name, Exec, TargetEnvironment.
The set of properties for a group could be extended (I was thinking of Icon, and then maybe MimeType, for example for Print actions), but I would say they're fine for the first iteration.

It really needs to be discussed on the list first, I think there's quite a few questionable choices here that I'd like to see some explanation for. The 'Unity uses this' argument is not very convincing to me by itself.

One of the reasons why this needs to go into the spec first is this:

desktop-file-validate /builddir/build/BUILDROOT/gnome-screenshot-3.3.1-1.fc17.x86_64/usr/share/applications/gnome-screenshot.desktop
/builddir/build/BUILDROOT/gnome-screenshot-3.3.1-1.fc17.x86_64/usr/share/applications/gnome-screenshot.desktop: error: file contains group "Screen Shortcut Group", but groups extending the format should start with "X-"

elementary project is also interested in this format. We have the same use case as Unity and GNOME Shell. 
We also might use it for specifying Contractor actions, which will require specifying a list of supported mime types for each action.

Created attachment 206912 [details] [review]
gio: introduce desktop actions

This patch introduces support in GDesktopAppInfo for so called "desktop
actions". Those are extra names/exec pairs that can be associated
with an application, to create static launcher actions.
The format is included in the latest version of the Desktop Entry
Specification.

Rebased, and updated for the specification that was approved at freedesktop.

I must say that I don't really like the idea of exposing these actions as independent objects that can outlive their owners. I'd rather go with a much more minimal api, a la

const gchar **g_desktop_info_get_actions (GDesktopInfo *info,
                                          const gchar  *desktop_env)

gboolean g_desktop_info_action_launch_uris (GDesktopInfo *info,
                                            const gchar  *action,
                                            ...)

I'd also like to investigate if we should have similar support on os x or win32,
in which case the api should be

g_app_info_get_actions / g_app_info_action_launch_uris

I went with GDesktopAction because we may want to support more keys from action groups (for example Icon, which is already part of the spec), and because it allows to have the *ShowIn check explicit rather than done in get_actions(). It also made a nice parallel with GDesktopAppInfo.

GDesktopActions can't really outlive they're parent. In C the expectation is no memory management at all (hence (transfer none)) and rely on the parent GAppInfo to keep everything alive. Obviously, JS and Python can do all kinds of nasty things with objects, but they're already discouraged from doing so (it breaks object identity, for example).

As for other systems, Windows 7 interface is at http://msdn.microsoft.com/en-us/library/dd378402(v=vs.85).aspx, and it is the other way round: the application is invoked in a special mode and registers its actions with the desktop shell. There is no way for third parties to query jumplists (and I don't expect one would use gio to implement a replacement windows shell).
I'm not aware of standard jumplists for Mac OS.

Ping on this?

I agree with Matthias about the API minimalism and I also don't think we should support launching URIs on desktop actions.  I'll look into this.

Created attachment 246332 [details] [review]
Implement the Desktop Action specification

This is a much smaller API vs. the previous patch and it includes
support for DBusActivatable apps.

Created attachment 246337 [details] [review]
Implement the Desktop Action specification

Found a small bug in the sending of the D-Bus message.  Fixed in this revision.

One thing that should be pointed out: this patch ignores OnlyShowIn, so eg. actions that only make sense in Unity will show up in GNOME Shell.

It's also probably (unfortunately) going to also need GAppLaunchContext* on the activate_action() function... :/

1) what's wrong with GAppLaunchContext?
2) it's not needed for OnlyShowIn -- g_desktop_app_info_set_desktop_env sets a global.

Oh, and I should probably point out, since nobody else has, that the spec is now upstream. I'm not sure if this implementation is compliant:

http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s08.html#extra-actions-identifier

(In reply to comment #20)
> Oh, and I should probably point out, since nobody else has, that the spec is
> now upstream. I'm not sure if this implementation is compliant:
> 
> http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s08.html#extra-actions-identifier

The fact that this is upstream is what caused me to want to support it, and I worked from the spec...

Created attachment 246375 [details] [review]
Implement the Desktop Action specification

Now with 100% more OnlyShowIn/NotShowIn and GAppLaunchContext!

Review of attachment 246375 [details] [review]:

::: gio/gdesktopappinfo.c
@@ +1720,3 @@
 {
+  /* We may have a spurious failure, so loop... */
+  while (g_atomic_pointer_get (&g_desktop_env) == NULL)

GOnce?

@@ +3781,3 @@
+
+/**
+ * g_desktop_app_info_should_show_action:

bad doc?

@@ +3922,3 @@
+  session_bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL);
+
+  if (session_bus && info->app_id)

I think it would be nice to explicitly be able to explicitly specify which path you go down, especially as you mention explicitly in the documentation that this can be used to activate applications that don't necessarily have exposed actions in the desktop file.

Being able to say "I want to ensure that we never pull up the desktop file and use the Exec= line for an action" is a better API.

::: gio/tests/Makefile.am
@@ +228,2 @@
 dist_test_data += \
+	actions.desktop				\

appinfo-test-actions.desktop ?

::: gio/tests/desktop-app-info.c
@@ +386,3 @@
+   * be OK.
+   *
+   * 600 * 100ms = 60 seconds.

I'd like Colin's OK on this.

Review of attachment 246375 [details] [review]:

::: gio/gdesktopappinfo.c
@@ +1719,3 @@
 g_desktop_app_info_set_desktop_env (const gchar *desktop_env)
 {
+  /* We may have a spurious failure, so loop... */

I don't see how we can get a spurious failure here? Surely our atomics are not weak, so if the compare_and_exchange succeeds it is now non-null, and if it failed its because it was not NULL to begin with.

I don't see how we need anything more than a single compare_and_exchange.

@@ +3731,3 @@
+ * action should actually be shown.
+ *
+ * Returns: a list of strings, always non-%NULL

(array zero-terminated=1) (element-type utf8) (transfer full)

@@ +3836,3 @@
+ *
+ * Returns: the locale-specific action name
+ *

(transfer full)

@@ +3869,3 @@
+ * @action_name: the name of the action as from
+ *   g_desktop_app_info_list_actions()
+ * @parameter: should be %NULL, but see below

(allow-none)

@@ +3929,3 @@
+      g_variant_builder_init (&params, G_VARIANT_TYPE ("av"));
+      if (parameter)
+        g_variant_builder_add (&params, "v", parameter);

Don't you ever want to pass in an array of parameters? (i.e. more than one?

::: gio/tests/desktop-app-info.c
@@ +383,3 @@
+
+  /* I hate time-based conditions in tests, but this will wait up to one
+   * whole minute for "touch file" to finish running.  I think it should

Couldn't you use startup notification for this? Then we get a test for that working too.

Review of attachment 246375 [details] [review]:

::: gio/tests/actions.desktop
@@ +1,3 @@
+[Desktop Entry]
+Type=Application
+Actions=frob;tweak;twiddle

there's a terminal semicolon missing

Created attachment 248254 [details] [review]
Implement the Desktop Action specification

For some time, the desktop file specification has supported "additional
application actions".  This is intended to allow for additional methods
of starting an app, such as a mail client having a "Compose New Message"
action that brings up the compose window instead of the folder list.

This patch adds support for this with a relatively minimal API.

In the case that the application is a GApplication and DBusActivatable,
desktop actions are translated into GActions that have been added to the
application with g_action_map_add_action().  This more or less closes
the loop on being able to activate an application with an action
invocation (instead of 'activate').

Changes vs last patch:

 - using g_once_init_enter() now instead of the loop
 - fix wrong doc string
 - rename desktop file in testcase, as suggsted
 - add annotations
 - add trailing ; to Actions= line

also: I agree with Jasper's concern that it's weird to deal with arbitrary D-Bus action invocation (with parameters) through the same API as might also cause an exec.  As such, I've restricted the API to only work with actions that are in the desktop file and I've added g_return_if_fail() on this and other APIs to ensure that they are only used with listed actions.

If and when we have actual usecases for invocation of arbitrary unknown actions via GDesktopAppInfo then we can talk about it....

Review of attachment 248254 [details] [review]:

::: gio/gdesktopappinfo.c
@@ +3816,3 @@
+  g_return_val_if_fail (G_IS_DESKTOP_APP_INFO (info), FALSE);
+  g_return_val_if_fail (action_name != NULL, FALSE);
+  g_return_if_fail (app_info_has_action (info, action_name));

Should be g_return_val_if_fail()

@@ +3855,3 @@
+  g_return_val_if_fail (G_IS_DESKTOP_APP_INFO (info), NULL);
+  g_return_val_if_fail (action_name != NULL, NULL);
+  g_return_if_fail (app_info_has_action (info, action_name));

Should be g_return_val_if_fail()

@@ +3933,3 @@
+      if (exec_line)
+        g_desktop_app_info_launch_uris_with_spawn (info, session_bus, exec_line, NULL, launch_context,
+                                                   _SPAWN_FLAGS_DEFAULT, NULL, NULL, NULL, NULL, NULL);

exec_line is leaked.

Jasper doesn't like OnlyShowIn on actions so he's going to take his campaign to remove them from the spec to xdg-list.

Meanwhile, this patch is blocked.

http://lists.freedesktop.org/archives/xdg/2013-July/012835.html

I don't really want to block the patch on this. If we can take it out and then put it in later, even if in a slightly ugly way, I'd be fine.

Review of attachment 248254 [details] [review]:

::: gio/gdesktopappinfo.c
@@ -136,3 @@
 						g_desktop_app_info_iface_init))
 
-G_LOCK_DEFINE_STATIC (g_desktop_env);

Can you remove the lock in a separate commit?

Created attachment 248946 [details] [review]
Implement the Desktop Action specification

For some time, the desktop file specification has supported "additional
application actions".  This is intended to allow for additional methods
of starting an app, such as a mail client having a "Compose New Message"
action that brings up the compose window instead of the folder list.

This patch adds support for this with a relatively minimal API.

In the case that the application is a GApplication and DBusActivatable,
desktop actions are translated into GActions that have been added to the
application with g_action_map_add_action().  This more or less closes
the loop on being able to activate an application with an action
invocation (instead of 'activate').

Review of attachment 248946 [details] [review]:

This looks good to me. I'm not overly happy about sometimes spawning off a new process according to the Exec line if you can't connect to DBus (I think that should probably be a fail altogether), but it should be OK.

Review of attachment 248946 [details] [review]:

(whoops, should be marked as ACN)

this was committed