root: refactor transaction initiation

- require the caller to use Root.startTransaction() directly
- introduce View.applyPending() to unify logic
- introduce View.shouldTrackConfigure() to unify more logic
- update all callsites to intelligently rearrange only what is necessary

river/Cursor.zig

@@ -68,9 +68,8 @@ const Mode = union(enum) {
                 // Automatically float all views being moved by the pointer
                 if (!view.current.float) {
                     view.pending.float = true;
-                    // Start a transaction to apply the pending state of the grabbed
+                    view.float_box = view.current.box;
-                    // view and rearrange the layout to fill the hole.
+                    view.applyPending();
-                    view.output.root.arrange();
                 }
 
                 // Clear cursor focus, so that the surface does not receive events
@@ -145,8 +144,7 @@ const Mode = union(enum) {
                     @intToFloat(f64, view.pending.box.y - view.current.box.y),
                 );
 
-                // Apply new pending state (no need for a configure as size didn't change)
+                view.applyPending();
-                view.current = view.pending;
             },
             .resize => |data| {
                 var output_width: c_int = undefined;
@@ -163,7 +161,7 @@ const Mode = union(enum) {
                 box.width = std.math.min(box.width, @intCast(u32, output_width - box.x - @intCast(i32, border_width)));
                 box.height = std.math.min(box.height, @intCast(u32, output_height - box.y - @intCast(i32, border_width)));
 
-                if (data.view.needsConfigure()) data.view.configure();
+                data.view.applyPending();
 
                 // Keep cursor locked to the original offset from the bottom right corner
                 c.wlr_cursor_warp_closest(

river/Output.zig

@@ -295,6 +295,8 @@ fn layoutExternal(self: *Self, visible_count: u32) !void {
 /// pending state, the changes are not appplied until a transaction is started
 /// and completed.
 pub fn arrangeViews(self: *Self) void {
+    if (self == &self.root.noop_output) return;
+
     const full_area = Box.fromWlrBox(c.wlr_output_layout_get_box(self.root.wlr_output_layout, self.wlr_output).*);
 
     // Count up views that will be arranged by the layout
@@ -353,7 +355,7 @@ pub fn arrangeLayers(self: *Self) void {
     // If the the usable_box has changed, we need to rearrange the output
     if (!std.meta.eql(self.usable_box, usable_box)) {
         self.usable_box = usable_box;
-        self.root.arrange();
+        self.arrangeViews();
     }
 
     // Arrange the layers without exclusive zones
@@ -392,6 +394,8 @@ pub fn arrangeLayers(self: *Self) void {
             }
         }
     }
+
+    self.root.startTransaction();
 }
 
 /// Arrange the layer surfaces of a given layer
@@ -603,7 +607,8 @@ fn handleDestroy(listener: ?*c.wl_listener, data: ?*c_void) callconv(.C) void {
     util.gpa.destroy(node);
 
     // Arrange the root in case evacuated views affect the layout
-    root.arrange();
+    fallback_output.arrangeViews();
+    root.startTransaction();
 }
 
 fn handleFrame(listener: ?*c.wl_listener, data: ?*c_void) callconv(.C) void {
@@ -616,5 +621,6 @@ fn handleFrame(listener: ?*c.wl_listener, data: ?*c_void) callconv(.C) void {
 fn handleMode(listener: ?*c.wl_listener, data: ?*c_void) callconv(.C) void {
     const self = @fieldParentPtr(Self, "listen_mode", listener.?);
     self.arrangeLayers();
-    self.root.arrange();
+    self.arrangeViews();
+    self.root.startTransaction();
 }

river/Root.zig

@@ -112,23 +112,20 @@ pub fn addOutput(self: *Self, wlr_output: *c.wlr_output) void {
     }
 }
 
-/// Arrange all views on all outputs and then start a transaction.
+/// Arrange all views on all outputs
-pub fn arrange(self: *Self) void {
+pub fn arrangeAll(self: *Self) void {
     var it = self.outputs.first;
-    while (it) |output_node| : (it = output_node.next) {
+    while (it) |node| : (it = node.next) node.data.arrangeViews();
-        output_node.data.arrangeViews();
-    }
-    self.startTransaction();
 }
 
 /// Initiate an atomic change to the layout. This change will not be
 /// applied until all affected clients ack a configure and commit a buffer.
-fn startTransaction(self: *Self) void {
+pub fn startTransaction(self: *Self) void {
     // If a new transaction is started while another is in progress, we need
     // to reset the pending count to 0 and clear serials from the views
     self.pending_configures = 0;
 
-    // Iterate over all views of all outputs
+    // Iterate over all layout views of all outputs
     var output_it = self.outputs.first;
     while (output_it) |node| : (output_it = node.next) {
         const output = &node.data;
@@ -136,23 +133,25 @@ fn startTransaction(self: *Self) void {
         while (view_it.next()) |view_node| {
             const view = &view_node.view;
 
-            // Clear the serial in case this transaction is interrupting a prior one.
+            if (view.shouldTrackConfigure()) {
-            view.pending_serial = null;
+                // Clear the serial in case this transaction is interrupting a prior one.
+                view.pending_serial = null;
 
-            if (view.needsConfigure()) {
+                if (view.needsConfigure()) {
-                view.configure();
+                    view.configure();
-                if (!view.pending.float) self.pending_configures += 1;
+                    self.pending_configures += 1;
 
-                // Send a frame done that the client will commit a new frame
+                    // Send a frame done that the client will commit a new frame
-                // with the dimensions we sent in the configure. Normally this
+                    // with the dimensions we sent in the configure. Normally this
-                // event would be sent in the render function.
+                    // event would be sent in the render function.
-                view.sendFrameDone();
+                    view.sendFrameDone();
-            }
+                }
 
-            // If there are saved buffers present, then this transaction is interrupting
+                // If there are saved buffers present, then this transaction is interrupting
-            // a previous transaction and we should keep the old buffers.
+                // a previous transaction and we should keep the old buffers.
-            if (view.saved_buffers.items.len == 0) {
+                if (view.saved_buffers.items.len == 0) view.saveBuffers();
-                view.saveBuffers();
+            } else {
+                if (view.needsConfigure()) view.configure();
             }
         }
     }
@@ -183,6 +182,8 @@ fn handleTimeout(data: ?*c_void) callconv(.C) c_int {
 
     log.err(.transaction, "timeout occurred, some imperfect frames may be shown", .{});
 
+    self.pending_configures = 0;
+
     self.commitTransaction();
 
     return 0;
@@ -203,10 +204,7 @@ pub fn notifyConfigured(self: *Self) void {
 /// layout. Should only be called after all clients have configured for
 /// the new layout. If called early imperfect frames may be drawn.
 fn commitTransaction(self: *Self) void {
-    // TODO: apply damage properly
+    std.debug.assert(self.pending_configures == 0);
-
-    // Ensure this is set to 0 to avoid entering invalid state (e.g. if called due to timeout)
-    self.pending_configures = 0;
 
     // Iterate over all views of all outputs
     var output_it = self.outputs.first;
@@ -231,6 +229,8 @@ fn commitTransaction(self: *Self) void {
         var view_it = ViewStack(View).iterator(output.views.first, std.math.maxInt(u32));
         while (view_it.next()) |view_node| {
             const view = &view_node.view;
+            if (!view.shouldTrackConfigure() and view.pending_serial != null) continue;
+
             // Apply pending state of the view
             view.pending_serial = null;
             if (view.pending.tags != view.current.tags) view_tags_changed = true;

river/View.zig

@@ -145,6 +145,49 @@ pub fn deinit(self: Self) void {
     self.saved_buffers.deinit();
 }
 
+/// Handle changes to pending state and start a transaction to apply them
+pub fn applyPending(self: *Self) void {
+    var arrange_output = false;
+
+    if (self.current.tags != self.pending.tags)
+        arrange_output = true;
+
+    // If switching from float -> layout or layout -> float arrange the output
+    // to get assigned a new size or fill the hole in the layout left behind
+    if (self.current.float != self.pending.float)
+        arrange_output = true;
+
+    // If switching from float to something else save the dimensions
+    if (self.current.float and !self.pending.float)
+        self.float_box = self.current.box;
+
+    // If switching from something else to float restore the dimensions
+    if ((!self.current.float and self.pending.float) or
+        (self.current.fullscreen and !self.pending.fullscreen and self.pending.float))
+        self.pending.box = self.float_box;
+
+    // If switching to fullscreen set the dimensions to the full area of the output
+    if (!self.current.fullscreen and self.pending.fullscreen) {
+        self.pending.box = Box.fromWlrBox(
+            c.wlr_output_layout_get_box(self.output.root.wlr_output_layout, self.output.wlr_output).*,
+        );
+        // TODO: move this to configure
+        switch (self.impl) {
+            .xdg_toplevel => |xdg_toplevel| xdg_toplevel.setFullscreen(self.pending.fullscreen),
+            .xwayland_view => |xwayland_view| xwayland_view.setFullscreen(self.pending.fullscreen),
+        }
+    }
+
+    // If switching from fullscreen to layout, arrange the output to get
+    // assigned the proper size.
+    if (self.current.fullscreen and !self.pending.fullscreen and !self.pending.float)
+        arrange_output = true;
+
+    if (arrange_output) self.output.arrangeViews();
+
+    self.output.root.startTransaction();
+}
+
 pub fn needsConfigure(self: Self) bool {
     return switch (self.impl) {
         .xdg_toplevel => |xdg_toplevel| xdg_toplevel.needsConfigure(),
@@ -171,11 +214,7 @@ pub fn dropSavedBuffers(self: *Self) void {
 }
 
 pub fn saveBuffers(self: *Self) void {
-    if (self.saved_buffers.items.len > 0) {
+    std.debug.assert(self.saved_buffers.items.len == 0);
-        log.err(.transaction, "view already has buffers saved, overwriting", .{});
-        self.saved_buffers.items.len = 0;
-    }
-
     self.saved_surface_box = self.surface_box;
     self.forEachSurface(saveBuffersIterator, &self.saved_buffers);
 }
@@ -204,36 +243,6 @@ fn saveBuffersIterator(
     }
 }
 
-/// Set the pending state, set the size, and inform the client.
-pub fn setFullscreen(self: *Self, fullscreen: bool) void {
-    self.pending.fullscreen = fullscreen;
-
-    if (fullscreen) {
-        // If transitioning from float -> fullscreen, save the floating
-        // dimensions.
-        if (self.pending.float) self.float_box = self.current.box;
-
-        const output = self.output;
-        self.pending.box = Box.fromWlrBox(
-            c.wlr_output_layout_get_box(output.root.wlr_output_layout, output.wlr_output).*,
-        );
-        self.configure();
-    } else if (self.pending.float) {
-        // If transitioning from fullscreen -> float, return to the saved
-        // floating dimensions.
-        self.pending.box = self.float_box;
-        self.configure();
-    } else {
-        // Transitioning to layout, arrange and start a transaction
-        self.output.root.arrange();
-    }
-
-    switch (self.impl) {
-        .xdg_toplevel => |xdg_toplevel| xdg_toplevel.setFullscreen(fullscreen),
-        .xwayland_view => |xwayland_view| xwayland_view.setFullscreen(fullscreen),
-    }
-}
-
 /// Move a view from one output to another, sending the required enter/leave
 /// events.
 pub fn sendToOutput(self: *Self, destination_output: *Output) void {
@@ -319,6 +328,16 @@ pub fn fromWlrSurface(wlr_surface: *c.wlr_surface) ?*Self {
     return null;
 }
 
+pub fn shouldTrackConfigure(self: Self) bool {
+    // There are exactly three cases in which we do not track configures
+    // 1. the view was and remains floating
+    // 2. the view is changing from float/layout to fullscreen
+    // 3. the view is changing from fullscreen to float
+    return !((self.pending.float and self.current.float) or
+        (self.pending.fullscreen and !self.current.fullscreen) or
+        (self.pending.float and !self.pending.fullscreen and self.current.fullscreen));
+}
+
 /// Called by the impl when the surface is ready to be displayed
 pub fn map(self: *Self) void {
     const root = self.output.root;
@@ -338,7 +357,8 @@ pub fn map(self: *Self) void {
 
     self.output.sendViewTags();
 
-    if (self.pending.float) self.configure() else root.arrange();
+    self.output.arrangeViews();
+    self.output.root.startTransaction();
 }
 
 /// Called by the impl when the surface will no longer be displayed
@@ -363,7 +383,10 @@ pub fn unmap(self: *Self) void {
     self.output.sendViewTags();
 
     // Still need to arrange if fullscreened from the layout
-    if (!self.current.float) root.arrange();
+    if (!self.current.float) {
+        self.output.arrangeViews();
+        root.startTransaction();
+    }
 }
 
 /// Destory the view and free the ViewStack node holding it.

river/XdgToplevel.zig

@@ -193,6 +193,7 @@ fn handleMap(listener: ?*c.wl_listener, data: ?*c_void) callconv(.C) void {
         // Make views with app_ids listed in the float filter float
         for (root.server.config.float_filter.items) |filter_app_id| {
             if (std.mem.eql(u8, std.mem.span(app_id), std.mem.span(filter_app_id))) {
+                view.current.float = true;
                 view.pending.float = true;
                 view.pending.box = view.float_box;
                 break;
@@ -245,11 +246,11 @@ fn handleCommit(listener: ?*c.wl_listener, data: ?*c_void) callconv(.C) void {
         view.surface_box = new_box;
 
         if (s == self.wlr_xdg_surface.configure_serial) {
-            // If this commit is in response to our configure and the view is
+            // If this commit is in response to our configure and the
-            // part of the layout, notify the transaction code. If floating or
+            // transaction code is tracking this configure, notify it.
-            // fullscreen apply the pending state immediately.
+            // Otherwise, apply the pending state immediately.
             view.pending_serial = null;
-            if (!view.pending.float and !view.pending.fullscreen)
+            if (view.shouldTrackConfigure())
                 view.output.root.notifyConfigured()
             else
                 view.current = view.pending;
@@ -286,5 +287,6 @@ fn handleNewPopup(listener: ?*c.wl_listener, data: ?*c_void) callconv(.C) void {
 fn handleRequestFullscreen(listener: ?*c.wl_listener, data: ?*c_void) callconv(.C) void {
     const self = @fieldParentPtr(Self, "listen_request_fullscreen", listener.?);
     const event = util.voidCast(c.wlr_xdg_toplevel_set_fullscreen_event, data.?);
-    self.view.setFullscreen(event.fullscreen);
+    self.view.pending.fullscreen = event.fullscreen;
+    self.view.applyPending();
 }

river/command/config.zig

@@ -31,7 +31,8 @@ pub fn borderWidth(
 
     const server = seat.input_manager.server;
     server.config.border_width = try std.fmt.parseInt(u32, args[1], 10);
-    server.root.arrange();
+    server.root.arrangeAll();
+    server.root.startTransaction();
 }
 
 pub fn viewPadding(
@@ -45,7 +46,8 @@ pub fn viewPadding(
 
     const server = seat.input_manager.server;
     server.config.view_padding = try std.fmt.parseInt(u32, args[1], 10);
-    server.root.arrange();
+    server.root.arrangeAll();
+    server.root.startTransaction();
 }
 
 pub fn outerPadding(
@@ -59,7 +61,8 @@ pub fn outerPadding(
 
     const server = seat.input_manager.server;
     server.config.outer_padding = try std.fmt.parseInt(u32, args[1], 10);
-    server.root.arrange();
+    server.root.arrangeAll();
+    server.root.startTransaction();
 }
 
 pub fn backgroundColor(

river/command/mod_master_count.zig

@@ -33,5 +33,6 @@ pub fn modMasterCount(
     const delta = try std.fmt.parseInt(i32, args[1], 10);
     const output = seat.focused_output;
     output.master_count = @intCast(u32, std.math.max(0, @intCast(i32, output.master_count) + delta));
-    seat.input_manager.server.root.arrange();
+    output.arrangeViews();
+    output.root.startTransaction();
 }

river/command/mod_master_factor.zig

@@ -35,6 +35,7 @@ pub fn modMasterFactor(
     const new_master_factor = std.math.min(std.math.max(output.master_factor + delta, 0.05), 0.95);
     if (new_master_factor != output.master_factor) {
         output.master_factor = new_master_factor;
-        seat.input_manager.server.root.arrange();
+        output.arrangeViews();
+        output.root.startTransaction();
     }
 }

river/command/send_to_output.zig

@@ -54,7 +54,9 @@ pub fn sendToOutput(
         seat.focused.view.sendToOutput(destination_output);
 
         // Handle the change and focus whatever's next in the focus stack
-        root.arrange();
         seat.focus(null);
+        seat.focused_output.arrangeViews();
+        destination_output.arrangeViews();
+        root.startTransaction();
     }
 }

river/command/tags.zig

@@ -30,7 +30,8 @@ pub fn setFocusedTags(
     const tags = try parseTags(allocator, args, out);
     if (seat.focused_output.pending.tags != tags) {
         seat.focused_output.pending.tags = tags;
-        seat.input_manager.server.root.arrange();
+        seat.focused_output.arrangeViews();
+        seat.focused_output.root.startTransaction();
     }
 }
 
@@ -44,7 +45,7 @@ pub fn setViewTags(
     const tags = try parseTags(allocator, args, out);
     if (seat.focused == .view) {
         seat.focused.view.pending.tags = tags;
-        seat.focused.view.output.root.arrange();
+        seat.focused.view.applyPending();
     }
 }
 
@@ -60,7 +61,8 @@ pub fn toggleFocusedTags(
     const new_focused_tags = output.pending.tags ^ tags;
     if (new_focused_tags != 0) {
         output.pending.tags = new_focused_tags;
-        seat.input_manager.server.root.arrange();
+        output.arrangeViews();
+        output.root.startTransaction();
     }
 }
 
@@ -76,7 +78,7 @@ pub fn toggleViewTags(
         const new_tags = seat.focused.view.current.tags ^ tags;
         if (new_tags != 0) {
             seat.focused.view.pending.tags = new_tags;
-            seat.focused.view.output.root.arrange();
+            seat.focused.view.applyPending();
         }
     }
 }

river/command/toggle_float.zig

@@ -40,18 +40,6 @@ pub fn toggleFloat(
         if (seat.input_manager.isCursorActionTarget(view)) return;
 
         view.pending.float = !view.pending.float;
-
+        view.applyPending();
-        if (view.pending.float) {
-            // If switching from layout to float, restore the previous floating
-            // dimensions.
-            view.pending.box = view.float_box;
-            view.configure();
-        } else {
-            // If switching from float to layout save the floating dimensions
-            // for next time.
-            view.float_box = view.current.box;
-        }
-
-        view.output.root.arrange();
     }
 }

river/command/toggle_fullscreen.zig

@@ -38,6 +38,7 @@ pub fn toggleFullscreen(
         // Don't modify views which are the target of a cursor action
         if (seat.input_manager.isCursorActionTarget(view)) return;
 
-        view.setFullscreen(!view.pending.fullscreen);
+        view.pending.fullscreen = !view.pending.fullscreen;
+        view.applyPending();
     }
 }

river/command/zoom.zig

@@ -56,8 +56,9 @@ pub fn zoom(
         if (zoom_node) |to_bump| {
             output.views.remove(to_bump);
             output.views.push(to_bump);
-            seat.input_manager.server.root.arrange();
             seat.focus(&to_bump.view);
+            output.arrangeViews();
+            output.root.startTransaction();
         }
     }
 }