Skip to content

Commit

Permalink
terminal: fix off-by-one tracked pin issues when page is pruned
Browse files Browse the repository at this point in the history
  • Loading branch information
mitchellh committed Mar 17, 2024
1 parent 1979347 commit 37eaae0
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 3 deletions.
39 changes: 38 additions & 1 deletion src/terminal/PageList.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1962,7 +1962,7 @@ pub fn pointFromPin(self: *const PageList, tag: point.Tag, p: Pin) ?point.Point
if (tl.y > p.y) return null;
coord.y = p.y - tl.y;
} else {
coord.y += tl.page.data.size.rows - tl.y - 1;
coord.y += tl.page.data.size.rows - tl.y;
var page_ = tl.page.next;
while (page_) |page| : (page_ = page.next) {
if (page == p.page) {
Expand Down Expand Up @@ -2879,6 +2879,43 @@ test "PageList pointFromPin active from prior page" {
}
}

test "PageList pointFromPin traverse pages" {
const testing = std.testing;
const alloc = testing.allocator;

var s = try init(alloc, 80, 24, null);
defer s.deinit();
const page = &s.pages.last.?.data;
for (0..page.capacity.rows * 2) |_| {
_ = try s.grow();
}

{
const pages = s.totalPages();
const page_cap = page.capacity.rows;
const expected_y = page_cap * (pages - 2) + 5;

try testing.expectEqual(point.Point{
.screen = .{
.y = expected_y,
.x = 2,
},
}, s.pointFromPin(.screen, .{
.page = s.pages.last.?.prev.?,
.y = 5,
.x = 2,
}).?);
}

// Prior page
{
try testing.expect(s.pointFromPin(.active, .{
.page = s.pages.first.?,
.y = 0,
.x = 0,
}) == null);
}
}
test "PageList active after grow" {
const testing = std.testing;
const alloc = testing.allocator;
Expand Down
30 changes: 28 additions & 2 deletions src/terminal/Screen.zig
Original file line number Diff line number Diff line change
Expand Up @@ -479,10 +479,35 @@ pub fn cursorDownScroll(self: *Screen) !void {
page_pin.page.data.getCells(self.cursor.page_row),
);
} else {
const old_pin = self.cursor.page_pin.*;

// Grow our pages by one row. The PageList will handle if we need to
// allocate, prune scrollback, whatever.
_ = try self.pages.grow();
const page_pin = self.cursor.page_pin.down(1).?;

// If our pin page change it means that the page that the pin
// was on was pruned. In this case, grow() moves the pin to
// the top-left of the new page. This effectively moves it by
// one already, we just need to fix up the x value.
const page_pin = if (old_pin.page == self.cursor.page_pin.page)
self.cursor.page_pin.down(1).?
else reuse: {
var pin = self.cursor.page_pin.*;
pin.x = self.cursor.x;
break :reuse pin;
};

// These assertions help catch some pagelist math errors. Our
// x/y should be unchanged after the grow.
if (comptime std.debug.runtime_safety) {
const active = self.pages.pointFromPin(
.active,
page_pin,
).?.active;
assert(active.x == self.cursor.x);
assert(active.y == self.cursor.y);
}

const page_rac = page_pin.rowAndCell();
self.cursor.page_pin.* = page_pin;
self.cursor.page_row = page_rac.row;
Expand Down Expand Up @@ -2745,14 +2770,15 @@ test "Screen: scrolling when viewport is pruned" {

// Our viewport is now somewhere pinned. Create so much scrollback
// that we prune it.
try s.testWriteString("\n");
for (0..1000) |_| try s.testWriteString("1ABCD\n2EFGH\n3IJKL\n");
try s.testWriteString("1ABCD\n2EFGH\n3IJKL");

{
// Test our contents rotated
const contents = try s.dumpStringAlloc(alloc, .{ .viewport = .{} });
defer alloc.free(contents);
try testing.expectEqualStrings("2EFGH\n3IJKL\n1ABCD", contents);
try testing.expectEqualStrings("1ABCD\n2EFGH\n3IJKL", contents);
}

{
Expand Down

0 comments on commit 37eaae0

Please sign in to comment.