GNOME Bugzilla – Bug 700918
Add GtkMainToolbar
Last modified: 2016-06-06 01:51:04 UTC
This is a widget to replace the toolbar as used in: - gnome-music - gnome-documents - totem - gnome-weather - gnome-clocks - gnome-boxes GtkHeaderBar subclass (all the functions it offers should be available in the subclass), or API additions (it's new in 3.10, we could change the behaviour right now). The colour of the bar will change depending on whether the search mode is toggled on. The title will change depending on the search-mode and the select-mode: - search mode OFF, select mode OFF: developer defined title - search mode ON, select mode OFF: empty when search-string is empty, "Results for" when not empty - search mode OFF or ON, select mode: ON: "Click on items to select them" with drop-down Properties to handle buttons: - show-search-button / search-mode-available / search-mode - show-select-mode / select-mode-available / select-mode - show-back-button - show-new-button Signals: - back-button-clicked - new-button-clicked Properties to handle titles: - search-string (Used to display "Results for "bleh" when search mode is on and selection is off) - title/subtitle/custom title (as in header bar) GtkHeaderBar's packing functions will allow adding new widgets to the left/right of the bar (cogwheel for example) We might also need a way to add new selection functions to the "select" drop-down, we have "select-all", "select-none" by default, Boxes also has "select running".
(In reply to comment #0) > GtkHeaderBar subclass (all the functions it offers should be available in the > subclass), or API additions (it's new in 3.10, we could change the behaviour > right now). I don't think we should be adding new API to GtkHeaderBar, it's used to implement CSD so having additional widgets in there muddles its use as a general purpose container.
Created attachment 245720 [details] [review] Add GtkMainToolbar widget https://bugzilla.gnome.org/show_bug.cgi?id=700918 v1: - With GtkBuildable support - Lacks API docs
Created attachment 245721 [details] [review] Add GtkMainToolbar demo to gtk3-demo
Some general comments from looking at the header: > typedef struct _GtkMainToolbar GtkMainToolbar; > I don't like the name very much, because a "toolbar" has historically been a widget that you add toolitems to. I'm not sure it's much of an issue, but something like GtkTopBar or GtkMainBar? Though "bar" is just a filler word, too... > struct _GtkMainToolbar > { > Can we make the struct private? I don't think we want to support people subclassing this widget. > /*< private >*/ > GtkBox parent; > Can we make the widget a GtkBin (I'd prefer GtkContainer, but that'd require extra work...) that contains a GtkBox as the only child? That should not be harder to implement and we avoid people doing gtk_box_pack_start (GTK_BOX (maintoolbar), some_widget, TRUE, TRUE); or similar. Plus, we can change the box to smeting else if we want to. > void gtk_main_toolbar_set_search_mode (GtkMainToolbar *bar, > gboolean search_mode); > In general, a "mode" in the API is an enum. Booleans should have names that can be answered with yes/no so we've usually taken adjectives (visible, mapped) or verbs, usually "has". So the simple change would be to have gtk_main_toolbar_set_has_search_mode() or if it's not a mode gtk_main_toolbar_set_has_search() and if it's only about visibility gtk_main_toolbar_set_show_search(). Same for all the other "mode" properties. > guint gtk_main_toolbar_get_n_selected (GtkMainToolbar *bar); > Is that about the number of selected items? Or about the nth item that is selected? Because APIs named get_n_somethings() are used to get the number of somethings. Otherwise just use gtk_main_toolbar_get_selected() or maybe gtk_main_toolbar_get_selected_action(). > GtkWidget * gtk_main_toolbar_get_custom_title (GtkMainToolbar *bar); > I suppose we have a default title and that's why we accept NULL as the title? Because otherwise gtk_main_toolbar_get_title() would suffice. > void gtk_main_toolbar_pack_start (GtkMainToolbar *bar, > GtkWidget *child); > Can we use "add" instead of "pack", that seems to be the common thing we do with widgets these days. So gtk_main_toolbar_add_start() or gtk_main_toolbar_add_at_start() sounds more like gtk lingo. No comments in here about what these function actually do. I have no clue about that.
Created attachment 248321 [details] [review] Add GtkMainToolbar widget https://bugzilla.gnome.org/show_bug.cgi?id=700918 v1: - With GtkBuildable support - Lacks API docs
Note that there are design updates for selection mode here: https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern A few apps have been updated. I'm hoping that the others will follow before 3.10.
(In reply to comment #4) > Some general comments from looking at the header: > > > typedef struct _GtkMainToolbar GtkMainToolbar; > > > I don't like the name very much, because a "toolbar" has historically been a > widget that you add toolitems to. I'm not sure it's much of an issue, but > something like GtkTopBar or GtkMainBar? Though "bar" is just a filler word, > too... GtkSelectionBar? Seeing as it's based on the selection pattern. > > struct _GtkMainToolbar > > { > > > Can we make the struct private? I don't think we want to support people > subclassing this widget. How come? I can imagine some apps subclassing it for their own use. > > /*< private >*/ > > GtkBox parent; > > > Can we make the widget a GtkBin (I'd prefer GtkContainer, but that'd require > extra work...) that contains a GtkBox as the only child? That should not be > harder to implement and we avoid people doing gtk_box_pack_start (GTK_BOX > (maintoolbar), some_widget, TRUE, TRUE); or similar. Plus, we can change the > box to smeting else if we want to. It's really a GtkBox with a GtkHeaderBar as an internal child. I should be able to make it a GtkContainer without too much trouble. A GtkBin would seem weird, as I'm adding multiple children to the same widget (even if I can because of an implementation detail). > > void gtk_main_toolbar_set_search_mode (GtkMainToolbar *bar, > > gboolean search_mode); > > > In general, a "mode" in the API is an enum. Booleans should have names that can > be answered with yes/no so we've usually taken adjectives (visible, mapped) or > verbs, usually "has". So the simple change would be to have > gtk_main_toolbar_set_has_search_mode() or if it's not a mode > gtk_main_toolbar_set_has_search() and if it's only about visibility > gtk_main_toolbar_set_show_search(). > Same for all the other "mode" properties. I'm guessing a bitfield here: GTK_SELECTION_BAR_MODE_SEARCH = 1, GTK_SELECTION_BAR_MODE_SELECTION = 1 << 1 as we can enable the 2 modes independently. Would clean up the API a bit too. > > guint gtk_main_toolbar_get_n_selected (GtkMainToolbar *bar); > > > Is that about the number of selected items? Or about the nth item that is > selected? Because APIs named get_n_somethings() are used to get the number of > somethings. Otherwise just use gtk_main_toolbar_get_selected() or maybe > gtk_main_toolbar_get_selected_action(). That's to get the number of selected items. This is mainly used to display "X selected items" in the toolbar. > > GtkWidget * gtk_main_toolbar_get_custom_title (GtkMainToolbar *bar); > > > I suppose we have a default title and that's why we accept NULL as the title? > Because otherwise gtk_main_toolbar_get_title() would suffice. We have a different title when in search mode, or in selection mode. This sets a title to be used in cases where the title isn't provided by the bar. > > void gtk_main_toolbar_pack_start (GtkMainToolbar *bar, > > GtkWidget *child); > > > Can we use "add" instead of "pack", that seems to be the common thing we do > with widgets these days. So gtk_main_toolbar_add_start() or > gtk_main_toolbar_add_at_start() sounds more like gtk lingo. Those were modelled on the GtkHeaderBar API.
Review of attachment 248321 [details] [review]: ::: gtk/gtkmaintoolbar.c @@ +38,3 @@ + * + * #GtkMainToolbar is a toolbar that contains oft-used buttons such as toggles + * for select mode, and find mode, or a new button. The widget will also be Should name the modes consistently, throughout: selection mode and search mode. @@ +90,3 @@ + PROP_SHOW_SEARCH_BUTTON, + PROP_SELECT_MODE, + PROP_SELECT_MODE_AVAILABLE, This one seems unused ? @@ +243,3 @@ + case PROP_CUSTOM_TITLE: + gtk_main_toolbar_set_custom_title (bar, g_value_get_object (value)); + break; Looks like you lose title/subtitle functionality from GtkHeaderBar ? Or can one get at the embedded header bar to set those directly ? @@ +409,3 @@ + P_("Whether the header bar is in select mode"), + FALSE, + G_PARAM_READWRITE | G_PARAM_CONSTRUCT)); Are search-mode and select-mode independent ? What happens when you turn on both at the same time ? @@ +432,3 @@ + P_("Show New Button"), + P_("Whether the new button is visible"), + FALSE, do back and new buttons really have to be part of this, with the need for dedicated signals ? Could just pack_start the right buttons ?
(In reply to comment #7) > GtkSelectionBar? Seeing as it's based on the selection pattern. > That sounds like it involves handling the currently selected text or so. Not a fan. > > Can we make the struct private? I don't think we want to support people > > subclassing this widget. > > How come? I can imagine some apps subclassing it for their own use. > I want to get away from people subclassing GTK widgets. That way, we can do more refactoring (like moving code from init to constructed vfuncs for example). And just embedding a widget in your ow GtkBin subclass is usually fine. > It's really a GtkBox with a GtkHeaderBar as an internal child. I should be able > to make it a GtkContainer without too much trouble. > great. > I'm guessing a bitfield here: > GTK_SELECTION_BAR_MODE_SEARCH = 1, > GTK_SELECTION_BAR_MODE_SELECTION = 1 << 1 > > as we can enable the 2 modes independently. Would clean up the API a bit too. > After getting explained the "modes", I think the API you have now is correct. I don't like the "mode" term, but can't come up with anything good. Maybe gtk_main_toolbar_set_enable_search()? Not very good either... I think a bitfield is the wrong thing to use here, because you want to toggle the modes independantly and don't want to have code that reads like gtk_main_toolbar_set_mode (toolbar, gtk_main_toolbar_get_mode (toolbar) & ~GTK_SELECTION_BAR_MODE_SEARCH); > We have a different title when in search mode, or in selection mode. This sets > a title to be used in cases where the title isn't provided by the bar. > I'd go with set_title() then I think. > > Can we use "add" instead of "pack", that seems to be the common thing we do > > with widgets these days. So gtk_main_toolbar_add_start() or > > gtk_main_toolbar_add_at_start() sounds more like gtk lingo. > > Those were modelled on the GtkHeaderBar API. > Meh. I'd have liked those to use add instead of pack, too. But I think I'm the one that's too pedantic here. :)
Using the name toolbar is really bad I think. Asked Cosimo about this and we thought perhaps GtkApplicationHeaderBar or GtkAppHeaderBar would be better. Don't mean to speak for him but he also felt the choice of using a Box was not a great idea since it would be possible to add other arbitrary children to the toolbar's box. He preferred subclassing first (also to avoid duplicating API) and use of Bin second.
Review of attachment 248321 [details] [review]: ::: gtk/gtkmaintoolbar.c @@ +493,3 @@ + if (gtk_widget_get_direction (GTK_WIDGET (bar)) == GTK_TEXT_DIR_RTL) + gtk_image_set_from_icon_name (GTK_IMAGE (bar->priv->back_button_image), + "go-next-symbolic", Use in the "go-previous-rtl-symbolic" instead "go-next-symbolic". @@ +494,3 @@ + gtk_image_set_from_icon_name (GTK_IMAGE (bar->priv->back_button_image), + "go-next-symbolic", + GTK_ICON_SIZE_MENU); Add here: else gtk_image_set_from_icon_name (..., "go-previous-symbolic"...); In fact, much simpler: if (gtk_widget_get_direction (GTK_WIDGET (bar)) == GTK_TEXT_DIR_RTL) g_object_set (bar->priv->back_button_image, "icon-name", "go-previous-rtl-symbolic", NULL); else g_object_set (bar->priv->back_button_image, "icon-name", "go-previous-symbolic", NULL); or: gboolean rtl; rtl = gtk_widget_get_direction (GTK_WIDGET (bar)) == GTK_TEXT_DIR_RT; g_object_set (bar->priv->back_button_image, "icon-name", rtl ? "go-previous-symbolic" : "go-previous-symbolic", NULL); or gtk_image_set_from_icon_name (GTK_IMAGE (bar->priv->back_button_image), rtl ? "go-previous-symbolic" : "go-previous-symbolic", GTK_ICON_SIZE_MENU); You will choose yourself :-) ::: gtk/gtkmaintoolbar.ui @@ +25,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="icon_name">go-previous-symbolic</property> Now, in RTL mode, the icon-name is set twice: in the UI file and in the gtk_main_toolbar_init function. Remove the property "icon_name" from UI file, and set him only in the code, to LTR mode and also to RTL mode.
don't think this is going to happen for 3.12. too late to start using it in applications anyway.