After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 643736 - GApplication doesn't emit dbus signals on action updates
GApplication doesn't emit dbus signals on action updates
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gapplication
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
: 664328 (view as bug list)
Depends on:
Blocks: 621203 622874
 
 
Reported: 2011-03-02 22:33 UTC by Jonathan Matthew
Modified: 2011-12-16 16:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (12.05 KB, patch)
2011-03-02 22:36 UTC, Jonathan Matthew
needs-work Details | Review
don't crash on receiving an action update signal (2.61 KB, patch)
2011-03-03 13:16 UTC, Jonathan Matthew
none Details | Review
don't crash on receiving an action update signal (2.61 KB, patch)
2011-03-03 13:26 UTC, Jonathan Matthew
none Details | Review
this one actually works (2.69 KB, patch)
2011-03-06 02:53 UTC, Jonathan Matthew
needs-work Details | Review
add vfuncs for action group changes (20.72 KB, patch)
2011-03-27 10:26 UTC, Jonathan Matthew
none Details | Review
fixed (2.70 KB, patch)
2011-03-27 10:27 UTC, Jonathan Matthew
none Details | Review
extract action state correctly (20.72 KB, patch)
2011-03-31 13:28 UTC, Jonathan Matthew
none Details | Review
style fixes (21.11 KB, patch)
2011-05-04 13:16 UTC, Jonathan Matthew
none Details | Review
GApplication: emit signals on action changes (2.49 KB, patch)
2011-12-16 16:43 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Jonathan Matthew 2011-03-02 22:33:08 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.
Comment 1 Jonathan Matthew 2011-03-02 22:36:52 UTC
Created attachment 182310 [details] [review]
patch
Comment 2 Jonathan Matthew 2011-03-03 13:16:25 UTC
Created attachment 182341 [details] [review]
don't crash on receiving an action update signal
Comment 3 Jonathan Matthew 2011-03-03 13:26:48 UTC
Created attachment 182344 [details] [review]
don't crash on receiving an action update signal

slightly better version, doesn't add an unused variable
Comment 4 Jonathan Matthew 2011-03-06 02:53:07 UTC
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.
Comment 5 Allison Karlitskaya (desrt) 2011-03-19 01:15:16 UTC
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 :)
Comment 6 Allison Karlitskaya (desrt) 2011-03-19 01:16:38 UTC
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 :)
Comment 7 Allison Karlitskaya (desrt) 2011-03-19 01:16:38 UTC
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 :)
Comment 8 Allison Karlitskaya (desrt) 2011-03-19 01:16:39 UTC
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 :)
Comment 9 Allison Karlitskaya (desrt) 2011-03-19 01:16:39 UTC
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 :)
Comment 10 Allison Karlitskaya (desrt) 2011-03-19 01:16:40 UTC
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 :)
Comment 11 Allison Karlitskaya (desrt) 2011-03-19 01:22:23 UTC
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
Comment 12 Jonathan Matthew 2011-03-19 05:38:09 UTC
(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.
Comment 13 Jonathan Matthew 2011-03-27 10:26:22 UTC
Created attachment 184346 [details] [review]
add vfuncs for action group changes
Comment 14 Jonathan Matthew 2011-03-27 10:27:03 UTC
Created attachment 184347 [details] [review]
fixed
Comment 15 Jonathan Matthew 2011-03-31 13:28:31 UTC
Created attachment 184778 [details] [review]
extract action state correctly

was calling g_variant_get instead of g_variant_get_child.  oops.
Comment 16 Jonathan Matthew 2011-05-04 13:16:00 UTC
Created attachment 187188 [details] [review]
style fixes
Comment 17 Matthias Clasen 2011-05-05 19:40:52 UTC
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.
Comment 18 Allison Karlitskaya (desrt) 2011-05-19 15:10:32 UTC
Matthias: this is part of the price that we pay for having the application and its proxy be the same object....
Comment 19 Allison Karlitskaya (desrt) 2011-05-19 15:19:36 UTC
> 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.
Comment 20 Allison Karlitskaya (desrt) 2011-11-21 19:00:38 UTC
*** Bug 664328 has been marked as a duplicate of this bug. ***
Comment 21 Giovanni Campagna 2011-12-09 12:40:32 UTC
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.
Comment 22 Allison Karlitskaya (desrt) 2011-12-16 16:43:59 UTC
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.
Comment 23 Allison Karlitskaya (desrt) 2011-12-16 16:46:47 UTC
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