xdg-surface: fix window geometry handling

It was completely wrong: according to the protocol, the effective
geometry is only updated on commit time if there pending state has
new state from xdg_surface.set_window_geometry or
xdg_surface.set_window_geometry has never been sent at all.

This commit adds wlr_xdg_surface.geometry which correctly matches the
effective window geometry and removes now-useless
wlr_xdg_surface_get_geometry().

include/wlr/types/wlr_xdg_shell.h

@@ -228,9 +228,16 @@ struct wlr_xdg_surface_configure {
 	};
 };
 
+enum wlr_xdg_surface_state_field {
+	WLR_XDG_SURFACE_STATE_WINDOW_GEOMETRY = 1 << 0,
+};
+
 struct wlr_xdg_surface_state {
-	uint32_t configure_serial;
+	uint32_t committed; // enum wlr_xdg_surface_state_field
+
 	struct wlr_box geometry;
+
+	uint32_t configure_serial;
 };
 
 /**
@@ -274,6 +281,8 @@ struct wlr_xdg_surface {
 	// Whether the latest commit is an initial commit
 	bool initial_commit;
 
+	struct wlr_box geometry;
+
 	struct {
 		struct wl_signal destroy;
 		struct wl_signal ping_timeout;
@@ -523,17 +532,6 @@ struct wlr_xdg_toplevel *wlr_xdg_toplevel_try_from_wlr_surface(struct wlr_surfac
  */
 struct wlr_xdg_popup *wlr_xdg_popup_try_from_wlr_surface(struct wlr_surface *surface);
 
-/**
- * Get the surface geometry.
- *
- * This is either the geometry as set by the client, or defaulted to the bounds
- * of the surface + the subsurfaces (as specified by the protocol).
- *
- * The x and y value can be < 0.
- */
-void wlr_xdg_surface_get_geometry(struct wlr_xdg_surface *surface,
-		struct wlr_box *box);
-
 /**
  * Call `iterator` on each mapped surface and popup in the xdg-surface tree
  * (whether or not this xdg-surface is mapped), with the surface's position

tinywl/tinywl.c

@@ -427,10 +427,9 @@ static void process_cursor_resize(struct tinywl_server *server, uint32_t time) {
 		}
 	}
 
-	struct wlr_box geo_box;
+	struct wlr_box *geo_box = &toplevel->xdg_toplevel->base->geometry;
-	wlr_xdg_surface_get_geometry(toplevel->xdg_toplevel->base, &geo_box);
 	wlr_scene_node_set_position(&toplevel->scene_tree->node,
-		new_left - geo_box.x, new_top - geo_box.y);
+		new_left - geo_box->x, new_top - geo_box->y);
 
 	int new_width = new_right - new_left;
 	int new_height = new_bottom - new_top;
@@ -727,17 +726,16 @@ static void begin_interactive(struct tinywl_toplevel *toplevel,
 		server->grab_x = server->cursor->x - toplevel->scene_tree->node.x;
 		server->grab_y = server->cursor->y - toplevel->scene_tree->node.y;
 	} else {
-		struct wlr_box geo_box;
+		struct wlr_box *geo_box = &toplevel->xdg_toplevel->base->geometry;
-		wlr_xdg_surface_get_geometry(toplevel->xdg_toplevel->base, &geo_box);
 
-		double border_x = (toplevel->scene_tree->node.x + geo_box.x) +
+		double border_x = (toplevel->scene_tree->node.x + geo_box->x) +
-			((edges & WLR_EDGE_RIGHT) ? geo_box.width : 0);
+			((edges & WLR_EDGE_RIGHT) ? geo_box->width : 0);
-		double border_y = (toplevel->scene_tree->node.y + geo_box.y) +
+		double border_y = (toplevel->scene_tree->node.y + geo_box->y) +
-			((edges & WLR_EDGE_BOTTOM) ? geo_box.height : 0);
+			((edges & WLR_EDGE_BOTTOM) ? geo_box->height : 0);
 		server->grab_x = server->cursor->x - border_x;
 		server->grab_y = server->cursor->y - border_y;
 
-		server->grab_geobox = geo_box;
+		server->grab_geobox = *geo_box;
 		server->grab_geobox.x += toplevel->scene_tree->node.x;
 		server->grab_geobox.y += toplevel->scene_tree->node.y;
 

types/scene/xdg_shell.c

@@ -34,10 +34,8 @@ static void scene_xdg_surface_update_position(
 		struct wlr_scene_xdg_surface *scene_xdg_surface) {
 	struct wlr_xdg_surface *xdg_surface = scene_xdg_surface->xdg_surface;
 
-	struct wlr_box geo = {0};
-	wlr_xdg_surface_get_geometry(xdg_surface, &geo);
 	wlr_scene_node_set_position(&scene_xdg_surface->surface_tree->node,
-		-geo.x, -geo.y);
+		-xdg_surface->geometry.x, -xdg_surface->geometry.y);
 
 	if (xdg_surface->role == WLR_XDG_SURFACE_ROLE_POPUP) {
 		struct wlr_xdg_popup *popup = xdg_surface->popup;

types/wlr_layer_shell_v1.c

@@ -574,8 +574,8 @@ void wlr_layer_surface_v1_for_each_popup_surface(struct wlr_layer_surface_v1 *su
 		}
 
 		double popup_sx, popup_sy;
-		popup_sx = popup->current.geometry.x - popup->base->current.geometry.x;
+		popup_sx = popup->current.geometry.x - popup->base->geometry.x;
-		popup_sy = popup->current.geometry.y - popup->base->current.geometry.y;
+		popup_sy = popup->current.geometry.y - popup->base->geometry.y;
 
 		struct layer_surface_iterator_data data = {
 			.user_iterator = iterator,
@@ -609,8 +609,8 @@ struct wlr_surface *wlr_layer_surface_v1_popup_surface_at(
 		}
 
 		double popup_sx, popup_sy;
-		popup_sx = popup->current.geometry.x - popup->base->current.geometry.x;
+		popup_sx = popup->current.geometry.x - popup->base->geometry.x;
-		popup_sy = popup->current.geometry.y - popup->base->current.geometry.y;
+		popup_sy = popup->current.geometry.y - popup->base->geometry.y;
 
 		struct wlr_surface *sub = wlr_xdg_surface_surface_at(
 			popup->base, sx - popup_sx, sy - popup_sy, sub_x, sub_y);

types/xdg_shell/wlr_xdg_popup.c

@@ -498,8 +498,8 @@ void wlr_xdg_popup_get_toplevel_coords(struct wlr_xdg_popup *popup,
 			popup_sy += xdg_surface->popup->current.geometry.y;
 			parent = xdg_surface->popup->parent;
 		} else {
-			popup_sx += xdg_surface->current.geometry.x;
+			popup_sx += xdg_surface->geometry.x;
-			popup_sy += xdg_surface->current.geometry.y;
+			popup_sy += xdg_surface->geometry.y;
 			break;
 		}
 	}

types/xdg_shell/wlr_xdg_surface.c

@@ -219,12 +219,12 @@ static void xdg_surface_handle_set_window_geometry(struct wl_client *client,
 	}
 
 	if (width <= 0 || height <= 0) {
-		wl_resource_post_error(resource,
+		wl_resource_post_error(resource, XDG_SURFACE_ERROR_INVALID_SIZE,
-			XDG_SURFACE_ERROR_INVALID_SIZE,
 			"Tried to set invalid xdg-surface geometry");
 		return;
 	}
 
+	surface->pending.committed |= WLR_XDG_SURFACE_STATE_WINDOW_GEOMETRY;
 	surface->pending.geometry.x = x;
 	surface->pending.geometry.y = y;
 	surface->pending.geometry.width = width;
@@ -256,6 +256,21 @@ static const struct xdg_surface_interface xdg_surface_implementation = {
 	.set_window_geometry = xdg_surface_handle_set_window_geometry,
 };
 
+// The window geometry is updated on commit, unless the commit is going to map
+// the surface, in which case it's updated on map, so that subsurfaces are
+// mapped and surface extents are computed correctly.
+static void update_geometry(struct wlr_xdg_surface *surface) {
+	if (!wlr_box_empty(&surface->current.geometry)) {
+		if ((surface->current.committed & WLR_XDG_SURFACE_STATE_WINDOW_GEOMETRY) != 0) {
+			wlr_surface_get_extents(surface->surface, &surface->geometry);
+			wlr_box_intersection(&surface->geometry,
+				&surface->current.geometry, &surface->geometry);
+		}
+	} else {
+		wlr_surface_get_extents(surface->surface, &surface->geometry);
+	}
+}
+
 static void xdg_surface_role_client_commit(struct wlr_surface *wlr_surface) {
 	struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface);
 	assert(surface != NULL);
@@ -320,11 +335,20 @@ static void xdg_surface_role_commit(struct wlr_surface *wlr_surface) {
 		break;
 	}
 
-	if (wlr_surface_has_buffer(wlr_surface)) {
+	if (wlr_surface->mapped) {
+		update_geometry(surface);
+	} else if (wlr_surface_has_buffer(wlr_surface)) {
 		wlr_surface_map(wlr_surface);
 	}
 }
 
+static void xdg_surface_role_map(struct wlr_surface *wlr_surface) {
+	struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface);
+	assert(surface != NULL);
+
+	update_geometry(surface);
+}
+
 static void xdg_surface_role_destroy(struct wlr_surface *wlr_surface) {
 	struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface);
 	if (surface == NULL) {
@@ -339,11 +363,19 @@ static const struct wlr_surface_role xdg_surface_role = {
 	.name = "xdg_surface",
 	.client_commit = xdg_surface_role_client_commit,
 	.commit = xdg_surface_role_commit,
+	.map = xdg_surface_role_map,
 	.destroy = xdg_surface_role_destroy,
 };
 
+static void surface_synced_move_state(void *_dst, void *_src) {
+	struct wlr_xdg_surface_state *dst = _dst, *src = _src;
+	*dst = *src;
+	src->committed = 0;
+}
+
 static const struct wlr_surface_synced_impl surface_synced_impl = {
 	.state_size = sizeof(struct wlr_xdg_surface_state),
+	.move_state = surface_synced_move_state,
 };
 
 struct wlr_xdg_surface *wlr_xdg_surface_try_from_wlr_surface(
@@ -522,12 +554,8 @@ void wlr_xdg_popup_get_position(struct wlr_xdg_popup *popup,
 		double *popup_sx, double *popup_sy) {
 	struct wlr_xdg_surface *parent = wlr_xdg_surface_try_from_wlr_surface(popup->parent);
 	assert(parent != NULL);
-	struct wlr_box parent_geo;
+	*popup_sx = parent->geometry.x + popup->current.geometry.x - popup->base->geometry.x;
-	wlr_xdg_surface_get_geometry(parent, &parent_geo);
+	*popup_sy = parent->geometry.y + popup->current.geometry.y - popup->base->geometry.y;
-	*popup_sx = parent_geo.x + popup->current.geometry.x -
-		popup->base->current.geometry.x;
-	*popup_sy = parent_geo.y + popup->current.geometry.y -
-		popup->base->current.geometry.y;
 }
 
 struct wlr_surface *wlr_xdg_surface_surface_at(
@@ -611,15 +639,3 @@ void wlr_xdg_surface_for_each_popup_surface(struct wlr_xdg_surface *surface,
 		wlr_surface_iterator_func_t iterator, void *user_data) {
 	xdg_surface_for_each_popup_surface(surface, 0, 0, iterator, user_data);
 }
-
-void wlr_xdg_surface_get_geometry(struct wlr_xdg_surface *surface,
-		struct wlr_box *box) {
-	wlr_surface_get_extents(surface->surface, box);
-
-	/* The client never set the geometry */
-	if (wlr_box_empty(&surface->current.geometry)) {
-		return;
-	}
-
-	wlr_box_intersection(box, &surface->current.geometry, box);
-}