xdg-shell: Rework the state system

The states system, so far, has been a complicated mix of weird APIs
that solved a real race condition, but have been particularly ugly
for both compositors and clients to implement.

clients/simple-egl.c

@@ -268,40 +268,39 @@ init_gl(struct window *window)
 
 static void
 handle_surface_configure(void *data, struct xdg_surface *surface,
-			 int32_t width, int32_t height)
+			 int32_t width, int32_t height,
+			 struct wl_array *states, uint32_t serial)
 {
 	struct window *window = data;
+	uint32_t *p;
 
-	if (window->native)
+	window->fullscreen = 0;
-		wl_egl_window_resize(window->native, width, height, 0, 0);
+	wl_array_for_each(p, states) {
-
+		uint32_t state = *p;
-	window->geometry.width = width;
+		switch (state) {
-	window->geometry.height = height;
+		case XDG_SURFACE_STATE_FULLSCREEN:
-
+			window->fullscreen = 1;
-	if (!window->fullscreen)
+			break;
-		window->window_size = window->geometry;
+		}
-}
-
-static void
-handle_surface_change_state(void *data, struct xdg_surface *xdg_surface,
-			    uint32_t state,
-			    uint32_t value,
-			    uint32_t serial)
-{
-	struct window *window = data;
-
-	switch (state) {
-	case XDG_SURFACE_STATE_FULLSCREEN:
-		window->fullscreen = value;
-
-		if (!value)
-			handle_surface_configure(window, window->xdg_surface,
-						 window->window_size.width,
-						 window->window_size.height);
-		break;
 	}
 
-	xdg_surface_ack_change_state(xdg_surface, state, value, serial);
+	if (width > 0 && height > 0) {
+		if (!window->fullscreen) {
+			window->window_size.width = width;
+			window->window_size.height = height;
+		}
+		window->geometry.width = width;
+		window->geometry.height = height;
+	} else if (!window->fullscreen) {
+		window->geometry = window->window_size;
+	}
+
+	if (window->native)
+		wl_egl_window_resize(window->native,
+				     window->geometry.width,
+				     window->geometry.height, 0, 0);
+
+	xdg_surface_ack_configure(surface, serial);
 }
 
 static void
@@ -322,7 +321,6 @@ handle_surface_delete(void *data, struct xdg_surface *xdg_surface)
 
 static const struct xdg_surface_listener xdg_surface_listener = {
 	handle_surface_configure,
-	handle_surface_change_state,
 	handle_surface_activated,
 	handle_surface_deactivated,
 	handle_surface_delete,
@@ -343,8 +341,8 @@ create_surface(struct window *window)
 
 	window->native =
 		wl_egl_window_create(window->surface,
-				     window->window_size.width,
+				     window->geometry.width,
-				     window->window_size.height);
+				     window->geometry.height);
 	window->egl_surface =
 		eglCreateWindowSurface(display->egl.dpy,
 				       display->egl.conf,
@@ -359,9 +357,8 @@ create_surface(struct window *window)
 	if (!window->frame_sync)
 		eglSwapInterval(display->egl.dpy, 0);
 
-	xdg_surface_request_change_state(window->xdg_surface,
+	if (window->fullscreen)
-					 XDG_SURFACE_STATE_FULLSCREEN,
+		xdg_surface_set_fullscreen(window->xdg_surface, NULL);
-					 window->fullscreen, 0);
 }
 
 static void
@@ -620,11 +617,12 @@ keyboard_handle_key(void *data, struct wl_keyboard *keyboard,
 {
 	struct display *d = data;
 
-	if (key == KEY_F11 && state)
+	if (key == KEY_F11 && state) {
-		xdg_surface_request_change_state(d->window->xdg_surface,
+		if (d->window->fullscreen)
-						 XDG_SURFACE_STATE_FULLSCREEN,
+			xdg_surface_unset_fullscreen(d->window->xdg_surface);
-						 !d->window->fullscreen, 0);
+		else
-	else if (key == KEY_ESC && state)
+			xdg_surface_set_fullscreen(d->window->xdg_surface, NULL);
+	} else if (key == KEY_ESC && state)
 		running = 0;
 }
 
@@ -772,8 +770,9 @@ main(int argc, char **argv)
 
 	window.display = &display;
 	display.window = &window;
-	window.window_size.width  = 250;
+	window.geometry.width  = 250;
-	window.window_size.height = 250;
+	window.geometry.height = 250;
+	window.window_size = window.geometry;
 	window.buffer_size = 32;
 	window.frame_sync = 1;
 

clients/simple-shm.c

@@ -117,16 +117,10 @@ create_shm_buffer(struct display *display, struct buffer *buffer,
 
 static void
 handle_configure(void *data, struct xdg_surface *surface,
-		 int32_t width, int32_t height)
+		 int32_t width, int32_t height,
-{
+		 struct wl_array *states, uint32_t serial)
-}
-
-static void
-handle_change_state(void *data, struct xdg_surface *xdg_surface,
-		    uint32_t state,
-		    uint32_t value,
-		    uint32_t serial)
 {
+	xdg_surface_ack_configure(surface, serial);
 }
 
 static void
@@ -147,7 +141,6 @@ handle_delete(void *data, struct xdg_surface *xdg_surface)
 
 static const struct xdg_surface_listener xdg_surface_listener = {
 	handle_configure,
-	handle_change_state,
 	handle_activated,
 	handle_deactivated,
 	handle_delete,

clients/window.c

@@ -230,6 +230,8 @@ struct window {
 	int fullscreen;
 	int maximized;
 
+	int next_attach_serial;
+
 	enum preferred_format preferred_format;
 
 	window_key_handler_t key_handler;
@@ -1338,6 +1340,12 @@ surface_flush(struct surface *surface)
 		surface->input_region = NULL;
 	}
 
+	if (surface->window->next_attach_serial > 0) {
+		xdg_surface_ack_configure(surface->window->xdg_surface,
+					  surface->window->next_attach_serial);
+		surface->window->next_attach_serial = 0;
+	}
+
 	surface->toysurface->swap(surface->toysurface,
 				  surface->buffer_transform, surface->buffer_scale,
 				  &surface->server_allocation);
@@ -3849,37 +3857,39 @@ widget_schedule_resize(struct widget *widget, int32_t width, int32_t height)
 
 static void
 handle_surface_configure(void *data, struct xdg_surface *xdg_surface,
-			 int32_t width, int32_t height)
+			 int32_t width, int32_t height,
+			 struct wl_array *states, uint32_t serial)
 {
 	struct window *window = data;
+	uint32_t *p;
 
-	window_schedule_resize(window, width, height);
+	if (width > 0 && height > 0) {
-}
+		window_schedule_resize(window, width, height);
-
+	} else {
-static void
-handle_surface_change_state(void *data, struct xdg_surface *xdg_surface,
-			    uint32_t state,
-			    uint32_t value,
-			    uint32_t serial)
-{
-	struct window *window = data;
-
-	switch (state) {
-	case XDG_SURFACE_STATE_MAXIMIZED:
-		window->maximized = value;
-		break;
-	case XDG_SURFACE_STATE_FULLSCREEN:
-		window->fullscreen = value;
-		break;
-	}
-
-	if (!window->fullscreen && !window->maximized)
 		window_schedule_resize(window,
 				       window->saved_allocation.width,
 				       window->saved_allocation.height);
+	}
 
-	xdg_surface_ack_change_state(xdg_surface, state, value, serial);
+	window->maximized = 0;
-	window_schedule_redraw(window);
+	window->fullscreen = 0;
+
+	wl_array_for_each(p, states) {
+		uint32_t state = *p;
+		switch (state) {
+		case XDG_SURFACE_STATE_MAXIMIZED:
+			window->maximized = 1;
+			break;
+		case XDG_SURFACE_STATE_FULLSCREEN:
+			window->fullscreen = 1;
+			break;
+		default:
+			/* Unknown state */
+			break;
+		}
+	}
+
+	window->next_attach_serial = serial;
 }
 
 static void
@@ -3905,7 +3915,6 @@ handle_surface_delete(void *data, struct xdg_surface *xdg_surface)
 
 static const struct xdg_surface_listener xdg_surface_listener = {
 	handle_surface_configure,
-	handle_surface_change_state,
 	handle_surface_activated,
 	handle_surface_deactivated,
 	handle_surface_delete,
@@ -4145,10 +4154,10 @@ window_set_fullscreen(struct window *window, int fullscreen)
 	if (window->fullscreen == fullscreen)
 		return;
 
-	xdg_surface_request_change_state(window->xdg_surface,
+	if (fullscreen)
-					 XDG_SURFACE_STATE_FULLSCREEN,
+		xdg_surface_set_fullscreen(window->xdg_surface, NULL);
-					 fullscreen ? 1 : 0,
+	else
-					 0);
+		xdg_surface_unset_fullscreen(window->xdg_surface);
 }
 
 int
@@ -4166,10 +4175,10 @@ window_set_maximized(struct window *window, int maximized)
 	if (window->maximized == maximized)
 		return;
 
-	xdg_surface_request_change_state(window->xdg_surface,
+	if (maximized)
-					 XDG_SURFACE_STATE_MAXIMIZED,
+		xdg_surface_set_maximized(window->xdg_surface);
-					 maximized ? 1 : 0,
+	else
-					 0);
+		xdg_surface_unset_maximized(window->xdg_surface);
 }
 
 void

desktop-shell/shell.c

@@ -3418,78 +3418,9 @@ xdg_surface_resize(struct wl_client *client, struct wl_resource *resource,
 }
 
 static void
-xdg_surface_set_output(struct wl_client *client,
+xdg_surface_ack_configure(struct wl_client *client,
-		       struct wl_resource *resource,
+			  struct wl_resource *resource,
-		       struct wl_resource *output_resource)
+			  uint32_t serial)
-{
-	struct shell_surface *shsurf = wl_resource_get_user_data(resource);
-	struct weston_output *output;
-
-	if (output_resource)
-		output = wl_resource_get_user_data(output_resource);
-	else
-		output = NULL;
-
-	shsurf->recommended_output = output;
-}
-
-static void
-xdg_surface_change_state(struct shell_surface *shsurf,
-			 uint32_t state, uint32_t value, uint32_t serial)
-{
-	xdg_surface_send_change_state(shsurf->resource, state, value, serial);
-	shsurf->state_requested = true;
-
-	switch (state) {
-	case XDG_SURFACE_STATE_MAXIMIZED:
-		shsurf->requested_state.maximized = value;
-		if (value)
-			set_maximized(shsurf, NULL);
-		break;
-	case XDG_SURFACE_STATE_FULLSCREEN:
-		shsurf->requested_state.fullscreen = value;
-
-		if (value)
-			set_fullscreen(shsurf,
-				       WL_SHELL_SURFACE_FULLSCREEN_METHOD_DEFAULT,
-				       0, shsurf->recommended_output);
-		break;
-	}
-}
-
-static void
-xdg_surface_request_change_state(struct wl_client *client,
-				 struct wl_resource *resource,
-				 uint32_t state,
-				 uint32_t value,
-				 uint32_t serial)
-{
-	struct shell_surface *shsurf = wl_resource_get_user_data(resource);
-
-	/* The client can't know what the current state is, so we need
-	   to always send a state change in response. */
-
-	if (shsurf->type != SHELL_SURFACE_TOPLEVEL)
-		return;
-
-	switch (state) {
-	case XDG_SURFACE_STATE_MAXIMIZED:
-	case XDG_SURFACE_STATE_FULLSCREEN:
-		break;
-	default:
-		/* send error? ignore? send change state with value 0? */
-		return;
-	}
-
-	xdg_surface_change_state(shsurf, state, value, serial);
-}
-
-static void
-xdg_surface_ack_change_state(struct wl_client *client,
-			     struct wl_resource *resource,
-			     uint32_t state,
-			     uint32_t value,
-			     uint32_t serial)
 {
 	struct shell_surface *shsurf = wl_resource_get_user_data(resource);
 
@@ -3500,6 +3431,62 @@ xdg_surface_ack_change_state(struct wl_client *client,
 	}
 }
 
+static void
+xdg_surface_set_maximized(struct wl_client *client,
+			  struct wl_resource *resource)
+{
+	struct shell_surface *shsurf = wl_resource_get_user_data(resource);
+
+	shsurf->state_requested = true;
+	shsurf->requested_state.maximized = true;
+	set_maximized(shsurf, NULL);
+}
+
+static void
+xdg_surface_unset_maximized(struct wl_client *client,
+			    struct wl_resource *resource)
+{
+	struct shell_surface *shsurf = wl_resource_get_user_data(resource);
+
+	shsurf->state_requested = true;
+	shsurf->requested_state.maximized = false;
+	shsurf->client->send_configure(shsurf->surface, 0, 0);
+}
+
+static void
+xdg_surface_set_fullscreen(struct wl_client *client,
+			   struct wl_resource *resource,
+			   struct wl_resource *output_resource)
+{
+	struct shell_surface *shsurf = wl_resource_get_user_data(resource);
+	struct weston_output *output;
+
+	shsurf->state_requested = true;
+	shsurf->requested_state.fullscreen = true;
+
+	if (output_resource)
+		output = wl_resource_get_user_data(output_resource);
+	else
+		output = NULL;
+
+	shsurf->recommended_output = output;
+
+	set_fullscreen(shsurf,
+		       WL_SHELL_SURFACE_FULLSCREEN_METHOD_DEFAULT,
+		       0, shsurf->recommended_output);
+}
+
+static void
+xdg_surface_unset_fullscreen(struct wl_client *client,
+			     struct wl_resource *resource)
+{
+	struct shell_surface *shsurf = wl_resource_get_user_data(resource);
+
+	shsurf->state_requested = true;
+	shsurf->requested_state.fullscreen = false;
+	shsurf->client->send_configure(shsurf->surface, 0, 0);
+}
+
 static void
 xdg_surface_set_minimized(struct wl_client *client,
 			    struct wl_resource *resource)
@@ -3521,10 +3508,12 @@ static const struct xdg_surface_interface xdg_surface_implementation = {
 	xdg_surface_set_app_id,
 	xdg_surface_move,
 	xdg_surface_resize,
-	xdg_surface_set_output,
+	xdg_surface_ack_configure,
-	xdg_surface_request_change_state,
+	xdg_surface_set_maximized,
-	xdg_surface_ack_change_state,
+	xdg_surface_unset_maximized,
-	xdg_surface_set_minimized
+	xdg_surface_set_fullscreen,
+	xdg_surface_unset_fullscreen,
+	xdg_surface_set_minimized,
 };
 
 static void
@@ -3532,11 +3521,28 @@ xdg_send_configure(struct weston_surface *surface,
 		   int32_t width, int32_t height)
 {
 	struct shell_surface *shsurf = get_shell_surface(surface);
+	uint32_t *s;
+	struct wl_array states;
+	uint32_t serial;
 
 	assert(shsurf);
 
-	if (shsurf->resource)
+	if (!shsurf->resource)
-		xdg_surface_send_configure(shsurf->resource, width, height);
+		return;
+
+	wl_array_init(&states);
+	if (shsurf->requested_state.fullscreen) {
+		s = wl_array_add(&states, sizeof *s);
+		*s = XDG_SURFACE_STATE_FULLSCREEN;
+	} else if (shsurf->requested_state.maximized) {
+		s = wl_array_add(&states, sizeof *s);
+		*s = XDG_SURFACE_STATE_MAXIMIZED;
+	}
+
+	serial = wl_display_next_serial(shsurf->surface->compositor->wl_display);
+	xdg_surface_send_configure(shsurf->resource, width, height, &states, serial);
+
+	wl_array_release(&states);
 }
 
 static const struct weston_shell_client xdg_client = {
@@ -4107,7 +4113,6 @@ maximize_binding(struct weston_seat *seat, uint32_t time, uint32_t button, void
 	struct weston_surface *focus = seat->keyboard->focus;
 	struct weston_surface *surface;
 	struct shell_surface *shsurf;
-	uint32_t serial;
 
 	surface = weston_surface_get_main_surface(focus);
 	if (surface == NULL)
@@ -4120,9 +4125,10 @@ maximize_binding(struct weston_seat *seat, uint32_t time, uint32_t button, void
 	if (!shell_surface_is_xdg_surface(shsurf))
 		return;
 
-	serial = wl_display_next_serial(seat->compositor->wl_display);
+	shsurf->state_requested = true;
-	xdg_surface_change_state(shsurf, XDG_SURFACE_STATE_MAXIMIZED,
+	shsurf->requested_state.maximized = !shsurf->state.maximized;
-				 !shsurf->state.maximized, serial);
+	if (shsurf->requested_state.maximized)
+		set_maximized(shsurf, NULL);
 }
 
 static void
@@ -4131,7 +4137,6 @@ fullscreen_binding(struct weston_seat *seat, uint32_t time, uint32_t button, voi
 	struct weston_surface *focus = seat->keyboard->focus;
 	struct weston_surface *surface;
 	struct shell_surface *shsurf;
-	uint32_t serial;
 
 	surface = weston_surface_get_main_surface(focus);
 	if (surface == NULL)
@@ -4144,9 +4149,12 @@ fullscreen_binding(struct weston_seat *seat, uint32_t time, uint32_t button, voi
 	if (!shell_surface_is_xdg_surface(shsurf))
 		return;
 
-	serial = wl_display_next_serial(seat->compositor->wl_display);
+	shsurf->state_requested = true;
-	xdg_surface_change_state(shsurf, XDG_SURFACE_STATE_FULLSCREEN,
+	shsurf->requested_state.fullscreen = !shsurf->state.fullscreen;
-				 !shsurf->state.fullscreen, serial);
+	if (shsurf->requested_state.fullscreen)
+		set_fullscreen(shsurf,
+			       WL_SHELL_SURFACE_FULLSCREEN_METHOD_DEFAULT,
+			       0, shsurf->recommended_output);
 }
 
 static void

protocol/xdg-shell.xml

@@ -241,40 +241,6 @@
       <arg name="edges" type="uint" summary="which edge or corner is being dragged"/>
     </request>
 
-    <event name="configure">
-      <description summary="suggest resize">
-	The configure event asks the client to resize its surface.
-
-	The size is a hint, in the sense that the client is free to
-	ignore it if it doesn't resize, pick a smaller size (to
-	satisfy aspect ratio or resize in steps of NxM pixels).
-
-	The client is free to dismiss all but the last configure
-	event it received.
-
-	The width and height arguments specify the size of the window
-	in surface local coordinates.
-      </description>
-
-      <arg name="width" type="int"/>
-      <arg name="height" type="int"/>
-    </event>
-
-    <request name="set_output">
-      <description summary="set the default output used by this surface">
-	Set the default output used by this surface when it is first mapped.
-
-	If this value is NULL (default), it's up to the compositor to choose
-	which display will be used to map this surface.
-
-	When fullscreen or maximized state are set on this surface, and it
-	wasn't mapped yet, the output set with this method will be used.
-	Otherwise, the output where the surface is currently mapped will be
-	used.
-      </description>
-      <arg name="output" type="object" interface="wl_output" allow-null="true"/>
-    </request>
-
     <enum name="state">
       <description summary="types of state on the surface">
         The different state values used on the surface. This is designed for
@@ -297,67 +263,67 @@
         0x1000 - 0x1FFF: GNOME
       </description>
       <entry name="maximized" value="1" summary="the surface is maximized">
-        A non-zero value indicates the surface is maximized. Otherwise,
+        The surface is maximized. The window geometry specified in the configure
-        the surface is unmaximized.
+        event must be obeyed by the client.
       </entry>
       <entry name="fullscreen" value="2" summary="the surface is fullscreen">
-        A non-zero value indicates the surface is fullscreen. Otherwise,
+        The surface is fullscreen. The window geometry specified in the configure
-        the surface is not fullscreen.
+        event must be obeyed by the client.
       </entry>
     </enum>
 
-    <request name="request_change_state">
+    <event name="configure">
-      <description summary="client requests to change a surface's state">
+      <description summary="suggest a surface chnage">
-        This asks the compositor to change the state. If the compositor wants
+	The configure event asks the client to resize its surface.
-        to change the state, it will send a change_state event with the same
-        state_type, value, and serial, and the event flow continues as if it
-        it was initiated by the compositor.
 
-        If the compositor does not want to change the state, it will send a
+	The width and height arguments specify a hint to the window
-        change_state to the client with the old value of the state.
+        about how its surface should be resized in surface local
-      </description>
+        coordinates. The states listed in the event specify how the
-      <arg name="state_type" type="uint" summary="the state to set"/>
+        width/height arguments should be interpreted.
-      <arg name="value" type="uint" summary="the value to change the state to"/>
-      <arg name="serial" type="uint" summary="an event serial">
-        This serial is so the client can know which change_state event corresponds
-        to which request_change_state request it sent out.
-      </arg>
-    </request>
 
-    <event name="change_state">
+        A client should arrange a new surface, and then send a
-      <description summary="compositor wants to change a surface's state">
+        ack_configure request with the serial sent in this configure
-        This event tells the client to change a surface's state. The client
+        event before attaching a new surface.
-        should respond with an ack_change_state request to the compositor to
+
-        guarantee that the compositor knows that the client has seen it.
+	If the client receives multiple configure events before it
+        can respond to one, it is free to discard all but the last
+        event it received.
       </description>
 
-      <arg name="state_type" type="uint" summary="the state to set"/>
+      <arg name="width" type="int"/>
-      <arg name="value" type="uint" summary="the value to change the state to"/>
+      <arg name="height" type="int"/>
-      <arg name="serial" type="uint" summary="a serial for the compositor's own tracking"/>
+      <arg name="states" type="array"/>
+      <arg name="serial" type="uint"/>
     </event>
 
-    <request name="ack_change_state">
+    <request name="ack_configure">
-      <description summary="ack a change_state event">
+      <description summary="ack a configure event">
-        When a change_state event is received, a client should then ack it
+        When a configure event is received, a client should then ack it
-        using the ack_change_state request to ensure that the compositor
+        using the ack_configure request to ensure that the compositor
         knows the client has seen the event.
 
         By this point, the state is confirmed, and the next attach should
-        contain the buffer drawn for the new state value.
+        contain the buffer drawn for the configure event you are acking.
-
-        The values here need to be the same as the values in the cooresponding
-        change_state event.
       </description>
-      <arg name="state_type" type="uint" summary="the state to set"/>
+      <arg name="serial" type="uint" summary="a serial to configure for"/>
-      <arg name="value" type="uint" summary="the value to change the state to"/>
-      <arg name="serial" type="uint" summary="a serial to pass to change_state"/>
     </request>
 
-    <request name="set_minimized">
+    <request name="set_maximized" />
-      <description summary="minimize the surface">
+    <request name="unset_maximized" />
-        Minimize the surface.
+
+    <request name="set_fullscreen">
+      <description summary="set the window as fullscreen on a monitor">
+	Make the surface fullscreen.
+
+        You can specify an output that you would prefer to be fullscreen.
+	If this value is NULL, it's up to the compositor to choose which
+        display will be used to map this surface.
       </description>
+      <arg name="output" type="object" interface="wl_output" allow-null="true"/>
     </request>
+    <request name="unset_fullscreen" />
+
+    <request name="set_minimized" />
 
     <event name="activated">
       <description summary="surface was activated">