From 40cac97c863ed12565c941aa33b17dcc39ac5d8c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 21 Mar 2024 21:26:29 -0700 Subject: [PATCH] terminal: insertChars/deleteChars needs to account properly --- src/terminal/Terminal.zig | 40 +-------------- src/terminal/page.zig | 105 ++++++++++++++++---------------------- 2 files changed, 46 insertions(+), 99 deletions(-) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index c8cad3780..fc441e89e 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1544,25 +1544,7 @@ pub fn insertBlanks(self: *Terminal, count: usize) void { while (@intFromPtr(x) >= @intFromPtr(left)) : (x -= 1) { const src: *Cell = @ptrCast(x); const dst: *Cell = @ptrCast(x + adjusted_count); - - // If the destination has graphemes we need to delete them. - // Graphemes are stored by cell offset so we have to do this - // now before we move. - if (dst.hasGrapheme()) { - page.clearGrapheme(self.screen.cursor.page_row, dst); - } - - // Copy our src to our dst - const old_dst = dst.*; - dst.* = src.*; - src.* = old_dst; - - // If the original source (now copied to dst) had graphemes, - // we have to move them since they're stored by cell offset. - if (dst.hasGrapheme()) { - assert(!src.hasGrapheme()); - page.moveGraphemeWithinRow(src, dst); - } + page.swapCells(src, dst); } } @@ -1636,25 +1618,7 @@ pub fn deleteChars(self: *Terminal, count: usize) void { while (@intFromPtr(x) <= @intFromPtr(right)) : (x += 1) { const src: *Cell = @ptrCast(x + count); const dst: *Cell = @ptrCast(x); - - // If the destination has graphemes we need to delete them. - // Graphemes are stored by cell offset so we have to do this - // now before we move. - if (dst.hasGrapheme()) { - page.clearGrapheme(self.screen.cursor.page_row, dst); - } - - // Copy our src to our dst - const old_dst = dst.*; - dst.* = src.*; - src.* = old_dst; - - // If the original source (now copied to dst) had graphemes, - // we have to move them since they're stored by cell offset. - if (dst.hasGrapheme()) { - assert(!src.hasGrapheme()); - page.moveGraphemeWithinRow(src, dst); - } + page.swapCells(src, dst); } } diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 01b04b1fe..a6245afcc 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -498,6 +498,8 @@ pub const Page = struct { // Required for moveGrapheme assertions dst.content_tag = .codepoint; self.moveGrapheme(src, dst); + src.content_tag = .codepoint; + dst.content_tag = .codepoint_grapheme; } // The destination row must be marked @@ -519,6 +521,43 @@ pub const Page = struct { } } + /// Swap two cells within the same row as quickly as possible. + pub fn swapCells( + self: *Page, + src: *Cell, + dst: *Cell, + ) void { + defer self.assertIntegrity(); + + // Graphemes are keyed by cell offset so we do have to move them. + // We do this first so that all our grapheme state is correct. + if (src.hasGrapheme() or dst.hasGrapheme()) { + if (src.hasGrapheme() and !dst.hasGrapheme()) { + self.moveGrapheme(src, dst); + } else if (!src.hasGrapheme() and dst.hasGrapheme()) { + self.moveGrapheme(dst, src); + } else { + // Both had graphemes, so we have to manually swap + const src_offset = getOffset(Cell, self.memory, src); + const dst_offset = getOffset(Cell, self.memory, dst); + var map = self.grapheme_map.map(self.memory); + const src_entry = map.getEntry(src_offset).?; + const dst_entry = map.getEntry(dst_offset).?; + const src_value = src_entry.value_ptr.*; + const dst_value = dst_entry.value_ptr.*; + src_entry.value_ptr.* = dst_value; + dst_entry.value_ptr.* = src_value; + } + } + + // Copy the metadata. Note that we do NOT have to worry about + // styles because styles are keyed by ID and we're preserving the + // exact ref count and row state here. + const old_dst = dst.*; + dst.* = src.*; + src.* = old_dst; + } + /// Clear the cells in the given row. This will reclaim memory used /// by graphemes and styles. Note that if the style cleared is still /// active, Page cannot know this and it will still be ref counted down. @@ -630,7 +669,11 @@ pub const Page = struct { /// Move the graphemes from one cell to another. This can't fail /// because we avoid any allocations since we're just moving data. - pub fn moveGrapheme(self: *Page, src: *Cell, dst: *Cell) void { + /// + /// WARNING: This will NOT change the content_tag on the cells because + /// there are scenarios where we want to move graphemes without changing + /// the content tag. Callers beware but assertIntegrity should catch this. + fn moveGrapheme(self: *Page, src: *Cell, dst: *Cell) void { if (comptime std.debug.runtime_safety) { assert(src.hasGrapheme()); assert(!dst.hasGrapheme()); @@ -643,8 +686,6 @@ pub const Page = struct { const value = entry.value_ptr.*; map.removeByPtr(entry.key_ptr); map.putAssumeCapacity(dst_offset, value); - src.content_tag = .codepoint; - dst.content_tag = .codepoint_grapheme; } /// Clear the graphemes for a given cell. @@ -678,28 +719,6 @@ pub const Page = struct { return self.grapheme_map.map(self.memory).count(); } - /// Move graphemes to another cell in the same row. - pub fn moveGraphemeWithinRow(self: *Page, src: *Cell, dst: *Cell) void { - // Note: we don't assert src has graphemes here because one of - // the places we call this is from insertBlanks where the cells have - // already swapped cell data but not grapheme data. - - defer self.assertIntegrity(); - - // Get our entry in the map, which must exist - const src_offset = getOffset(Cell, self.memory, src); - var map = self.grapheme_map.map(self.memory); - const entry = map.getEntry(src_offset).?; - const value = entry.value_ptr.*; - - // Remove the entry so we know we have space - map.removeByPtr(entry.key_ptr); - - // Add the entry for the new cell - const dst_offset = getOffset(Cell, self.memory, dst); - map.putAssumeCapacity(dst_offset, value); - } - pub const Layout = struct { total_size: usize, rows_start: usize, @@ -1717,39 +1736,3 @@ test "Page verifyIntegrity styles ref count mismatch" { page.verifyIntegrity(testing.allocator), ); } - -test "Page verifyIntegrity styles extra" { - var page = try Page.init(.{ - .cols = 10, - .rows = 10, - .styles = 8, - }); - defer page.deinit(); - - // Upsert a style we'll use - const md = try page.styles.upsert(page.memory, .{ .flags = .{ - .bold = true, - } }); - - const md2 = try page.styles.upsert(page.memory, .{ .flags = .{ - .italic = true, - } }); - md2.ref += 1; - - // Write - for (0..page.size.cols) |x| { - const rac = page.getRowAndCell(x, 0); - rac.row.styled = true; - rac.cell.* = .{ - .content_tag = .codepoint, - .content = .{ .codepoint = @intCast(x + 1) }, - .style_id = md.id, - }; - md.ref += 1; - } - - try testing.expectError( - Page.IntegrityError.InvalidStyleCount, - page.verifyIntegrity(testing.allocator), - ); -}