GNOME Bugzilla – Bug 643736
GApplication doesn't emit dbus signals on action updates
Last modified: 2011-12-16 16:46:52 UTC
There's code in gapplicationimpl-dbus.c for processing action update signals when remote, but not for emitting them. I'm attaching a patch to 1) make GApplication proxy signals from its internal action group, 2) make the dbus code convert those into dbus signals.
Created attachment 182310 [details] [review] patch
Created attachment 182341 [details] [review] don't crash on receiving an action update signal
Created attachment 182344 [details] [review] don't crash on receiving an action update signal slightly better version, doesn't add an unused variable
Created attachment 182587 [details] [review] this one actually works not sure what I was thinking with the last one. I've actually tested this a bit.
Review of attachment 182587 [details] [review]: ::: gio/gapplicationimpl-dbus.c @@ +485,3 @@ + + if (g_variant_iter_init (&state_iter, state)) { + g_variant_iter_next (&state_iter, "v", &info->state); info->state may end up not being set at all here (and g_slice_new doesn't give you initialised memory). also: this is a very odd use of an iterator. i'd do something more like this instead: if (g_variant_n_children (state)) g_variant_get (state, 0, "v", &info->state); else info->state = NULL; @@ +832,1 @@ + impl->actions = g_hash_table_new (g_str_hash, g_str_equal); oops. looks like i forgot about that :)
Review of attachment 182310 [details] [review]: ::: gio/gapplication.c @@ +378,3 @@ + GApplication *app) +{ + g_action_group_action_added (G_ACTION_GROUP (app), action); this makes me slightly unhappy because it removes the ability for the GApplication subclass to provide its own signals in a safe way. consider, for example, that GtkApplication always wants to provide a "quit" action whenever the user-provided action group doesn't do it for itself. then imagine that the "removed" signal happens on the "quit" action. the GtkApplication class needs some way to get in on the middle of this to impose its will. i almost feel like we need 4 new virtual functions here, but i'm not sure if that's insane. hmmmmmm........... @@ +433,2 @@ application->priv->actions = action_group; + g_signal_connect_object (action_group, i have to admit i don't know: does this function correctly disconnect the signal when the target object stops existing? ::: gio/gapplicationimpl-dbus.c @@ +283,3 @@ +static GVariant * +actions_to_variant (GActionGroup *action_group, + gchar **actions) trivial whitespace bug. you should have an extra space in the args. @@ +763,3 @@ *remote_actions = NULL; + + /* Emit dbus signals for action group changes. */ please add a comment here that the signals never need to be disconnected because you want to report the events for as long as the app exists
(In reply to comment #11) > Review of attachment 182310 [details] [review]: > > ::: gio/gapplication.c > @@ +378,3 @@ > + GApplication *app) > +{ > + g_action_group_action_added (G_ACTION_GROUP (app), action); > > this makes me slightly unhappy because it removes the ability for the > GApplication subclass to provide its own signals in a safe way. > > consider, for example, that GtkApplication always wants to provide a "quit" > action whenever the user-provided action group doesn't do it for itself. > > then imagine that the "removed" signal happens on the "quit" action. > > the GtkApplication class needs some way to get in on the middle of this to > impose its will. i almost feel like we need 4 new virtual functions here, but > i'm not sure if that's insane. Sounds reasonable to me. I guess GtkApplication in this case would eat the added/removed signals for 'quit' and emit enabled/state changed signals if its quit action was different to the app's action in those respects. How are you thinking this would work? added/removed/state changed/enabled changed vfuncs, taking action group, action, enabled/state parameters, with default implementations that call the GActionGroup methods? In the end it looks like it'd be pretty tricky to implement actions in the GApplication subclass like this - you'd have to get the interaction with the app's action group right for each of the GActionGroup methods, then handle these vfuncs correctly too. Maybe it'd be easier to allow the GApplication subclass to provide an action group and have GApplication itself handle combining that with the app's actions? Might be complicating GApplication unnecessarily though. > hmmmmmm........... > > @@ +433,2 @@ > application->priv->actions = action_group; > + g_signal_connect_object (action_group, > > i have to admit i don't know: does this function correctly disconnect the > signal when the target object stops existing? It invalidates the signal handler closure when the target object (by which I mean 'application' here - the action group disappearing isn't a concern as we hold a ref on it) is finalized, which is probably close enough here. I always forget this too.
Created attachment 184346 [details] [review] add vfuncs for action group changes
Created attachment 184347 [details] [review] fixed
Created attachment 184778 [details] [review] extract action state correctly was calling g_variant_get instead of g_variant_get_child. oops.
Created attachment 187188 [details] [review] style fixes
Review of attachment 187188 [details] [review]: I must say that I hate the amount of plumbing that is going on here. There's action-added/removed signals on GActionGroup already, now GApplication gets vfuncs for them, and both GApplication and GApplicationImpl connect to signals of their private parts... Maybe this is just the messy nature of the entire GApplication thing.
Matthias: this is part of the price that we pay for having the application and its proxy be the same object....
> Sounds reasonable to me. I guess GtkApplication in this case would eat the > added/removed signals for 'quit' and emit enabled/state changed signals if its > quit action was different to the app's action in those respects. > > How are you thinking this would work? added/removed/state changed/enabled > changed vfuncs, taking action group, action, enabled/state parameters, with > default implementations that call the GActionGroup methods? > > In the end it looks like it'd be pretty tricky to implement actions in the > GApplication subclass like this - you'd have to get the interaction with the > app's action group right for each of the GActionGroup methods, then handle > these vfuncs correctly too. Maybe it'd be easier to allow the GApplication > subclass to provide an action group and have GApplication itself handle > combining that with the app's actions? Might be complicating GApplication > unnecessarily though. I agree with your concerns here, but I am also concerned that there is no clear way that we could provide this 'internal action group' facility in a way that is accessible to subclasses but not mere 'users' of the application class. It also clashes with the fact that the application itself is an action group and people expect to be able to *handle* actions by implementing the interface in the obvious way (and chaining up where appropriate). This is actually quite elegant and the only place that it really gets messy is the change notification.
*** Bug 664328 has been marked as a duplicate of this bug. ***
This bug is still relevant after the merge and it's blocking the shell implementation of the menu (as we don't receive state changes). Gtk currently uses GActionObservable / GActionObserver, and in examples/bloatpad expects users to handle GAction::activate, without messing with GActionGroups. You could consider making GApplicationImpl a GActionObserver, and the GSimpleActionGroup a GActionObservable. The alternative would be killing completely the internal GSimpleActionGroup (since g_application_set_action_group() is deprecated, and GApplication exposes GActionMap), and connecting directly to the GActions.
Created attachment 203683 [details] [review] GApplication: emit signals on action changes Now that we're a GActionMap the story about propagating signals from our (now-constant) internal action group is vastly simplified. If someone calls g_application_set_action_group() then signals will stop working -- but this function is deprecated and they never worked before, so no big loss there.
This simple solution is now possible due to the deprecation of _set_action_group() and introduction of GActionMap. Conditional introduction of actions like 'quit' can be managed via observing the calls on the GActionMap interface (which is now the only non-deprecated way of providing actions) and intercepting at that point. This means that the GSimpleActionGroup contained in the GApplication is now completely authorative and can be directly wired up. Jonathan: I'm sorry for putting you through the process of rewriting that patch. I think Matthias was right though, that the process was getting just a little too crazy. I feel much better with what we have now. Attachment 203683 [details] pushed as e370631 - GApplication: emit signals on action changes