Created attachment 266662 [details] [review]
GtkActionMuxer: implement GRemoteActionGroup

If we insert a GRemoteActionGroup in the muxer, it makes sense
that the interface is exposed outside so that the platform data
can be sent togheter with the activation.

Created attachment 266663 [details] [review]
GtkMenuTrackerItem: make use of GRemoteActionGroup

When paired to a GtkActionMuxer, pass down the timestamp of the
action in the form of platform data.
The timestamp must come from an outside source (usually,
shell_global_get_current_time ()), so the API is changed.

Created attachment 266664 [details] [review]
RemoteMenu: adjust to GtkMenuTrackerItem change

Pass down the event time.

Created attachment 266665 [details] [review]
ShellApp: implement open_new_window and request_quit using GActionGroup

Reserve the action names "app.new-window" and "app.quit" as well known
names for the standard actions, and use them (when available)
instead of the X/launcher fallback.
The advantage of this is that the application can support
a new window action even if its Activate method would present
the most recent window, and that "Quit" would really quit the
application, even in face of inactivity timeouts or background
services.

Created attachment 266666 [details] [review]
AppDisplay: don't show a "New Window" menu item if not possible

If the application reports itself as single window, not show a
"New window" item that doesn't actually open a new window.

Review of attachment 266663 [details] [review]:

This needs to go upstream.

Review of attachment 266662 [details] [review]:

Same with this.

They are going upstream, see bug 722553.

Review of attachment 266664 [details] [review]:

Merge this with the patch that changes GtkMenuTrackerItem so bisect doesn't break.

Review of attachment 266666 [details] [review]:

::: src/shell-app.c
@@ +744,3 @@
+  /* Check if the desktop file says anything about the behavior of Activate()... */
+  return !g_desktop_app_info_get_boolean (G_DESKTOP_APP_INFO (app->info),
+                                          "X-GNOME-SingleWindow");

Do any applications set this, or is this just a convention you invented? If the latter, this doesn't really help us right now, since no desktop files will set this to TRUE. Perhaps better heuristics would be "if the app supports app.activate, but not app.new-window, then we know it's single-window", which should cover most of our single-window apps already without modifying .desktop files.

Review of attachment 266665 [details] [review]:

::: src/shell-app.c
@@ +657,3 @@
 
+static GVariant *
+get_platform_data (void)

I'd love to see this shared with gtkmenutrackeritem.c but not going to care about it too much. We already reimplement this in notificationDaemon.js, too.

::: tests/interactive/gapplication.js
@@ +29,2 @@
 function main() {
+    GLib.set_prgname('test-gapplication');

Any reason for this?

(In reply to comment #10)
> Review of attachment 266666 [details] [review]:
> 
> ::: src/shell-app.c
> @@ +744,3 @@
> +  /* Check if the desktop file says anything about the behavior of
> Activate()... */
> +  return !g_desktop_app_info_get_boolean (G_DESKTOP_APP_INFO (app->info),
> +                                          "X-GNOME-SingleWindow");
> 
> Do any applications set this, or is this just a convention you invented? If the
> latter, this doesn't really help us right now, since no desktop files will set
> this to TRUE. Perhaps better heuristics would be "if the app supports
> app.activate, but not app.new-window, then we know it's single-window", which
> should cover most of our single-window apps already without modifying .desktop
> files.

It's something I invented, and I hoped to send a notice to d-d-l to at least cover our apps. It's a very small desktop file change, and the default is conservative.

The issue is, some apps open a new window if you call Activate (or the equivalent for apps that don't use fdo.Application), just present the old one. Some of the apps in the first group have also a separate new-window action.
The other part of the issue is that, beside a small subset of apps, we don't call Activate() directly. Instead, we spawn the launcher binary and let the app do it's thing. Which means that using the ability to call Activate (that is, DBusActivatable=true) is not a good heuristics to know if that Activate means new window or not.
Additionally, switching to DBus activation is a lot harder than adding the SingleWindow boolean (you need to rename the desktop file, to provide a DBus service file and to work properly in service mode), so I expect that many more app authors will be willing to add the flag (like they did with UsesNotifications).

(In reply to comment #11)
> ::: tests/interactive/gapplication.js
> @@ +29,2 @@
>  function main() {
> +    GLib.set_prgname('test-gapplication');
> 
> Any reason for this?

Heh, it was an old patch I had lying around, and I rewrote it but kept the tests. I don't remember actually.

(In reply to comment #12)
> The issue is, some apps open a new window if you call Activate (or the
> equivalent for apps that don't use fdo.Application), just present the old one.
> Some of the apps in the first group have also a separate new-window action.
> The other part of the issue is that, beside a small subset of apps, we don't
> call Activate() directly. Instead, we spawn the launcher binary and let the app
> do it's thing. Which means that using the ability to call Activate (that is,
> DBusActivatable=true) is not a good heuristics to know if that Activate means
> new window or not.

Erm, I just picked an action at random that our apps can be expected to implement. I wasn't suggesting calling "app.activate" at all. How about "supports app.quit but not app.new-window"?

(In reply to comment #13)
> 
> Erm, I just picked an action at random that our apps can be expected to
> implement. I wasn't suggesting calling "app.activate" at all. How about
> "supports app.quit but not app.new-window"?

I don't get it, why would app.quit be relevant to app.new-window?

(In reply to comment #14)
> I don't get it, why would app.quit be relevant to app.new-window?

We're trying to detect whether an app can open a new window.. If we see that there's an action called "app.quit", we can guess that the app implements all the actions it cares about. So, we can make a pretty educated guess that if "app.new-window" isn't implemented, the app is a single-instance app.

For an application doesn't use GApplication, e.g. Firefox, there's no muxer, so we just take a guess that it supports "New Window" through spawning the process again.

For an application that uses GApplication but doesn't implement any actions, we don't see either "app.quit" or "app.new-window", so we just take a guess that it supports "New Window" through Activate() or spawning the process again.

For an application that uses GApplication but only implements "app.quit", e.g. Documents, we see that "app.quit" exists but "app.new-window" doesn't, so we can assume that it can't support "New Window".

For an application that uses GApplication and supports "app.new-window", we know it supports "New Window".

Created attachment 269074 [details] [review]
AppDisplay: don't show a "New Window" menu item if not possible

If the application reports itself as single window (through
an explicit indication in the desktop file or some heuristics),
not show a "New window" item that doesn't actually open a new window.

Created attachment 269075 [details] [review]
ShellApp: remove impossible check

The action muxer is created and destroyed with the running state,
it can never be NULL.

Review of attachment 266664 [details] [review]:

Gtk+ wants to solve this differently.

Review of attachment 269075 [details] [review]:

::: src/shell-app.c
@@ +586,3 @@
         }
 
+      g_assert (app->running_state->muxer);

Maybe just assert on app->running_state? As noted, muxer is created with running_state anyway, and we catch a segfault if running_state is indeed unset (well, not that an assert makes much of a difference from a user's POV ...)

Review of attachment 266665 [details] [review]:

Ok, I'm going to reject this, because don't doing startup notification regresses the ability to launch an app on a specific desktop.
As for request_quit(), it's only used for legacy apps, so there is little point in a GtkApplication code path there.

Review of attachment 269075 [details] [review]:

Yep, looks correct.

Review of attachment 269074 [details] [review]:

::: src/shell-app.c
@@ +726,3 @@
+
+     We don't consider non-unique GtkApplications here to handle cases like
+     evince, which don't export a new-window action because each window is in

Maybe it's late, but I don't see how any of the conditions below has anything to do with uniqueness?

Review of attachment 269074 [details] [review]:

It still doesn't implement my suggestions from before.

Comment on attachment 269075 [details] [review]
ShellApp: remove impossible check

Attachment 269075 [details] pushed as d555fd7 - ShellApp: remove impossible check

(In reply to comment #22)
> Review of attachment 269074 [details] [review]:
> 
> ::: src/shell-app.c
> @@ +726,3 @@
> +
> +     We don't consider non-unique GtkApplications here to handle cases like
> +     evince, which don't export a new-window action because each window is in
> 
> Maybe it's late, but I don't see how any of the conditions below has anything
> to do with uniqueness?

A non-unique gtkapplication has a null gtk-application-id (which is the well-known dbus name). A non gtkapplication is not unique by definition.

There were no really negative comments (sorry Jasper but looking at app.quit is not a useful suggestion in addition to simple gtkapplication), and we had duplicates, so I'm going to push and close this one.

Attachment 269074 [details] pushed as 3182aba - AppDisplay: don't show a "New Window" menu item if not possible

*** Bug 647105 has been marked as a duplicate of this bug. ***