sway/tree: Simplify sway_node teardown

A sway_node may end up being referenced in either a queued transaction,
pending transaction or as a dirty node. To manage this, the transaction
system has been responsible for destroying containers, workspaces and
outputs at the end of their last referenced transaction.

This significantly complicates the teardown flow of surfaces and
outputs. Instead, remove the node from transactions and dirty lists so
that the callsite can remove and free the node immediately.

include/sway/desktop/transaction.h

@@ -23,6 +23,7 @@
 
 struct sway_transaction_instruction;
 struct sway_view;
+struct sway_node;
 
 /**
  * Find all dirty containers, create and commit a transaction containing them,
@@ -30,6 +31,11 @@ struct sway_view;
  */
 void transaction_commit_dirty(void);
 
+/**
+ * Remove a node that will be destroyed from transactions and dirty node lists.
+ */
+void transaction_remove_node(struct sway_node *node);
+
 /*
  * Same as transaction_commit_dirty, but signalling that this is a
  * client-initiated change has already taken effect.

include/sway/output.h

@@ -84,8 +84,6 @@ struct sway_output *output_create(struct wlr_output *wlr_output);
 
 void output_destroy(struct sway_output *output);
 
-void output_begin_destroy(struct sway_output *output);
-
 struct sway_output *output_from_wlr_output(struct wlr_output *output);
 
 struct sway_output *output_get_in_direction(struct sway_output *reference,

include/sway/tree/container.h

@@ -155,8 +155,6 @@ struct sway_container *container_create(struct sway_view *view);
 
 void container_destroy(struct sway_container *con);
 
-void container_begin_destroy(struct sway_container *con);
-
 /**
  * Search a container's descendants a container based on test criteria. Returns
  * the first container that passes the test.

include/sway/tree/node.h

@@ -39,7 +39,6 @@ struct sway_node {
 
 	struct sway_transaction_instruction *instruction;
 	size_t ntxnrefs;
-	bool destroying;
 
 	// If true, indicates that the container has pending state that differs from
 	// the current.

include/sway/tree/view.h

@@ -101,8 +101,6 @@ struct sway_view {
 	struct wl_listener foreign_close_request;
 	struct wl_listener foreign_destroy;
 
-	bool destroying;
-
 	list_t *executed_criteria; // struct criteria *
 
 	union {
@@ -296,8 +294,6 @@ bool view_init(struct sway_view *view, enum sway_view_type type,
 
 void view_destroy(struct sway_view *view);
 
-void view_begin_destroy(struct sway_view *view);
-
 /**
  * Map a view, ie. make it visible in the tree.
  *

include/sway/tree/workspace.h

@@ -62,9 +62,7 @@ struct sway_workspace *workspace_create(struct sway_output *output,
 
 void workspace_destroy(struct sway_workspace *workspace);
 
-void workspace_begin_destroy(struct sway_workspace *workspace);
+bool workspace_consider_destroy(struct sway_workspace *ws);
-
-void workspace_consider_destroy(struct sway_workspace *ws);
 
 char *workspace_next_name(const char *output_name);
 

sway/commands/move.c

@@ -611,14 +611,16 @@ static struct cmd_results *cmd_move_container(bool no_auto_back_and_forth,
 	if (old_parent) {
 		container_reap_empty(old_parent);
 	} else if (old_ws) {
-		workspace_consider_destroy(old_ws);
+		if (workspace_consider_destroy(old_ws)) {
+			old_ws = NULL;
+		}
 	}
 
 	// arrange windows
 	if (root->fullscreen_global) {
 		arrange_root();
 	} else {
-		if (old_ws && !old_ws->node.destroying) {
+		if (old_ws) {
 			arrange_workspace(old_ws);
 		}
 		arrange_node(node_get_parent(destination));
@@ -753,7 +755,9 @@ static struct cmd_results *cmd_move_in_direction(
 	if (old_parent) {
 		container_reap_empty(old_parent);
 	} else if (old_ws) {
-		workspace_consider_destroy(old_ws);
+		if (workspace_consider_destroy(old_ws)) {
+			old_ws = NULL;
+		}
 	}
 
 	struct sway_workspace *new_ws = container->pending.workspace;
@@ -761,7 +765,9 @@ static struct cmd_results *cmd_move_in_direction(
 	if (root->fullscreen_global) {
 		arrange_root();
 	} else {
-		arrange_workspace(old_ws);
+		if (old_ws) {
+			arrange_workspace(old_ws);
+		}
 		if (new_ws != old_ws) {
 			arrange_workspace(new_ws);
 		}

sway/desktop/launcher.c

@@ -153,7 +153,7 @@ static void ctx_handle_node_destroy(struct wl_listener *listener, void *data) {
 		// same output
 		free(ctx->fallback_name);
 		ctx->fallback_name = strdup(ws->name);
-		if (!ws->output || ws->output->node.destroying) {
+		if (!ws->output) {
 			// If the output is being destroyed it would be pointless to track
 			// If the output is being disabled, we'll find out if it's still
 			// disabled when we try to match it.

sway/desktop/output.c

@@ -428,7 +428,7 @@ void force_modeset(void) {
 	apply_stored_output_configs();
 }
 
-static void begin_destroy(struct sway_output *output) {
+static void output_teardown(struct sway_output *output) {
 
 	wl_list_remove(&output->layout_destroy.link);
 	wl_list_remove(&output->destroy.link);
@@ -444,7 +444,6 @@ static void begin_destroy(struct sway_output *output) {
 	if (output->enabled) {
 		output_disable(output);
 	}
-	output_begin_destroy(output);
 	wl_list_remove(&output->link);
 
 	output->wlr_output->data = NULL;
@@ -453,17 +452,19 @@ static void begin_destroy(struct sway_output *output) {
 	wl_event_source_remove(output->repaint_timer);
 	output->repaint_timer = NULL;
 
+	output_destroy(output);
+
 	request_modeset();
 }
 
 static void handle_destroy(struct wl_listener *listener, void *data) {
 	struct sway_output *output = wl_container_of(listener, output, destroy);
-	begin_destroy(output);
+	output_teardown(output);
 }
 
 static void handle_layout_destroy(struct wl_listener *listener, void *data) {
 	struct sway_output *output = wl_container_of(listener, output, layout_destroy);
-	begin_destroy(output);
+	output_teardown(output);
 }
 
 static void handle_present(struct wl_listener *listener, void *data) {

sway/desktop/transaction.c

@@ -59,22 +59,6 @@ static void transaction_destroy(struct sway_transaction *transaction) {
 		if (node->instruction == instruction) {
 			node->instruction = NULL;
 		}
-		if (node->destroying && node->ntxnrefs == 0) {
-			switch (node->type) {
-			case N_ROOT:
-				sway_assert(false, "Never reached");
-				break;
-			case N_OUTPUT:
-				output_destroy(node->sway_output);
-				break;
-			case N_WORKSPACE:
-				workspace_destroy(node->sway_workspace);
-				break;
-			case N_CONTAINER:
-				container_destroy(node->sway_container);
-				break;
-			}
-		}
 		free(instruction);
 	}
 	list_free(transaction->instructions);
@@ -239,7 +223,7 @@ static void apply_container_state(struct sway_container *container,
 
 	if (view) {
 		if (view->saved_surface_tree) {
-			if (!container->node.destroying || container->node.ntxnrefs == 1) {
+			if (container->node.ntxnrefs == 1) {
 				view_remove_saved_buffer(view);
 			}
 		}
@@ -788,9 +772,6 @@ static bool should_configure(struct sway_node *node,
 	if (!node_is_view(node)) {
 		return false;
 	}
-	if (node->destroying) {
-		return false;
-	}
 	if (!instruction->server_request) {
 		return false;
 	}
@@ -825,7 +806,7 @@ static void transaction_commit(struct sway_transaction *transaction) {
 		struct sway_transaction_instruction *instruction =
 			transaction->instructions->items[i];
 		struct sway_node *node = instruction->node;
-		bool hidden = node_is_view(node) && !node->destroying &&
+		bool hidden = node_is_view(node) &&
 			!view_is_visible(node->sway_container->view);
 		if (should_configure(node, instruction)) {
 			instruction->serial = view_configure(node->sway_container->view,
@@ -967,3 +948,39 @@ void transaction_commit_dirty(void) {
 void transaction_commit_dirty_client(void) {
 	_transaction_commit_dirty(false);
 }
+
+static void _transaction_remove_node(struct sway_transaction *transaction,
+		struct sway_node *node) {
+	if (!transaction || !node) {
+		return;
+	}
+	for (int idx = 0; idx < transaction->instructions->length; idx++) {
+		struct sway_transaction_instruction *instruction =
+			transaction->instructions->items[idx];
+		struct sway_node *n = instruction->node;
+		if (n != node) {
+			continue;
+		}
+
+		n->ntxnrefs--;
+		n->instruction = NULL;
+		free(instruction);
+		list_del(transaction->instructions, idx);
+		idx--;
+	}
+}
+
+void transaction_remove_node(struct sway_node *node) {
+	_transaction_remove_node(server.pending_transaction, node);
+	_transaction_remove_node(server.queued_transaction, node);
+
+	for (int idx = 0; idx < server.dirty_nodes->length; idx++) {
+		struct sway_node *n = server.dirty_nodes->items[idx];
+		if (n != node) {
+			continue;
+		}
+		n->dirty = false;
+		list_del(server.dirty_nodes, idx);
+		idx--;
+	}
+}

sway/desktop/xdg_shell.c

@@ -535,7 +535,7 @@ static void handle_destroy(struct wl_listener *listener, void *data) {
 	if (view->xdg_decoration) {
 		view->xdg_decoration->view = NULL;
 	}
-	view_begin_destroy(view);
+	view_destroy(view);
 }
 
 struct sway_view *view_from_wlr_xdg_surface(

sway/desktop/xwayland.c

@@ -482,7 +482,7 @@ static void handle_destroy(struct wl_listener *listener, void *data) {
 	wl_list_remove(&xwayland_view->associate.link);
 	wl_list_remove(&xwayland_view->dissociate.link);
 	wl_list_remove(&xwayland_view->override_redirect.link);
-	view_begin_destroy(&xwayland_view->view);
+	view_destroy(&xwayland_view->view);
 }
 
 static void handle_unmap(struct wl_listener *listener, void *data) {

sway/tree/container.c

@@ -55,7 +55,7 @@ static void handle_destroy(
 	struct sway_container *con = wl_container_of(
 			listener, con, output_handler_destroy);
 
-	container_begin_destroy(con);
+	container_destroy(con);
 }
 
 static bool handle_point_accepts_input(
@@ -501,36 +501,6 @@ void container_update_title_bar(struct sway_container *con) {
 }
 
 void container_destroy(struct sway_container *con) {
-	if (!sway_assert(con->node.destroying,
-				"Tried to free container which wasn't marked as destroying")) {
-		return;
-	}
-	if (!sway_assert(con->node.ntxnrefs == 0, "Tried to free container "
-				"which is still referenced by transactions")) {
-		return;
-	}
-	free(con->title);
-	free(con->formatted_title);
-	free(con->title_format);
-	list_free(con->pending.children);
-	list_free(con->current.children);
-
-	list_free_items_and_destroy(con->marks);
-
-	if (con->view && con->view->container == con) {
-		con->view->container = NULL;
-		wlr_scene_node_destroy(&con->output_handler->node);
-		if (con->view->destroying) {
-			view_destroy(con->view);
-		}
-	}
-
-	scene_node_disown_children(con->content_tree);
-	wlr_scene_node_destroy(&con->scene_tree->node);
-	free(con);
-}
-
-void container_begin_destroy(struct sway_container *con) {
 	if (con->view) {
 		ipc_event_window(con, "close");
 	}
@@ -547,9 +517,6 @@ void container_begin_destroy(struct sway_container *con) {
 
 	container_end_mouse_operation(con);
 
-	con->node.destroying = true;
-	node_set_dirty(&con->node);
-
 	if (con->scratchpad) {
 		root_scratchpad_remove_container(con);
 	}
@@ -567,6 +534,28 @@ void container_begin_destroy(struct sway_container *con) {
 		wl_list_remove(&con->output_leave.link);
 		wl_list_remove(&con->output_handler_destroy.link);
 	}
+
+	transaction_remove_node(&con->node);
+	if (!sway_assert(con->node.ntxnrefs == 0, "Tried to free container "
+				"which is still referenced by transactions")) {
+		return;
+	}
+	free(con->title);
+	free(con->formatted_title);
+	free(con->title_format);
+	list_free(con->pending.children);
+	list_free(con->current.children);
+
+	list_free_items_and_destroy(con->marks);
+
+	if (con->view && con->view->container == con) {
+		con->view->container = NULL;
+		wlr_scene_node_destroy(&con->output_handler->node);
+	}
+
+	scene_node_disown_children(con->content_tree);
+	wlr_scene_node_destroy(&con->scene_tree->node);
+	free(con);
 }
 
 void container_reap_empty(struct sway_container *con) {
@@ -579,7 +568,7 @@ void container_reap_empty(struct sway_container *con) {
 			return;
 		}
 		struct sway_container *parent = con->pending.parent;
-		container_begin_destroy(con);
+		container_destroy(con);
 		con = parent;
 	}
 	if (ws) {
@@ -595,7 +584,7 @@ struct sway_container *container_flatten(struct sway_container *container) {
 		struct sway_container *child = container->pending.children->items[0];
 		struct sway_container *parent = container->pending.parent;
 		container_replace(container, child);
-		container_begin_destroy(container);
+		container_destroy(container);
 		container = parent;
 	}
 	return container;

sway/tree/output.c

@@ -2,6 +2,7 @@
 #include <ctype.h>
 #include <string.h>
 #include <strings.h>
+#include "sway/desktop/transaction.h"
 #include "sway/ipc-server.h"
 #include "sway/layers.h"
 #include "sway/output.h"
@@ -235,7 +236,7 @@ static void output_evacuate(struct sway_output *output) {
 			}
 
 			if (workspace_num_sticky_containers(workspace) == 0) {
-				workspace_begin_destroy(workspace);
+				workspace_destroy(workspace);
 				continue;
 			}
 		}
@@ -253,27 +254,6 @@ static void output_evacuate(struct sway_output *output) {
 	}
 }
 
-void output_destroy(struct sway_output *output) {
-	if (!sway_assert(output->node.destroying,
-				"Tried to free output which wasn't marked as destroying")) {
-		return;
-	}
-	if (!sway_assert(output->wlr_output == NULL,
-				"Tried to free output which still had a wlr_output")) {
-		return;
-	}
-	if (!sway_assert(output->node.ntxnrefs == 0, "Tried to free output "
-				"which is still referenced by transactions")) {
-		return;
-	}
-
-	destroy_scene_layers(output);
-	list_free(output->workspaces);
-	list_free(output->current.workspaces);
-	wlr_color_transform_unref(output->color_transform);
-	free(output);
-}
-
 void output_disable(struct sway_output *output) {
 	if (!sway_assert(output->enabled, "Expected an enabled output")) {
 		return;
@@ -295,15 +275,24 @@ void output_disable(struct sway_output *output) {
 	output_evacuate(output);
 }
 
-void output_begin_destroy(struct sway_output *output) {
+void output_destroy(struct sway_output *output) {
 	if (!sway_assert(!output->enabled, "Expected a disabled output")) {
 		return;
 	}
-	sway_log(SWAY_DEBUG, "Destroying output '%s'", output->wlr_output->name);
 	wl_signal_emit_mutable(&output->node.events.destroy, &output->node);
 
-	output->node.destroying = true;
+	transaction_remove_node(&output->node);
-	node_set_dirty(&output->node);
+
+	if (!sway_assert(output->node.ntxnrefs == 0, "Tried to free output "
+				"which is still referenced by transactions")) {
+		return;
+	}
+
+	destroy_scene_layers(output);
+	list_free(output->workspaces);
+	list_free(output->current.workspaces);
+	wlr_color_transform_unref(output->color_transform);
+	free(output);
 }
 
 struct sway_output *output_from_wlr_output(struct wlr_output *output) {

sway/tree/view.c

@@ -70,11 +70,10 @@ bool view_init(struct sway_view *view, enum sway_view_type type,
 }
 
 void view_destroy(struct sway_view *view) {
-	if (!sway_assert(view->surface == NULL, "Tried to free mapped view")) {
+	if (!sway_assert(view->surface == NULL, "Tried to destroy a mapped view")) {
 		return;
 	}
-	if (!sway_assert(view->destroying,
+	if (!sway_assert(view->surface == NULL, "Tried to free mapped view")) {
-				"Tried to free view which wasn't marked as destroying")) {
 		return;
 	}
 	if (!sway_assert(view->container == NULL,
@@ -95,17 +94,6 @@ void view_destroy(struct sway_view *view) {
 	}
 }
 
-void view_begin_destroy(struct sway_view *view) {
-	if (!sway_assert(view->surface == NULL, "Tried to destroy a mapped view")) {
-		return;
-	}
-	view->destroying = true;
-
-	if (!view->container) {
-		view_destroy(view);
-	}
-}
-
 const char *view_get_title(struct sway_view *view) {
 	if (view->impl->get_string_prop) {
 		return view->impl->get_string_prop(view, VIEW_PROP_TITLE);
@@ -935,17 +923,19 @@ void view_unmap(struct sway_view *view) {
 
 	struct sway_container *parent = view->container->pending.parent;
 	struct sway_workspace *ws = view->container->pending.workspace;
-	container_begin_destroy(view->container);
+	container_destroy(view->container);
 	if (parent) {
 		container_reap_empty(parent);
 	} else if (ws) {
-		workspace_consider_destroy(ws);
+		if (workspace_consider_destroy(ws)) {
+			ws = NULL;
+		}
 	}
 
 	if (root->fullscreen_global) {
 		// Container may have been a child of the root fullscreen container
 		arrange_root();
-	} else if (ws && !ws->node.destroying) {
+	} else if (ws) {
 		arrange_workspace(ws);
 		workspace_detect_urgent(ws);
 	}
@@ -1099,9 +1089,6 @@ void view_update_title(struct sway_view *view, bool force) {
 }
 
 bool view_is_visible(struct sway_view *view) {
-	if (view->container->node.destroying) {
-		return false;
-	}
 	struct sway_workspace *workspace = view->container->pending.workspace;
 	if (!workspace && view->container->pending.fullscreen_mode != FULLSCREEN_GLOBAL) {
 		bool fs_global_descendant = false;

sway/tree/workspace.c

@@ -5,6 +5,7 @@
 #include <stdio.h>
 #include <strings.h>
 #include "stringop.h"
+#include "sway/desktop/transaction.h"
 #include "sway/input/input-manager.h"
 #include "sway/input/cursor.h"
 #include "sway/input/seat.h"
@@ -134,10 +135,15 @@ struct sway_workspace *workspace_create(struct sway_output *output,
 }
 
 void workspace_destroy(struct sway_workspace *workspace) {
-	if (!sway_assert(workspace->node.destroying,
+	sway_log(SWAY_DEBUG, "Destroying workspace '%s'", workspace->name);
-				"Tried to free workspace which wasn't marked as destroying")) {
+	ipc_event_workspace(NULL, workspace, "empty"); // intentional
-		return;
+	wl_signal_emit_mutable(&workspace->node.events.destroy, &workspace->node);
+
+	if (workspace->output) {
+		workspace_detach(workspace);
 	}
+	transaction_remove_node(&workspace->node);
+
 	if (!sway_assert(workspace->node.ntxnrefs == 0, "Tried to free workspace "
 				"which is still referenced by transactions")) {
 		return;
@@ -158,36 +164,25 @@ void workspace_destroy(struct sway_workspace *workspace) {
 	free(workspace);
 }
 
-void workspace_begin_destroy(struct sway_workspace *workspace) {
+bool workspace_consider_destroy(struct sway_workspace *ws) {
-	sway_log(SWAY_DEBUG, "Destroying workspace '%s'", workspace->name);
-	ipc_event_workspace(NULL, workspace, "empty"); // intentional
-	wl_signal_emit_mutable(&workspace->node.events.destroy, &workspace->node);
-
-	if (workspace->output) {
-		workspace_detach(workspace);
-	}
-	workspace->node.destroying = true;
-	node_set_dirty(&workspace->node);
-}
-
-void workspace_consider_destroy(struct sway_workspace *ws) {
 	if (ws->tiling->length || ws->floating->length) {
-		return;
+		return false;
 	}
 
 	if (ws->output && output_get_active_workspace(ws->output) == ws) {
-		return;
+		return false;
 	}
 
 	struct sway_seat *seat;
 	wl_list_for_each(seat, &server.input->seats, link) {
 		struct sway_node *node = seat_get_focus_inactive(seat, &root->node);
 		if (node == &ws->node) {
-			return;
+			return false;
 		}
 	}
 
-	workspace_begin_destroy(ws);
+	workspace_destroy(ws);
+	return true;
 }
 
 static bool workspace_valid_on_output(const char *output_name,
@@ -596,9 +591,6 @@ bool workspace_switch(struct sway_workspace *workspace) {
 }
 
 bool workspace_is_visible(struct sway_workspace *ws) {
-	if (ws->node.destroying) {
-		return false;
-	}
 	return output_get_active_workspace(ws->output) == ws;
 }