After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 745108 - Add specific CSS classes for close/minimize/maximize buttons
Add specific CSS classes for close/minimize/maximize buttons
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2015-02-24 18:04 UTC by horst3180
Modified: 2015-03-04 20:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add style classes to the titlebuttons (3.35 KB, patch)
2015-02-24 18:04 UTC, horst3180
none Details | Review
Add style classes to the titlebuttons (2.24 KB, patch)
2015-02-25 16:31 UTC, horst3180
none Details | Review
Add style classes to the titlebuttons (1.73 KB, patch)
2015-03-04 15:45 UTC, horst3180
committed Details | Review

Description horst3180 2015-02-24 18:04:22 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.
Comment 1 Florian Müllner 2015-02-24 19:41:37 UTC
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 ...)
Comment 2 horst3180 2015-02-24 22:22:09 UTC
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?
Comment 3 Florian Müllner 2015-02-24 22:24:50 UTC
Not if you remove the class at the end of the for loop (by the call to cairo_restore()) ...
Comment 4 Florian Müllner 2015-02-24 22:29:21 UTC
gtk_style_context_save()/gtk_style_context_restore() might also work.
Comment 5 horst3180 2015-02-25 16:31:43 UTC
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.
Comment 6 Florian Müllner 2015-02-26 21:48:28 UTC
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(...);
Comment 7 horst3180 2015-03-04 15:45:43 UTC
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.
Comment 8 Florian Müllner 2015-03-04 16:21:59 UTC
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 ...
Comment 9 horst3180 2015-03-04 19:19:56 UTC
I think for now this should do, maybe I'll come back to it later.
Comment 10 Florian Müllner 2015-03-04 20:04:46 UTC
Attachment 298549 [details] pushed as e5d9766 - Add style classes to the titlebuttons