GNOME Bugzilla – Bug 646882
Theming fixes for GtkButton and GtkCombobox
Last modified: 2011-04-12 21:09:58 UTC
Here's a patchset that fixes some holes in GtkButton and GtkCombobox themability, as for bug 645824.
Created attachment 185251 [details] [review] button: make gtk_button_get_props() return the padding too
Created attachment 185252 [details] [review] button: don't use the border values for padding It's wrong, and makes it impossible to theme the button properly.
Created attachment 185253 [details] [review] combobox: don't add the button style class to the whole widget It already has a toggle button inside, and this way we cannot theme them separately.
Created attachment 185254 [details] [review] combobox: don't use the border as a padding This causes the combobox to behave badly from the theme.
Created attachment 185255 [details] [review] combobox: rename border->padding for code clarity
Created attachment 185256 [details] [review] combobox: allocate the right border to the arrow and not to the label When the combobox is in menu mode, the right padding of the togglebutton inside, should be allocated to its rightmost children, which is the arrow.
Review of attachment 185251 [details] [review]: Looks fine, apart from the minor coding style thing. ::: gtk/gtkbutton.c @@ +1464,3 @@ + { + gtk_style_context_get_padding (context, state, padding); + } GTK+ coding style calls for no braces here.
Review of attachment 185252 [details] [review]: Not 100% sure I understand why it is wrong, but I'll trust your insight into the css machinery.
Review of attachment 185253 [details] [review]: Ok
Review of attachment 185254 [details] [review]: Again I'll have to defer to your understanding of css parameter interactions.
Review of attachment 185255 [details] [review]: No problems with the renaming ::: gtk/gtkcombobox.c @@ +2425,3 @@ + allocation->y += padding.top; + allocation->width -= padding.left + padding.right; + allocation->height -= padding.top + padding.bottom; This looks like it is not just a renaming, but an actual behaviour change. Should be explained and committed separately.
Review of attachment 185256 [details] [review]: ok
Created attachment 185820 [details] [review] combobox: always give the full allocation to the button in menu mode The button is what draws the background/frame outline of the combobox, and padding is defined as the spacing *inside* the widget between the border and the content.
(In reply to comment #11) > This looks like it is not just a renaming, but an actual behaviour change. > Should be explained and committed separately. I splitted it into the last patch attached here.
Review of attachment 185820 [details] [review]: Makes sense.
Thanks for the review, pushed to master with the code style fixed for the first patch. Attachment 185251 [details] pushed as 29e45c4 - button: make gtk_button_get_props() return the padding too Attachment 185252 [details] pushed as 2c29311 - button: don't use the border values for padding Attachment 185253 [details] pushed as 9f4183c - combobox: don't add the button style class to the whole widget Attachment 185254 [details] pushed as 22eb1f5 - combobox: don't use the border as a padding Attachment 185255 [details] pushed as 503466c - combobox: rename border->padding for code clarity Attachment 185256 [details] pushed as e94b311 - combobox: allocate the right border to the arrow and not to the label Attachment 185820 [details] pushed as 713488e - combobox: always give the full allocation to the button in menu mode