From 0d94fb61c9d529d28f42dbf0d6d1d205d6b02571 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 7 Jun 2024 15:03:02 -0700 Subject: [PATCH] terminal: all cursor movement needs to mark the old and new page dirty --- src/terminal/Screen.zig | 21 ++++++++++----------- src/terminal/Terminal.zig | 39 +++++++++++++++++++-------------------- 2 files changed, 29 insertions(+), 31 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 94053d21d..df00546fd 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -481,10 +481,6 @@ pub fn cursorAbsolute(self: *Screen, x: size.CellCountInt, y: size.CellCountInt) assert(y < self.pages.rows); defer self.assertIntegrity(); - // Moving the cursor affects text run splitting (ligatures) so - // we must mark the old and new page dirty. - self.cursor.page_pin.markDirty(); - var page_pin = if (y < self.cursor.y) self.cursor.page_pin.up(self.cursor.y - y).? else if (y > self.cursor.y) @@ -498,11 +494,6 @@ pub fn cursorAbsolute(self: *Screen, x: size.CellCountInt, y: size.CellCountInt) const page_rac = page_pin.rowAndCell(); self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; - - // Mark the new page dirty. This might be the same page as the old - // but this is a fairly cheap operation and cursor movement isn't - // super common. - self.cursor.page_pin.markDirty(); } /// Reloads the cursor pointer information into the screen. This is expensive @@ -717,6 +708,14 @@ pub fn cursorCopy(self: *Screen, other: Cursor) !void { /// 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 { + // Moving the cursor affects text run splitting (ligatures) so + // we must mark the old and new page dirty. We do this as long + // as the pins are not equal + if (!self.cursor.page_pin.eql(new)) { + self.cursor.page_pin.markDirty(); + new.markDirty(); + } + // 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. @@ -2983,8 +2982,8 @@ test "Screen: scrolling with a single-row screen with scrollback" { // Active should be dirty try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); - // Our scrollback should not be dirty - try testing.expect(!s.pages.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); + // Scrollback also dirty because cursor moved from there + try testing.expect(s.pages.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); s.scroll(.{ .delta_row = -1 }); { diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 6ef7f1ddb..b2783ba82 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -2621,7 +2621,8 @@ test "Terminal: input with basic wraparound dirty" { t.clearDirty(); try t.print('w'); - try testing.expect(!t.isDirty(.{ .screen = .{ .x = 4, .y = 0 } })); + // Old row is dirty because cursor moved from there + try testing.expect(t.isDirty(.{ .screen = .{ .x = 4, .y = 0 } })); try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 1 } })); } @@ -2747,7 +2748,8 @@ test "Terminal: print wide char at edge creates spacer head" { // Our first row just had a spacer head added which does not affect // rendering so only the place where the wide char was printed // should be marked. - try testing.expect(!t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); + // BUT old row is dirty because cursor moved from there + try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 1 } })); } @@ -3708,10 +3710,11 @@ test "Terminal: print right margin wrap dirty tracking" { try testing.expect(t.isDirty(.{ .screen = .{ .x = 4, .y = 0 } })); try testing.expect(!t.isDirty(.{ .screen = .{ .x = 2, .y = 1 } })); - // Writing our Y should wrap and only mark the second line dirty. + // Writing our Y should wrap. It marks both rows dirty because the + // cursor moved. t.clearDirty(); try t.print('Y'); - try testing.expect(!t.isDirty(.{ .screen = .{ .x = 4, .y = 0 } })); + try testing.expect(t.isDirty(.{ .screen = .{ .x = 4, .y = 0 } })); try testing.expect(t.isDirty(.{ .screen = .{ .x = 2, .y = 1 } })); { @@ -3769,8 +3772,8 @@ test "Terminal: print wide char at right margin does not create spacer head" { try testing.expectEqual(@as(usize, 1), t.screen.cursor.y); try testing.expectEqual(@as(usize, 4), t.screen.cursor.x); - // Only wrapped row should be dirty - try testing.expect(!t.isDirty(.{ .screen = .{ .x = 4, .y = 0 } })); + // Both rows dirty because the cursor moved + try testing.expect(t.isDirty(.{ .screen = .{ .x = 4, .y = 0 } })); try testing.expect(t.isDirty(.{ .screen = .{ .x = 4, .y = 1 } })); { @@ -3809,9 +3812,9 @@ test "Terminal: linefeed and carriage return" { try t.linefeed(); - // LF should not mark row dirty - try testing.expect(!t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); - try testing.expect(!t.isDirty(.{ .screen = .{ .x = 0, .y = 1 } })); + // LF marks row dirty due to cursor movement + try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); + try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 1 } })); for ("world") |c| try t.print(c); try testing.expectEqual(@as(usize, 1), t.screen.cursor.y); @@ -3832,8 +3835,8 @@ test "Terminal: linefeed unsets pending wrap" { try testing.expect(t.screen.cursor.pending_wrap == true); t.clearDirty(); try t.linefeed(); - try testing.expect(!t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); - try testing.expect(!t.isDirty(.{ .screen = .{ .x = 0, .y = 1 } })); + try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); + try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 1 } })); try testing.expect(t.screen.cursor.pending_wrap == false); } @@ -5648,9 +5651,7 @@ test "Terminal: index" { try t.index(); try t.print('A'); - // Only the row we write to is dirty. Moving the cursor itself - // does not make a row dirty. - try testing.expect(!t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); + try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); { @@ -5673,9 +5674,7 @@ test "Terminal: index from the bottom" { try t.index(); try t.print('B'); - // Only the row we write to is dirty. Moving the cursor itself - // does not make a row dirty. - try testing.expect(!t.isDirty(.{ .active = .{ .x = 0, .y = 3 } })); + try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 3 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 4 } })); { @@ -5726,7 +5725,7 @@ test "Terminal: index no scroll region, top of screen" { try t.index(); try t.print('X'); - try testing.expect(!t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); + try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); { @@ -5747,7 +5746,7 @@ test "Terminal: index bottom of primary screen" { try t.index(); try t.print('X'); - try testing.expect(!t.isDirty(.{ .active = .{ .x = 0, .y = 3 } })); + try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 3 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 4 } })); { @@ -5801,7 +5800,7 @@ test "Terminal: index inside scroll region" { try t.index(); try t.print('X'); - try testing.expect(!t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); + try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); {