GNOME Bugzilla – Bug 723878
GtkMenuButton: Support popovers
Last modified: 2014-02-09 01:44:14 UTC
Add api to allow explicitly setting a GtkPopover instead of a GtkMenu as the popup of a GtkMenuButton. Also, add api to instruct the menu button to construct a popover when given a menu model.
Created attachment 268467 [details] [review] GtkMenuButton: Support popovers
Created attachment 268468 [details] how it looks in the wild
Created attachment 268469 [details] [review] GtkMenuButton: Support popovers Add api to allow explicitly setting a GtkPopover instead of a GtkMenu as the popup of a GtkMenuButton. Also, add api to instruct the menu button to construct a popover when given a menu model.
Review of attachment 268467 [details] [review]: Looks great in general :), and looks like will make GtkPopover adoption quite faster where it's most expected. just a few minor comments ::: gtk/gtkmenubutton.c @@ +224,3 @@ + if (priv->menu) + gtk_menu_shell_deactivate (GTK_MENU_SHELL (priv->menu)); + if (priv->popover) If menu and popover are mutually exclusive, maybe this would be clearer as if ... else if ... for future reads, or check use_popover, dunno... @@ +423,2 @@ } + if (priv->popover) same here @@ +437,3 @@ + if (priv->menu) + popup_menu (menu_button, event); + if (priv->popover) and here @@ +1085,3 @@ + + g_clear_object (&priv->model); + gtk_menu_button_set_popup (menu_button, NULL); maybe gtk_menu_button_set_popup() should also unset the popover? both should protect for reentrancy in that case. Also, I guess the popup must be unset only if popover != NULL.
Thanks for the review. One thing to note in the screenshot is that we should make Adwaita not give the button this special treatment when it is not popping up a menu. Probably need to add a style class to differentiate the two cases.
Created attachment 268470 [details] [review] GtkMenuButton: Support popovers Add api to allow explicitly setting a GtkPopover instead of a GtkMenu as the popup of a GtkMenuButton. Also, add api to instruct the menu button to construct a popover when given a menu model. We set the style class "menu-button" on the button only when it pops up a menu, to allow different treatment for the active state of the button in the two cases.
Created attachment 268471 [details] [review] Restrict menubutton styling For menu buttons, we create an active state that makes the button appear attached to the menu. That does not look so hot when the button is actually popping up something else. GTK+ introduced a style class, 'menu-button', to differentiate the case of a button with a menu. Use it.
Attachment 268470 [details] pushed as 9822d51 - GtkMenuButton: Support popovers
Comment on attachment 268471 [details] [review] Restrict menubutton styling Attachment 268471 [details] pushed as 22882a4 - Restrict menubutton styling