transactions: handle preemption properly

when a transaction interrupts an ongoing transaction, we must be careful
to handle the configures properly. This commit adds a new member to view
so that we can store the dimensions sent with the last configure in
order to determine if the preempting transaction should override the
ongoing configure or not.

Additionally, some views do not ack a configure if they already have the
requested dimensions. This can happen if a pending configure setting
alternative dimensions is overridden, so in this case we do not wait for
an ack before committing the transaction.

6 files changed, 94 insertions, 37 deletions

diff --git a/river/Output.zig b/river/Output.zig
index d97ebfb..c15bb1f 100644
--- a/river/Output.zig
+++ b/river/Output.zig
@@ -334,8 +334,8 @@ pub fn layoutMasterStack(self: *Self, visible_count: u32, output_tags: u32, posi
         new_box.width -= delta_size;
         new_box.height -= delta_size;
 
-        // Set the view's pending box to the new dimensions
-        view.pending_box = new_box;
+        // Set the view's next box to the new dimensions
+        view.next_box = new_box;
 
         i += 1;
     }
@@ -391,7 +391,7 @@ pub fn layoutFull(self: *Self, visible_count: u32, output_tags: u32) void {
             .height = layout_height,
         };
 
-        view.pending_box = new_box;
+        view.next_box = new_box;
 
         i += 1;
     }
diff --git a/river/Root.zig b/river/Root.zig
index 11ec344..b84a568 100644
--- a/river/Root.zig
+++ b/river/Root.zig
@@ -132,27 +132,57 @@ fn startTransaction(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;
-            // Clear the serial in case this transaction is interrupting a prior one.
-            view.pending_serial = null;
 
-            if (view.needsConfigure()) {
-                view.configure();
-                self.pending_configures += 1;
+            switch (view.configureAction()) {
+                .override => {
+                    view.configure();
+
+                    // Some clients do not ack a configure if the requested
+                    // size is the same as their current size. Configures of
+                    // this nature may be sent if a pending configure is
+                    // interrupted by a configure returning to the original
+                    // size.
+                    if (view.pending_box.?.width == view.current_box.width and
+                        view.pending_box.?.height == view.current_box.height)
+                    {
+                        view.pending_serial = null;
+                    } else {
+                        std.debug.assert(view.pending_serial != null);
+                        self.pending_configures += 1;
+                    }
+                },
+                .new_configure => {
+                    view.configure();
+                    self.pending_configures += 1;
+                    std.debug.assert(view.pending_serial != null);
+                },
+                .old_configure => {
+                    self.pending_configures += 1;
+                    view.next_box = null;
+                    std.debug.assert(view.pending_serial != null);
+                },
+                .noop => {
+                    view.next_box = null;
+                    std.debug.assert(view.pending_serial == null);
+                },
+            }
+
+            // If there is a saved buffer present, then this transaction is
+            // interrupting a previous transaction and we should keep the old
+            // buffer.
+            if (view.stashed_buffer == null) {
+                view.stashBuffer();
 
                 // We save the current buffer, so we can send an early
                 // frame done event to give the client a head start on
                 // redrawing.
                 view.sendFrameDone();
             }
-
-            // If there is a saved buffer present, then this transaction is interrupting
-            // a previous transaction and we should keep the old buffer.
-            if (view.stashed_buffer == null) {
-                view.stashBuffer();
-            }
         }
     }
 
+    // If there are views that need configures, start a timer and wait for
+    // configure events before committing.
     if (self.pending_configures > 0) {
         Log.Debug.log(
             "Started transaction with {} pending configures.",
@@ -160,11 +190,15 @@ fn startTransaction(self: *Self) void {
         );
 
         // Set timeout to 200ms
-        if (c.wl_event_source_timer_update(self.transaction_timer, 200) < 0) {
+        if (c.wl_event_source_timer_update(self.transaction_timer, 1000) < 0) {
             Log.Error.log("failed to update timer.", .{});
             self.commitTransaction();
         }
     } else {
+        // No views need configures, clear the current timer in case we are
+        // interrupting another transaction and commit.
+        if (c.wl_event_source_timer_update(self.transaction_timer, 0) < 0)
+            Log.Error.log("Error disarming timer", .{});
         self.commitTransaction();
     }
 }
@@ -183,7 +217,7 @@ pub fn notifyConfigured(self: *Self) void {
     self.pending_configures -= 1;
     if (self.pending_configures == 0) {
         // Disarm the timer, as we didn't timeout
-        if (c.wl_event_source_timer_update(self.transaction_timer, 0) == -1)
+        if (c.wl_event_source_timer_update(self.transaction_timer, 0) < 0)
             Log.Error.log("Error disarming timer", .{});
         self.commitTransaction();
     }
@@ -223,6 +257,7 @@ fn commitTransaction(self: *Self) void {
             const view = &view_node.view;
             // Ensure that all pending state is cleared
             view.pending_serial = null;
+            std.debug.assert(view.next_box == null);
             if (view.pending_box) |state| {
                 view.current_box = state;
                 view.pending_box = null;
diff --git a/river/View.zig b/river/View.zig
index af5d9b9..dcb6027 100644
--- a/river/View.zig
+++ b/river/View.zig
@@ -55,8 +55,13 @@ focused: bool,
 
 /// The current output-relative coordinates and dimensions of the view
 current_box: Box,
+
+/// The dimensions sent with the most recent configure
 pending_box: ?Box,
 
+/// The dimensions to be used for the next configure
+next_box: ?Box,
+
 /// The dimensions the view would have taken if we didn't force it to tile
 natural_width: u32,
 natural_height: u32,
@@ -111,23 +116,40 @@ pub fn deinit(self: Self) void {
     }
 }
 
-pub fn needsConfigure(self: Self) bool {
-    if (self.pending_box) |pending_box| {
-        return pending_box.width != self.current_box.width or
-            pending_box.height != self.current_box.height;
-    } else {
-        return false;
-    }
+/// Returns true if a configure needs to be sent to ensure the next_box is
+/// applied correctly.
+pub fn configureAction(self: Self) enum { override, new_configure, old_configure, noop } {
+    // If we have a pending box, check if the next box is different from the
+    // pending box. If we do not have a pending box, check if the next box is
+    // different from the current box.
+    if (self.pending_box) |pending_box|
+        if (self.next_box) |next_box| {
+            if (next_box.width != pending_box.width or next_box.height != pending_box.height) {
+                return .override;
+            } else {
+                return .old_configure;
+            }
+        };
+
+    if (self.next_box) |next_box|
+        if (next_box.width != self.current_box.width or next_box.height != self.current_box.height)
+            return .new_configure;
+
+    return .noop;
 }
 
-pub fn configure(self: Self) void {
-    if (self.pending_box) |pending_box| {
+/// Tell the client to assume the size of next_box. Set pending_box to
+/// next_box and next_box to null.
+pub fn configure(self: *Self) void {
+    if (self.next_box) |next_box| {
         switch (self.impl) {
-            .xdg_toplevel => |xdg_toplevel| xdg_toplevel.configure(pending_box),
-            .xwayland_view => |xwayland_view| xwayland_view.configure(pending_box),
+            .xdg_toplevel => |xdg_toplevel| xdg_toplevel.configure(next_box),
+            .xwayland_view => |xwayland_view| xwayland_view.configure(next_box),
         }
+        self.pending_box = next_box;
+        self.next_box = null;
     } else {
-        Log.Error.log("Configure called on a View with no pending box", .{});
+        Log.Error.log("configure called on View with null next_box", .{});
     }
 }
 
diff --git a/river/VoidView.zig b/river/VoidView.zig
index fca2321..4f71d81 100644
--- a/river/VoidView.zig
+++ b/river/VoidView.zig
@@ -23,7 +23,7 @@ const c = @import("c.zig");
 
 const Box = @import("Box.zig");
 
-pub fn configure(self: Self, pending_box: Box) void {
+pub fn configure(self: Self, box: Box) void {
     unreachable;
 }
 
diff --git a/river/XdgToplevel.zig b/river/XdgToplevel.zig
index 88e7dce..4726146 100644
--- a/river/XdgToplevel.zig
+++ b/river/XdgToplevel.zig
@@ -63,11 +63,11 @@ pub fn init(self: *Self, view: *View, wlr_xdg_surface: *c.wlr_xdg_surface) void
     c.wl_signal_add(&self.wlr_xdg_surface.events.unmap, &self.listen_unmap);
 }
 
-pub fn configure(self: Self, pending_box: Box) void {
+pub fn configure(self: Self, box: Box) void {
     self.view.pending_serial = c.wlr_xdg_toplevel_set_size(
         self.wlr_xdg_surface,
-        pending_box.width,
-        pending_box.height,
+        box.width,
+        box.height,
     );
 }
 
diff --git a/river/XwaylandView.zig b/river/XwaylandView.zig
index 681d9d1..bb0b07a 100644
--- a/river/XwaylandView.zig
+++ b/river/XwaylandView.zig
@@ -58,13 +58,13 @@ pub fn init(self: *Self, view: *View, wlr_xwayland_surface: *c.wlr_xwayland_surf
 }
 
 /// Tell the client to take a new size
-pub fn configure(self: Self, pending_box: Box) void {
+pub fn configure(self: Self, box: Box) void {
     c.wlr_xwayland_surface_configure(
         self.wlr_xwayland_surface,
-        @intCast(i16, pending_box.x),
-        @intCast(i16, pending_box.y),
-        @intCast(u16, pending_box.width),
-        @intCast(u16, pending_box.height),
+        @intCast(i16, box.x),
+        @intCast(i16, box.y),
+        @intCast(u16, box.width),
+        @intCast(u16, box.height),
     );
     // Xwayland surfaces don't use serials, so we will just assume they have
     // configured the next time they commit. Set pending serial to a dummy