terminal: preserve multi-point grapheme clusters on scrollback deletion

This codepath was not previously tested (an accident). Upon testing
this codepath its clear to see the logic was incorrect. When we have to
remove rows from our scrollback to fit new rows in the circular buffer,
we have to delete graphemes, but we were deleting them from the wrong
row offset.

For the row offset, we previously used the _active_ screen but the
proper offset is the _full_ screen. Tests verify.
This commit is contained in:
Mitchell Hashimoto
2024-01-25 09:35:56 -08:00
parent 066202b378
commit 2d612ab168
2 changed files with 27 additions and 1 deletions

View File

@ -2174,7 +2174,7 @@ fn scrollDelta(self: *Screen, delta: isize, viewport_only: bool) Allocator.Error
if (self.graphemes.count() > 0) { if (self.graphemes.count() > 0) {
var y: usize = 0; var y: usize = 0;
while (y < rows_to_delete) : (y += 1) { while (y < rows_to_delete) : (y += 1) {
const row = self.getRow(.{ .active = y }); const row = self.getRow(.{ .screen = y });
if (row.storage[0].header.flags.grapheme) row.clear(.{}); if (row.storage[0].header.flags.grapheme) row.clear(.{});
} }
} }
@ -4306,6 +4306,7 @@ test "Screen: history region with scrollback" {
try testing.expectEqualStrings(expected, contents); try testing.expectEqualStrings(expected, contents);
} }
} }
test "Screen: row copy" { test "Screen: row copy" {
const testing = std.testing; const testing = std.testing;
const alloc = testing.allocator; const alloc = testing.allocator;

View File

@ -7254,3 +7254,28 @@ test "Terminal: printAttributes" {
try testing.expectEqualStrings("0", buf); try testing.expectEqualStrings("0", buf);
} }
} }
test "Terminal: preserve grapheme cluster on large scrollback" {
const alloc = testing.allocator;
var t = try init(alloc, 5, 3);
defer t.deinit(alloc);
// This is the label emoji + the VS16 variant selector
const label = "\u{1F3F7}\u{FE0F}";
// This bug required a certain behavior around scrollback interacting
// with the circular buffer that we use at the time of writing this test.
// Mainly, we want to verify that in certain scroll scenarios we preserve
// grapheme clusters. This test is admittedly somewhat brittle but we
// should keep it around to prevent this regression.
for (0..t.screen.max_scrollback * 2) |_| {
try t.printString(label ++ "\n");
}
try t.scrollViewport(.{ .delta = -1 });
{
const str = try t.screen.testString(alloc, .viewport);
defer testing.allocator.free(str);
try testing.expectEqualStrings("🏷️\n🏷️\n🏷️", str);
}
}