GNOME Bugzilla – Bug 104679
misaligned icons/text in GtkToolbar buttons
Last modified: 2004-12-22 21:47:04 UTC
The label/image inside a toolbar button is not properly centered if the GtkToolbar style is icons-only or text-only. Even worse, the exact alignment depends on the style that was set previously. This happens because the image/label is packed into a GtkBox with the expand and fill flags set to either FALSE or the value used by the previous style. The packing options are always the same only for the 'both' and 'both_horiz' styles. The attached patch fixes the problem by setting expand = TRUE, fill = TRUE if there's only a single widget in the GtkBox.
Created attachment 13895 [details] [review] gtktoolbar_alignment_fix.diff
Could you regenerate the patch with cvs diff -uw? It's a little hard to see what changed in the version above. Thanks.
Created attachment 13932 [details] [review] gtktoolbar_alignment_fix_uw.diff
Oh cool. Didn't know about -uw ;-)
A) Isn't the only time you want anything other than TRUE/TRUE the icon for BOTH_HORIZONTAL and maybe the text for BOTH? B) Really, I'm think this patch needs to clean up the code not make things worse :-) The logic for deciding the packing options needs to be in only one place. I think it might be best to just pack them in an arbitrary fashion to begin with and have a function to set_child_packing() that is called as necessary.
Created attachment 13964 [details] [review] gtktoolbar_alignment_fix_proper.diff
This one is easier to read without -w :) Was there any reason not to use gtk_bin_get_child()? I couldn't think of any and thus deliberately removed the get_first_child() helper function.
I think the code didn't use gtk_bin_get_child() because it predated it. Hmm, you seem to be setting the magic object data on items of type CHILD_WIDGET now. What's the reason for this. - if (type != GTK_TOOLBAR_CHILD_WIDGET) - { - /* Mark child as ours */ - g_object_set_data (G_OBJECT (child->widget), - "gtk-toolbar-is-child", - GINT_TO_POINTER (TRUE)); - } + set_child_packing_and_visibility (toolbar, child); + Other than that it looks good. Reading over the code, I see that both the old code and your code has: + gtk_container_remove (GTK_CONTAINER (child->widget), hbox) The remove the old hbox or vbox and get rid of it. I would recommend: gtk_widget_destroy (hbox); instead ... less chance of a memory leak from a left over weak reference, and simpler as well.
The if (type != GTK_TOOLBAR_CHILD_WIDGET) is superfluous because it's always true. The switch() branch is: case GTK_TOOLBAR_CHILD_BUTTON: case GTK_TOOLBAR_CHILD_TOGGLEBUTTON: case GTK_TOOLBAR_CHILD_RADIOBUTTON: GTK_TOOLBAR_CHILD_WIDGET is handled before that, and there's no fallthrough.
Ah, makes sense.
Committed to gtk-2-2 and HEAD.
*** Bug 106337 has been marked as a duplicate of this bug. ***