Created attachment 163293 [details] [review]
[ShellApp] Split off running state handling into separate structure

This is a small memory usage optimization, and cleans up the code.
In particular, this will help for later patches which perform
more substantial operations on running apps.

Created attachment 163294 [details] [review]
Port to GDBus, except for gdmuser/

Primarily motivated by wanting the session bus as a singleton
hooked off of ShellGlobal for future work which will use GDBus.

gdmuser/ directory not ported because it's a huge hairy ball.
We do need to fix it eventually though; but I'm hoping there will
be some sane user management library to come out of the Fedora
account manager stuff that we can port to.

Created attachment 163295 [details] [review]
[ShellApp] Support GtkApplication, add actions API

Initial work to hook up with GtkApplication.  I need to make some
further changes to the GtkApplication DBus API so we don't need
to backref from the pid, among other things.

Review of attachment 163293 [details] [review]:

Looks good - minor comments follow:

::: src/shell-app.c
@@ +36,3 @@
  * @short_description: Object representing an application
  * SECTION:shell-app
+

Missing *

@@ +66,3 @@
 static guint shell_app_signals[LAST_SIGNAL] = { 0 };
+static ShellAppRunningState * ref_running_state (ShellAppRunningState *state) G_GNUC_UNUSED;
+static void create_running_state (ShellApp *app);

Should be *ref_running_state

@@ +621,2 @@
     return 1;
+  if (app->state == SHELL_APP_STATE_RUNNING)

I'm not sure if that should be part of the patch - saying that "an app which is not running compares equal to any other app" is changing the function's behavior. I'm sure there's some rationale behind it, but I can't figure it out in the context of this patch. I suspect that it will become clear when reading the other patches, but then the change should be done there.

@@ +694,3 @@
 {
                                 ShellApp   *app)
+  g_assert (app->running_state != NULL);

Is there a strong reason for using g_assert over g_return_if_fail here? Same question below.

Review of attachment 163294 [details] [review]:

::: src/shell-app-usage.c
@@ +356,2 @@
+  if (strcmp (signal_name, "StatusChanged") == 0
+      || !g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(u)")))

flip the sense of the conditional and just return early if it's not that, rather than indenting the entire rest of the function?

@@ +456,3 @@
   g_signal_connect (tracker, "app-state-changed", G_CALLBACK (on_app_state_changed), self);
 
+  g_bus_watch_name (G_BUS_TYPE_SESSION,

I think this would be simpler if you used g_bus_watch_proxy() instead of g_bus_watch_name()?

::: src/shell-global.c
@@ +879,3 @@
+  guint32 request_status;
+
+  reply = g_dbus_connection_call_sync (session,

Why don't you use g_bus_own_name here?

I suppose the answer is "because g_bus_own_name() makes it too difficult to abort if it can't acquire the name", but if so, that seems like people just need to beat davidz over the head more forcefully about this. It's not a convenience API if it's not convenient.

Review of attachment 163295 [details] [review]:

::: src/shell-app.c
@@ +910,3 @@
+                                    result, &error);
+  if (!reply)
+    return;

possibly leaking error

@@ +935,3 @@
+      g_variant_unref (action_data);
+      if (!enabled)
+        continue;

i think that's a bug? otherwise there's no point in having action->enabled, shell_app_action_get_enabled(), etc

@@ +1059,3 @@
+    }
+
+  app->running_state->name_watch_id = g_bus_watch_name (G_BUS_TYPE_SESSION,

again, can probably simplify by using g_bus_watch_proxy

(In reply to comment #5)
> I think this would be simpler if you used g_bus_watch_proxy() instead of
> g_bus_watch_name()?

Note that g_bus_watch_proxy() has been "nuked": http://git.gnome.org/browse/glib/commit/?id=32f2e9a85be

oh, I was going by the current docs on library.gnome.org, which I guess are from the last tarball. Anyway, "the API g_bus_watch_proxy() has been replaced with" then.

(In reply to comment #4)
>
> I'm not sure if that should be part of the patch - saying that "an app which is
> not running compares equal to any other app" is changing the function's
> behavior.

Not quite, actually the behavior of the function has always been effectively that not-running apps compare equal.  Not that a not-running app compares equal to any other.

> @@ +694,3 @@
>  {
>                                  ShellApp   *app)
> +  g_assert (app->running_state != NULL);
> 
> Is there a strong reason for using g_assert over g_return_if_fail here? Same
> question below.

Mmmm...I did think about it but I think I prefer g_return_if_fail to be used solely as preconditions, whereas the purpose of g_assert() is to trap things more obviously if we later add another application state or something.

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

Note from 622914: you should be able to remove dbus-glib-1 from the MUTTER_PLUGIN pkg-config line in configure.ac since it's now only used indirectly via the gdmuser library.

Created attachment 187857 [details] [review]
ShellApp: port to new GApplication API

DBus GApplication API changed from 2.25 to 2.27/2.28. Update accordingly,
and also make ShellApp implement the new GActionGroup interface,
and change the way the dbus id is associated to the app so that
already active applications are picked up when the shell starts.

Created attachment 187858 [details] [review]
Application Menu: add support for showing GApplication actions

Use the new GApplication support in ShellApp to create the application
menu. Supports plain (no state), boolean and double actions.
Includes a test application (as no other application uses GApplication
for actions)

Created attachment 201495 [details] [review]
ShellApp: port to new GDBusActionGroup and GMenuProxy API

GDBusActionGroup and GMenuProxy are new objects in GIO 2.32 that
help with accessing menus and actions of remote applications.
This patch makes it possible for the shell to associate an
application with a dbus name and from that a GMenu, that will
be shown as the application menu.

Created attachment 201496 [details] [review]
Application Menu: add support for showing GApplication actions

Use the new GApplication support in ShellApp to create the application
menu. Supports plain (no state), boolean and double actions.
Includes a test application (as no other application uses GApplication
for actions)

Gave this a quick try, but couldn't get it to work.

First I ran into an assertion in g_dbus_action_group_new_finish, after fixing that, your test application gave me assertions about various label or detailed_action arguments being NULL. After replacing all those, I end up with

    JS ERROR: !!!     message = '"model.get_item_link is not a function"'


Btw, if you want another testcase, the wip/gmenu branch of gtk has tests/testgmenu, which you can run with --export to get an dynamic menu exported.

Sounds like your gobject-introspection bindings aren't up to date.

  $ cd gobject-introspection/misc
  $ python update-glib-annotations.py ../../glib
  $ jhbuild make

I did rebuild gobject-introspection, and Gio-2.0.gir does have GMenuModel stuff in it. If there is other extra-special sauce involved, then I may be missing it, indeed.

Doing that made the warnings about model.get_item_link go away...but still no menu :-( other than 'Quit gjs'

Ok, now I got to see a menu - exciting !


After adjusting testgmenu in gtk+ to the bus naming convention expected here, I got to see its menu too. 

Issues I found so far: 

- the menu does not get populated initially, I need to move the focus off and back for the menu to be filled.

- testgmenu has a label with an underline mnemonic in it: _Undo. I expect that we'll see that quite a bit in practice. It would be good to be robust against that and strip such underlines out

- testgmenu also has an example of radio items, using a single action with a string state - each item has a string parameter, and activating the item is expected to change the state to that string. This is not currently supported by your code, it seems

Created attachment 201636 [details]
quick shot

One more issue: 

- the appmenu does not pick up changes to the action group / menu while the app is in focus; I have to move the focus away and back to have new items show up. Sensitivity and state changes do show up without a focus change.

(In reply to comment #22)
> One more issue: 
> 
> - the appmenu does not pick up changes to the action group / menu while the app
> is in focus; I have to move the focus away and back to have new items show up.
> Sensitivity and state changes do show up without a focus change.

Actually, it should be the opposite: GApplication does not propagate changes from the attached GSimpleActionGroup (I have a patch for that, but was waiting for wip/menus to merge before proposing it; now on bug 664328). On the other hand, GMenu changes should work, because GMenuExporter connects to the object provided by the app.

I'll look at it, using testgmenu from gtk

testgmenu has a problem, at the moment: ShellWindowTracker uses org.gtk.Application.Hello to find applications that support GAction/GMenu, so anything not using GApplication won't be tracked, unless the shell is restarted.
If possible, it should be rewritten using GApplication and g_application_set_action_group / g_application_set_menu.

testgmenu has both a --export and a --import, if you want to play with that.
And the --import side clearly picks up changes. I guess you are right that we are not actually listening for changes of the action group, but of the menu model.

I want to keep testgmenu as the low-level handrolled example it is now.

You can also look at examples/bloatpad for a GtkApplication-using example.

To make the shell pick up its menus, you currently need to patch gtk_application_get_menu to return NULL (see the FIXME in there)

One more thing to work out: Do we want the shell to enforce the presence of a 'quit' item ? Or do we rely on apps to provide one in their app menu ?

(In reply to comment #26)
> One more thing to work out: Do we want the shell to enforce the presence of a
> 'quit' item ? Or do we rely on apps to provide one in their app menu ?

I'm not particularly attached to "quit", but we need at least something for applications which don't export an app menu - both an empty menu and an unreactive app menu button are rather weird in my opinion.

(Now, our "quit" implementation is rather dumb, so it is probably a good idea to allow applications to implement their own)

The patch currently keeps the 'dumb quit' for apps which don't have an app menu.

I'm just wondering if we want the shell to look for an item with the 'quit' action, and if it isn't there, provide its own. The other alternative would be to provide a default 'quit' action from the GtkApplication side. Might be nicer.

Created attachment 201663 [details] [review]
Application Menu: add support for showing GApplication actions

Use the new GApplication support in ShellApp to create the application
menu. Supports plain (no state), boolean and double actions.
Includes a test application (as no other application uses GApplication
for actions)

Tested more, now it works with testgmenu (although the shell must be
restarted, for the GApplication issue). Also, includes stripping of
_, and supports radio actions.

Works great now, excellent work!

Created attachment 201828 [details] [review]
Application Menu: add support for showing GApplication actions

Switch to strings instead of quarks

Review of attachment 201495 [details] [review]:

::: src/shell-app.c
@@ +996,3 @@
+                                                          &error);
+
+  if (error) {

Wrong brace style.

@@ +1005,3 @@
+    g_clear_error (&error);
+
+    if (state->dbus_cancellable) {

Wrong brace style.

@@ +1007,3 @@
+    if (state->dbus_cancellable) {
+      g_object_unref (state->dbus_cancellable);
+      state->dbus_cancellable = NULL;

I recommend g_clear_object (&state->dbus_cancellable)

@@ +1010,3 @@
+    }
+
+    if (state->name_watcher_id) {

Wrong brace style.

@@ +1059,3 @@
+                           state->dbus_cancellable,
+                           on_action_group_acquired,
+                           g_object_ref (self));

We're adding a ref on the ShellApp here, but I don't see it being unreffed in on_action_group_acquired() where it should be, right?

@@ +1078,3 @@
+      g_cancellable_cancel (state->dbus_cancellable);
+      g_object_unref (state->dbus_cancellable);
+      state->dbus_cancellable = NULL;

g_clear_object()

@@ +1084,3 @@
+    {
+      g_object_unref (state->remote_actions);
+      state->remote_actions = NULL;

g_clear_object()

@@ +1090,3 @@
+    {
+      g_object_unref (state->remote_menu);
+      state->remote_menu = NULL;

g_clear_object()

@@ +1111,3 @@
+         (can only happen if you restart the shell
+         in the middle of the session)
+      */

Hmm?  I don't understand how that can happen - if you restart the shell, it'll stop all of the dbus requests the previous process was doing, no?

@@ +1361,3 @@
+
+  if (state->remote_actions)
+    g_object_unref (state->remote_actions);

g_clear_object()

@@ +1364,3 @@
+
+  if (state->remote_menu)
+    g_object_unref (state->remote_menu);

g_clear_object()

@@ +1548,3 @@
+  /* We should have been transitioned when we removed all of our windows */
+  g_assert (app->state == SHELL_APP_STATE_STOPPED);
+  g_assert (app->running_state == NULL);

You could probably break out some of these assertion additions as a separate patch (up to you, fine with me to leave them in here)

@@ +1624,3 @@
+                                   g_param_spec_string ("dbus-id",
+                                                        "Application DBus Id",
+                                                        "The DBus well-known name of the application",

Honestly I just use "" for gobject property descriptions.  Nothing is going to read or use them here.  For gtk+, glade does use them, but long term we want it to read the docs from the .gir.

::: src/shell-window-tracker.c
@@ +616,3 @@
+    {
+      /* FIXME delete this - the connection may have gone away */
+      g_warning ("%s\n", error->message);

Yeah, it is slightly lame to warn here if the user starts and app and then immediately closes it and we're still trying the GetConnectionPid call.

@@ +631,3 @@
+  if (app)
+    _shell_app_set_dbus_name (app, data->bus_name);
+  else {

Wrong brace style.

@@ +679,3 @@
+  gchar *bus_name = NULL;
+
+  g_variant_get (parameters, "(s)", &bus_name);

You can use &s to avoid the strdup/free.

@@ +708,3 @@
+
+  g_variant_iter_init (&iter, g_variant_get_child_value (res, 0));
+  while (g_variant_iter_loop (&iter, "s", &bus_name)) {

Wrong brace style.

@@ +740,3 @@
+  g_dbus_connection_call (g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL),
+                          "org.freedesktop.DBus",
+                          "/",

Should be /org/freedesktop/DBus, I guess the bus driver doesn't care though.

@@ +752,3 @@
   self->window_to_app = g_hash_table_new_full (g_direct_hash, g_direct_equal,
                                                NULL, (GDestroyNotify) g_object_unref);
+  self->pid_to_dbus_connection = g_hash_table_new_full (g_direct_hash, g_direct_equal,

You can just use NULL for g_direct_hash and equal.

Created attachment 201843 [details] [review]
ShellApp: port to new GDBusActionGroup and GMenuProxy API

GDBusActionGroup and GMenuProxy are new objects in GIO 2.32 that
help with accessing menus and actions of remote applications.
This patch makes it possible for the shell to associate an
application with a dbus name and from that a GMenu, that will
be shown as the application menu.

Fixed according to comments. I left in the g_assert() stuff
(not worth a patch, IMO) and the GParamSpec nick/blurbs (they're
not harming anyone there, and they're G_PARAM_STATIC_STRINGS).

Created attachment 202080 [details] [review]
Application Menu: add support for showing GApplication actions

Use the new GApplication support in ShellApp to create the application
menu. Supports plain (no state), boolean and double actions.
Includes a test application (as no other application uses GApplication
for actions)

Rebased on current master (previous patch had conflicts with Lang.Class mass port)

Review of attachment 201843 [details] [review]:

One minor bit.

I think we should replace the ListNames code though with an X property that GtkApplication puts on the window.  Ryan was going to look at that.

::: src/shell-window-tracker.c
@@ +708,3 @@
+
+  g_variant_iter_init (&iter, g_variant_get_child_value (res, 0));
+  while (g_variant_iter_loop (&iter, "s", &bus_name))

This leaks bus_name; you want to use "&s" (and make bus_name const char *).

(In reply to comment #35)
> Review of attachment 201843 [details] [review]:
> 
> One minor bit.
> 
> I think we should replace the ListNames code though with an X property that
> GtkApplication puts on the window.  Ryan was going to look at that.

Ok. Is there any place to track this work, or should I just wait for commit to glib master?

> ::: src/shell-window-tracker.c
> @@ +708,3 @@
> +
> +  g_variant_iter_init (&iter, g_variant_get_child_value (res, 0));
> +  while (g_variant_iter_loop (&iter, "s", &bus_name))
> 
> This leaks bus_name; you want to use "&s" (and make bus_name const char *).

No, g_variant_iter_loop frees before each round. (or at least, it's documented as such).
In any case, we can avoid the allocation, and use &s.

Created attachment 202159 [details] [review]
ShellApp: port to new GDBusActionGroup and GMenuProxy API

Rebased - use _DBUS_APPLICATION_ID property, drop ListNames code.

(In reply to comment #36)
> (In reply to comment #35)
> > Review of attachment 201843 [details] [review] [details]:
> > 
> > One minor bit.
> > 
> > I think we should replace the ListNames code though with an X property that
> > GtkApplication puts on the window.  Ryan was going to look at that.
> 
> Ok. Is there any place to track this work, or should I just wait for commit to
> glib master?

I just went ahead and pushed a commit to do it to the wip/gmenus branch of gtk+.  This bug now depends on the mutter bug to read it, and I rebased your patch to consume it.

(In reply to comment #36)
>
> No, g_variant_iter_loop frees before each round. (or at least, it's documented
> as such).

Oh, I see...weird.

> In any case, we can avoid the allocation, and use &s.

Yeah.  This code is gone now though =)

Some comments from looking at the code on the branch:

- I've recently changed the GTK+ implementation to do activation instead of change_state calls for stateful a tions. This is how things are supposed to work. For a boolean action, just activating it without a parameter is toggling it, and for a string-valued action, activating it with a string parameter is setting the state to that value.  We need to do the same here.

- I don't see any reason for supporting float-valued actions here. Its not called for in the designs, and doing it will only encourage crazy stuff.

Created attachment 202316 [details] [review]
Application Menu: add support for showing GApplication actions

Use the new GApplication support in ShellApp to create the application
menu. Supports plain (no state), boolean and double actions.
Includes a test application (as no other application uses GApplication
for actions)

Support for 'd' removed, change_action_state() replaced with activate_action()
(although I now wonder what change_action_state() means)

change_action_state does just what the name says, it changes the state. But it is just conceptually cleaner to have items always activate actions, and have the state change happen as a side-effect of the activation.

Can we get the latest patch on the wip/menus branch ?

I've just updated wip/menus-rebase2 to accomodate the latest changes in gio.

(In reply to comment #44)
> I've just updated wip/menus-rebase2 to accomodate the latest changes in gio.

No wait, gio changed again...

Attachment 202159 [details] pushed as 8764253 - ShellApp: port to new GDBusActionGroup and GMenuProxy API
Attachment 202316 [details] pushed as 4debedb - Application Menu: add support for showing GApplication actions