diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 6a5d0d251..79aa96f18 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3116,6 +3116,8 @@ pub fn scrollClear(self: *PageList) Allocator.Error!void { /// the sole remaining page, or shortening the source page of a split. Without /// a new generation, a cached pointer, serial, and coordinate could still pass /// `nodeIsValid` while referring to a different row than it originally did. +/// Screen and Terminal fast paths which manipulate Page rows directly must use +/// this because they bypass PageList's own row-mutation helpers. /// /// The caller must pass a node which is currently live in this PageList. Call /// this at the point the operation commits to changing the layout and before @@ -3142,7 +3144,7 @@ pub fn scrollClear(self: *PageList) Allocator.Error!void { /// adjust row or memory accounting, mark cells dirty, or notify incremental /// compression. The surrounding operation remains responsible for all such /// bookkeeping required by its layout change. -fn invalidateNodeLayout(self: *PageList, node: *List.Node) void { +pub fn invalidateNodeLayout(self: *PageList, node: *List.Node) void { node.serial = self.page_serial; self.page_serial += 1; } diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 8798411b0..5ca5d957c 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -960,8 +960,8 @@ pub fn cursorScrollAbove(self: *Screen) !void { // lot cheaper in 99% of cases. const old_pin = self.cursor.page_pin.*; - if (try self.pages.grow()) |_| { - try self.cursorScrollAboveRotate(); + if (try self.pages.grow()) |new_node| { + try self.cursorScrollAboveRotate(new_node); } else { // In this case, it means grow() didn't allocate a new page. @@ -981,6 +981,8 @@ pub fn cursorScrollAbove(self: *Screen) !void { // Rotate the rows so that the newly created empty row is at the // beginning. e.g. [ 0 1 2 3 ] in to [ 3 0 1 2 ]. var rows = page.rows.ptr(page.memory.ptr); + // Rotating this suffix changes which logical row its coordinates identify. + self.pages.invalidateNodeLayout(pin.node); fastmem.rotateOnceR(Row, rows[pin.y..page.size.rows]); // Mark the whole page as dirty. @@ -1014,7 +1016,7 @@ pub fn cursorScrollAbove(self: *Screen) !void { // 1 |5E00000000| | 4 // +----------+ : // +-------------+ - try self.cursorScrollAboveRotate(); + try self.cursorScrollAboveRotate(null); } } @@ -1029,7 +1031,10 @@ pub fn cursorScrollAbove(self: *Screen) !void { } } -fn cursorScrollAboveRotate(self: *Screen) !void { +fn cursorScrollAboveRotate( + self: *Screen, + fresh_node: ?*PageList.List.Node, +) !void { self.cursorChangePin(self.cursor.page_pin.down(1).?); // Go through each of the pages following our pin, shift all rows @@ -1042,6 +1047,12 @@ fn cursorScrollAboveRotate(self: *Screen) !void { const prev_rows = prev_page.rows.ptr(prev_page.memory.ptr); const cur_rows = cur_page.rows.ptr(cur_page.memory.ptr); + // A newly allocated tail has no earlier references to invalidate. + if (fresh_node == null or current != fresh_node.?) { + // Rotating this page moves every cached row coordinate down by one. + self.pages.invalidateNodeLayout(current); + } + // Rotate the pages down: [ 0 1 2 3 ] => [ 3 0 1 2 ] fastmem.rotateOnceR(Row, cur_rows[0..cur_page.size.rows]); @@ -1061,6 +1072,8 @@ fn cursorScrollAboveRotate(self: *Screen) !void { assert(current == self.cursor.page_pin.node); const cur_page = current.page(); const cur_rows = cur_page.rows.ptr(cur_page.memory.ptr); + // Rotating the cursor-page suffix changes its cached row coordinates. + self.pages.invalidateNodeLayout(current); fastmem.rotateOnceR(Row, cur_rows[self.cursor.page_pin.y..cur_page.size.rows]); self.clearCells( cur_page, @@ -1145,6 +1158,8 @@ pub fn cursorScrollRegionUp(self: *Screen, limit: usize) !void { // Rotate the region rows so the now-blank top row moves to the // bottom (the cursor row) and everything else shifts up by one. + // Rotating the region changes which logical row its coordinates identify. + self.pages.invalidateNodeLayout(pin.node); fastmem.rotateOnce(Row, rows); // Mark the whole page as dirty. @@ -4919,6 +4934,22 @@ test "Screen: cursorScrollRegionUp simple" { } } +test "Screen: cursorScrollRegionUp renews page generation" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, .{ .cols = 5, .rows = 5, .max_scrollback = 0 }); + defer s.deinit(); + try s.testWriteString("1ABCD\n2EFGH\n3IJKL\n4MNOP\n5QRST"); + s.cursorAbsolute(0, 2); + + const node = s.cursor.page_pin.node; + const serial = node.serial; + try s.cursorScrollRegionUp(2); + + try testing.expect(!s.pages.nodeIsValid(node, serial)); +} + test "Screen: cursorScrollRegionUp moves selection" { const testing = std.testing; const alloc = testing.allocator; @@ -5281,7 +5312,10 @@ test "Screen: scroll above same page" { // +----------+ : // +-------------+ + const node = s.cursor.page_pin.node; + const serial = node.serial; try s.cursorScrollAbove(); + try testing.expect(!s.pages.nodeIsValid(node, serial)); // +----------+ = PAGE 0 // 0 |1ABCD00000| @@ -5356,7 +5390,13 @@ test "Screen: scroll above same page but cursor on previous page" { // +----------+ : // +-------------+ + const first_node = s.pages.pages.first.?; + const second_node = first_node.next.?; + const first_serial = first_node.serial; + const second_serial = second_node.serial; try s.cursorScrollAbove(); + try testing.expect(!s.pages.nodeIsValid(first_node, first_serial)); + try testing.expect(!s.pages.nodeIsValid(second_node, second_serial)); // +----------+ = PAGE 0 // ... : : @@ -5521,7 +5561,10 @@ test "Screen: scroll above creates new page" { // 4307 |3IJKL00000| | 2 // +----------+ : // +-------------+ + const node = s.pages.pages.first.?; + const serial = node.serial; try s.cursorScrollAbove(); + try testing.expect(!s.pages.nodeIsValid(node, serial)); // +----------+ = PAGE 0 // ... : : diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index e5bfd93b7..089201276 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -2429,6 +2429,21 @@ fn rowWillBeShifted( } } +/// Renew every live page generation in an inclusive range before a full-width +/// line operation moves logical rows between their coordinates. +fn invalidateFullWidthRowRange( + self: *Terminal, + first: *PageList.List.Node, + last: *PageList.List.Node, +) void { + var node = first; + while (true) : (node = node.next.?) { + // Full-width line movement remaps cached row coordinates in this page. + self.screens.active.pages.invalidateNodeLayout(node); + if (node == last) break; + } +} + // TODO(qwerasd): `insertLines` and `deleteLines` are 99% identical, // the majority of their logic can (and should) be abstracted in to // a single shared helper function, probably on `Screen` not here. @@ -2504,6 +2519,12 @@ pub fn insertLines(self: *Terminal, count: usize) void { }; defer self.screens.active.pages.untrackPin(cur_p); + // Partial-width margins edit cells in stable rows; full-width moves rows. + if (!left_right) self.invalidateFullWidthRowRange( + self.screens.active.cursor.page_pin.node, + cur_p.node, + ); + // Our current y position relative to the cursor var y: usize = rem; @@ -2699,6 +2720,12 @@ pub fn deleteLines(self: *Terminal, count: usize) void { }; defer self.screens.active.pages.untrackPin(cur_p); + // Partial-width margins edit cells in stable rows; full-width moves rows. + if (!left_right) self.invalidateFullWidthRowRange( + cur_p.node, + cur_p.down(rem - 1).?.node, + ); + // Our current y position relative to the cursor var y: usize = 0; @@ -6867,8 +6894,11 @@ test "Terminal: insertLines simple" { try t.printString("GHI"); t.setCursorPos(2, 2); + const node = t.screens.active.cursor.page_pin.node; + const serial = node.serial; t.clearDirty(); t.insertLines(1); + try testing.expect(!t.screens.active.pages.nodeIsValid(node, serial)); try testing.expect(!t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); @@ -7046,11 +7076,15 @@ test "Terminal: insertLines across page boundary marks all shifted rows dirty" { try t.printString("5EEEE"); // Verify we now have a second page - try testing.expect(first_page.next != null); + const second_page = first_page.next.?; + const first_serial = first_page.serial; + const second_serial = second_page.serial; t.setCursorPos(1, 1); t.clearDirty(); t.insertLines(1); + try testing.expect(!t.screens.active.pages.nodeIsValid(first_page, first_serial)); + try testing.expect(!t.screens.active.pages.nodeIsValid(second_page, second_serial)); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); @@ -9754,8 +9788,11 @@ test "Terminal: deleteLines simple" { try t.printString("GHI"); t.setCursorPos(2, 2); + const node = t.screens.active.cursor.page_pin.node; + const serial = node.serial; t.clearDirty(); t.deleteLines(1); + try testing.expect(!t.screens.active.pages.nodeIsValid(node, serial)); try testing.expect(!t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); @@ -9837,11 +9874,15 @@ test "Terminal: deleteLines across page boundary marks all shifted rows dirty" { try t.printString("5EEEE"); // Verify we now have a second page - try testing.expect(first_page.next != null); + const second_page = first_page.next.?; + const first_serial = first_page.serial; + const second_serial = second_page.serial; t.setCursorPos(1, 1); t.clearDirty(); t.deleteLines(1); + try testing.expect(!t.screens.active.pages.nodeIsValid(first_page, first_serial)); + try testing.expect(!t.screens.active.pages.nodeIsValid(second_page, second_serial)); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } }));