native: draw a combining mark that starts a chunk (#1651)

A combining mark in another rope chunk than its base glyph was not
drawn. Edits, `TextBuffer.append`, and streamed text split chunks, so
typing U+0301 after `abc` in an editor showed `abc`, while the text held
`abc\u0301`. A chunk of only marks has no columns, and the draw loop
stopped at the chunk's last column, so it never drew the mark. A mark
that starts a chunk with more text was dropped when its base was in the
last column of the target, the viewport, or the scissor, because those
checks ran on the mark's column.

The draw loop now walks the chunk's bytes, and a zero-width cluster
joins the glyph this draw wrote before it as soon as it is read. That
glyph already passed the edge and scissor checks. The skip for chunks
past the right edge waits while such a glyph can still take marks. A
mark with no drawn glyph before it is still dropped: at line start,
after control notation, after a base that is scrolled out or clipped,
and after a truncation ellipsis, which now never takes a mark. A
truncated line's prefix keeps a mark-only chunk that follows its last
glyph. A mark that would change the cluster's width, such as U+FE0F
after a 1-cell glyph, is dropped instead of putting a 2-cell emoji in a
1-cell slot.

The test that attaches marks is now a table. Each row draws split chunks
and the same text in one chunk, through `TextBuffer.append` (controls
hidden) and through editor inserts (controls shown as notation). Rows
vary the x offset, viewport, scissor, wrap, truncation, selection, and
opacity, and name the cell that must hold the mark. Each of eleven
plausible breaks of the change (column loop, chunk skip, attach order,
base reset, ellipsis, prefix, width check) fails a row. In random tests,
split chunks now draw the same as one chunk in every case except joiners
(ZWJ, U+FE0F that widens a glyph), word wrap, and one truncation prefix
shape that main also has.

Fixes #1646

packages/native/src/buffer.zig

@@ -2392,7 +2392,8 @@ pub const OptimizedBuffer = struct {
                 };
                 const line_col_offset = vline.document_cell_offset;
 
-                if (currentX >= @as(i32, @intCast(self.width))) {
+                // A glyph in the last column can still take marks that start this chunk.
+                if (currentX >= @as(i32, @intCast(self.width)) and mark_base == null) {
                     document_cell_offset += vchunk.width_cols;
                     currentX += @intCast(vchunk.width_cols);
                     continue;
@@ -2407,7 +2408,7 @@ pub const OptimizedBuffer = struct {
                     special_idx += 1;
                 }
 
-                text_buffer_loop: while (byte_offset < byte_end and col < col_end) {
+                text_buffer_loop: while (byte_offset < byte_end) {
                     const at_special = special_idx < render_clusters.len and render_clusters[special_idx].byte_start == byte_offset;
 
                     var grapheme_bytes: []const u8 = undefined;
@@ -2432,8 +2433,14 @@ pub const OptimizedBuffer = struct {
                         cluster_width_cols = if (ascii) 1 else utf8.getWidthAt(grapheme_bytes, 0, text_buffer.tabWidth(), text_buffer.controlDisplay(), text_buffer.widthMethod());
                         byte_offset = next_byte_offset;
                     }
-                    // Only a glyph that this iteration draws, or a zero-width cluster, sets it again.
-                    const previous_glyph = mark_base;
+                    // A combining mark joins the glyph before it, which passed the checks below; other
+                    // zero-width clusters keep that glyph. Only a glyph that this iteration draws sets it again.
+                    if (cluster_width_cols == 0) {
+                        if (mark_base != null and utf8.isCombiningMark(grapheme_bytes)) {
+                            mark_base = try self.attachCombiningMark(checked, grapheme_bytes, mark_base.?, @intCast(currentY), text_buffer.widthMethod());
+                        }
+                        continue;
+                    }
                     mark_base = null;
 
                     if (rendered_col_in_vline < horizontal_offset) {
@@ -2616,17 +2623,6 @@ pub const OptimizedBuffer = struct {
                         }
                     }
 
-                    if (cluster_width_cols == 0) {
-                        // A combining mark joins the glyph before it; other zero-width clusters keep it.
-                        if (previous_glyph) |glyph| {
-                            mark_base = if (utf8.isCombiningMark(grapheme_bytes))
-                                try self.attachCombiningMark(checked, grapheme_bytes, glyph, @intCast(currentY))
-                            else
-                                glyph;
-                        }
-                        continue;
-                    }
-
                     var drawFg = finalFg;
                     var drawBg = finalBg;
                     const drawAttributes = finalAttributes;
@@ -2727,6 +2723,8 @@ pub const OptimizedBuffer = struct {
                     rendered_col_in_vline += cluster_width_cols;
                     col += cluster_width_cols;
                 }
+                // The ellipsis stands for dropped text, so the marks after it have no base glyph.
+                if (chunk == &layout_view.ellipsis_chunk) mark_base = null;
             }
 
             const is_last_vline_of_logical_line = (slice_idx + 1 >= virtual_lines[firstVisibleLine..lastPossibleLine].len) or
@@ -2746,8 +2744,8 @@ pub const OptimizedBuffer = struct {
     /// Combines a zero-width mark with the glyph that this draw wrote just before it, as if both
     /// were one cluster. The cell keeps its colors, which already have opacity applied.
     /// Returns the combined glyph, or null when the cell does not hold that glyph or cannot
-    /// hold the combination.
-    fn attachCombiningMark(self: *OptimizedBuffer, comptime checked: bool, mark: []const u8, base: MarkBase, y: u32) !?MarkBase {
+    /// hold the combination in the same cells (U+FE0F can widen a cluster beyond its layout).
+    fn attachCombiningMark(self: *OptimizedBuffer, comptime checked: bool, mark: []const u8, base: MarkBase, y: u32, width_method: utf8.WidthMethod) !?MarkBase {
         const index = self.coordsToIndex(base.x, y);
         // A blended space keeps the glyph under it, which this draw did not write.
         if (self.buffer.char[index] != base.char) return null;
@@ -2765,6 +2763,7 @@ pub const OptimizedBuffer = struct {
         if (base_bytes.len + mark.len > combined.len) return null;
         @memcpy(combined[0..base_bytes.len], base_bytes);
         @memcpy(combined[base_bytes.len..][0..mark.len], mark);
+        if (utf8.getGraphemeWidthAt(combined[0 .. base_bytes.len + mark.len], 0, 1, .hidden, width_method) != gp.encodedCharWidth(base.char)) return null;
         const id = self.acquireGlyph(combined[0 .. base_bytes.len + mark.len]) catch |err| {
             if (checked) return err;
             self.logger.warn("Failed to allocate grapheme: {}", .{err});

packages/native/src/tests/buffer_test.zig

@@ -1136,50 +1136,101 @@ test "OptimizedBuffer text views attach a combining mark only to the glyph drawn
     defer pools.deinit();
     const fg = ansi.rgbColor(200, 100, 50, 255);
     const bg = ansi.rgbColor(10, 20, 30, 255);
-    // A mark in its own chunk combines with the glyph before it, like the same cluster in one chunk.
-    for ([_]f32{ 1.0, 0.5 }) |opacity| {
-        const one_chunk = try OptimizedBuffer.init(std.testing.allocator, 4, 1, .{ .pool = &pools.graphemes, .link_pool = &pools.links });
+    const selected = ansi.rgbColor(0, 90, 0, 255);
+    // A mark in another chunk than its base glyph draws like the same text in one chunk. Text views
+    // append chunks and hide controls; editors insert them at the end and show controls as notation.
+    const Source = enum { append, insert };
+    const Row = struct {
+        chunks: []const []const u8,
+        x: i32 = 0,
+        viewport: text_buffer_view.Viewport = .{ .x = 0, .y = 0, .width = 4, .height = 2 },
+        wrap: bool = false,
+        scissor_width: u32 = 4,
+        truncate: bool = false,
+        /// The one-chunk text that draws the same cells, when it is not the joined chunks.
+        reference: ?[]const u8 = null,
+        /// Per source, the cell that holds the mark, or null when no drawn glyph comes before it.
+        mark: [2]?u32,
+    };
+    const rows = [_]Row{
+        .{ .chunks = &.{ "a", "\u{301}b" }, .mark = .{ 0, 0 } },
+        .{ .chunks = &.{ "abc", "\u{301}" }, .mark = .{ 2, 2 } },
+        .{ .chunks = &.{ "abc", "\u{301}d" }, .x = 1, .mark = .{ 3, 3 } },
+        .{ .chunks = &.{ "abc", "\u{301}d" }, .viewport = .{ .x = 1, .y = 0, .width = 2, .height = 2 }, .mark = .{ 1, 1 } },
+        .{ .chunks = &.{ "abc", "\u{301}d" }, .scissor_width = 3, .mark = .{ 2, 2 } },
+        .{ .chunks = &.{ "abc", "\u{301}d" }, .scissor_width = 2, .mark = .{ null, null } },
+        .{ .chunks = &.{ "abc\u{4e2d}", "\u{301}" }, .truncate = true, .mark = .{ null, null } },
+        .{ .chunks = &.{ "abcde", "\u{301}" }, .truncate = true, .mark = .{ 3, 3 } },
+        .{ .chunks = &.{ "abcd", "\u{301}e" }, .truncate = true, .mark = .{ null, null } },
+        .{ .chunks = &.{ "a", "\u{301}", "bcdef" }, .viewport = .{ .x = 0, .y = 0, .width = 5, .height = 2 }, .truncate = true, .mark = .{ 0, 0 } },
+        .{ .chunks = &.{ "\u{2714}", "\u{fe0f}" }, .reference = "\u{2714}", .mark = .{ null, null } },
+        .{ .chunks = &.{ "a\t", "\u{301}" }, .mark = .{ null, null } },
+        .{ .chunks = &.{ "abc", "\u{301}d" }, .viewport = .{ .x = 0, .y = 0, .width = 3, .height = 2 }, .wrap = true, .mark = .{ 2, 2 } },
+        .{ .chunks = &.{ "\u{4e2d}", "\u{301}", "\u{301}" }, .mark = .{ 0, 0 } },
+        .{ .chunks = &.{ "a\x01", "\u{301}" }, .mark = .{ 0, null } },
+        .{ .chunks = &.{ "ab\n", "\u{301}c" }, .mark = .{ null, null } },
+        .{ .chunks = &.{"\u{301}ab"}, .x = 1, .mark = .{ null, null } },
+        .{ .chunks = &.{ "x", "\u{301}ab" }, .x = 1, .viewport = .{ .x = 1, .y = 0, .width = 4, .height = 2 }, .mark = .{ null, null } },
+        .{ .chunks = &.{ "\u{4e2d}", "\u{301}ab" }, .x = 1, .viewport = .{ .x = 2, .y = 0, .width = 4, .height = 2 }, .mark = .{ null, null } },
+    };
+    for ([_]f32{ 1.0, 0.5 }) |opacity| for (std.enums.values(Source)) |source| for (rows) |row| {
+        // The editor scrolls to its cursor, so it cannot keep a viewport that starts right of column 0.
+        if (source == .insert and (row.viewport.x != 0 or row.truncate)) continue;
+        errdefer std.debug.print("opacity={d} source={t} chunks={any}\n", .{ opacity, source, row.chunks });
+        const text = try std.mem.concat(std.testing.allocator, u8, if (row.reference) |reference| &.{reference} else row.chunks);
+        defer std.testing.allocator.free(text);
+        const one_chunk = try OptimizedBuffer.init(std.testing.allocator, 4, 2, .{ .pool = &pools.graphemes, .link_pool = &pools.links });
         defer one_chunk.deinit();
-        const two_chunks = try OptimizedBuffer.init(std.testing.allocator, 4, 1, .{ .pool = &pools.graphemes, .link_pool = &pools.links });
-        defer two_chunks.deinit();
-        for ([_]*OptimizedBuffer{ one_chunk, two_chunks }, [_][]const []const u8{ &.{"a\u{301}b"}, &.{ "a", "\u{301}b" } }) |target, chunks| {
+        const chunks = try OptimizedBuffer.init(std.testing.allocator, 4, 2, .{ .pool = &pools.graphemes, .link_pool = &pools.links });
+        defer chunks.deinit();
+        for ([_]*OptimizedBuffer{ one_chunk, chunks }, [_][]const []const u8{ &.{text}, row.chunks }) |target, parts| {
             target.clear(ansi.rgbColor(0, 0, 0, 255), null);
-            const content = try TextBuffer.init(std.testing.allocator, &pools.graphemes, &pools.links, .unicode);
-            defer content.deinit();
-            const view = try TextBufferView.init(std.testing.allocator, content);
-            defer view.deinit();
-            try content.setText(chunks[0]);
-            for (chunks[1..]) |chunk| try content.append(chunk);
-            content.setDefaultFg(fg);
-            content.setDefaultBg(bg);
+            try target.pushScissorRect(0, 0, row.scissor_width, 2);
             try target.pushOpacity(opacity);
-            try target.drawTextBufferChecked(view, 0, 0);
+            switch (source) {
+                .append => {
+                    const content = try TextBuffer.init(std.testing.allocator, &pools.graphemes, &pools.links, .unicode);
+                    defer content.deinit();
+                    const view = try TextBufferView.init(std.testing.allocator, content);
+                    defer view.deinit();
+                    try content.setText(parts[0]);
+                    for (parts[1..]) |part| try content.append(part);
+                    content.setDefaultFg(fg);
+                    content.setDefaultBg(bg);
+                    view.setViewport(row.viewport);
+                    view.setTruncate(row.truncate);
+                    view.setSelection(1, 3, selected, null);
+                    if (row.wrap) view.setWrapMode(.char);
+                    if (row.wrap) view.setWrapWidth(row.viewport.width);
+                    try target.drawTextBufferChecked(view, row.x, 0);
+                },
+                .insert => {
+                    const content = try edit_buffer.EditBuffer.init(std.testing.allocator, &pools.graphemes, &pools.links, .unicode, null);
+                    defer content.deinit();
+                    const view = try editor_view.EditorView.init(std.testing.allocator, content, row.viewport.width, 2);
+                    defer view.deinit();
+                    try content.setText(parts[0]);
+                    for (parts[1..]) |part| {
+                        try content.setCursor(std.math.maxInt(u32), std.math.maxInt(u32));
+                        try content.insertText(part);
+                    }
+                    try content.setCursor(0, 0);
+                    content.getTextBuffer().setDefaultFg(fg);
+                    content.getTextBuffer().setDefaultBg(bg);
+                    if (row.wrap) view.setWrapMode(.char);
+                    view.setSelection(1, 3, selected, null);
+                    try target.drawEditorViewChecked(view, row.x, 0);
+                },
+            }
         }
-        try std.testing.expectEqualStrings("a\u{301}", try pools.graphemes.get(gp.graphemeIdFromChar(two_chunks.buffer.char[0])));
-        try expectSameCells(one_chunk, two_chunks);
-    }
-
-    // A mark whose base glyph this draw did not write leaves the cell left of the view unchanged.
-    const target = try OptimizedBuffer.init(std.testing.allocator, 6, 1, .{ .pool = &pools.graphemes, .link_pool = &pools.links });
-    defer target.deinit();
-    const Case = struct { chunks: []const []const u8, scroll_x: u32 };
-    for ([_]Case{
-        .{ .chunks = &.{"\u{301}ab"}, .scroll_x = 0 },
-        .{ .chunks = &.{ "x", "\u{301}ab" }, .scroll_x = 1 },
-        .{ .chunks = &.{ "\u{4e2d}", "\u{301}ab" }, .scroll_x = 2 },
-    }) |case| {
-        target.clear(ansi.rgbColor(0, 0, 0, 255), null);
-        target.set(1, 0, .{ .char = 'Z', .fg = fg, .bg = bg, .attributes = 0 });
-        const content = try TextBuffer.init(std.testing.allocator, &pools.graphemes, &pools.links, .unicode);
-        defer content.deinit();
-        const view = try TextBufferView.init(std.testing.allocator, content);
-        defer view.deinit();
-        try content.setText(case.chunks[0]);
-        for (case.chunks[1..]) |chunk| try content.append(chunk);
-        view.setViewport(.{ .x = case.scroll_x, .y = 0, .width = 4, .height = 1 });
-        try target.drawTextBufferChecked(view, 2, 0);
-        try std.testing.expectEqualSlices(u32, &.{ ' ', 'Z', 'a', 'b', ' ', ' ' }, target.buffer.char);
-    }
+        for (chunks.buffer.char, 0..) |char, index| {
+            const bytes = if (gp.isGraphemeChar(char)) try pools.graphemes.get(gp.graphemeIdFromChar(char)) else "";
+            const holds_mark = bytes.len > 0 and try std.unicode.utf8ByteSequenceLength(bytes[0]) < bytes.len;
+            try std.testing.expectEqual(if (row.mark[@intFromEnum(source)]) |cell| cell == index else false, holds_mark);
+        }
+        try expectSameCells(one_chunk, chunks);
+        try expectNoControlCells(chunks);
+    };
     try std.testing.expectEqual(0, pools.graphemes.interned_live_ids.count());
 }
 

packages/native/src/text-buffer-view.zig

@@ -1433,7 +1433,8 @@ pub const UnifiedTextBufferView = struct {
 
             var prefix_accumulated: u32 = 0;
             for (vline.chunks.items) |chunk| {
-                if (prefix_accumulated >= prefix_width) break;
+                // Marks right after the prefix belong to its last glyph.
+                if (prefix_accumulated >= prefix_width and chunk.width_cols != 0) break;
 
                 const space_left = prefix_width - prefix_accumulated;
                 if (chunk.width_cols <= space_left) {