View: fix unmap/destroy control flow

Currently the view destruction sequence is started as soon as a view
is unmapped. However, this is incorrect as a client may map the view
again instead of destroying it.

Instead, only start the view destruction sequence when the underlying
xdg toplevel or xwayland surface is destroyed.

river/Cursor.zig

@@ -630,7 +630,9 @@ fn xwaylandUnmanagedSurfaceAt(lx: f64, ly: f64) ?SurfaceAtResult {
 }
 
 fn surfaceAtFilter(view: *View, filter_tags: u32) bool {
-    return !view.destroying and view.current.tags & filter_tags != 0;
+    // TODO(wlroots): we can remove this view.surface != null check as surfaceAt
+    // will start filtering by mapped views by default in 0.15.0
+    return view.surface != null and view.current.tags & filter_tags != 0;
 }
 
 pub fn enterMode(self: *Self, mode: std.meta.Tag((Mode)), view: *View) void {

river/Output.zig

@@ -168,7 +168,7 @@ pub fn sendUrgentTags(self: Self) void {
 }
 
 pub fn arrangeFilter(view: *View, filter_tags: u32) bool {
-    return !view.destroying and !view.pending.float and !view.pending.fullscreen and
+    return view.surface != null and !view.pending.float and !view.pending.fullscreen and
         view.pending.tags & filter_tags != 0;
 }
 

river/OutputStatus.zig

@@ -74,7 +74,7 @@ pub fn sendViewTags(self: Self) void {
 
     var it = self.output.views.first;
     while (it) |node| : (it = node.next) {
-        if (node.view.destroying) continue;
+        if (node.view.surface == null) continue;
         view_tags.append(node.view.current.tags) catch {
             self.output_status.postNoMemory();
             log.crit("out of memory", .{});

river/Root.zig

@@ -291,7 +291,7 @@ pub fn startTransaction(self: *Self) void {
         while (view_it) |view_node| : (view_it = view_node.next) {
             const view = &view_node.view;
 
-            if (view.destroying) continue;
+            if (view.surface == null) continue;
 
             if (view.shouldTrackConfigure()) {
                 // Clear the serial in case this transaction is interrupting a prior one.
@@ -386,10 +386,13 @@ fn commitTransaction(self: *Self) void {
             const view = &view_node.view;
             view_it = view_node.next;
 
-            if (view.destroying) {
-                view.destroy();
+            if (view.surface == null) {
+                view.dropSavedBuffers();
+                view.output.views.remove(view_node);
+                if (view.destroying) view.destroy();
                 continue;
             }
+            assert(!view.destroying);
 
             if (view.pending_serial != null and !view.shouldTrackConfigure()) continue;
 

river/Seat.zig

@@ -183,7 +183,7 @@ pub fn focus(self: *Self, _target: ?*View) void {
 }
 
 fn pendingFilter(view: *View, filter_tags: u32) bool {
-    return !view.destroying and view.pending.tags & filter_tags != 0;
+    return view.surface != null and view.pending.tags & filter_tags != 0;
 }
 
 /// Switch focus to the target, handling unfocus and input inhibition

river/View.zig

@@ -88,12 +88,11 @@ impl: Impl = undefined,
 /// The output this view is currently associated with
 output: *Output,
 
-/// This is non-null from the point where the view is mapped until the
-/// surface is destroyed by wlroots.
+/// This is non-null exactly when the view is mapped
 surface: ?*wlr.Surface = null,
 
-/// This View struct outlasts the wlroots object it wraps. This bool is set to
-/// true when the backing wlr.XdgToplevel or equivalent has been destroyed.
+/// This indicates that the view should be destroyed when the current
+/// transaction completes. See View.destroy()
 destroying: bool = false,
 
 /// The double-buffered state of the view
@@ -153,28 +152,22 @@ pub fn init(self: *Self, output: *Output, tags: u32, surface: anytype) void {
     } else unreachable;
 }
 
-/// Deinit the view, remove it from the view stack and free the memory.
+/// If saved buffers of the view are currently in use by a transaction,
+/// mark this view for destruction when the transaction completes. Otherwise
+/// destroy immediately.
 pub fn destroy(self: *Self) void {
-    self.dropSavedBuffers();
-    self.saved_buffers.deinit();
+    assert(self.surface == null);
+    self.destroying = true;
 
-    if (self.foreign_toplevel_handle) |handle| {
-        self.foreign_activate.link.remove();
-        self.foreign_fullscreen.link.remove();
-        self.foreign_close.link.remove();
-        handle.destroy();
+    // If there are still saved buffers, then this view needs to be kept
+    // around until the current transaction completes. This function will be
+    // called again in Root.commitTransaction()
+    if (self.saved_buffers.items.len == 0) {
+        self.saved_buffers.deinit();
+
+        const node = @fieldParentPtr(ViewStack(Self).Node, "view", self);
+        util.gpa.destroy(node);
     }
-
-    switch (self.impl) {
-        .xdg_toplevel => |*xdg_toplevel| xdg_toplevel.deinit(),
-        .xwayland_view => |*xwayland_view| xwayland_view.deinit(),
-    }
-
-    self.request_activate.link.remove();
-
-    const node = @fieldParentPtr(ViewStack(Self).Node, "view", self);
-    self.output.views.remove(node);
-    util.gpa.destroy(node);
 }
 
 /// Handle changes to pending state and start a transaction to apply them
@@ -457,7 +450,8 @@ pub fn shouldTrackConfigure(self: Self) bool {
 pub fn map(self: *Self) !void {
     log.debug("view '{s}' mapped", .{self.getTitle()});
 
-    if (self.foreign_toplevel_handle == null) {
+    {
+        assert(self.foreign_toplevel_handle == null);
         const handle = try wlr.ForeignToplevelHandleV1.create(server.foreign_toplevel_manager);
         self.foreign_toplevel_handle = handle;
 
@@ -494,15 +488,24 @@ pub fn map(self: *Self) !void {
 pub fn unmap(self: *Self) void {
     log.debug("view '{s}' unmapped", .{self.getTitle()});
 
-    assert(!self.destroying);
-    self.destroying = true;
-
     if (self.saved_buffers.items.len == 0) self.saveBuffers();
 
+    assert(self.surface != null);
+    self.surface = null;
+
     // Inform all seats that the view has been unmapped so they can handle focus
     var it = server.input_manager.seats.first;
     while (it) |seat_node| : (it = seat_node.next) seat_node.data.handleViewUnmap(self);
 
+    assert(self.foreign_toplevel_handle != null);
+    self.foreign_activate.link.remove();
+    self.foreign_fullscreen.link.remove();
+    self.foreign_close.link.remove();
+    self.foreign_toplevel_handle.?.destroy();
+    self.foreign_toplevel_handle = null;
+
+    self.request_activate.link.remove();
+
     self.output.sendViewTags();
 
     // Still need to arrange if fullscreened from the layout

river/VoidView.zig

@@ -23,10 +23,6 @@ const wlr = @import("wlroots");
 const Box = @import("Box.zig");
 const View = @import("View.zig");
 
-pub fn deinit(self: *Self) void {
-    unreachable;
-}
-
 pub fn needsConfigure(self: Self) bool {
     unreachable;
 }

river/XdgToplevel.zig

@@ -77,20 +77,6 @@ pub fn init(self: *Self, view: *View, xdg_surface: *wlr.XdgSurface) void {
     Subsurface.handleExisting(xdg_surface.surface, .{ .xdg_toplevel = self });
 }
 
-pub fn deinit(self: *Self) void {
-    if (self.view.surface != null) {
-        // Remove listeners that are active for the entire lifetime of the view
-        self.destroy.link.remove();
-        self.map.link.remove();
-        self.unmap.link.remove();
-        self.new_popup.link.remove();
-        self.new_subsurface.link.remove();
-
-        Subsurface.destroySubsurfaces(self.xdg_surface.surface);
-        XdgPopup.destroyPopups(self.xdg_surface);
-    }
-}
-
 /// Returns true if a configure must be sent to ensure that the pending
 /// dimensions are applied.
 pub fn needsConfigure(self: Self) bool {
@@ -168,8 +154,18 @@ pub fn getConstraints(self: Self) View.Constraints {
 
 fn handleDestroy(listener: *wl.Listener(*wlr.XdgSurface), xdg_surface: *wlr.XdgSurface) void {
     const self = @fieldParentPtr(Self, "destroy", listener);
-    self.deinit();
-    self.view.surface = null;
+
+    // Remove listeners that are active for the entire lifetime of the view
+    self.destroy.link.remove();
+    self.map.link.remove();
+    self.unmap.link.remove();
+    self.new_popup.link.remove();
+    self.new_subsurface.link.remove();
+
+    Subsurface.destroySubsurfaces(self.xdg_surface.surface);
+    XdgPopup.destroyPopups(self.xdg_surface);
+
+    self.view.destroy();
 }
 
 /// Called when the xdg surface is mapped, or ready to display on-screen.

river/XwaylandView.zig

@@ -58,16 +58,6 @@ pub fn init(self: *Self, view: *View, xwayland_surface: *wlr.XwaylandSurface) vo
     xwayland_surface.events.request_configure.add(&self.request_configure);
 }
 
-pub fn deinit(self: *Self) void {
-    if (self.view.surface != null) {
-        // Remove listeners that are active for the entire lifetime of the view
-        self.destroy.link.remove();
-        self.map.link.remove();
-        self.unmap.link.remove();
-        self.request_configure.link.remove();
-    }
-}
-
 pub fn needsConfigure(self: Self) bool {
     const output = self.view.output;
     const output_box = server.root.output_layout.getBox(output.wlr_output).?;
@@ -153,8 +143,14 @@ pub fn getConstraints(self: Self) View.Constraints {
 /// Called when the xwayland surface is destroyed
 fn handleDestroy(listener: *wl.Listener(*wlr.XwaylandSurface), xwayland_surface: *wlr.XwaylandSurface) void {
     const self = @fieldParentPtr(Self, "destroy", listener);
-    self.deinit();
-    self.view.surface = null;
+
+    // Remove listeners that are active for the entire lifetime of the view
+    self.destroy.link.remove();
+    self.map.link.remove();
+    self.unmap.link.remove();
+    self.request_configure.link.remove();
+
+    self.view.destroy();
 }
 
 /// Called when the xwayland surface is mapped, or ready to display on-screen.

river/command/focus_view.zig

@@ -72,5 +72,5 @@ pub fn focusView(
 }
 
 fn filter(view: *View, filter_tags: u32) bool {
-    return !view.destroying and view.pending.tags & filter_tags != 0;
+    return view.surface != null and view.pending.tags & filter_tags != 0;
 }

river/command/swap.zig

@@ -80,6 +80,6 @@ pub fn swap(
 }
 
 fn filter(view: *View, filter_tags: u32) bool {
-    return !view.destroying and !view.pending.float and
+    return view.surface != null and !view.pending.float and
         !view.pending.fullscreen and view.pending.tags & filter_tags != 0;
 }

river/command/zoom.zig

@@ -61,6 +61,6 @@ pub fn zoom(
 }
 
 fn filter(view: *View, filter_tags: u32) bool {
-    return !view.destroying and !view.pending.float and
+    return view.surface != null and !view.pending.float and
         !view.pending.fullscreen and view.pending.tags & filter_tags != 0;
 }