From c0d7e71ec499d3e520c5567f4fbf6c6bda723adc Mon Sep 17 00:00:00 2001 From: Isaac Freund Date: Mon, 8 Jun 2020 00:10:30 +0200 Subject: [PATCH] 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. --- river/Output.zig | 6 ++--- river/Root.zig | 61 +++++++++++++++++++++++++++++++++--------- river/View.zig | 46 ++++++++++++++++++++++--------- river/VoidView.zig | 2 +- river/XdgToplevel.zig | 6 ++--- river/XwaylandView.zig | 10 +++---- 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