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 694381 - Add support for client side decorations
Add support for client side decorations
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
: 587475 670490 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-02-21 17:59 UTC by Rob Bradford
Modified: 2013-03-17 16:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] application-window: Respect allocation of any decorations (1.36 KB, patch)
2013-02-21 18:01 UTC, Rob Bradford
reviewed Details | Review
[PATCH] window: Add initial support for client-side decorations under Wayland (27.32 KB, patch)
2013-02-21 18:01 UTC, Rob Bradford
reviewed Details | Review
[PATCH] css: Add style entries for client side decorations to default CSS (1.98 KB, patch)
2013-02-21 18:01 UTC, Rob Bradford
reviewed Details | Review
[PATCH] window: Don't set a background pattern/colour when client decorated (1.77 KB, patch)
2013-02-21 18:01 UTC, Rob Bradford
reviewed Details | Review
[PATCH] window: Hide the decorations on fullscreen (4.55 KB, patch)
2013-02-21 18:01 UTC, Rob Bradford
reviewed Details | Review
[PATCH] window: Use same title fallback mechanism as X backend (1.65 KB, patch)
2013-02-21 18:02 UTC, Rob Bradford
reviewed Details | Review
[PATCH] window: Add support for enabling client decorations on non-Wayland (2.53 KB, patch)
2013-02-21 18:02 UTC, Rob Bradford
reviewed Details | Review
[PATCH] window: Move decoration creation to _realize (1.06 KB, patch)
2013-02-21 18:02 UTC, Rob Bradford
reviewed Details | Review
[PATCH] window: Don't require get_preferred_height/width to be run before allocate (19.43 KB, patch)
2013-02-21 18:02 UTC, Rob Bradford
reviewed Details | Review
screenshot (44.51 KB, image/png)
2013-03-15 23:16 UTC, Matthias Clasen
  Details

Description Rob Bradford 2013-02-21 17:59:07 UTC
Here are a set of patches that myself and Kristian have been working on over a substantial period of time to enable client side decorations for clients running against Wayland (but one of the patches optionally allows you to use this on X11)

I'd really appreciate some review on these - although they still have some flaws (the combobox demo in gtk3-demo doesn't render quite right - something to do with gtk_container_set_border_width)

My goal is to get this off-branch and into master ahead for the feature freeze next month.
Comment 1 Rob Bradford 2013-02-21 18:00:17 UTC
The patches are also on wip/csd-for-review
Comment 2 Rob Bradford 2013-02-21 18:01:41 UTC
Created attachment 237079 [details] [review]
[PATCH] application-window: Respect allocation of any decorations


Since _gtk_window_set_allocation () updates the allocation in place and may
update it to a smaller allocation (to allow the application of decorations) we
shouldn't setup the basis for the menubar's allocation until after this
function has been called.
---
 gtk/gtkapplicationwindow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Comment 3 Rob Bradford 2013-02-21 18:01:45 UTC
Created attachment 237080 [details] [review]
[PATCH] window: Add initial support for client-side decorations under Wayland


This change comprises four main parts:
 * the creation of the widgets that form the decorations,
 * implementation of get_preferred_height/width, and the for_width/for_height
   variants,
 * taking the decorations into account when allocating,
 * and drawing the decorations themselves.

Kristian did the bulk of the original work on this but any bugs are almost
certainly mine through the many refactorings and rebasings.
---
 gtk/gtkwindow.c | 574 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 558 insertions(+), 16 deletions(-)
Comment 4 Rob Bradford 2013-02-21 18:01:49 UTC
Created attachment 237081 [details] [review]
[PATCH] css: Add style entries for client side decorations to default CSS

 gtk/gtk-default.css | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
Comment 5 Rob Bradford 2013-02-21 18:01:53 UTC
Created attachment 237082 [details] [review]
[PATCH] window: Don't set a background pattern/colour when client decorated


Otherwise we'll potentially get some background sticking through our rounded
corners in our decorations. The actual background will get drawn as part of
the decoration drawing.
---
 gtk/gtkwindow.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
Comment 6 Rob Bradford 2013-02-21 18:01:56 UTC
Created attachment 237083 [details] [review]
[PATCH] window: Hide the decorations on fullscreen

 gtk/gtkwindow.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)
Comment 7 Rob Bradford 2013-02-21 18:02:01 UTC
Created attachment 237084 [details] [review]
[PATCH] window: Use same title fallback mechanism as X backend


This looks at the application name or program name and uses that as the title
if gtk_window_set_title has not been called.
---
 gtk/gtkwindow.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
Comment 8 Rob Bradford 2013-02-21 18:02:05 UTC
Created attachment 237085 [details] [review]
[PATCH] window: Add support for enabling client decorations on non-Wayland


Client side decorations can be enabled on non-Wayland platforms by setting the
GTK_CSD="1" environment variable.

We must ensure we have a GdkVisual that has an alpha channel since the
decorations rely on transparency. If we cannot get a visual with an alpha
channel then we do not enable client side decorations.
---
 gtk/gtkwindow.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)
Comment 9 Rob Bradford 2013-02-21 18:02:09 UTC
Created attachment 237086 [details] [review]
[PATCH] window: Move decoration creation to _realize


Evince directly calls gtk_widget_realize () in its window setup therefore we
need to ensure the decorations are created at this point.
---
 gtk/gtkwindow.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Comment 10 Rob Bradford 2013-02-21 18:02:14 UTC
Created attachment 237087 [details] [review]
[PATCH] window: Don't require get_preferred_height/width to be run before allocate


gtk_window_get_preferred_width/height were retrieving the border and title
height and then saving that into the private structure of the window. This was
then used inside the _gtk_window_set_allocation and gtk_window_draw methods.

If a subclass did not chain up for get_preferred_height / get_preferred_width
(like Nautilus) then these values would not be saved and the allocation and
drawing would not work correctly.

This resolves this by spinning out the retrieval of that information into a
separate function and using that every time the data is required.
---
 gtk/gtkwindow.c | 257 ++++++++++++++++++++++++++++++--------------------------
 1 file changed, 136 insertions(+), 121 deletions(-)
Comment 11 William Jon McCann 2013-02-22 20:54:43 UTC
*** Bug 670490 has been marked as a duplicate of this bug. ***
Comment 12 Emmanuele Bassi (:ebassi) 2013-02-26 17:22:20 UTC
Review of attachment 237079 [details] [review]:

::: gtk/gtkapplicationwindow.c
@@ +708,3 @@
       GtkWidget *child;
 
+      /* Updates allocation to be the child allocation area for the window */

this comment (and the commit) is a bit weird: _gtk_window_set_allocation() sets the allocation of the window, and updates internal details like the grip bar.

the allocation passed to _gtk_window_set_allocation() is not touched; the argument should be constified to reflect this, like gtk_widget_set_allocation() takes a const GtkAllocation*.

if you changed the behaviour of _gtk_window_set_allocation() and made the allocation argument an in-out in a later commit, you should probably squash this commit with it.
Comment 13 Emmanuele Bassi (:ebassi) 2013-02-26 17:48:35 UTC
Review of attachment 237080 [details] [review]:

::: gtk/gtkwindow.c
@@ +4835,3 @@
+    return;
+#else
+  return;

not a huge fan of an unconditional return with a conditional compilation option, really.

@@ +4839,3 @@
+
+  if (!priv->decorated)
+    return;

you probably want to move this above the display manager check.

@@ +4843,3 @@
+  priv->client_decorated = TRUE;
+
+  if (priv->type != GTK_WINDOW_POPUP)

below you check for TOPLEVEL, so I'd do a priv->type == GTK_WINDOW_TOPLEVEL check here as well, instead of !POPUP.

@@ +4854,3 @@
+        title = priv->title;
+      gtk_label_set_markup (GTK_LABEL (priv->title_label), title);
+      gtk_box_pack_start (GTK_BOX (priv->title_box),

I'd use the GtkWidget alignment and expansion flags, as well as a GtkGrid - though, obviously, CSS styling becomes an issue unless you add a class for each control.

@@ +4857,3 @@
+                          priv->title_label, TRUE, TRUE, 0);
+
+      priv->title_close_button = gtk_button_new_with_label ("×");

cute. ;-)

I bet that the design team will want to use an image, though, so this should probably be a GtkButton with icon-name. also, it should have a specific CSS id or a specific CSS class, for theme authors to (ab)use.

I know that we only have the close button, but we should probably add a minimize and a maximize buttons as well, for use on Windows and MacOS, given that they do have those there - assuming we want to turn on this on non-Wayland platforms.

@@ +5010,3 @@
     gtk_widget_map (child);
 
+  if (priv->title_box && !gtk_widget_get_mapped (priv->title_box))

gtk_widget_map() will do a "is mapped" check for you, so you can call it unconditionally.

coding style: you should do an explicit NULL check, and not use the thruthy value of pointers.

@@ +5167,3 @@
   priv->below_initially = (state & GDK_WINDOW_STATE_BELOW) != 0;
 
+  if (priv->title_box)

coding style: explicit NULL check.

@@ +5651,3 @@
+  child_allocation.x += border_width;
+  child_allocation.y += border_width;
+  child_allocation.width -= border_width * 2;

you want to do a negative value check for the width, here.

@@ +5652,3 @@
+  child_allocation.y += border_width;
+  child_allocation.width -= border_width * 2;
+  child_allocation.height -= border_width * 2;

and do the same for the height.

@@ +5655,3 @@
+
+  if (priv->client_decorated && priv->decorated &&
+      priv->title_box &&

coding style: explicit NULL check.

@@ +5704,3 @@
     }
+
+  *allocation = child_allocation;

okay, this is where you change the allocation argument of _gtk_window_set_allocation() from a strict in argument to an in-out argument; you should note that in the documentation of the function, so that it's easy to *not* miss.

I'd probably decouple it, and use a constified in argument, and add an out argument, so that internally you're always operating on a copy, but that's just my general dislike of in-out arguments, so it's not a blocker.

I think you should merge the previous commit with this one, because it really makes more sense to have the behaviour change atomically replaceable (and greppable in the log).

@@ +5716,1 @@
+  /* Updates the allocation to the child allocation in place */

this is why I'd rather have two arguments: you're passing an argument that's to be considered in-only to a function that manipulates it as an in-out parameter. there's no telling what the signal emission will do, so I'd rather the internals of GTK be resilient against people injecting crap values.

@@ +6257,3 @@
+          event->y < allocation.y + border_width + allocation.height)
+        {
+          gdk_window_begin_move_drag_for_device (gtk_widget_get_window(widget),

coding style: missing space between function and brace.

@@ +6258,3 @@
+        {
+          gdk_window_begin_move_drag_for_device (gtk_widget_get_window(widget),
+                                                 gdk_event_get_device((GdkEvent *) event),

coding style: missing space between function and brace.

@@ +6434,3 @@
+static void
+gtk_window_forall (GtkContainer *container,
+                   gboolean	     include_internals,

coding style: too much whitespace.

@@ +6444,3 @@
+  child = gtk_bin_get_child (GTK_BIN (container));
+  if (child)
+    (* callback) (child, callback_data);

no need to de-reference the callback function pointer: it just adds visual noise.

@@ +6447,3 @@
+
+  if (priv->title_box)
+    (* callback) (priv->title_box, callback_data);

same as above.

also, I'd consider the title box an internal widget, and add a check on include_internals.

@@ +6458,3 @@
+  gboolean widget_was_visible;
+
+  /* Modified from gtk_bin_remove() to work with the decoration widgets. */

yeah, I don't really like the ability to remove widgets from the title bar.

hiding widgets, and maybe adding widgets, is fine by me - but being able to screw up the title bar itself is a bit too much "bazooka targeted at your feet" for an API.

@@ +6674,3 @@
+  if (priv->client_decorated &&
+      priv->decorated &&
+      priv->type == GTK_WINDOW_TOPLEVEL)

can the client_decorated and decorated bits be set on windows that are not TOPLEVELs? you check for !POPUP, so I'd expect no.

@@ +6741,3 @@
+  if (priv->client_decorated &&
+      priv->decorated &&
+      priv->type == GTK_WINDOW_TOPLEVEL)

same as in get_preferred_height_for_width(), above.

@@ +6809,3 @@
+  if (priv->client_decorated &&
+      priv->decorated &&
+      priv->type == GTK_WINDOW_TOPLEVEL)

same as above.

@@ +6876,3 @@
+  if (priv->client_decorated &&
+      priv->decorated &&
+      priv->type == GTK_WINDOW_TOPLEVEL)

same as above.

I'm a tad worried about the code duplication, here, with GtkBin. it probably cannot be avoided, tho.

@@ +8078,3 @@
+      else
+        {
+          gtk_widget_get_allocation (widget, &allocation);

you could call gtk_widget_get_allocation() outside the check, and do it once.

@@ +8090,3 @@
+      gtk_style_context_save (context);
+      gtk_style_context_add_class (context, "titlebar");
+      gtk_widget_get_allocation (priv->title_box, &allocation);  

same here.

@@ +8092,3 @@
+      gtk_widget_get_allocation (priv->title_box, &allocation);  
+
+      /* Why do these subtract ? */

I don't know, you should probably tell us... :-)
Comment 14 Emmanuele Bassi (:ebassi) 2013-02-26 17:49:25 UTC
Review of attachment 237081 [details] [review]:

in general, if you added a bunch of CSS ids or classes it would be easier to theme the title bar widgets, instead of using selectors with low specificity.
Comment 15 Emmanuele Bassi (:ebassi) 2013-02-26 17:50:44 UTC
Review of attachment 237082 [details] [review]:

the comments could be reworded to be a bit better, but I'm not entirely sure they are needed at all.
Comment 16 Emmanuele Bassi (:ebassi) 2013-02-26 17:52:39 UTC
Review of attachment 237083 [details] [review]:

::: gtk/gtkwindow.c
@@ +5666,3 @@
   if (priv->client_decorated && priv->decorated &&
       priv->title_box &&
+      gtk_widget_get_visual(priv->title_box) &&

coding style: missing space.

@@ +6268,3 @@
+           priv->decorated &&
+           priv->title_box &&
+           !priv->fullscreen)

this is getting a bit ridiculous, though; I'd use a macro, here:

#define HAS_DECORATIONS(w) ((w)->priv->client_decorated && ...)

@@ +6700,3 @@
       priv->decorated &&
+      priv->type == GTK_WINDOW_TOPLEVEL &&
+      !priv->fullscreen)

same as above.
Comment 17 Emmanuele Bassi (:ebassi) 2013-02-26 17:53:56 UTC
Review of attachment 237084 [details] [review]:

::: gtk/gtkwindow.c
@@ +4820,3 @@
 }
 
+/* copied from gdkwindow-x11.c */

do we want some public API for this? something like:

  const char *gdk_get_default_title (void);

maybe?
Comment 18 Emmanuele Bassi (:ebassi) 2013-02-26 17:55:57 UTC
Review of attachment 237085 [details] [review]:

::: gtk/gtkwindow.c
@@ +3387,3 @@
   if (gdk_window)
     {
+      if (priv->decorated && !priv->client_decorated)

this looks like it belongs with a previous commit.

in theory, this commit should only add the GTK_CSD environment variable, and the check for it.

@@ +4859,3 @@
+  if (!priv->client_decorated &&
+      g_getenv ("GTK_CSD") &&
+      g_str_equal (g_getenv ("GTK_CSD"), "1"))

use g_strcmp0() instead of g_str_equal() - it's NULL-safe.

@@ +5416,3 @@
     gdk_window_set_role (gdk_window, priv->wm_role);
   
+  if (!priv->decorated || priv->client_decorated)

this too should probably go in a previous commit.
Comment 19 Emmanuele Bassi (:ebassi) 2013-02-26 17:56:22 UTC
Review of attachment 237086 [details] [review]:

this commit is squashable.
Comment 20 Emmanuele Bassi (:ebassi) 2013-02-26 18:00:51 UTC
Review of attachment 237087 [details] [review]:

I wonder if there should be an equivalent public API for this. we have gtk_window_get_resize_grip_area() as well, so having a gtk_window_get_titlebar_border() and a gtk_window_get_frame_border() may be good. on the other hand, I can't really think of any legal use case, unless you want to do some weird-ass event handling yourself...

::: gtk/gtkwindow.c
@@ +5670,3 @@
+  GtkStateFlags state;
+
+  if (!title_border && !window_border)

coding style: explicit NULL check.

I'd also assert() that both are non NULL, because if they are then calling this function is a pointless waste of time, and it should be caught during development.

@@ +5676,3 @@
+  state = gtk_style_context_get_state (context);
+
+  if (title_border)

coding style: explicit NULL check.

@@ +5684,3 @@
+    }
+
+  if (window_border)

coding style: explicit NULL check.

@@ +6844,3 @@
                                                      &title_min, &title_nat);
+          get_decoration_borders (widget, &title_border, &window_border);
+        } else {

coding style: braces on another line.

@@ +6905,3 @@
+                                           &title_height);
+          get_decoration_borders (widget, &title_border, &window_border);
+        } else {

coding style: braces on another line.

@@ +6966,3 @@
+          get_decoration_borders (widget, &title_border, &window_border);
+        } else {
+          get_decoration_borders (widget, NULL, &window_border);

coding style: braces on another line.
Comment 21 Rob Bradford 2013-02-26 19:57:02 UTC
Thanks Emmanuele - i'm working to integrate your feedback and will post a revised set of patches as soon as possible.
Comment 22 Javier Jardón (IRC: jjardon) 2013-03-05 10:30:51 UTC
*** Bug 587475 has been marked as a duplicate of this bug. ***
Comment 23 Xavier Claessens 2013-03-09 10:46:55 UTC
I was currious how this is implemented, so I quickly looked at the code. It seems wrong that gtk_window_forall() includes priv->title_box even if include_internals is FALSE. If that's fixed then gtk_window_remove() has no reason to be called with title_box anymore, and title_box would be leaked. So it will have to be unparented from gtk_window_dispose() I guess.

Or is there something I did not understand?
Comment 24 Matthias Clasen 2013-03-09 23:21:02 UTC
After taking a brief look, I must say this is a little limited.
Codys old client-side-decorations branch (https://github.com/bratsche/gtk/tree/client-side-decorations) had many more things worked out: 
 min/max buttons
 titlebar clicks
 rounded corners
 resize handles 
 shadows

We can probably merge this right after 3.8, to have some decorations on Wayland at least, but we should see if we can revive some of the things from that branch.
Comment 25 Jasper St. Pierre (not reading bugmail) 2013-03-10 17:01:38 UTC
Review of attachment 237080 [details] [review]:

::: gtk/gtkwindow.c
@@ +6691,3 @@
+
+      gtk_style_context_save (context);
+      gtk_style_context_add_class (context, "window-border");

I really don't like how we add/remove the window-border style context from a window. Perhaps we should have a window style class that we have permanently.
Comment 26 Jasper St. Pierre (not reading bugmail) 2013-03-10 17:02:36 UTC
This is very limited. Perhaps we should land this for 3.8, but I'd like to get to a state of "be able to draw Adwaita with CSD".
Comment 27 Matthias Clasen 2013-03-11 02:23:04 UTC
I couldn't stop myself from spending a day on this, and added a number of features, including a port to a new GtkHeaderBar (nee GdHeaderBar) widget, and a version of invisible borders.

Still missing:

- shadows
- a way to tell the wm to ignore the invisible borders for tiling, etc
- api to add custom widgets to the titlebar, or replace the titlebar altogether
Comment 28 Matthias Clasen 2013-03-11 17:12:59 UTC
I should probably add some hints for how to try my additions:

My gtk-default.css has this:

* {
  color: @fg_color;
  border-color: shade (@bg_color, 0.6);
  padding: 2px;
  -GtkWindow-resize-grip-width: 0;
  -GtkWindow-resize-grip-height: 0;
  -GtkWindow-decoration-button-layout: 'icon:minimize,maximize,close';
}

.window-border {
  border-color: darker (@bg_color);
  border-radius: 10px;
  #border-width: 5px 5px 5px 5px;
  border-width: 0px;
  border-style: solid;
}

.window-outer-border {
  border-color: transparent;
  border-radius: 10px;
  border-width: 10px 10px 10px 10px;
  border-style: solid;
}


.titlebar GtkButton:first-child {
  border-radius: 10px 0px 0px 0px;
}

.titlebar GtkButton:last-child {
  border-radius: 0px 10px 0px 0px;
}


That should give you window icon, minimize, maximize and close buttons, as well as rounded corners on the outer buttons, and an invisible border
Comment 29 Matthias Clasen 2013-03-12 11:38:16 UTC
oh, one more thing that we should do when here: better fallback for app menu when we have csd - we can just put the menu in the titlebar, no need to add a full menubar in that case.
Comment 30 Matthias Clasen 2013-03-14 02:02:59 UTC
I would love to get some feedback and review on whats in the branch now; we still need to work through Emmanuele's review too.
Comment 31 Matthias Clasen 2013-03-15 23:16:05 UTC
I've added some of the css in comment 28 in git now.

Somewhat ironically, you have to set the system theme to Raleigh to see the Awaita-like decorations...
Comment 32 Matthias Clasen 2013-03-15 23:16:55 UTC
Created attachment 239012 [details]
screenshot
Comment 33 Matthias Clasen 2013-03-17 16:35:44 UTC
I've merged a version of this now, lets continue working on this on master.
I'll file individual bugs for leftover items.