GNOME Bugzilla – Bug 694036
Controller widget for GdStack
Last modified: 2013-02-18 22:04:39 UTC
I think it would be good to have a controller widget that can drive a GdStack. This would probably look like an exclusive button bar. I would like to just connect the controller to the stack and have it populated automatically. Clicking the buttons on the controller should change the view of the stack. I could then add this widget to the custom-title property of GdHeaderBar.
see also https://bugzilla.gnome.org/show_bug.cgi?id=693113 where I proposed something similar for GdMainToolbar I'll leave that one open in case we want to improve the main toolbar api, but I like the idea of an ad hoc widget.
Created attachment 236551 [details] [review] Add GdStackSwitcher
Review of attachment 236551 [details] [review]: Looks pretty good to me; I have two smaller comments below. ::: libgd/gd-stack-switcher.c @@ +168,3 @@ + + button = g_hash_table_lookup (self->priv->buttons, widget); + gtk_container_add (GTK_CONTAINER (self), button); gtk_container_remove maybe? ::: libgd/gd-stack.c @@ +51,3 @@ + CHILD_REMOVED, + NUM_SIGNALS +}; Can't you use the "add" and "remove" signals of GtkContainer for this?
Review of attachment 236551 [details] [review]: ::: libgd/gd-stack-switcher.c @@ +168,3 @@ + + button = g_hash_table_lookup (self->priv->buttons, widget); + gtk_container_add (GTK_CONTAINER (self), button); oops. ::: libgd/gd-stack.c @@ +51,3 @@ + CHILD_REMOVED, + NUM_SIGNALS +}; Unfortunately not. I think they are emitted before the item is added.
Created attachment 236621 [details] [review] Add GdStackSwitcher
Review of attachment 236621 [details] [review]: Not even if you use g_signal_connect_after? The default handler should run first. If that's not an option, this looks good with the following minor fix. ::: libgd/gd-stack-switcher.c @@ +98,3 @@ + NULL); + if (title == NULL || title[0] == '\0') + g_warning ("Title not set for stack"); I'd say "Title not set for stack child" here.
Created attachment 236632 [details] [review] Add GdStackSwitcher
Review of attachment 236632 [details] [review]: Much better. I have just a few more comments that went unnoticed before, sorry for the back and forth. ::: libgd/gd-stack-switcher.c @@ +49,3 @@ + context = gtk_widget_get_style_context (GTK_WIDGET (switcher)); + gtk_style_context_add_class (context, "linked"); + gtk_style_context_add_class (context, GTK_STYLE_CLASS_HORIZONTAL); I think you should just call gtk_orientable_set_orientation (GTK_ORIENTATION_HORIZONTAL) on the box. The parent class will set the orientable style classes automatically. @@ +75,3 @@ +{ + char *title; + char *name; Seems unused now @@ +193,3 @@ + * gd_stack_switcher_set_stack: + * @switcher: a #GdStackSwitcher + * @stack: a #GdStack Should have (allow-none) I think @@ +240,3 @@ + * + * Return value: (transfer none): the stack, or %NULL if + * none has been set explicitely. "explicitly"? @@ +329,3 @@ + +GtkWidget * +gd_stack_switcher_new (void) Maybe make this take a GdStack too? It's convenient to create it that way from C.
Review of attachment 236632 [details] [review]: ::: libgd/gd-stack-switcher.c @@ +329,3 @@ + +GtkWidget * +gd_stack_switcher_new (void) I had that in the first version I made. But I took it out. I'm not sure I like _new functions taking arguments in general. It makes it much harder to maintain api stability. We can add it later if we want.
Created attachment 236661 [details] [review] Add GdStackSwitcher
Review of attachment 236661 [details] [review]: OK, this looks good then! Thanks.
Attachment 236661 [details] pushed as 4c44014 - Add GdStackSwitcher