GNOME Bugzilla – Bug 738083
GtkMenuButton is not disabled even if corresponding GAction is
Last modified: 2014-10-09 22:16:40 UTC
Imagine some code like this: this._gearMenu = Application.application.lookup_action('gear-menu'); this._gearMenu.enabled = false; let previewMenu = this._getPreviewMenu(); let menuButton = new Gtk.MenuButton({ image: new Gtk.Image ({ icon_name: 'open-menu-symbolic' }), menu_model: previewMenu, action_name: 'app.gear-menu' }); Even though the GAction has been disabled, the GtkMenuButton retains its sensitivity. What is happening here is that the GtkActionable:action-name is set before the menu-model property. When action-name is set, the half-constructed GtkMenuButton is insensitive (because it has no menu, yet), which is fine. Later on, when menu-model is set, the presence of a menu makes the GtkMenuButton sensitive, even if the GAction says otherwise. One option would be to check the state of the action using the GtkActionHelper held in the parent GtkButton class whenever a new menu widget is added to the button. PS: Reading the code, it looks like the GtkMenuToolButton might be suffering from similar (if not exactly the same) issue. Its parent, GtkToolButton, implements GtkActionable and passes on the set_action_name method to its internal GtkButton. Now GtkMenuToolButton adds another button to this (ie. arrow_widget), and I don't see anything that would render it insensitive when the action is disabled.
Created attachment 287955 [details] [review] GtkMenuButton: Respect the disabled state of the GAction
I was never really happy with having actions on submenu-carrying items to determine their sensitivity - feels a bit like a hack, because the action is never actually used as an action. This seems to extend the hack to menubuttons ? Is there an independent reason that you already have an action associated with the gear menu ?
(In reply to comment #2) > I was never really happy with having actions on submenu-carrying items to > determine their sensitivity - feels a bit like a hack, because the action is > never actually used as an action. > > This seems to extend the hack to menubuttons ? Well, right now MenuButtons can be made insensitive based on the GAction if the application sets action-name after menu-model. Basically, create the widget without an action-name and set it later. That already works. > Is there an independent reason that you already have an action associated with > the gear menu ? A GAction gives us an accelerator, and the ability to have multiple widgets proxying it. eg., app.gear-menu is proxied by MenuButtons in PreviewToolbar and PreviewFullscreenToolbar.
Oh, and we actually connect to 'action-state-changed::gear-menu' to reveal the toolbar if someone pressed the accelerator in the fullscreen mode. Don't know if that counts as 'using the action as an action'.
I guess thats valid reasons... I'll overcome my aversion :-)
Review of attachment 287955 [details] [review]: This is not quite enough though, or is it ? I mean, the button has an actionhelper which will happily turn the menubutton sensitive when the action gets enabled, even if there is no menu, or am I missing something ?
(In reply to comment #6) > Review of attachment 287955 [details] [review]: > > This is not quite enough though, or is it ? I mean, the button has an > actionhelper which will happily turn the menubutton sensitive when the action > gets enabled, even if there is no menu, or am I missing something ? You are right. This only covers the "not getting disabled when GAction is disabled" case. I will write a patch to cover the "getting enabled when there is no menu" case.
Created attachment 288053 [details] [review] GtkMenuButton: Stop the GAction from enabling a menu-less button I am not confident of my GObject-fu here. I noticed that when both action-name and menu-model were set at construction, my notify::action-name handler was not being invoked. To correctly cover both 1) setting via property, and 2) setting via setter cases, I couldn't think of anything else other than to override the property and listen to notify::action-name.
Review of attachment 288053 [details] [review]: ::: gtk/gtkmenubutton.c @@ +246,3 @@ + if (priv->action_helper == NULL) + { + priv->action_helper = GTK_BUTTON (menu_button)->priv->action_helper; Is it really worth storing the actionhelper locally, here ? I think we could just get away with button->priv->action_helper in the 3 places where it is used.
Review of attachment 288053 [details] [review]: ::: gtk/gtkmenubutton.c @@ +707,3 @@ atk_object_set_name (accessible, _("Menu")); + + g_signal_connect (menu_button, "notify::action-name", G_CALLBACK (action_name_notify_cb), NULL); We generally try to avoid objects connecting to their own signals, including ::notify for their own properties. Why do you need this anyway - now that you are overriding the property, it will _always_ be set through your setter, I don't see any need for this handler. If there was a need, the more correct way to do this would be to override the notify vfunc. Don't forget to chain up if you do.
(In reply to comment #10) > Review of attachment 288053 [details] [review]: > > ::: gtk/gtkmenubutton.c > @@ +707,3 @@ > atk_object_set_name (accessible, _("Menu")); > + > + g_signal_connect (menu_button, "notify::action-name", G_CALLBACK > (action_name_notify_cb), NULL); > > We generally try to avoid objects connecting to their own signals, including > ::notify for their own properties. Yeah, I am not sure it is even supposed to work when the object is under construction because I was seeing some weird behaviour. As I mentioned before, the notify::action-name callback was not getting called when I was setting both menu-model and action-name at construction time. It was getting invoked if I only set action-name. FWIW, I was using the code in https://bugzilla.gnome.org/attachment.cgi?id=287958 (the menu button in src/preview.js) as a test case. > Why do you need this anyway - now that you > are overriding the property, it will _always_ be set through your setter, I > don't see any need for this handler. Overriding the property only takes care of the case where action-name is being set via g_object_set (menu_button, ...), not when gtk_actionable_set_action_name (GTK_ACTIONABLE (menu_button), ...) is used. > If there was a need, the more correct way to do this would be to override the > notify vfunc. Don't forget to chain up if you do. Ok, I will try that.
(In reply to comment #9) > Review of attachment 288053 [details] [review]: > > ::: gtk/gtkmenubutton.c > @@ +246,3 @@ > + if (priv->action_helper == NULL) > + { > + priv->action_helper = GTK_BUTTON (menu_button)->priv->action_helper; > > Is it really worth storing the actionhelper locally, here ? I think we could > just get away with > button->priv->action_helper in the 3 places where it is used. We don't really need it in the case of set_popup and set_popover. But when we want to connect to GtkActionHelper:enabled, we need to track when the helper is getting created because GtkButton creates it when set_action_item is called for the first time, and not in gtk_button_init. We could track it using other means too. eg., storing the handler ID. Since we are using the helper in other places, I chose to store the helper itself for the minor gain of being able to use "priv->action_helper" instead of "GTK_BUTTON (menu_button)->priv->action_helper" No strong opinion, though. Whatever you prefer.
(In reply to comment #11) > Overriding the property only takes care of the case where action-name is being > set via g_object_set (menu_button, ...), not when > gtk_actionable_set_action_name (GTK_ACTIONABLE (menu_button), ...) is used. I think this points to the underlying problem here - you probably need to re-implement the entire GtkActionable interface, not just override the action-name property for this to work correctly.
Created attachment 288136 [details] [review] GtkMenuButton: Respect the disabled state of the GAction
Created attachment 288137 [details] [review] GtkMenuButton: Stop the GAction from enabling a menu-less button After trying a few options, I think the easiest thing to do is create the GtkActionHelper unconditionally during the construction of the GtkButton, so that we don't have to hook into property changes to get to the helper. As I said before, whenever the menu-model is set during construction emission of property notifications stop. Either it is not supposed to work, or there is a bug. All tests pass, except testsuite/reftests/inherit-and-initial.ui, but that one was failing even before.
Created attachment 288138 [details] C test case to play with
(In reply to comment #13) > (In reply to comment #11) > > > Overriding the property only takes care of the case where action-name is being > > set via g_object_set (menu_button, ...), not when > > gtk_actionable_set_action_name (GTK_ACTIONABLE (menu_button), ...) is used. > > I think this points to the underlying problem here - you probably need to > re-implement the entire GtkActionable interface, not just override the > action-name property for this to work correctly. I was trying to avoid duplicating the entire GtkActionable logic that GtkButton already has. But, yes, that is another option.
thanks for the testcase, I'll give it a try
Sorry for misdirecting you here. After looking at your testcase, and playing with it a bit, I decided that I was wrong. What I actually pushed now is a patch to make GtkMenuButton just not fiddle with sensitivity at all when it has an action associated - in that case, the action is what controls it, and it is the responsibility of the action owner to ensure that the enabled state is only TRUE when it makes sense.