xdg-shell: rework roles

include/types/wlr_xdg_shell.h

@@ -5,16 +5,13 @@
 #include <wlr/types/wlr_xdg_shell.h>
 #include "xdg-shell-protocol.h"
 
-extern const struct wlr_surface_role xdg_toplevel_surface_role;
-extern const struct wlr_surface_role xdg_popup_surface_role;
-
 void create_xdg_surface(struct wlr_xdg_client *client, struct wlr_surface *wlr_surface,
 	uint32_t id);
 void destroy_xdg_surface(struct wlr_xdg_surface *surface);
-void destroy_xdg_surface_role_object(struct wlr_xdg_surface *surface);
+
-void xdg_surface_role_commit(struct wlr_surface *wlr_surface);
+bool set_xdg_surface_role(struct wlr_xdg_surface *surface, enum wlr_xdg_surface_role role);
-void xdg_surface_role_unmap(struct wlr_surface *wlr_surface);
+void set_xdg_surface_role_object(struct wlr_xdg_surface *surface,
-void xdg_surface_role_destroy(struct wlr_surface *wlr_surface);
+	struct wl_resource *role_resource);
 
 void create_xdg_positioner(struct wlr_xdg_client *client, uint32_t id);
 

include/wlr/types/wlr_xdg_shell.h

@@ -231,8 +231,18 @@ struct wlr_xdg_surface {
 	struct wl_resource *resource;
 	struct wlr_surface *surface;
 	struct wl_list link; // wlr_xdg_client.surfaces
-	enum wlr_xdg_surface_role role;
 
+	/**
+	 * The lifetime-bound role of the xdg_surface. WLR_XDG_SURFACE_ROLE_NONE
+	 * if the role was never set.
+	 */
+	enum wlr_xdg_surface_role role;
+	/**
+	 * The role object representing the role. NULL if the object was destroyed.
+	 */
+	struct wl_resource *role_resource;
+
+	// NULL if the role resource is inert
 	union {
 		struct wlr_xdg_toplevel *toplevel;
 		struct wlr_xdg_popup *popup;
@@ -247,8 +257,6 @@ struct wlr_xdg_surface {
 
 	struct wlr_xdg_surface_state current, pending;
 
-	struct wl_listener surface_commit;
-
 	struct {
 		struct wl_signal destroy;
 		struct wl_signal ping_timeout;
@@ -260,6 +268,11 @@ struct wlr_xdg_surface {
 	} events;
 
 	void *data;
+
+	// private state
+
+	bool client_mapped;
+	struct wl_listener role_resource_destroy;
 };
 
 struct wlr_xdg_toplevel_move_event {

types/xdg_shell/wlr_xdg_popup.c

@@ -361,13 +361,6 @@ static void xdg_popup_handle_resource_destroy(struct wl_resource *resource) {
 	wlr_xdg_popup_destroy(popup);
 }
 
-const struct wlr_surface_role xdg_popup_surface_role = {
-	.name = "xdg_popup",
-	.commit = xdg_surface_role_commit,
-	.unmap = xdg_surface_role_unmap,
-	.destroy = xdg_surface_role_destroy,
-};
-
 void create_xdg_popup(struct wlr_xdg_surface *surface,
 		struct wlr_xdg_surface *parent,
 		struct wlr_xdg_positioner *positioner, uint32_t id) {
@@ -379,15 +372,7 @@ void create_xdg_popup(struct wlr_xdg_surface *surface,
 		return;
 	}
 
-	if (surface->role != WLR_XDG_SURFACE_ROLE_NONE) {
+	if (!set_xdg_surface_role(surface, WLR_XDG_SURFACE_ROLE_POPUP)) {
-		wl_resource_post_error(surface->resource,
-			XDG_SURFACE_ERROR_ALREADY_CONSTRUCTED,
-			"xdg-surface has already been constructed");
-		return;
-	}
-
-	if (!wlr_surface_set_role(surface->surface, &xdg_popup_surface_role,
-			surface->resource, XDG_WM_BASE_ERROR_ROLE)) {
 		return;
 	}
 
@@ -412,8 +397,6 @@ void create_xdg_popup(struct wlr_xdg_surface *surface,
 		&xdg_popup_implementation, surface->popup,
 		xdg_popup_handle_resource_destroy);
 
-	wlr_surface_set_role_object(surface->surface, surface->resource);
-
 	surface->role = WLR_XDG_SURFACE_ROLE_POPUP;
 
 	wlr_xdg_positioner_rules_get_geometry(
@@ -429,6 +412,8 @@ void create_xdg_popup(struct wlr_xdg_surface *surface,
 	} else {
 		wl_list_init(&surface->popup->link);
 	}
+
+	set_xdg_surface_role_object(surface, surface->popup->resource);
 }
 
 void reset_xdg_popup(struct wlr_xdg_popup *popup) {
@@ -460,6 +445,17 @@ void reset_xdg_popup(struct wlr_xdg_popup *popup) {
 }
 
 void destroy_xdg_popup(struct wlr_xdg_popup *popup) {
+	wlr_surface_unmap(popup->base->surface);
+	reset_xdg_popup(popup);
+
+	// TODO: improve events
+	if (popup->base->added) {
+		wl_signal_emit_mutable(&popup->base->events.destroy, NULL);
+		popup->base->added = false;
+	}
+
+	popup->base->popup = NULL;
+
 	wl_list_remove(&popup->link);
 	wl_resource_set_user_data(popup->resource, NULL);
 	free(popup);
@@ -476,8 +472,7 @@ void wlr_xdg_popup_destroy(struct wlr_xdg_popup *popup) {
 	}
 
 	xdg_popup_send_popup_done(popup->resource);
-	wl_resource_set_user_data(popup->resource, NULL);
+	destroy_xdg_popup(popup);
-	destroy_xdg_surface_role_object(popup->base);
 }
 
 void wlr_xdg_popup_get_toplevel_coords(struct wlr_xdg_popup *popup,

types/xdg_shell/wlr_xdg_shell.c

@@ -67,7 +67,7 @@ static void xdg_client_handle_resource_destroy(struct wl_resource *resource) {
 
 	struct wlr_xdg_surface *surface, *tmp = NULL;
 	wl_list_for_each_safe(surface, tmp, &client->surfaces, link) {
-		wlr_surface_destroy_role_object(surface->surface);
+		destroy_xdg_surface(surface);
 	}
 
 	if (client->ping_timer != NULL) {

types/xdg_shell/wlr_xdg_surface.c

@@ -5,18 +5,6 @@
 #include <wlr/util/log.h>
 #include "types/wlr_xdg_shell.h"
 
-struct wlr_xdg_surface *wlr_xdg_surface_try_from_wlr_surface(
-		struct wlr_surface *surface) {
-	if (surface->role != &xdg_toplevel_surface_role &&
-			surface->role != &xdg_popup_surface_role) {
-		return NULL;
-	}
-	if (surface->role_resource == NULL) {
-		return NULL;
-	}
-	return wlr_xdg_surface_from_resource(surface->role_resource);
-}
-
 static void xdg_surface_configure_destroy(
 		struct wlr_xdg_surface_configure *configure) {
 	if (configure == NULL) {
@@ -27,7 +15,16 @@ static void xdg_surface_configure_destroy(
 	free(configure);
 }
 
+// An xdg_surface implementation is reset, when:
+// 1) a surface is unmapped due to a commit with NULL buffer, or
+// 2) the xdg_surface role object is destroyed, or
+// 3) wlr_xdg_surface is destroyed
+// An xdg_surface role object implementation is reset, when:
+// 1) a surface is unmapped due to a commit with NULL buffer, or
+// 2) the xdg_surface role object implementation is destroyed
+
 static void reset_xdg_surface(struct wlr_xdg_surface *surface) {
+	surface->client_mapped = false;
 	surface->configured = false;
 
 	struct wlr_xdg_popup *popup, *popup_tmp;
@@ -35,6 +32,18 @@ static void reset_xdg_surface(struct wlr_xdg_surface *surface) {
 		wlr_xdg_popup_destroy(popup);
 	}
 
+	struct wlr_xdg_surface_configure *configure, *tmp;
+	wl_list_for_each_safe(configure, tmp, &surface->configure_list, link) {
+		xdg_surface_configure_destroy(configure);
+	}
+
+	if (surface->configure_idle) {
+		wl_event_source_remove(surface->configure_idle);
+		surface->configure_idle = NULL;
+	}
+}
+
+static void reset_xdg_surface_role_object(struct wlr_xdg_surface *surface) {
 	switch (surface->role) {
 	case WLR_XDG_SURFACE_ROLE_TOPLEVEL:
 	 	if (surface->toplevel != NULL) {
@@ -49,16 +58,6 @@ static void reset_xdg_surface(struct wlr_xdg_surface *surface) {
 	case WLR_XDG_SURFACE_ROLE_NONE:
 		break;
 	}
-
-	struct wlr_xdg_surface_configure *configure, *tmp;
-	wl_list_for_each_safe(configure, tmp, &surface->configure_list, link) {
-		xdg_surface_configure_destroy(configure);
-	}
-
-	if (surface->configure_idle) {
-		wl_event_source_remove(surface->configure_idle);
-		surface->configure_idle = NULL;
-	}
 }
 
 static void xdg_surface_handle_ack_configure(struct wl_client *client,
@@ -241,7 +240,7 @@ static void xdg_surface_handle_destroy(struct wl_client *client,
 		return;
 	}
 
-	if (surface->role != WLR_XDG_SURFACE_ROLE_NONE) {
+	if (surface->role_resource != NULL) {
 		wl_resource_post_error(resource,
 			XDG_SURFACE_ERROR_DEFUNCT_ROLE_OBJECT,
 			"surface was destroyed before its role object");
@@ -259,54 +258,48 @@ static const struct xdg_surface_interface xdg_surface_implementation = {
 	.set_window_geometry = xdg_surface_handle_set_window_geometry,
 };
 
-static void xdg_surface_handle_resource_destroy(struct wl_resource *resource) {
+static void xdg_surface_role_commit(struct wlr_surface *wlr_surface) {
-	struct wlr_xdg_surface *surface =
+	struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface);
-		wlr_xdg_surface_from_resource(resource);
+	assert(surface != NULL);
-	if (surface != NULL) {
-		wlr_surface_destroy_role_object(surface->surface);
-	}
-}
 
-static void xdg_surface_handle_surface_commit(struct wl_listener *listener,
+	if (wlr_surface_has_buffer(wlr_surface) && !surface->configured) {
-		void *data) {
-	struct wlr_xdg_surface *surface =
-		wl_container_of(listener, surface, surface_commit);
-
-	if (wlr_surface_has_buffer(surface->surface) && !surface->configured) {
 		wl_resource_post_error(surface->resource,
 			XDG_SURFACE_ERROR_UNCONFIGURED_BUFFER,
 			"xdg_surface has never been configured");
 		return;
 	}
 
-	// surface->role might be NONE for inert popups
+	if (surface->role_resource == NULL) {
-	// So we check surface->surface->role
-	if (surface->surface->role == NULL) {
 		wl_resource_post_error(surface->resource,
 			XDG_SURFACE_ERROR_NOT_CONSTRUCTED,
-			"xdg_surface must have a role");
+			"xdg_surface must have a role object");
 		return;
 	}
-}
 
-void xdg_surface_role_commit(struct wlr_surface *wlr_surface) {
+	if (surface->client_mapped && !wlr_surface_has_buffer(wlr_surface)) {
-	struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface);
+		// This commit has unmapped the surface
-	assert(surface != NULL);
+		reset_xdg_surface_role_object(surface);
+		reset_xdg_surface(surface);
+	}
 
 	surface->current = surface->pending;
 
 	switch (surface->role) {
 	case WLR_XDG_SURFACE_ROLE_NONE:
-		// inert toplevel or popup
+		assert(0 && "not reached");
 		return;
 	case WLR_XDG_SURFACE_ROLE_TOPLEVEL:
 		if (surface->toplevel != NULL) {
 			handle_xdg_toplevel_committed(surface->toplevel);
+		} else {
+			return;
 		}
 		break;
 	case WLR_XDG_SURFACE_ROLE_POPUP:
 		if (surface->popup != NULL) {
 			handle_xdg_popup_committed(surface->popup);
+		} else {
+			return;
 		}
 		break;
 	}
@@ -317,33 +310,43 @@ void xdg_surface_role_commit(struct wlr_surface *wlr_surface) {
 			surface);
 	}
 
-	if (surface->configured && wlr_surface_has_buffer(wlr_surface)) {
+	if (wlr_surface_has_buffer(wlr_surface)) {
+		surface->client_mapped = true;
 		wlr_surface_map(wlr_surface);
 	}
 }
 
-void xdg_surface_role_unmap(struct wlr_surface *wlr_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);
-	assert(surface != NULL);
+	if (surface == NULL) {
-
+		// This is the only time xdg_surface can be inert
-	reset_xdg_surface(surface);
+		return;
 	}
 
-void xdg_surface_role_destroy(struct wlr_surface *wlr_surface) {
+	destroy_xdg_surface(surface);
-	struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface);
+}
-	assert(surface != NULL);
 
-	destroy_xdg_surface_role_object(surface);
+static struct wlr_surface_role xdg_surface_role = {
+	.name = "xdg_surface",
+	.commit = xdg_surface_role_commit,
+	.destroy = xdg_surface_role_destroy,
+};
 
-	wl_list_remove(&surface->link);
+struct wlr_xdg_surface *wlr_xdg_surface_try_from_wlr_surface(
-	wl_list_remove(&surface->surface_commit.link);
+		struct wlr_surface *surface) {
-
+	if (surface->role != &xdg_surface_role || surface->role_resource == NULL) {
-	wl_resource_set_user_data(surface->resource, NULL);
+		return NULL;
-	free(surface);
+	}
+	return wlr_xdg_surface_from_resource(surface->role_resource);
 }
 
 void create_xdg_surface(struct wlr_xdg_client *client, struct wlr_surface *wlr_surface,
 		uint32_t id) {
+	if (!wlr_surface_set_role(wlr_surface, &xdg_surface_role, client->resource,
+			XDG_WM_BASE_ERROR_ROLE)) {
+		return;
+	}
+
 	struct wlr_xdg_surface *surface =
 		calloc(1, sizeof(struct wlr_xdg_surface));
 	if (surface == NULL) {
@@ -381,49 +384,91 @@ void create_xdg_surface(struct wlr_xdg_client *client, struct wlr_surface *wlr_s
 	wl_signal_init(&surface->events.configure);
 	wl_signal_init(&surface->events.ack_configure);
 
-	wl_signal_add(&surface->surface->events.commit,
-		&surface->surface_commit);
-	surface->surface_commit.notify = xdg_surface_handle_surface_commit;
-
 	wlr_log(WLR_DEBUG, "new xdg_surface %p (res %p)", surface,
 		surface->resource);
 	wl_resource_set_implementation(surface->resource,
-		&xdg_surface_implementation, surface,
+		&xdg_surface_implementation, surface, NULL);
-		xdg_surface_handle_resource_destroy);
 	wl_list_insert(&client->surfaces, &surface->link);
+
+	wlr_surface_set_role_object(wlr_surface, surface->resource);
 }
 
-void destroy_xdg_surface_role_object(struct wlr_xdg_surface *surface) {
+bool set_xdg_surface_role(struct wlr_xdg_surface *surface, enum wlr_xdg_surface_role role) {
-	if (surface->surface->mapped) {
+	assert(role != WLR_XDG_SURFACE_ROLE_NONE);
-		wlr_surface_unmap(surface->surface);
+
-	} else {
+	static const char *role_names[] = {
-		reset_xdg_surface(surface);
+		[WLR_XDG_SURFACE_ROLE_TOPLEVEL] = "xdg_toplevel",
+		[WLR_XDG_SURFACE_ROLE_POPUP] = "xdg_popup",
+	};
+
+	if (surface->role != WLR_XDG_SURFACE_ROLE_NONE && surface->role != role) {
+		wl_resource_post_error(surface->client->resource, XDG_WM_BASE_ERROR_ROLE,
+			"Cannot assign role %s to xdg_surface@%" PRIu32 ", already has role %s",
+			role_names[role], wl_resource_get_id(surface->resource),
+			role_names[surface->role]);
+		return false;
+	}
+	if (surface->role_resource != NULL) {
+		wl_resource_post_error(surface->client->resource, XDG_WM_BASE_ERROR_ROLE,
+			"Cannot reassign role %s to xdg_surface@%" PRIu32 ", role object still exists",
+			role_names[role], wl_resource_get_id(surface->resource));
+		return false;
 	}
 
-	if (surface->added) {
+	surface->role = role;
-		wl_signal_emit_mutable(&surface->events.destroy, NULL);
+	return true;
-		surface->added = false;
+}
+
+static void destroy_xdg_surface_role_object(struct wlr_xdg_surface *surface) {
+	if (surface->role_resource == NULL) {
+		return;
 	}
 
 	switch (surface->role) {
+	case WLR_XDG_SURFACE_ROLE_NONE:
+		assert(0 && "not reached");
+		break;
 	case WLR_XDG_SURFACE_ROLE_TOPLEVEL:
 		if (surface->toplevel != NULL) {
 			destroy_xdg_toplevel(surface->toplevel);
-			surface->toplevel = NULL;
 		}
 		break;
 	case WLR_XDG_SURFACE_ROLE_POPUP:
 		if (surface->popup != NULL) {
 			destroy_xdg_popup(surface->popup);
-			surface->popup = NULL;
 		}
 		break;
-	case WLR_XDG_SURFACE_ROLE_NONE:
-		// This space is intentionally left blank
-		break;
 	}
 
-	surface->role = WLR_XDG_SURFACE_ROLE_NONE;
+	surface->role_resource = NULL;
+	wl_list_remove(&surface->role_resource_destroy.link);
+	wl_list_init(&surface->role_resource_destroy.link);
+}
+
+static void xdg_surface_handle_role_resource_destroy(struct wl_listener *listener, void *data) {
+	struct wlr_xdg_surface *surface = wl_container_of(listener, surface, role_resource_destroy);
+	destroy_xdg_surface_role_object(surface);
+	reset_xdg_surface(surface);
+}
+
+void set_xdg_surface_role_object(struct wlr_xdg_surface *surface,
+		struct wl_resource *role_resource) {
+	assert(surface->role != WLR_XDG_SURFACE_ROLE_NONE);
+	assert(surface->role_resource == NULL);
+	assert(role_resource != NULL);
+	surface->role_resource = role_resource;
+	surface->role_resource_destroy.notify = xdg_surface_handle_role_resource_destroy;
+	wl_resource_add_destroy_listener(role_resource, &surface->role_resource_destroy);
+}
+
+void destroy_xdg_surface(struct wlr_xdg_surface *surface) {
+	destroy_xdg_surface_role_object(surface);
+	reset_xdg_surface(surface);
+
+	wl_list_remove(&surface->link);
+
+	wl_resource_set_user_data(surface->resource, NULL);
+	free(surface);
 }
 
 struct wlr_xdg_surface *wlr_xdg_surface_from_resource(

types/xdg_shell/wlr_xdg_toplevel.c

@@ -469,33 +469,9 @@ static const struct xdg_toplevel_interface xdg_toplevel_implementation = {
 	.set_minimized = xdg_toplevel_handle_set_minimized,
 };
 
-static void xdg_toplevel_handle_resource_destroy(struct wl_resource *resource) {
-	struct wlr_xdg_toplevel *toplevel =
-		wlr_xdg_toplevel_from_resource(resource);
-	if (toplevel == NULL) {
-		return;
-	}
-	destroy_xdg_surface_role_object(toplevel->base);
-}
-
-const struct wlr_surface_role xdg_toplevel_surface_role = {
-	.name = "xdg_toplevel",
-	.commit = xdg_surface_role_commit,
-	.unmap = xdg_surface_role_unmap,
-	.destroy = xdg_surface_role_destroy,
-};
-
 void create_xdg_toplevel(struct wlr_xdg_surface *surface,
 		uint32_t id) {
-	if (!wlr_surface_set_role(surface->surface, &xdg_toplevel_surface_role,
+	if (!set_xdg_surface_role(surface, WLR_XDG_SURFACE_ROLE_TOPLEVEL)) {
-			surface->resource, XDG_WM_BASE_ERROR_ROLE)) {
-		return;
-	}
-
-	if (surface->role != WLR_XDG_SURFACE_ROLE_NONE) {
-		wl_resource_post_error(surface->resource,
-			XDG_SURFACE_ERROR_ALREADY_CONSTRUCTED,
-			"xdg-surface has already been constructed");
 		return;
 	}
 
@@ -527,12 +503,9 @@ void create_xdg_toplevel(struct wlr_xdg_surface *surface,
 		return;
 	}
 	wl_resource_set_implementation(surface->toplevel->resource,
-		&xdg_toplevel_implementation, surface->toplevel,
+		&xdg_toplevel_implementation, surface->toplevel, NULL);
-		xdg_toplevel_handle_resource_destroy);
 
-	wlr_surface_set_role_object(surface->surface, surface->resource);
+	set_xdg_surface_role_object(surface, surface->toplevel->resource);
-
-	surface->role = WLR_XDG_SURFACE_ROLE_TOPLEVEL;
 }
 
 void reset_xdg_toplevel(struct wlr_xdg_toplevel *toplevel) {
@@ -557,6 +530,17 @@ void reset_xdg_toplevel(struct wlr_xdg_toplevel *toplevel) {
 }
 
 void destroy_xdg_toplevel(struct wlr_xdg_toplevel *toplevel) {
+	wlr_surface_unmap(toplevel->base->surface);
+
+	reset_xdg_toplevel(toplevel);
+
+	// TODO: improve events
+	if (toplevel->base->added) {
+		wl_signal_emit_mutable(&toplevel->base->events.destroy, NULL);
+		toplevel->base->added = false;
+	}
+
+	toplevel->base->toplevel = NULL;
 	wl_resource_set_user_data(toplevel->resource, NULL);
 	free(toplevel);
 }