From 8d4f454e3062d4c77f57abb59c337f002c592c60 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 27 Mar 2024 13:42:11 -0700 Subject: [PATCH 1/2] terminal: add integrity assertion that cursor pin matches x/y --- src/terminal/Screen.zig | 44 ++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 7c5d925f4..e49a5fcbe 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -182,6 +182,17 @@ pub fn assertIntegrity(self: *const Screen) void { assert(self.cursor.x < self.pages.cols); assert(self.cursor.y < self.pages.rows); + // Our cursor x/y should always match the pin. If this doesn't + // match then it indicates that the tracked pin moved and we didn't + // account for it by either calling cursorReload or manually + // adjusting. + const pt: point.Point = self.pages.pointFromPin( + .active, + self.cursor.page_pin.*, + ) orelse unreachable; + assert(self.cursor.x == pt.active.x); + assert(self.cursor.y == pt.active.y); + if (self.cursor.style_id == style.default_id) { // If our style is default, we should have no refs. assert(self.cursor.style.default()); @@ -407,12 +418,12 @@ pub fn cursorUp(self: *Screen, n: size.CellCountInt) void { assert(self.cursor.y >= n); defer self.assertIntegrity(); + self.cursor.y -= n; // Must be set before cursorChangePin const page_pin = self.cursor.page_pin.up(n).?; self.cursorChangePin(page_pin); const page_rac = page_pin.rowAndCell(); self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; - self.cursor.y -= n; } pub fn cursorRowUp(self: *Screen, n: size.CellCountInt) *pagepkg.Row { @@ -431,6 +442,8 @@ pub fn cursorDown(self: *Screen, n: size.CellCountInt) void { assert(self.cursor.y + n < self.pages.rows); defer self.assertIntegrity(); + self.cursor.y += n; // Must be set before cursorChangePin + // We move the offset into our page list to the next row and then // get the pointers to the row/cell and set all the cursor state up. const page_pin = self.cursor.page_pin.down(n).?; @@ -438,9 +451,6 @@ pub fn cursorDown(self: *Screen, n: size.CellCountInt) void { const page_rac = page_pin.rowAndCell(); self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; - - // Y of course increases - self.cursor.y += n; } /// Move the cursor to some absolute horizontal position. @@ -460,6 +470,12 @@ pub fn cursorAbsolute(self: *Screen, x: size.CellCountInt, y: size.CellCountInt) assert(y < self.pages.rows); defer self.assertIntegrity(); + const pt: point.Point = self.pages.pointFromPin( + .active, + self.cursor.page_pin.*, + ) orelse unreachable; + std.log.warn("pt={} cur_y={} y={}", .{ pt, self.cursor.y, y }); + var page_pin = if (y < self.cursor.y) self.cursor.page_pin.up(self.cursor.y - y).? else if (y > self.cursor.y) @@ -467,12 +483,12 @@ pub fn cursorAbsolute(self: *Screen, x: size.CellCountInt, y: size.CellCountInt) else self.cursor.page_pin.*; page_pin.x = x; + self.cursor.x = x; // Must be set before cursorChangePin + self.cursor.y = y; self.cursorChangePin(page_pin); const page_rac = page_pin.rowAndCell(); self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; - self.cursor.x = x; - self.cursor.y = y; } /// Reloads the cursor pointer information into the screen. This is expensive @@ -545,6 +561,14 @@ pub fn cursorDownScroll(self: *Screen) !void { // eraseRow will shift everything below it up. try self.pages.eraseRow(.{ .active = .{} }); + // We need to move our cursor down one because eraseRows will + // preserve our pin directly and we're erasing one row. + const page_pin = self.cursor.page_pin.down(1).?; + self.cursorChangePin(page_pin); + const page_rac = page_pin.rowAndCell(); + self.cursor.page_row = page_rac.row; + self.cursor.page_cell = page_rac.cell; + // The above may clear our cursor so we need to update that // again. If this fails (highly unlikely) we just reset // the cursor. @@ -558,14 +582,6 @@ pub fn cursorDownScroll(self: *Screen) !void { self.cursor.style_id = 0; self.cursor.style_ref = null; }; - - // We need to move our cursor down one because eraseRows will - // preserve our pin directly and we're erasing one row. - const page_pin = self.cursor.page_pin.down(1).?; - self.cursorChangePin(page_pin); - const page_rac = page_pin.rowAndCell(); - self.cursor.page_row = page_rac.row; - self.cursor.page_cell = page_rac.cell; } } else { const old_pin = self.cursor.page_pin.*; From dce96847a9bc291a1d605de4047a7059113abf97 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 27 Mar 2024 20:23:45 -0700 Subject: [PATCH 2/2] terminal: test eraseRowBounded, fix off by ones --- src/terminal/PageList.zig | 145 ++++++++++++++++++++++++++++++++++++-- src/terminal/Screen.zig | 6 -- src/terminal/Terminal.zig | 22 +++--- 3 files changed, 155 insertions(+), 18 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index b7ab72049..6a159d463 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -2042,7 +2042,11 @@ pub fn eraseRow( // 5 5 5 | 6 // 6 6 6 | 7 // 7 7 7 <' 4 - try page.data.cloneRowFrom(&next.data, &rows[page.data.size.rows - 1], &next_rows[0]); + try page.data.cloneRowFrom( + &next.data, + &rows[page.data.size.rows - 1], + &next_rows[0], + ); page = next; rows = next_rows; @@ -2101,7 +2105,9 @@ pub fn eraseRowBounded( var pin_it = self.tracked_pins.keyIterator(); while (pin_it.next()) |p_ptr| { const p = p_ptr.*; - if (p.page == page and p.y > pn.y and p.y < pn.y + limit) p.y -= 1; + if (p.page == page and + p.y >= pn.y and + p.y <= pn.y + limit) p.y -= 1; } return; @@ -2119,14 +2125,18 @@ pub fn eraseRowBounded( var pin_it = self.tracked_pins.keyIterator(); while (pin_it.next()) |p_ptr| { const p = p_ptr.*; - if (p.page == page and p.y > pn.y) p.y -= 1; + if (p.page == page and p.y >= pn.y) p.y -= 1; } } while (page.next) |next| { const next_rows = next.data.rows.ptr(next.data.memory.ptr); - try page.data.cloneRowFrom(&next.data, &rows[page.data.size.rows - 1], &next_rows[0]); + try page.data.cloneRowFrom( + &next.data, + &rows[page.data.size.rows - 1], + &next_rows[0], + ); page = next; rows = next_rows; @@ -4439,6 +4449,133 @@ test "PageList erase a one-row active" { } } +test "PageList eraseRowBounded less than full row" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 10, null); + defer s.deinit(); + + // Pins + const p_top = try s.trackPin(s.pin(.{ .active = .{ .y = 5, .x = 0 } }).?); + defer s.untrackPin(p_top); + const p_bot = try s.trackPin(s.pin(.{ .active = .{ .y = 8, .x = 0 } }).?); + defer s.untrackPin(p_bot); + const p_out = try s.trackPin(s.pin(.{ .active = .{ .y = 9, .x = 0 } }).?); + defer s.untrackPin(p_out); + + // Erase only a few rows in our active + try s.eraseRowBounded(.{ .active = .{ .y = 5 } }, 3); + try testing.expectEqual(s.rows, s.totalRows()); + + try testing.expectEqual(s.pages.first.?, p_top.page); + try testing.expectEqual(@as(usize, 4), p_top.y); + try testing.expectEqual(@as(usize, 0), p_top.x); + + try testing.expectEqual(s.pages.first.?, p_bot.page); + try testing.expectEqual(@as(usize, 7), p_bot.y); + try testing.expectEqual(@as(usize, 0), p_bot.x); + + try testing.expectEqual(s.pages.first.?, p_out.page); + try testing.expectEqual(@as(usize, 9), p_out.y); + try testing.expectEqual(@as(usize, 0), p_out.x); +} + +test "PageList eraseRowBounded full rows single page" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 10, null); + defer s.deinit(); + + // Pins + const p_in = try s.trackPin(s.pin(.{ .active = .{ .y = 7, .x = 0 } }).?); + defer s.untrackPin(p_in); + const p_out = try s.trackPin(s.pin(.{ .active = .{ .y = 9, .x = 0 } }).?); + defer s.untrackPin(p_out); + + // Erase only a few rows in our active + try s.eraseRowBounded(.{ .active = .{ .y = 5 } }, 10); + try testing.expectEqual(s.rows, s.totalRows()); + + // Our pin should move to the first page + try testing.expectEqual(s.pages.first.?, p_in.page); + try testing.expectEqual(@as(usize, 6), p_in.y); + try testing.expectEqual(@as(usize, 0), p_in.x); + + try testing.expectEqual(s.pages.first.?, p_out.page); + try testing.expectEqual(@as(usize, 8), p_out.y); + try testing.expectEqual(@as(usize, 0), p_out.x); +} + +test "PageList eraseRowBounded full rows two pages" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 10, null); + defer s.deinit(); + + // Grow to two pages so our active area straddles + { + const page = &s.pages.last.?.data; + for (0..page.capacity.rows - page.size.rows) |_| _ = try s.grow(); + try s.growRows(5); + try testing.expectEqual(@as(usize, 2), s.totalPages()); + try testing.expectEqual(@as(usize, 5), s.pages.last.?.data.size.rows); + } + + // Pins + const p_first = try s.trackPin(s.pin(.{ .active = .{ .y = 4, .x = 0 } }).?); + defer s.untrackPin(p_first); + const p_first_out = try s.trackPin(s.pin(.{ .active = .{ .y = 3, .x = 0 } }).?); + defer s.untrackPin(p_first_out); + const p_in = try s.trackPin(s.pin(.{ .active = .{ .y = 8, .x = 0 } }).?); + defer s.untrackPin(p_in); + const p_out = try s.trackPin(s.pin(.{ .active = .{ .y = 9, .x = 0 } }).?); + defer s.untrackPin(p_out); + + { + try testing.expectEqual(s.pages.last.?.prev.?, p_first.page); + try testing.expectEqual(@as(usize, p_first.page.data.size.rows - 1), p_first.y); + try testing.expectEqual(@as(usize, 0), p_first.x); + + try testing.expectEqual(s.pages.last.?.prev.?, p_first_out.page); + try testing.expectEqual(@as(usize, p_first_out.page.data.size.rows - 2), p_first_out.y); + try testing.expectEqual(@as(usize, 0), p_first_out.x); + + try testing.expectEqual(s.pages.last.?, p_in.page); + try testing.expectEqual(@as(usize, 3), p_in.y); + try testing.expectEqual(@as(usize, 0), p_in.x); + + try testing.expectEqual(s.pages.last.?, p_out.page); + try testing.expectEqual(@as(usize, 4), p_out.y); + try testing.expectEqual(@as(usize, 0), p_out.x); + } + + // Erase only a few rows in our active + try s.eraseRowBounded(.{ .active = .{ .y = 4 } }, 4); + + // In page in first page is shifted + try testing.expectEqual(s.pages.last.?.prev.?, p_first.page); + try testing.expectEqual(@as(usize, p_first.page.data.size.rows - 2), p_first.y); + try testing.expectEqual(@as(usize, 0), p_first.x); + + // Out page in first page should not be shifted + try testing.expectEqual(s.pages.last.?.prev.?, p_first_out.page); + try testing.expectEqual(@as(usize, p_first_out.page.data.size.rows - 2), p_first_out.y); + try testing.expectEqual(@as(usize, 0), p_first_out.x); + + // In page is shifted + try testing.expectEqual(s.pages.last.?, p_in.page); + try testing.expectEqual(@as(usize, 2), p_in.y); + try testing.expectEqual(@as(usize, 0), p_in.x); + + // Out page is not shifted + try testing.expectEqual(s.pages.last.?, p_out.page); + try testing.expectEqual(@as(usize, 4), p_out.y); + try testing.expectEqual(@as(usize, 0), p_out.x); +} + test "PageList clone" { const testing = std.testing; const alloc = testing.allocator; diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index e49a5fcbe..a94a24c7a 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -470,12 +470,6 @@ pub fn cursorAbsolute(self: *Screen, x: size.CellCountInt, y: size.CellCountInt) assert(y < self.pages.rows); defer self.assertIntegrity(); - const pt: point.Point = self.pages.pointFromPin( - .active, - self.cursor.page_pin.*, - ) orelse unreachable; - std.log.warn("pt={} cur_y={} y={}", .{ pt, self.cursor.y, y }); - var page_pin = if (y < self.cursor.y) self.cursor.page_pin.up(self.cursor.y - y).? else if (y > self.cursor.y) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 52267fa13..2c17c051a 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1095,19 +1095,25 @@ pub fn index(self: *Terminal) !void { // Otherwise use a fast path function from PageList to efficiently // scroll the contents of the scrolling region. - // eraseRow and eraseRowBounded will end up moving the cursor pin - // up by 1, so we save its current position and restore it after. - const cursor_x = self.screen.cursor.x; - const cursor_y = self.screen.cursor.y; - defer { - self.screen.cursorAbsolute(cursor_x, cursor_y); - } + // Preserve old cursor just for assertions + const old_cursor = self.screen.cursor; try self.screen.pages.eraseRowBounded( .{ .active = .{ .y = self.scrolling_region.top } }, - self.scrolling_region.bottom - self.scrolling_region.top + self.scrolling_region.bottom - self.scrolling_region.top, ); + // eraseRow and eraseRowBounded will end up moving the cursor pin + // up by 1, so we need to move it back down. A `cursorReload` + // would be better option but this is more efficient and this is + // a super hot path so we do this instead. + if (comptime std.debug.runtime_safety) { + assert(self.screen.cursor.x == old_cursor.x); + assert(self.screen.cursor.y == old_cursor.y); + } + self.screen.cursor.y -= 1; + self.screen.cursorDown(1); + // The operations above can prune our cursor style so we need to // update. This should never fail because the above can only FREE // memory.