GNOME Bugzilla – Bug 667394
Gtk(Tool)Button: add an 'action-name' property
Last modified: 2012-01-11 05:31:32 UTC
When the button is placed inside the widget hierarchy of a GtkApplicationWindow, this property can refer to the name of an action in the same way that GMenu does. This allows for easy association of buttons (and particularly toolbar items) with actions defined on the window and application.
Created attachment 204724 [details] [review] Gtk(Tool)Button: add an 'action-name' property
Review of attachment 204724 [details] [review]: This looks less involved than I had feared. Do we want to make this and the GtkAction stuff mutually exclusive (ie emit some nasty warning if you try to bind a button to both a GAction and a GtkAction ?
One piece of feedback I wanted: It seems like we should add 'action-target' property as I mentioned in one of the docs comments, and it should probably be a GVariant*, but this is inconvenient. Would it be evil to notice strings of the form "action::target" on _set_action_name() and split them into the action name ('action') and string-typed target value? This is a bit weird because it means that 'action-name' would read back with a different value than you just set on it... Alternatively, we could have a separate convenience setter for this purpose and tell people using properties (ie: from GtkBuilder) that they simply have to set both fields.
I could see us having gtk_button_set_action (GtkButton *button, const gchar *detailed_action) which would not be a direct setter for the action_name property, but do the action::target thing, in addition to straight property setters: gtk_button_set_action_name (GtkButton *button, const gchar *action); gtk_button_set_action_target (GtkButton *button, GVariant *target); That looks perfectly fine to me
Created attachment 204764 [details] [review] GtkBuilder: support parsing GVariant properties
Created attachment 204765 [details] [review] Add new GtkActionable interface This is the interface for GtkWidgets that can be associated with an action on a GtkAppicationWindow or associated GtkApplication. It essentially features 'action-name' and 'action-target' properties with some associated convenience API. This interface is implemented by GtkButton and GtkToolButton.
Created attachment 204766 [details] [review] bloatpad: add left/centre/right toolbar buttons
Created attachment 204767 [details] [review] GtkButton: do not allow both types of actions Only allow one of 'action-name' or 'related-action' to be set.
It's my opinion that we need the interface. GtkSwitch will probably be next and I can think of a few more less-likely-but-still-plausible candidates after that. Hopefully we are on a path toward deprecating GtkAction (and friends), so the GtkActionable/GtkActivatable confusion won't last for long.
Review of attachment 204764 [details] [review]: Looks fine to me
Comment on attachment 204764 [details] [review] GtkBuilder: support parsing GVariant properties Attachment 204764 [details] pushed as d47c3ac - GtkBuilder: support parsing GVariant properties
Review of attachment 204765 [details] [review]: ::: gtk/gsimpleactionobserver.c @@ +251,3 @@ + g_action_observable_register_observer (observable, action_name, G_ACTION_OBSERVER (observer)); + + if (g_action_group_query_action (observer->action_group, action_name, &enabled, &type, NULL, NULL, &state)) Should you query the parameter type here, and check that target is the right type ? Do we do that elsewhere ? ::: gtk/gtkactionable.c @@ +60,3 @@ + GTK_ACTIONABLE_GET_IFACE (actionable) + ->set_action_name (actionable, action_name); +} Why do you have properties and virtualized setters, isn't that a bit redundant ? I would expect g_object_set (actionable, "action-name", action_name, NULL); to work just as well here... ::: gtk/gtkapplicationwindow.c @@ +987,3 @@ +gtk_application_window_get_observer (GtkApplicationWindow *window, + const gchar *action_name, + GVariant *target) This looks like a reffing getter, which we don't normally do in gtk, and surprised me when I saw it used. Maybe it should be called create_observer ? @@ +990,3 @@ +{ + g_return_val_if_fail (GTK_IS_APPLICATION_WINDOW (window), NULL); + Should you check muxer_initialized here ? ::: gtk/gtkbutton.c @@ +734,3 @@ + g_return_if_fail (GTK_IS_BUTTON (button)); + + if (g_strcmp0 (action_name, button->priv->action_name) != 0) We normally don't do strcmp checks in string setters - do you think it is warranted here ?
Review of attachment 204766 [details] [review]: Looks fine to me
Review of attachment 204767 [details] [review]: Sure, thats fine. Do we also want to have checks to ensure that the parameter type is suitable for the button subclass ?
Review of attachment 204765 [details] [review]: set patch status
Review of attachment 204765 [details] [review]: ::: gtk/gsimpleactionobserver.c @@ +251,3 @@ + g_action_observable_register_observer (observable, action_name, G_ACTION_OBSERVER (observer)); + + if (g_action_group_query_action (observer->action_group, action_name, &enabled, &type, NULL, NULL, &state)) That's part of what this code is doing. The parameter type ends up in 'type' which gets passed to action_added() which inspects it and sets the value of 'can_activate' accordingly. ::: gtk/gtkactionable.c @@ +60,3 @@ + GTK_ACTIONABLE_GET_IFACE (actionable) + ->set_action_name (actionable, action_name); +} That works fine (except for being slow). On the flip-side, it doesn't work so well for getters because they usually don't return references whereas properties do, so you'd have to do that ugly thing where you decrease the reference count and return. For action-name it actually wouldn't work at all because that's a string... and you can't really free and return that. So since we have to have getters, we also have setters for consistency. It's worth noting that I actually had a default implementation of the setters that did exactly this that I dropped for being too hacky. As to the potential question about why we have properties at all (and not just pure vfuncs): GtkBuilder. What would be really awesome is if the interface itself could implement the property by calling the vfuncs -- then subclasses would only need to implement those and not worry about the properties... I'm not really sure how that would work though, so we're stuck here. ::: gtk/gtkapplicationwindow.c @@ +987,3 @@ +gtk_application_window_get_observer (GtkApplicationWindow *window, + const gchar *action_name, + GVariant *target) Agree. @@ +990,3 @@ +{ + g_return_val_if_fail (GTK_IS_APPLICATION_WINDOW (window), NULL); + No. It could well be the case that the window is not associated with a GtkApplication yet. On realize, when the action groups get added, the muxer will fire signals and the observer will notice and update itself. ::: gtk/gtkbutton.c @@ +734,3 @@ + g_return_if_fail (GTK_IS_BUTTON (button)); + + if (g_strcmp0 (action_name, button->priv->action_name) != 0) No. Not in particular. I only did it because I thought that it was something that we do. I'll take it out.
Review of attachment 204766 [details] [review]: Waiting to commit this until after we're done with the others...
Review of attachment 204767 [details] [review]: Good to go
One thing I wonder here is: Do we really have to go this implicit route putting the name on the button and have the button contain the logic to find its window, and look for the action (slightly wrapped in that observer). Shouldn't we just connect the button directly to a GAction ? Of course, we'll have to have a GAction implementation that contains all the smarts of your observer to make it work 'magically'. But it seems to me that a direct button-action connection is nicer than tying the window directly into this. The general long-term direction is that we want widgets to be less dependent on their context.
I agree with this in principle as being substantially more efficient. It would also eliminate the need for the extra observer class. The trouble is that GActionMuxer is an actiongroup: it is not necessarily comprised of actual GActions. In practice, our particular GActionMuxer is composed of a GtkApplication and a GtkApplicationWindow -- each of which is a GActionMap (ie: can pull real GActions out of it). We could implement the GActionMap interface on GActionMuxer in order to pull the original GAction out of the underlying group of that group is a GActionMap. In our case, this would always succeed, of course. My only reservation is that invoking the actions directly is possibly annoying. You might expect your application or window subclass to be able to intercept action invocations by overriding the activate_action() vcall. I've also often thought that it might be a cool idea to have some sort of interface on the GtkApplicationWindow to introduce your own named action groups, so you could target actions like "doc.save". That would break our assumptions here, unless we required that the introduced GActionGroup is a GActionMap (which I really don't like) or we had a fallback to deal with the non-actionmap case (ie: what the code is doing in the patch as it is now). On balance I'm actually extremely fond of your idea -- not because of the "reducing of context" argument but rather because it cuts out the middleman in a rather substantial way. I'm going to think about this a bit.
Attachment 204765 [details] pushed as 88ec007 - Add new GtkActionable interface Attachment 204766 [details] pushed as 4dbd12b - bloatpad: add left/centre/right toolbar buttons Attachment 204767 [details] pushed as cf2590d - GtkButton: do not allow both types of actions Leaving open to address the remaning issues.
Created attachment 204867 [details] [review] Rename gtk_application_window_get_observer This should have been called _create_observer
Created attachment 204868 [details] [review] GtkButton: don't do string compare on property set Just set the property unconditionally.
The following fixes have been pushed: b7a28de GtkButton: don't do string compare on property set ab91527 Rename gtk_application_window_get_observer
Created attachment 204999 [details] [review] GtkButton: don't do string compare on property set Just set the property unconditionally.
Created attachment 205000 [details] [review] Rename gtk_application_window_get_observer This should have been called _create_observer https://bugzilla.gnome.org/show_bug.cgi?id=667394 Fixup switch