Broken gravity handling when window size is restricted by PMaxSize in WM_NORMAL_HINTS

[sigprof]

The test program https://github.com/ccg/metacity/blob/master/src/wm-tester/test-gravity.c also has some tests for window resize handling:

• Clicking the mouse button 2 invokes XResizeWindow(), toggling the window size between 100×100 and 200×200.
• Clicking the mouse button 3 invokes XMoveResizeWindow(), toggling the window size between 100×100 and 200×200 and simultaneously moving the window to the position appropriate for its new size and gravity.

The twist is that the test program also sets PMaxSize in WM_NORMAL_HINTS, and sets both max_width and max_height to 150 — therefore the size passed to XResizeWindow() violates the PMaxSize constraint.

The appropriate handling for this situation is apparently not specified in ICCCM or EWMH specifications. Currently Awesome in this case recalculates the window position according to gravity while assuming that the window will be resized to 200×200, and then applies the PMaxSize restriction at some later step; this results in a 150×150 window with the top left corner positioned as if it was 200×200 (looks wrong for all gravities except NorthWestGravity and StaticGravity). Repeated button 2 clicks result in the window traveling towards the top left corner of the screen.

I tested some other window managers:

• xfwm4 and openbox treat the out-of-range resize request as a resize to the maximum size (150×150), and handle the gravity as for resize to 150×150; after repeated button 2 clicks the window remains in the same location.
• sawfish ignores the PMaxSize restriction and resizes the window to 200×200 (but any attempt to resize it manually results in the size jumping to 150×150); gravity is handled as appropriate for the 200×200 resize; after repeated button 2 clicks the window remains in the same location.

[psychon]

This (untested) patch got a bit more complicated than intended. The idea is to do what xfwm4 and openbox do and restrict configure requests to the size hints. However, the function to apply size hints wants an area_t as argument and that lead to quite some changes.

diff --git a/event.c b/event.c
index 935f8bc4..7bc4e780 100644
--- a/event.c
+++ b/event.c
@@ -329,7 +329,7 @@ event_handle_configurerequest(xcb_configure_request_event_t *ev)
 
     if((c = client_getbywin(ev->window)))
     {
-        area_t geometry = c->geometry;
+        area_t old_geometry = c->geometry;
         uint16_t bw = c->border_width;
         uint16_t tb_left = c->titlebar[CLIENT_TITLEBAR_LEFT].size;
         uint16_t tb_right = c->titlebar[CLIENT_TITLEBAR_RIGHT].size;
@@ -341,38 +341,36 @@ event_handle_configurerequest(xcb_configure_request_event_t *ev)
         uint16_t deco_bottom = bw + tb_bottom;
         int16_t diff_w = 0, diff_h = 0, diff_border = 0;
 
-        lua_State *L = globalconf_get_lua_State();
-
+        /* Figure out the newly requested geometry */
+        area_t geometry = old_geometry;
         if(ev->value_mask & XCB_CONFIG_WINDOW_X)
-        {
-            int16_t diff = 0;
             geometry.x = ev->x;
-            xwindow_translate_for_gravity(c->size_hints.win_gravity, deco_left, 0, deco_right, 0, &diff, NULL);
-            geometry.x += diff;
-        }
         if(ev->value_mask & XCB_CONFIG_WINDOW_Y)
-        {
-            int16_t diff = 0;
             geometry.y = ev->y;
-            xwindow_translate_for_gravity(c->size_hints.win_gravity, 0, deco_top, 0, deco_bottom, NULL, &diff);
-            geometry.y += diff;
-        }
+        /* The ConfigureRequest specifies the size of the client window, we want the frame */
         if(ev->value_mask & XCB_CONFIG_WINDOW_WIDTH)
-        {
-            uint16_t old_w = geometry.width;
-            geometry.width = ev->width;
-            /* The ConfigureRequest specifies the size of the client window, we want the frame */
-            geometry.width += tb_left + tb_right;
-            diff_w = geometry.width - old_w;
-        }
+            geometry.width = ev->width + tb_left + tb_right;
         if(ev->value_mask & XCB_CONFIG_WINDOW_HEIGHT)
+            geometry.height = ev->height + tb_top + tb_bottom;
+
+        /* Enforce that the request is consistent with the size hints */
+        geometry = client_apply_size_hints(c, geometry);
+
+        diff_w = geometry.width - old_geometry.width;
+        diff_h = geometry.height - old_geometry.height;
+
+        /* Requests for a position need to be corrected for the frame size */
+        if(ev->value_mask & (XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y))
         {
-            uint16_t old_h = geometry.height;
-            geometry.height = ev->height;
-            /* The ConfigureRequest specifies the size of the client window, we want the frame */
-            geometry.height += tb_top + tb_bottom;
-            diff_h = geometry.height - old_h;
+            int16_t diff_x = 0, diff_y = 0;
+            xwindow_translate_for_gravity(c->size_hints.win_gravity, deco_left, deco_top, deco_right, deco_bottom, &diff_x, &diff_y);
+            if(ev->value_mask & XCB_CONFIG_WINDOW_X)
+                geometry.x += diff_x;
+            if(ev->value_mask & XCB_CONFIG_WINDOW_Y)
+                geometry.y += diff_y;
         }
+
+        lua_State *L = globalconf_get_lua_State();
         if(ev->value_mask & XCB_CONFIG_WINDOW_BORDER_WIDTH)
         {
             diff_border = ev->border_width - bw;
diff --git a/objects/client.c b/objects/client.c
index 81720563..2271a231 100644
--- a/objects/client.c
+++ b/objects/client.c
@@ -1650,7 +1650,7 @@ client_send_configure(client_t *c)
 
 /** Apply size hints to the client's new geometry.
  */
-static area_t
+area_t
 client_apply_size_hints(client_t *c, area_t geometry)
 {
     int32_t minw = 0, minh = 0;
diff --git a/objects/client.h b/objects/client.h
index f9115961..78d958f6 100644
--- a/objects/client.h
+++ b/objects/client.h
@@ -201,6 +201,7 @@ void client_send_configure(client_t *);
 void client_find_transient_for(client_t *);
 drawable_t *client_get_drawable(client_t *, int, int);
 drawable_t *client_get_drawable_offset(client_t *, int *, int *);
+area_t client_apply_size_hints(client_t *c, area_t geometry);
 
 /** Put client on top of the stack.
  * \param c The client to raise.

I'll test that patch when I have some time.

(And again: Thanks for your great bug reports)

[sigprof]

There probably should be a test for c->size_hints_honor before calling client_apply_size_hints(), otherwise that flag will be broken (currently it is used in luaA_client_geometry()).

Also, looks like this code has a problem with gravities which require centering, similar to #2324 (I did not yet write a bug report for this, because I did not prepare a reproducer): xwindow_translate_for_gravity() is called with deltas, which can get truncated during division.