Created attachment 299427 [details] [review]
Proposed patch

Goal here is to keep the patch as small as possible. So it reuses the existing resize handles, but make them input/output windows without a compositor.

Also uses CSS for the border width and color/look. Disclaimer, CSS is not my area of expertise...

Created attachment 299428 [details]
Screenshot

Left is the gtk3-demo with a compositor, right is the same without.

Thanks for looking into this.

So, currently under X, the situation is the following:

We can do either
- full csd (shadow, invisible borders, custom titlebar)
- custom titlebar with wm borders
- traditional decorations

For full csd, we need a rgba visual, compositor and frame extents support, and we rely on GDK to convince the window manager to not decorate the window at all.

For custom titlebar, we rely on mwm hints to get border-only wm decorations.

The 'custom titlebar' case is a bit of a fallback: the application sets a custom titlebar which we have to show somehow, but we lack some of the requirements for doing full csd.

And the suggestion here is to change the custom titlebar case to not rely on mwm hints and instead draw a visible border client-side, in the hope that even wm without good support for mwm hints will get the 'totally undecorated' case right.

Here are some preliminary comments on the patch, not a thorough review:

1) I think I would prefer to keep the border windows input-only, and always draw onto the main window. Actually, looking at the draw code, you do that anyway. Why did you make the border windows input-output ?

2) The flags we keep to differentiate the various case (csd_requested, decorated, client_decorated, custom_title) are already confusing. Instead of just adding another one, we should kill the custom_title one (it is only used to select GDK_DECOR_BORDER, and we're not going to do that anymore), and instead have a client_shadow (or similar) flag that indicates full csd.

3) I think we need to differentiate between maximized and fullscreen - we want borders drawn in the first case, but not the second.

Created attachment 299491 [details] [review]
Updated patch

(In reply to Matthias Clasen from comment #4)
> Here are some preliminary comments on the patch, not a thorough review:

Thanks for that preliminary comments, it helps a lot!
 
> 1) I think I would prefer to keep the border windows input-only, and always
> draw onto the main window. Actually, looking at the draw code, you do that
> anyway.

Absolutely, and it actually makes the code cleaner and the patch smaller...

> Why did you make the border windows input-output ?

Err, don't ask... :)
 
> 2) The flags we keep to differentiate the various case (csd_requested,
> decorated, client_decorated, custom_title) are already confusing. Instead of
> just adding another one, we should kill the custom_title one (it is only
> used to select GDK_DECOR_BORDER, and we're not going to do that anymore),
> and instead have a client_shadow (or similar) flag that indicates full csd.

Yeap, even more code removed, brilliant.

> 3) I think we need to differentiate between maximized and fullscreen - we
> want borders drawn in the first case, but not the second.

I don't quite agree here (assuming I understood what you meant), I think it works as expected wrt decorations when maximized and fullscreen.

We already differentiate between maxinized and fullscreen, but s far as the resize border go, it's the same.

In both cases (maximize and fullscreen), we don't want the resize borders to be visible which is the case as of now (users cannot resize neither maximized nor fullscreen windows).

When fullscreen, we also hide the title box which is in line with what mose window managers do with SSD.

So for now, updated patch attached does 1) and 2) but not 3).

Review of attachment 299491 [details] [review]:

This looks better, just a few more things to sort out. 

Wrt to the css, it is generated from _common.scss by the parse-sass script in the same directory. They are in git to avoid a hard ruby dependency, so you should move your changes into the sass file and run parse-sass.sh.

::: gtk/gtkwindow.c
@@ +6493,3 @@
+      add_window_frame_style_class (context);
+  else
+      gtk_style_context_add_class (context, "decoration-frame");

I'm a little worried about this - add_window_frame_style_class is called in various places to set up the right style classes for measuring things that depends on style margins and padding and so forth. Therefore, I'd rather see us pick the 'right' style classes in that functions, always.

I would also like to stick to a single window-frame class for the decorations, and just add extra classes as necessary to differentiate the various cases. Looking at what we have now, we have a .csd
class that is interpreted in the css pretty much as 'we are composited, use shadows and alpha'.

So, we use

.window-frame.csd for full csd with shadows

There is

.window-frame.ssd as special case for use of gtk decorations on the server-side in mutter (just the same as .window-frame.csd, but without shadows)

.popup.csd is used for client-side shadows on menus


I would almost say we can just use .window-frame without any extra class in the same way as your .decoration-frame. But I'm not 100% sure it will work out. There's quite a bit of css that applies to
.window-frame without extra classes now, so I'm curious why that is.

If that doesn't work out, we could do something like .window-frame.solid or .window-frame.no-csd

@@ +9721,3 @@
+
+              gtk_style_context_restore (context);
+            }

I don't think this extra drawing code should be necessary - add_window_frame_style_class should just set up the right style classes, and then the code a few lines above that draws the window frame should cover this case. Right ?

Created attachment 299729 [details] [review]
Updated patch v3

(In reply to Matthias Clasen from comment #6)
> Review of attachment 299491 [details] [review] [review]:
> 
> Wrt to the css, it is generated from _common.scss by the parse-sass script
> in the same directory. They are in git to avoid a hard ruby dependency, so
> you should move your changes into the sass file and run parse-sass.sh.

Oh right, done!

> ::: gtk/gtkwindow.c
> @@ +6493,3 @@
> +      add_window_frame_style_class (context);
> +  else
> +      gtk_style_context_add_class (context, "decoration-frame");
> 
> I'm a little worried about this - add_window_frame_style_class is called in
> various places to set up the right style classes for measuring things that
> depends on style margins and padding and so forth. Therefore, I'd rather see
> us pick the 'right' style classes in that functions, always.

Done (I think)

> I would also like to stick to a single window-frame class for the
> decorations, and just add extra classes as necessary to differentiate the
> various cases. Looking at what we have now, we have a .csd
> class that is interpreted in the css pretty much as 'we are composited, use
> shadows and alpha'.

Done.

> I would almost say we can just use .window-frame without any extra class in
> the same way as your .decoration-frame. But I'm not 100% sure it will work
> out. There's quite a bit of css that applies to
> .window-frame without extra classes now, so I'm curious why that is.

Nope, unfortunately it does not work because we want the window handles but not the shadow, and the code takes the larger of the two, so by using the class as it is, we end up with a huge black border around the CSD without a compositor.

> If that doesn't work out, we could do something like .window-frame.solid or
> .window-frame.no-csd

Sold! I used "sold-csd" for this purpose :-)

> @@ +9721,3 @@
> +
> +              gtk_style_context_restore (context);
> +            }
> 
> I don't think this extra drawing code should be necessary -
> add_window_frame_style_class should just set up the right style classes, and
> then the code a few lines above that draws the window frame should cover
> this case. Right ?

Nope, I think we need it, because the existing code paints within the window border so without this code, we end up with the borders being left unpainted.

And we want to keep the borders area as this is where the resize handles reside.

But I removed the extra add_window_frame_style_class()/ gtk_style_context_restore()

Note, I have also added /some/ cleanup, as not setting the ARGB visual twice in gtk_window_set_titlebar() as it's already done in gtk_window_enable_csd() - This way, we don't use ARGB if a compositor is not available.

Created attachment 299730 [details] [review]
Updated patch v3 (the right one, this time)

(facepalm, wrong patch attached...)

Committed with slight modifications:
- only set one of the .csd and .solid-csd style classes
- only render background+frame once for the border area