mirror of
https://github.com/ghostty-org/ghostty.git
synced 2026-04-14 03:25:50 +00:00
Terminal: keep cross-boundary rows dirty in {insert,delete}Lines (#10290)
This fixes #10265 and thus also the remaining part of #9718 and likely #10250. The issue was that when using `insertLines` and `deleteLines` to generate scrolling in a region that spans a page boundary, rows that are replaced by a row from a different page lose their dirty flags in the clone operation, since the flag is part of the data that gets cloned. The solution is to set the dirty flag again after the clone, just like the non-cloning branch does after the pointer swap. **AI disclosure:** Amp is the MVP here. I prompted it with the hypothesis I developed in #10265 (that this happens when the scrolling region spans a page boundary), supplemented with insight I gained from perusing asciicast files (that the offending scrolling operations are always triggered by `CSI 1 L` or `CSI 1 M`, that is, `Terminal.insertLines` or `Terminal.deleteLines`). Amp figured out the rest and drafted the fix and tests. For free! I cleaned up the tests and then pushed back a bit against the logic behind the fix, which led to a better understanding and what I think is a more appropriate fix. I can explain the details if there's interest, or people can just skim the thread here: https://ampcode.com/threads/T-019bb0d6-5334-744a-b78a-7c997ec1fade.
This commit is contained in:
@@ -1602,9 +1602,6 @@ pub fn insertLines(self: *Terminal, count: usize) void {
|
||||
const cur_rac = cur_p.rowAndCell();
|
||||
const cur_row: *Row = cur_rac.row;
|
||||
|
||||
// Mark the row as dirty
|
||||
cur_p.markDirty();
|
||||
|
||||
// If this is one of the lines we need to shift, do so
|
||||
if (y > adjusted_count) {
|
||||
const off_p = cur_p.up(adjusted_count).?;
|
||||
@@ -1699,9 +1696,6 @@ pub fn insertLines(self: *Terminal, count: usize) void {
|
||||
dst_row.* = src_row.*;
|
||||
src_row.* = dst;
|
||||
|
||||
// Make sure the row is marked as dirty though.
|
||||
dst_row.dirty = true;
|
||||
|
||||
// Ensure what we did didn't corrupt the page
|
||||
cur_p.node.data.assertIntegrity();
|
||||
} else {
|
||||
@@ -1728,6 +1722,9 @@ pub fn insertLines(self: *Terminal, count: usize) void {
|
||||
);
|
||||
}
|
||||
|
||||
// Mark the row as dirty
|
||||
cur_p.markDirty();
|
||||
|
||||
// We have successfully processed a line.
|
||||
y -= 1;
|
||||
// Move our pin up to the next row.
|
||||
@@ -1805,9 +1802,6 @@ pub fn deleteLines(self: *Terminal, count: usize) void {
|
||||
const cur_rac = cur_p.rowAndCell();
|
||||
const cur_row: *Row = cur_rac.row;
|
||||
|
||||
// Mark the row as dirty
|
||||
cur_p.markDirty();
|
||||
|
||||
// If this is one of the lines we need to shift, do so
|
||||
if (y < rem - adjusted_count) {
|
||||
const off_p = cur_p.down(adjusted_count).?;
|
||||
@@ -1897,9 +1891,6 @@ pub fn deleteLines(self: *Terminal, count: usize) void {
|
||||
dst_row.* = src_row.*;
|
||||
src_row.* = dst;
|
||||
|
||||
// Make sure the row is marked as dirty though.
|
||||
dst_row.dirty = true;
|
||||
|
||||
// Ensure what we did didn't corrupt the page
|
||||
cur_p.node.data.assertIntegrity();
|
||||
} else {
|
||||
@@ -1926,6 +1917,9 @@ pub fn deleteLines(self: *Terminal, count: usize) void {
|
||||
);
|
||||
}
|
||||
|
||||
// Mark the row as dirty
|
||||
cur_p.markDirty();
|
||||
|
||||
// We have successfully processed a line.
|
||||
y += 1;
|
||||
// Move our pin down to the next row.
|
||||
@@ -5419,6 +5413,52 @@ test "Terminal: insertLines top/bottom scroll region" {
|
||||
}
|
||||
}
|
||||
|
||||
test "Terminal: insertLines across page boundary marks all shifted rows dirty" {
|
||||
const alloc = testing.allocator;
|
||||
var t = try init(alloc, .{ .rows = 5, .cols = 10, .max_scrollback = 1024 });
|
||||
defer t.deinit(alloc);
|
||||
|
||||
const first_page = t.screens.active.pages.pages.first.?;
|
||||
const first_page_nrows = first_page.data.capacity.rows;
|
||||
|
||||
// Fill up the first page minus 3 rows
|
||||
for (0..first_page_nrows - 3) |_| try t.linefeed();
|
||||
|
||||
// Add content that will cross a page boundary
|
||||
try t.printString("1AAAA");
|
||||
t.carriageReturn();
|
||||
try t.linefeed();
|
||||
try t.printString("2BBBB");
|
||||
t.carriageReturn();
|
||||
try t.linefeed();
|
||||
try t.printString("3CCCC");
|
||||
t.carriageReturn();
|
||||
try t.linefeed();
|
||||
try t.printString("4DDDD");
|
||||
t.carriageReturn();
|
||||
try t.linefeed();
|
||||
try t.printString("5EEEE");
|
||||
|
||||
// Verify we now have a second page
|
||||
try testing.expect(first_page.next != null);
|
||||
|
||||
t.setCursorPos(1, 1);
|
||||
t.clearDirty();
|
||||
t.insertLines(1);
|
||||
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } }));
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } }));
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 2 } }));
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 3 } }));
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 4 } }));
|
||||
|
||||
{
|
||||
const str = try t.plainString(testing.allocator);
|
||||
defer testing.allocator.free(str);
|
||||
try testing.expectEqualStrings("\n1AAAA\n2BBBB\n3CCCC\n4DDDD", str);
|
||||
}
|
||||
}
|
||||
|
||||
test "Terminal: insertLines (legacy test)" {
|
||||
const alloc = testing.allocator;
|
||||
var t = try init(alloc, .{ .cols = 2, .rows = 5 });
|
||||
@@ -8042,6 +8082,52 @@ test "Terminal: deleteLines colors with bg color" {
|
||||
}
|
||||
}
|
||||
|
||||
test "Terminal: deleteLines across page boundary marks all shifted rows dirty" {
|
||||
const alloc = testing.allocator;
|
||||
var t = try init(alloc, .{ .rows = 5, .cols = 10, .max_scrollback = 1024 });
|
||||
defer t.deinit(alloc);
|
||||
|
||||
const first_page = t.screens.active.pages.pages.first.?;
|
||||
const first_page_nrows = first_page.data.capacity.rows;
|
||||
|
||||
// Fill up the first page minus 3 rows
|
||||
for (0..first_page_nrows - 3) |_| try t.linefeed();
|
||||
|
||||
// Add content that will cross a page boundary
|
||||
try t.printString("1AAAA");
|
||||
t.carriageReturn();
|
||||
try t.linefeed();
|
||||
try t.printString("2BBBB");
|
||||
t.carriageReturn();
|
||||
try t.linefeed();
|
||||
try t.printString("3CCCC");
|
||||
t.carriageReturn();
|
||||
try t.linefeed();
|
||||
try t.printString("4DDDD");
|
||||
t.carriageReturn();
|
||||
try t.linefeed();
|
||||
try t.printString("5EEEE");
|
||||
|
||||
// Verify we now have a second page
|
||||
try testing.expect(first_page.next != null);
|
||||
|
||||
t.setCursorPos(1, 1);
|
||||
t.clearDirty();
|
||||
t.deleteLines(1);
|
||||
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } }));
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } }));
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 2 } }));
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 3 } }));
|
||||
try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 4 } }));
|
||||
|
||||
{
|
||||
const str = try t.plainString(testing.allocator);
|
||||
defer testing.allocator.free(str);
|
||||
try testing.expectEqualStrings("2BBBB\n3CCCC\n4DDDD\n5EEEE", str);
|
||||
}
|
||||
}
|
||||
|
||||
test "Terminal: deleteLines (legacy)" {
|
||||
const alloc = testing.allocator;
|
||||
var t = try init(alloc, .{ .cols = 80, .rows = 80 });
|
||||
|
||||
Reference in New Issue
Block a user