win: split unmap_win into unmap and destroy

Instead of doing both unmapping and destroying in the same function,
since that complicates the logic.

Also since now destroy_win handles both the managed and unmanaged cases,
remove destroy_unmanage_win function.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

src/event.c

@@ -262,11 +262,7 @@ static inline void ev_configure_notify(session_t *ps, xcb_configure_notify_event
 static inline void ev_destroy_notify(session_t *ps, xcb_destroy_notify_event_t *ev) {
 	auto w = find_win(ps, ev->window);
 	if (w) {
-		if (w->managed) {
+		auto _ attr_unused = destroy_win(ps, w);
-			auto _ attr_unused = unmap_win(ps, (struct managed_win *)w, true);
-		} else {
-			destroy_unmanaged_win(ps, w);
-		}
 	}
 }
 
@@ -299,7 +295,7 @@ static inline void ev_map_notify(session_t *ps, xcb_map_notify_event_t *ev) {
 static inline void ev_unmap_notify(session_t *ps, xcb_unmap_notify_event_t *ev) {
 	auto w = find_managed_win(ps, ev->window);
 	if (w) {
-		auto _ attr_unused = unmap_win(ps, w, false);
+		unmap_win(ps, w);
 	}
 }
 
@@ -322,13 +318,10 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t
 		}
 	} else {
 		// otherwise, find and destroy the window first
-		auto w = find_win(ps, ev->window);
+		{
-		if (w) {
+			auto w = find_win(ps, ev->window);
-			if (w->managed) {
+			if (w) {
-				auto _ attr_unused =
+				auto _ attr_unused = destroy_win(ps, w);
-				    unmap_win(ps, (struct managed_win *)w, true);
-			} else {
-				destroy_unmanaged_win(ps, w);
 			}
 		}
 

src/win.c

@@ -1668,51 +1668,58 @@ static void finish_unmap_win(session_t *ps, struct managed_win *w) {
 
 /// Finish the destruction of a window (e.g. after fading has finished).
 /// Frees `w`
-static void finish_destroy_win(session_t *ps, struct managed_win *w) {
+static void finish_destroy_win(session_t *ps, struct win *w) {
-	log_trace("Trying to finish destroying (%#010x)", w->base.id);
+	log_trace("Trying to finish destroying (%#010x)", w->id);
 
-	if (w->state != WSTATE_UNMAPPED) {
+	auto next_w = win_stack_find_next_managed(ps, &w->stack_neighbour);
-		// Only UNMAPPED state has window resources freed, otherwise
+	list_remove(&w->stack_neighbour);
-		// we need to call finish_unmap_win to free them.
-		finish_unmap_win(ps, w);
-	}
 
-	// Invalidate reg_ignore of windows below this one
+	if (w->managed) {
-	// TODO what if w->next is not mapped??
+		auto mw = (struct managed_win *)w;
-	// TODO seriously figure out how reg_ignore behaves.
-	//      I think if `w` is unmapped, and destroyed after
-	//      paint happened at least once, w->reg_ignore_valid would
-	//      be true, and there is no need to invalid w->next->reg_ignore
-	//      when w is destroyed.
-	auto next_w = win_stack_find_next_managed(ps, &w->base.stack_neighbour);
-	if (next_w) {
-		rc_region_unref(&next_w->reg_ignore);
-		next_w->reg_ignore_valid = false;
-	}
 
-	log_trace("Trying to destroy (%#010x)", w->base.id);
+		if (mw->state != WSTATE_UNMAPPED) {
-	list_remove(&w->base.stack_neighbour);
+			// Only UNMAPPED state has window resources freed, otherwise
+			// we need to call unmap_win_finish to free them.
+			// XXX actually we unmap_win_finish only frees the rendering
+			//     resources, we still need to call free_win_res. will fix
+			//     later.
+			finish_unmap_win(ps, mw);
+		}
 
-	if (w == ps->active_win) {
+		// Invalidate reg_ignore of windows below this one
-		// Usually, the window cannot be the focused at destruction. FocusOut
+		// TODO what if next_w is not mapped??
-		// should be generated before the window is destroyed.
+		// TODO seriously figure out how reg_ignore behaves.
-		// We do this check just to be completely sure we don't have dangling
+		//      I think if `w` is unmapped, and destroyed after
-		// references.
+		//      paint happened at least once, w->reg_ignore_valid would
-		log_debug("window %#010x (%s) is destroyed while being focused",
+		//      be true, and there is no need to invalid w->next->reg_ignore
-		          w->base.id, w->name);
+		//      when w is destroyed.
-		ps->active_win = NULL;
+		if (next_w) {
-	}
+			rc_region_unref(&next_w->reg_ignore);
+			next_w->reg_ignore_valid = false;
+		}
 
-	free_win_res(ps, w);
+		if (mw == ps->active_win) {
+			// Usually, the window cannot be the focused at destruction.
+			// FocusOut should be generated before the window is destroyed. We
+			// do this check just to be completely sure we don't have dangling
+			// references.
+			log_debug("window %#010x (%s) is destroyed while being focused",
+			          w->id, mw->name);
+			ps->active_win = NULL;
+		}
 
-	// Drop w from all prev_trans to avoid accessing freed memory in
+		free_win_res(ps, mw);
-	// repair_win()
+
-	// TODO there can only be one prev_trans pointing to w
+		// Drop w from all prev_trans to avoid accessing freed memory in
-	win_stack_foreach_managed(w2, &ps->window_stack) {
+		// repair_win()
-		if (w == w2->prev_trans) {
+		// TODO there can only be one prev_trans pointing to w
-			w2->prev_trans = NULL;
+		win_stack_foreach_managed(w2, &ps->window_stack) {
+			if (mw == w2->prev_trans) {
+				w2->prev_trans = NULL;
+			}
 		}
 	}
+
 	free(w);
 }
 
@@ -1721,14 +1728,6 @@ static void finish_map_win(struct managed_win *w) {
 	w->state = WSTATE_MAPPED;
 }
 
-void destroy_unmanaged_win(session_t *ps, struct win *w) {
-	assert(!w->managed);
-	assert(!w->destroyed);
-	list_remove(&w->stack_neighbour);
-	HASH_DEL(ps->windows, w);
-	free(w);
-}
-
 /// Move window `w` so it's before `next` in the list
 static inline void restack_win(session_t *ps, struct win *w, struct list_node *next) {
 	struct managed_win *mw = NULL;
@@ -1812,86 +1811,93 @@ void restack_top(session_t *ps, struct win *w) {
 	restack_win(ps, w, ps->window_stack.next);
 }
 
-/// Unmap or destroy a window
+/// Start destroying a window. Windows cannot always be destroyed immediately
-/// @return whether the window is destroyed and freed
+/// because of fading and such.
-bool unmap_win(session_t *ps, struct managed_win *w, bool destroy) {
+///
-	winstate_t target_state = destroy ? WSTATE_DESTROYING : WSTATE_UNMAPPING;
+/// @return whether the window has finished destroying and is freed
+bool destroy_win(session_t *ps, struct win *w) {
+	auto mw = (struct managed_win *)w;
+	assert(w);
 
-	if (unlikely(!w)) {
+	log_debug("Destroying %#010x \"%s\", managed = %d", w->id,
-		return false;
+	          (w->managed ? mw->name : NULL), w->managed);
+
+	// Delete destroyed window from the hash table, even though the window might still
+	// be rendered for a while. We need to make sure future window with the same
+	// window id won't confuse us. Keep the window in the window stack if it's managed
+	// and mapped, since we might still need to render it (e.g. fading out). Window
+	// will be removed from the stack when it finishes destroying.
+	HASH_DEL(ps->windows, w);
+
+	if (!w->managed || mw->state == WSTATE_UNMAPPED) {
+		// Window is already unmapped, or is an unmanged window, just destroy it
+		finish_destroy_win(ps, w);
+		return true;
 	}
 
-	if (!destroy && w->a._class == XCB_WINDOW_CLASS_INPUT_ONLY) {
+	if (w->managed) {
-		// We don't care about mapping / unmapping of Input Only windows.
+		// Update state flags of a managed window
-		// But we need to remember to destroy them, so future window with
+		mw->state = WSTATE_DESTROYING;
-		// the same id won't be handled incorrectly.
+		mw->a.map_state = XCB_MAP_STATE_UNMAPPED;
-		// Issue #119 was caused by this.
+		mw->in_openclose = true;
-		return false;
 	}
 
-	log_trace("Unmapping %#010x \"%s\", destroy = %d", w->base.id,
+	// don't need win_ev_stop because the window is gone anyway
-	          (w ? w->name : NULL), destroy);
+#ifdef CONFIG_DBUS
+	// Send D-Bus signal
+	if (ps->o.dbus) {
+		cdbus_ev_win_destroyed(ps, w);
+	}
+#endif
 
-	if (unlikely(w->state == WSTATE_DESTROYING && !destroy)) {
+	if (!ps->redirected) {
+		// Skip transition if we are not rendering
+		return win_skip_fading(ps, mw);
+	}
+
+	return false;
+}
+
+void unmap_win(session_t *ps, struct managed_win *w) {
+	assert(w);
+	assert(w->base.managed);
+	assert(w->a._class != XCB_WINDOW_CLASS_INPUT_ONLY);
+
+	log_debug("Unmapping %#010x \"%s\"", w->base.id, w->name);
+
+	if (unlikely(w->state == WSTATE_DESTROYING)) {
 		log_warn("Trying to undestroy a window?");
 		assert(false);
 	}
 
-	// If the window is already in the state we want
+	if (unlikely(w->state == WSTATE_UNMAPPING || w->state == WSTATE_UNMAPPED)) {
-	if (unlikely(w->state == target_state)) {
+		log_warn("Trying to unmapping an already unmapped window %#010x "
-		log_warn("%s a window twice", destroy ? "Destroying" : "Unmapping");
+		         "\"%s\"",
-		return false;
+		         w->base.id, w->name);
-	}
+		assert(false);
-
+		return;
-	if (destroy) {
-		// Delete destroyed window from the hash table, so future window with the
-		// same window id won't confuse us.
-		// Keep the window in the window stack, since we might still need to
-		// render it (fading out). Window will be removed from the stack when
-		// fading finishes.
-		HASH_DEL(ps->windows, &w->base);
-	}
-
-	if (unlikely(w->state == WSTATE_UNMAPPED) || w->a._class == XCB_WINDOW_CLASS_INPUT_ONLY) {
-		if (unlikely(!destroy)) {
-			log_warn("Unmapping an already unmapped window %#010x %s twice",
-			         w->base.id, w->name);
-			return false;
-		}
-		// Window is already unmapped, or is an Input Only window, just destroy it
-		finish_destroy_win(ps, w);
-		return true;
 	}
 
 	// Note we don't update focused window here. This will either be
 	// triggered by subsequence Focus{In, Out} event, or by recheck_focus
 
 	w->a.map_state = XCB_MAP_STATE_UNMAPPED;
-	w->state = target_state;
+	w->state = WSTATE_UNMAPPING;
 	w->opacity_target = win_calc_opacity_target(ps, w, false);
 
-	w->in_openclose = destroy;
-
 	// don't care about properties anymore
-	if (!destroy) {
+	win_ev_stop(ps, &w->base);
-		win_ev_stop(ps, &w->base);
-	}
 
 #ifdef CONFIG_DBUS
 	// Send D-Bus signal
 	if (ps->o.dbus) {
-		if (destroy) {
+		cdbus_ev_win_unmapped(ps, &w->base);
-			cdbus_ev_win_destroyed(ps, &w->base);
-		} else {
-			cdbus_ev_win_unmapped(ps, &w->base);
-		}
 	}
 #endif
 
 	if (!ps->redirected) {
-		return win_skip_fading(ps, w);
+		CHECK(!win_skip_fading(ps, w));
 	}
-	return false;
 }
 
 /**
@@ -1908,7 +1914,7 @@ bool win_check_fade_finished(session_t *ps, struct managed_win *w) {
 	if (w->opacity == w->opacity_target) {
 		switch (w->state) {
 		case WSTATE_UNMAPPING: finish_unmap_win(ps, w); return false;
-		case WSTATE_DESTROYING: finish_destroy_win(ps, w); return true;
+		case WSTATE_DESTROYING: finish_destroy_win(ps, &w->base); return true;
 		case WSTATE_MAPPING: finish_map_win(w); return false;
 		case WSTATE_FADING: w->state = WSTATE_MAPPED; break;
 		default: unreachable;

src/win.h

@@ -253,6 +253,14 @@ struct managed_win {
 /// Process pending images flags on a window. Has to be called in X critical section
 void win_process_flags(session_t *ps, struct managed_win *w);
 
+/// Start the unmap of a window. We cannot unmap immediately since we might need to fade
+/// the window out.
+void unmap_win(struct session *, struct managed_win *);
+
+/// Start the destroying of a window. Windows cannot always be destroyed immediately
+/// because of fading and such.
+bool must_use destroy_win(session_t *ps, struct win *w);
+
 /// Release images bound with a window, set the *_NONE flags on the window. Only to be
 /// used when de-initializing the backend outside of win.c
 void win_release_images(struct backend_base *base, struct managed_win *w);
@@ -358,12 +366,6 @@ void restack_above(session_t *ps, struct win *w, xcb_window_t below);
 void restack_bottom(session_t *ps, struct win *w);
 /// Move window `w` to the top of the stack
 void restack_top(session_t *ps, struct win *w);
-/// Unmap or destroy a window
-/// @return whether the window is destroyed and freed
-bool must_use unmap_win(session_t *ps, struct managed_win *, bool destroy);
-/// Destroy and free an unmanaged window
-void destroy_unmanaged_win(session_t *ps, struct win *w);
-
 void map_win(session_t *ps, struct managed_win *w);
 
 /**