From 1b753a15bd37b68a9aa358bcc38eb29fd4fda186 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Mar 2024 20:10:23 -0700 Subject: [PATCH] terminal: in all cursor move cases, we need to account for page changes --- src/terminal/Screen.zig | 194 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 180 insertions(+), 14 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 27d1df2349..68b40569fc 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -367,8 +367,8 @@ pub fn cursorUp(self: *Screen, n: size.CellCountInt) void { assert(self.cursor.y >= n); const page_pin = self.cursor.page_pin.up(n).?; + self.cursorChangePin(page_pin); const page_rac = page_pin.rowAndCell(); - self.cursor.page_pin.* = page_pin; self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; self.cursor.y -= n; @@ -391,8 +391,8 @@ pub fn cursorDown(self: *Screen, n: size.CellCountInt) void { // 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).?; + self.cursorChangePin(page_pin); const page_rac = page_pin.rowAndCell(); - self.cursor.page_pin.* = page_pin; self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; @@ -422,8 +422,8 @@ pub fn cursorAbsolute(self: *Screen, x: size.CellCountInt, y: size.CellCountInt) else self.cursor.page_pin.*; page_pin.x = x; + self.cursorChangePin(page_pin); const page_rac = page_pin.rowAndCell(); - self.cursor.page_pin.* = page_pin; self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; self.cursor.x = x; @@ -467,8 +467,8 @@ pub fn cursorDownScroll(self: *Screen) !void { // 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_pin.* = page_pin; self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; @@ -510,8 +510,8 @@ pub fn cursorDownScroll(self: *Screen) !void { assert(active.y == self.cursor.y); } + self.cursorChangePin(page_pin); const page_rac = page_pin.rowAndCell(); - self.cursor.page_pin.* = page_pin; self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; @@ -527,15 +527,6 @@ pub fn cursorDownScroll(self: *Screen) !void { } if (self.cursor.style_id != style.default_id) { - // We need to ensure our new page has our style. This is a somewhat - // expensive operation so we only do it if our page pin y is on zero, - // which signals we're at the top of a page. - if (self.cursor.page_pin.y == 0) { - // This should never happen because if we're in a new - // page then we should have space for one style. - try self.manualStyleUpdate(); - } - // The newly created line needs to be styled according to // the bg color if it is set. if (self.cursor.style.bgCell()) |blank_cell| { @@ -582,6 +573,42 @@ pub fn cursorCopy(self: *Screen, other: Cursor) !void { try self.manualStyleUpdate(); } +/// Always use this to write to cursor.page_pin.*. +/// +/// This specifically handles the case when the new pin is on a different +/// page than the old AND we have a style set. In that case, we must release +/// our old style and upsert our new style since styles are stored per-page. +fn cursorChangePin(self: *Screen, new: Pin) void { + // If we have a style set, then we need to migrate it over to the + // new page. This is expensive so we do everything we can with cheap + // ops to avoid it. + if (self.cursor.style_id == style.default_id or + self.cursor.page_pin.page == new.page) + { + self.cursor.page_pin.* = new; + return; + } + + // Store our old full style so we can reapply it in the new page. + const old_style = self.cursor.style; + + // Clear our old style in the current pin + self.cursor.style = .{}; + self.manualStyleUpdate() catch unreachable; // Removing a style should never fail + + // Update our pin to the new page + self.cursor.page_pin.* = new; + self.cursor.style = old_style; + self.manualStyleUpdate() catch |err| { + // This failure should not happen because manualStyleUpdate + // handles page splitting, overflow, and more. This should only + // happen if we're out of RAM. In this case, we'll just degrade + // gracefully back to the default style. + log.err("failed to update style on cursor change err={}", .{err}); + self.cursor.style = .{}; + }; +} + /// Options for scrolling the viewport of the terminal grid. The reason /// we have this in addition to PageList.Scroll is because we have additional /// scroll behaviors that are not part of the PageList.Scroll enum. @@ -2489,6 +2516,145 @@ test "Screen: clearPrompt no prompt" { } } +test "Screen: cursorDown across pages preserves style" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 3, 1); + defer s.deinit(); + + // Scroll down enough to go to another page + const start_page = &s.pages.pages.last.?.data; + const rem = start_page.capacity.rows; + for (0..rem) |_| try s.cursorDownOrScroll(); + + // We need our page to change for this test o make sense. If this + // assertion fails then the bug is in the test: we should be scrolling + // above enough for a new page to show up. + { + const page = &s.cursor.page_pin.page.data; + try testing.expect(start_page != page); + } + + // Scroll back to the previous page + s.cursorUp(1); + { + const page = &s.cursor.page_pin.page.data; + try testing.expect(start_page == page); + } + + // Go back up, set a style + try s.setAttribute(.{ .bold = {} }); + { + const page = &s.cursor.page_pin.page.data; + const styleval = page.styles.lookupId( + page.memory, + s.cursor.style_id, + ).?; + try testing.expect(styleval.flags.bold); + } + + // Go back down into the next page and we should have that style + s.cursorDown(1); + { + const page = &s.cursor.page_pin.page.data; + const styleval = page.styles.lookupId( + page.memory, + s.cursor.style_id, + ).?; + try testing.expect(styleval.flags.bold); + } +} + +test "Screen: cursorUp across pages preserves style" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 3, 1); + defer s.deinit(); + + // Scroll down enough to go to another page + const start_page = &s.pages.pages.last.?.data; + const rem = start_page.capacity.rows; + for (0..rem) |_| try s.cursorDownOrScroll(); + + // We need our page to change for this test o make sense. If this + // assertion fails then the bug is in the test: we should be scrolling + // above enough for a new page to show up. + { + const page = &s.cursor.page_pin.page.data; + try testing.expect(start_page != page); + } + + // Go back up, set a style + try s.setAttribute(.{ .bold = {} }); + { + const page = &s.cursor.page_pin.page.data; + const styleval = page.styles.lookupId( + page.memory, + s.cursor.style_id, + ).?; + try testing.expect(styleval.flags.bold); + } + + // Go back down into the prev page and we should have that style + s.cursorUp(1); + { + const page = &s.cursor.page_pin.page.data; + try testing.expect(start_page == page); + + const styleval = page.styles.lookupId( + page.memory, + s.cursor.style_id, + ).?; + try testing.expect(styleval.flags.bold); + } +} + +test "Screen: cursorAbsolute across pages preserves style" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 3, 1); + defer s.deinit(); + + // Scroll down enough to go to another page + const start_page = &s.pages.pages.last.?.data; + const rem = start_page.capacity.rows; + for (0..rem) |_| try s.cursorDownOrScroll(); + + // We need our page to change for this test o make sense. If this + // assertion fails then the bug is in the test: we should be scrolling + // above enough for a new page to show up. + { + const page = &s.cursor.page_pin.page.data; + try testing.expect(start_page != page); + } + + // Go back up, set a style + try s.setAttribute(.{ .bold = {} }); + { + const page = &s.cursor.page_pin.page.data; + const styleval = page.styles.lookupId( + page.memory, + s.cursor.style_id, + ).?; + try testing.expect(styleval.flags.bold); + } + + // Go back down into the prev page and we should have that style + s.cursorAbsolute(1, 1); + { + const page = &s.cursor.page_pin.page.data; + try testing.expect(start_page == page); + + const styleval = page.styles.lookupId( + page.memory, + s.cursor.style_id, + ).?; + try testing.expect(styleval.flags.bold); + } +} test "Screen: scrolling" { const testing = std.testing; const alloc = testing.allocator;