Created attachment 231222 [details] [review]
main-toolbar: Add support for modes

Applications which do not need modes should not require any changes.

I tried using a GtkButtonBox to avoid having to set the minimum width of the buttons with:
gtk_widget_set_size_request (button, 100, -1);

However, I could not get it to honour the vexpand=TRUE of the buttons inside it. Since the buttons on the left and right have vexpand=TRUE, this causes their bottom edges to be misaligned.

I am not sure whether 100 is a good number or not. GtkButtonBox uses 85, gnome-calendar uses 80 and gnome-clocks uses 100.

Review of attachment 231222 [details] [review]:

Thanks, this looks good except for some minor comments

::: libgd/gd-main-toolbar.c
@@ +235,3 @@
 
+  /* centered mode group */
+  self->priv->modes_grid = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 0);

This is not a grid, so I think it should be renamed to modes_box.

@@ +439,3 @@
+GtkWidget *
+gd_main_toolbar_add_mode (GdMainToolbar *self,
+ * @self:

I think I'd rename "name" to "label" to avoid any confusion here.

@@ +446,3 @@
+  button = gtk_radio_button_new_with_label (NULL, name);
+  gtk_toggle_button_set_mode (GTK_TOGGLE_BUTTON (button), FALSE);
+ */

Why is this set_size_request needed at all?

(In reply to comment #2)
> ::: libgd/gd-main-toolbar.c
> @@ +235,3 @@
> 
> +  /* centered mode group */
> +  self->priv->modes_grid = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 0);
> 
> This is not a grid, so I think it should be renamed to modes_box.

Done.

> @@ +439,3 @@
> +GtkWidget *
> +gd_main_toolbar_add_mode (GdMainToolbar *self,
> + * @self:
> 
> I think I'd rename "name" to "label" to avoid any confusion here.

Done.

> @@ +446,3 @@
> +  button = gtk_radio_button_new_with_label (NULL, name);
> +  gtk_toggle_button_set_mode (GTK_TOGGLE_BUTTON (button), FALSE);
> + */
> 
> Why is this set_size_request needed at all?

To enforce a minimum width for the buttons, just like having them in GtkButtonBox. eg., if you have a button in a GtkButtonBox with GTK_STOCK_OK, the button is much wider than required because the minimum width of is set to be 85. Others, like gnome-calendar and gnome-clocks use 80 and 100 respectively.

I do not know what is the best way to do this.

Created attachment 231243 [details] [review]
main-toolbar: Add support for modes

Review of attachment 231243 [details] [review]:

Thanks, this looks good now.

Thanks for the review!

I took the liberty of committing an obvious fix: http://git.gnome.org/browse/libgd/commit/?id=ee7333b87b57d9c5f0ff614c11e43bd37d6a30ce

However when trying to use this api I find it a bit weird:

1) add_mode returns "GtkWidget", but you are supposed to connect to "toggled" so the caller must know that it is a ToggleButton... the api should retunr GtkToggleButton so that introspection/vala knows the right type

2) having to connect to each button is cumbersome: what about having a mode-changed signal on the toolbar? add mode could return an int and mode-changed pass the int to the handler

3) (bikeshedding) "mode" is a pretty bad name for this imho: I'd expect the "mode" of the toolbar to be for instance "normal" and "selection"... what about "add_page" instead?

Paolo, thanks for the fix, that must have been a copy/paste mistake :)

How does it make a difference for Vala/introspection to know that is a GtkToggleButton? I mean, even gtk_toggle_button_new returns a GtkWidget, so I think it's fine as long as we document that the returned widget is actually a toggle button.

I agree wrt. the other two points you make; can you please file them as separate bugs against libgd? Thanks.

(In reply to comment #8)
> How does it make a difference for Vala/introspection to know that is a
> GtkToggleButton?

Well, vala complains that the Widget class does not have a "toggle" signal, so you need to downcast which is a bit ugly.

Anyway I'll the other bugs separately.