terminal: all cursor movement needs to mark the old and new page dirty

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
+    // Scrollback also dirty because cursor moved from there
-    try testing.expect(!s.pages.isDirty(.{ .screen = .{ .x = 0, .y = 0 } }));
+    try testing.expect(s.pages.isDirty(.{ .screen = .{ .x = 0, .y = 0 } }));
 
     s.scroll(.{ .delta_row = -1 });
     {

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
+    // 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 = 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
+    // 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 = 0 } }));
-    try testing.expect(!t.isDirty(.{ .screen = .{ .x = 0, .y = 1 } }));
+    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 = 0 } }));
-    try testing.expect(!t.isDirty(.{ .screen = .{ .x = 0, .y = 1 } }));
+    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
+    try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } }));
-    // 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 = 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
+    try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 3 } }));
-    // 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 = 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 } }));
 
     {