GNOME Bugzilla – Bug 741917
Let GTK+ draw window decorations
Last modified: 2014-12-29 16:59:21 UTC
With CSD support in GTK+, we are now maintaining two very different (but supposedly matching) themes for the same elements. Just ditch our own theme format (which everyone hates anyway) and use GTK+ for server-side decorations as well. This has been lying around locally for a couple of months now and in a public branch for quite some time, so I consider this fairly well tested by now (mostly using the default theme).
Created attachment 293271 [details] [review] theme: Rename button_rect() to get_button_rect() Basically it's odd to have "button_rect" be a function with all the foo_rect GdkRectangles around - renaming to get_button_rect() will free the name for the generically named "rect" once buttons are the only movable pieces in the frame.
Created attachment 293272 [details] [review] theme: Add MetaStyleInfo for wrapping frame style context Our current use of style contexts is fairly limited - we don't use them for much more than picking up some color information. We will soon start to make more elaborate use of GTK style information, but a single context will no longer be enough to draw a frame then. To prepare for this, add a simple ref-counted type to wrap style information.
Created attachment 293273 [details] [review] theme: Build a StyleContext hierarchy that matches GTK+'s CSD In order to pick up all theme information from GTK+, a single style context is not enough; a style hierarchy that closely matches the widget hierarchy by GTK+'s client-side decorations will allow this soon.
Created attachment 293274 [details] [review] theme: Add titlebar_spacing Rather than defining the space to the left and right of buttons, add a simple spacing property that defines the space between buttons, which is what GTK+ does for client-side decorations (e.g. GtkButtons in a GtkBox). Unfortunately the value is hardcoded in GTK+; if it is exposed in the theme in the future, we should pick it up from there, but for now we just use the same value as GTK+.
Created attachment 293275 [details] [review] frames: Use title style to set up title layout Sounds obvious, doesn't it? After this change when titlebar-uses-system-font is set, the "system font" used will not be a generic one, but match what GTK+ uses in client-side decorations.
Created attachment 293276 [details] [review] theme: Remove get_title_scale() Not used anymore
Created attachment 293277 [details] [review] theme: Add method to adjust styles for frame state GTK+ expresses the window state as style classes and widget state for client-side decorations. Add a helper method to translate our own frame state to the corresponding changes to the style context hierarchy.
Created attachment 293278 [details] [review] theme: Add function to fill geometry information from GTK+ theme We want to eventually pick up all theme information from GTK+ instead of our own theme format; to prepare for this, add another helper method to fill in geometry information from the GTK+ theme.
Created attachment 293279 [details] [review] theme: Use style information from GTK+ We now have everything in place to pick up geometry and drawing information from GTK+ rather than the metacity theme, so do just that; the metacity theme is now only used for some constants (title_scale, hide_buttons, ...), which we will replace soon.
Created attachment 293280 [details] [review] Properly update on GTK+ theme changes With geometry information picked up from GTK+, we need to queue a resize on GTK+ theme changes to correctly update to the new geometry.
Created attachment 293281 [details] [review] frames: Adapt frame mask/bounds The frame shape is relevant in three places: - the window decoration we draw - the frame mask (used for the shape region) - the frame bounds (used for clipping) All three should match, so make sure to use the same GTK+ method for the first two, and bring the (non-antialiased) third closer to the other two by removing an obscure modifier from the corner radius.
Created attachment 293282 [details] [review] theme: Scale whitespace from theme with title_scale factor GTK+ doesn't deal with different frame types for its client-side decorations - it just treats dialogs the same as normal windows and ignores the odder frame types like UTILITY and MENU. That's fine as those have largely gone out of fashion anyway, but it's a different case for the WM - we still have to support them somehow. For now, just apply the existing title_scale factor to the geometry information picked up from the theme in addition to the title font. If it turns out that there's demand for something more sophisticated, we can still consider adding wm-only style information to the GTK+ theme.
Created attachment 293283 [details] [review] theme: Disable support for fringe buttons Few themes ever had support for those in the first place, and even less supported them properly; in particular support in the default theme has been broken for a while now. With this in mind (and considering that not even the tweak tool exposes any UI to configure them), let's (try to) remove support altogether - the corresponding rects are still kept around, so it's easy to add back in case we reconsider (and get the necessary artwork).
Created attachment 293284 [details] [review] theme: Don't load metacity themes All geometry/drawing information is now picked up from the GTK+ theme, so replace the remaining bits (hide_buttons + title_scale) with hardcoded values from the default Adwaita theme and stop loading the metacity theme altogether. If there is a need to theme those constants again in the future, we should make them available from GTK+ where they are available for client-side decorations as well. They certainly don't justify maintaining support for a complex theme format.
Created attachment 293285 [details] [review] theme: Use a singleton theme Different themes don't make sense when we are always using the current GTK+ theme for everything, so adapt the MetaTheme API to use a singleton.
Created attachment 293286 [details] [review] Remove all support for the metacity format Rest in peace you magnificent format, love-child of arcane X11 drawing API and markup craze, you will not be missed. We do remember however the bravery of a many men and women, who fearlessly descended into the guts of your intrinsics and turned ugliness into beauty; their work will still be spoken of when you will long have been forgotten.
Created attachment 293287 [details] [review] frames: Rename layout to text_layout ... to differentiate PangoLayout from MetaFrameLayout.
Created attachment 293288 [details] [review] theme: Remove FrameResize Really, styling windows differently based on how they can be resized is over the top ...
Created attachment 293289 [details] [review] theme: Remove MetaFrameStyle/MetaFrameStyleSet MetaFrameStyle now only holds a MetaFrameLayout, so we can cut out the middle man and use the layout directly. And as we are already using a single style/layout per frame set and handle frame state and focus by setting appropriate style flags, MetaFrameStyleSet is pointless too - just store one MetaFrameLayout per frame type directly in the theme.
Created attachment 293290 [details] [review] theme: Move the layout we save closer to GTK+'s model With support for the old metacity theme format gone, there's no reason to keep storing theme information in terms of the old theme properties. Just store the padding/border information for each element directly.
Review of attachment 293271 [details] [review]: OK.
Review of attachment 293272 [details] [review]: I have perhaps a dumb question. If this is all part of a singleton theme in the end, why do we need to refcount it? It seems like we should be able to embed this directly in MetaTheme / MetaFrameLayout.
Review of attachment 293274 [details] [review]: ::: src/ui/theme.c @@ +138,3 @@ layout->button_height = -1; + layout->titlebar_spacing = 6; /* hardcoded in GTK+ */ Can you link to the line of code that hardcodes it, for future reference?
Review of attachment 293275 [details] [review]: OK.
Review of attachment 293276 [details] [review]: Squash with prev. patch.
Review of attachment 293277 [details] [review]: ::: src/ui/theme.c @@ +5175,3 @@ + gtk_style_context_set_state (style, + backdrop ? state | GTK_STATE_FLAG_BACKDROP + : state & ~GTK_STATE_FLAG_BACKDROP); Not a big fan of ternary / bitfield use. Mind turning this into an if/else?
(I'm waiting on future patches before reviewing the rest of this one, FYI)
Comment on attachment 293276 [details] [review] theme: Remove get_title_scale() (In reply to comment #27) > (I'm waiting on future patches before reviewing the rest of this one, FYI) Ah, that was not obvious (given the minor comments) - I was actually about to just push it (with the comments addressed), given that our strict review policy has gone down the drain anyway. (In reply to comment #22) > It seems like we should be able to embed this directly in MetaTheme / MetaFrameLayout. No. MetaTheme is a singleton, MetaStyleInfo is not; MetaFrameLayout is per window type, while MetaStyleInfo is per theme variant. The only place we could embed this directly is MetaUIFrame, but not sharing identical style infos between frames using the same theme variant is certainly more wasteful than a refcount. Getting rid of the refcount here would certainly be possible (we only actually free the style variants on theme changes, not when the last frame using it goes away), but I don't see a very strong reason to do so to be honest. Attaching updated patches with comments addressed.
Created attachment 293443 [details] [review] theme: Add titlebar_spacing Added link to the GTK+ code in question.
Created attachment 293444 [details] [review] frames: Use title style to set up title layout Squashed follow-up patch.
Created attachment 293445 [details] [review] theme: Add method to adjust styles for frame state Turn ternary operator into if-else block
(In reply to comment #28) > Ah, that was not obvious (given the minor comments) - I was actually about to > just push it (with the comments addressed), given that our strict review policy > has gone down the drain anyway. I'm fine with that. I looked at the rest of the code, and while I have some minor complaints, it's a hell of a lot better than what we had before. So just go ahead and I'll do my fixups on what I find. And... it seems faster??? A few minor comments from trying it out. It seems the theme looks really different. I get a very obvious border at the top of the window contents. While I'm sure Endless would be happy with that[0], it's clearly not the effect we want. Do I need changes in the GTK+ theme on this? (In reply to comment #8) > Created an attachment (id=293278) [details] [review] > theme: Add function to fill geometry information from GTK+ theme When trying to rebase, gcc crashed on this one, since the function was unused. Either squash this into the next patch, or add G_GNUC_UNUSED and tear it out on the next commit. Can you go through and make sure it all builds at all steps? I know I'm not the patron saint for this either, having just broken the build twice this morning :) [0] https://github.com/endlessm/mutter/commit/7cd83589c926023bc950e155792b2c51143f435d
(In reply to comment #32) > (In reply to comment #28) > > Ah, that was not obvious (given the minor comments) - I was actually about to > > just push it (with the comments addressed), given that our strict review policy > > has gone down the drain anyway. > > I'm fine with that. I looked at the rest of the code, and while I have some > minor complaints Nah, if you already know you have comments, tell me about it :-p > It seems the theme looks really different. I get a very obvious border at the > top of the window contents. While I'm sure Endless would be happy with that[0], > it's clearly not the effect we want. I'm not sure about that - at this point, the GTK+ theme is much more maintained than the metacity one, so it could actually be the latter one which is "wrong". In any case yes, if this change is undesired, it is a GTK+ theme issue - the goal of these patches is not to match the old metacity theme (I'll leave that to the designers), but to get decorations closer to running a GTK+ application with GTK_CSD=1. > When trying to rebase, gcc crashed on this one, since the function was unused. > Either squash this into the next patch, or add G_GNUC_UNUSED and tear it out on > the next commit. > > Can you go through and make sure it all builds at all steps? Grr, looks like I didn't have that particular warning enabled, so rebase --exec "jhbuild make" didn't catch this.
Attachment 293271 [details] pushed as 75105e2 - theme: Rename button_rect() to get_button_rect() Attachment 293443 [details] pushed as db04ac9 - theme: Add titlebar_spacing Pushing these two since they're simple and let's get them out of the way.
Attachment 293272 [details] pushed as 472f2a4 - theme: Add MetaStyleInfo for wrapping frame style context Attachment 293273 [details] pushed as 2db71e7 - theme: Build a StyleContext hierarchy that matches GTK+'s CSD Attachment 293278 [details] pushed as fb14590 - theme: Add function to fill geometry information from GTK+ theme Attachment 293279 [details] pushed as 6eda784 - theme: Use style information from GTK+ Attachment 293280 [details] pushed as 26c4c21 - Properly update on GTK+ theme changes Attachment 293281 [details] pushed as 2cabc06 - frames: Adapt frame mask/bounds Attachment 293282 [details] pushed as 8a7a01b - theme: Scale whitespace from theme with title_scale factor Attachment 293283 [details] pushed as 34ac803 - theme: Disable support for fringe buttons Attachment 293284 [details] pushed as d5e6177 - theme: Don't load metacity themes Attachment 293285 [details] pushed as 662dd6a - theme: Use a singleton theme Attachment 293286 [details] pushed as 5e9db42 - Remove all support for the metacity format Attachment 293287 [details] pushed as ef32899 - frames: Rename layout to text_layout Attachment 293289 [details] pushed as ee461b5 - theme: Remove MetaFrameStyle/MetaFrameStyleSet Attachment 293290 [details] pushed as 6b92b45 - theme: Move the layout we save closer to GTK+'s model Attachment 293444 [details] pushed as 89a371e - frames: Use title style to set up title layout Attachment 293445 [details] pushed as bc9547f - theme: Add method to adjust styles for frame state OK. I fixed a few minor nits. Otherwise, let's just go. Nice work, Florian!