GNOME Bugzilla – Bug 745108
Add specific CSS classes for close/minimize/maximize buttons
Last modified: 2015-03-04 20:04:50 UTC
Created attachment 297796 [details] [review] Add style classes to the titlebuttons Since GTK+ has implemented specific style classes for the titlebuttons, this should be done in mutter too. See the GTK+ commit here https://git.gnome.org/browse/gtk+/commit/?id=b187773053098cca1b7c23e04e096d47fbb65a5f I tried to write a patch that does this, but I'm no programmer, so I'm sure it can be done better. However the attached patch seems to work fine but I'm not sure if it's acceptable.
Review of attachment 297796 [details] [review]: I'm afraid I don't like the patch too much - if possible, I'd rather see something along the lines of: ::: src/ui/theme.c @@ +747,3 @@ + style = style_info->styles[META_STYLE_ELEMENT_BUTTON_MINIMIZE]; + else + style = style_info->styles[META_STYLE_ELEMENT_BUTTON]; Is there a reason why something like: const char *button_class = get_class_from_button_type (button_type); if (button_class) gtk_style_context_add_class (style_info->styles[META_STYLE_ELEMENT_BUTTON], button_class); doesn't work? (To be really correct, you'd need something like add_toplevel_class() for buttons, e.g. add the class to the button style context and modify the image style's path - but then your patch doesn't handle image correctly either ...)
Sorry if I'm misunderstanding something, but wouldn't the classes get repeatedly added to META_STYLE_ELEMENT_BUTTON if I did something like this, i.e. every button has the three classes?
Not if you remove the class at the end of the for loop (by the call to cairo_restore()) ...
gtk_style_context_save()/gtk_style_context_restore() might also work.
Created attachment 297899 [details] [review] Add style classes to the titlebuttons Thanks for your help, I took your suggestions into account and maybe this patch is better.
Review of attachment 297899 [details] [review]: (In reply to horst3180 from comment #5) > maybe this patch is better. I think so, thanks. ::: src/ui/theme-private.h @@ +278,3 @@ PangoContext *context); +const char* get_class_from_button_type (MetaButtonType type); This is a helper method only used in theme.c, it shouldn't be in a header (hint: it should be "static const char *") ::: src/ui/theme.c @@ +759,3 @@ + const char *button_class = get_class_from_button_type (button_type); + if (button_class) + gtk_style_context_add_class (style, button_class); The if() is always true, even for the empty string. Adding a class "" may work fine, but it's still nonsense - you should return NULL in the no-class case (or alternatively check for (button_class && *button_class), but that's unconventional). It might be worth to add add_button_class()/remove_button_class() helpers (similar to add/remove_toplevel_class()) to also adjust the IMAGE style's widget path to make sure stuff like ".titlebutton.close GtkImage {}" is handled correctly, though I doubt many themes would actually use anything like this. @@ +843,3 @@ } cairo_restore (cr); + gtk_style_context_remove_class (style, button_class); Should use the same conditional as add_class(), e.g. if (button_class) gtk_style_context_remove_class(...);
Created attachment 298549 [details] [review] Add style classes to the titlebuttons Ok, I made some modifications to the patch. I see the problem with the IMAGE path now and I agree that it's unlikely that themes will use this, but it would probably be better to get this right.
Review of attachment 298549 [details] [review]: (In reply to horst3180 from comment #7) > I see the problem with the IMAGE path now and I agree that it's unlikely > that themes will use this, but it would probably be better to get this right. Do you want to work on that? For what it's worth, I'm fine with pushing the current patch and wait for someone to complain ...
I think for now this should do, maybe I'll come back to it later.
Attachment 298549 [details] pushed as e5d9766 - Add style classes to the titlebuttons