Skip to content

Commit 62af0c0

Browse files
authored
terminal: clear correct row on index operation in certain edge cases (#7093)
Fixes #7066 This fixes an issue where under certain conditions (expanded below), we would not clear the correct row, leading to the screen having duplicate data. This was triggered by a page state of the following: ``` +----------+ = PAGE 0 ... : : 4305 |1ABCD00000| 4306 |2EFGH00000| :^ : = PIN 0 +-------------+ ACTIVE 4307 |3IJKL00000| | 0 +----------+ : +----------+ : = PAGE 1 0 | | | 1 1 | | | 2 +----------+ : +-------------+ ``` Namely, the cursor had to NOT be on the last row of the first page, but somewhere on the first page. Then, when an `index` (LF) operation was performed the result would look like this: ``` +----------+ = PAGE 0 ... : : 4305 |1ABCD00000| 4306 |2EFGH00000| +-------------+ ACTIVE 4307 |3IJKL00000| | 0 :^ : : = PIN 0 +----------+ : +----------+ : = PAGE 1 0 |3IJKL00000| | 1 1 | | | 2 +----------+ : +-------------+ ``` The `3IJKL` line was duplicated. What was happening here is that we performed the index operation correctly but failed to clear the cursor line as expected. This is because we were always clearing the first row in the page instead of the row of the cursor. Test added.
2 parents 4aa875b + da6b478 commit 62af0c0

File tree

1 file changed

+90
-2
lines changed

1 file changed

+90
-2
lines changed

src/terminal/Screen.zig

+90-2
Original file line numberDiff line numberDiff line change
@@ -924,8 +924,8 @@ fn cursorScrollAboveRotate(self: *Screen) !void {
924924
fastmem.rotateOnceR(Row, cur_rows[self.cursor.page_pin.y..cur_page.size.rows]);
925925
self.clearCells(
926926
cur_page,
927-
&cur_rows[0],
928-
cur_page.getCells(&cur_rows[0]),
927+
&cur_rows[self.cursor.page_pin.y],
928+
cur_page.getCells(&cur_rows[self.cursor.page_pin.y]),
929929
);
930930

931931
// Set all the rows we rotated and cleared dirty
@@ -1256,6 +1256,17 @@ pub fn clearCells(
12561256
self.assertIntegrity();
12571257
}
12581258

1259+
if (comptime std.debug.runtime_safety) {
1260+
// Our row and cells should be within the page.
1261+
const page_rows = page.rows.ptr(page.memory.ptr);
1262+
assert(@intFromPtr(row) >= @intFromPtr(&page_rows[0]));
1263+
assert(@intFromPtr(row) <= @intFromPtr(&page_rows[page.size.rows - 1]));
1264+
1265+
const row_cells = page.getCells(row);
1266+
assert(@intFromPtr(&cells[0]) >= @intFromPtr(&row_cells[0]));
1267+
assert(@intFromPtr(&cells[cells.len - 1]) <= @intFromPtr(&row_cells[row_cells.len - 1]));
1268+
}
1269+
12591270
// If this row has graphemes, then we need go through a slow path
12601271
// and delete the cell graphemes.
12611272
if (row.grapheme) {
@@ -4818,6 +4829,83 @@ test "Screen: scroll above creates new page" {
48184829
try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 2 } }));
48194830
}
48204831

4832+
test "Screen: scroll above with cursor on non-final row" {
4833+
const testing = std.testing;
4834+
const alloc = testing.allocator;
4835+
4836+
var s = try init(alloc, 10, 4, 10);
4837+
defer s.deinit();
4838+
4839+
// Get the cursor to be 2 rows above a new page
4840+
const first_page_size = s.pages.pages.first.?.data.capacity.rows;
4841+
s.pages.pages.first.?.data.pauseIntegrityChecks(true);
4842+
for (0..first_page_size - 3) |_| try s.testWriteString("\n");
4843+
s.pages.pages.first.?.data.pauseIntegrityChecks(false);
4844+
4845+
// Write 3 lines of text, forcing the last line into the first
4846+
// row of a new page. Move our cursor onto the previous page.
4847+
try s.setAttribute(.{ .direct_color_bg = .{ .r = 155 } });
4848+
try s.testWriteString("1AB\n2BC\n3DE\n4FG");
4849+
s.cursorAbsolute(0, 1);
4850+
s.pages.clearDirty();
4851+
4852+
// Ensure we're still on the first page. So our cursor is on the first
4853+
// page but we have two pages of data.
4854+
try testing.expect(s.cursor.page_pin.node == s.pages.pages.first.?);
4855+
4856+
// +----------+ = PAGE 0
4857+
// ... : :
4858+
// +-------------+ ACTIVE
4859+
// 4305 |1AB0000000| | 0
4860+
// 4306 |2BC0000000| | 1
4861+
// :^ : : = PIN 0
4862+
// 4307 |3DE0000000| | 2
4863+
// +----------+ :
4864+
// +----------+ : = PAGE 1
4865+
// 0 |4FG0000000| | 3
4866+
// +----------+ :
4867+
// +-------------+
4868+
try s.cursorScrollAbove();
4869+
4870+
// +----------+ = PAGE 0
4871+
// ... : :
4872+
// 4305 |1AB0000000|
4873+
// +-------------+ ACTIVE
4874+
// 4306 |2BC0000000| | 0
4875+
// 4307 | | | 1
4876+
// :^ : : = PIN 0
4877+
// +----------+ :
4878+
// +----------+ : = PAGE 1
4879+
// 0 |3DE0000000| | 2
4880+
// 1 |4FG0000000| | 3
4881+
// +----------+ :
4882+
// +-------------+
4883+
// try s.pages.diagram(std.io.getStdErr().writer());
4884+
4885+
{
4886+
const contents = try s.dumpStringAlloc(alloc, .{ .viewport = .{} });
4887+
defer alloc.free(contents);
4888+
try testing.expectEqualStrings("2BC\n\n3DE\n4FG", contents);
4889+
}
4890+
{
4891+
const list_cell = s.pages.getCell(.{ .active = .{ .x = 0, .y = 1 } }).?;
4892+
const cell = list_cell.cell;
4893+
try testing.expect(cell.content_tag == .bg_color_rgb);
4894+
try testing.expectEqual(Cell.RGB{
4895+
.r = 155,
4896+
.g = 0,
4897+
.b = 0,
4898+
}, cell.content.color_rgb);
4899+
}
4900+
4901+
// Page 0's penultimate row is dirty because the cursor moved off of it.
4902+
try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 0 } }));
4903+
// Page 0's final row is dirty because it was cleared.
4904+
try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 1 } }));
4905+
// Page 1's row is dirty because it's new.
4906+
try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 2 } }));
4907+
}
4908+
48214909
test "Screen: scroll above no scrollback bottom of page" {
48224910
const testing = std.testing;
48234911
const alloc = testing.allocator;

0 commit comments

Comments
 (0)