From f9e2cb6aec854a05f1f5ad3793cd3c800d3bd073 Mon Sep 17 00:00:00 2001
From: Qwerasd <qwerasd205@users.noreply.github.com>
Date: Wed, 2 Oct 2024 16:38:31 -0400
Subject: [PATCH 1/3] fix(renderer): use 1-wide ul/st chars, ignore null shaper
 cells

This makes sure that underline styles are consistent and not stretched,
and avoids rendering overlapping text decorations or extraneous
background cells for the right halves of wide chars.
---
 src/renderer/Metal.zig  | 42 ++++++++++++++++++++++++++---
 src/renderer/OpenGL.zig | 58 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 92 insertions(+), 8 deletions(-)

diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig
index 1790711c8..0fda9cbe4 100644
--- a/src/renderer/Metal.zig
+++ b/src/renderer/Metal.zig
@@ -2274,6 +2274,10 @@ fn rebuildCells(
             };
 
             for (shaper_cells) |shaper_cell| {
+                // The shaper can emit null glyphs representing the right half
+                // of wide characters, we don't need to do anything with them.
+                if (shaper_cell.glyph_index == null) continue;
+
                 const coord: terminal.Coordinate = .{
                     .x = shaper_cell.x,
                     .y = y,
@@ -2541,7 +2545,7 @@ fn updateCell(
             font.sprite_index,
             @intFromEnum(sprite),
             .{
-                .cell_width = if (cell.wide == .wide) 2 else 1,
+                .cell_width = 1,
                 .grid_metrics = self.grid_metrics,
             },
         );
@@ -2551,7 +2555,7 @@ fn updateCell(
         try self.cells.add(self.alloc, .underline, .{
             .mode = .fg,
             .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
-            .constraint_width = cell.gridWidth(),
+            .constraint_width = 1,
             .color = .{ color.r, color.g, color.b, alpha },
             .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y },
             .glyph_size = .{ render.glyph.width, render.glyph.height },
@@ -2560,6 +2564,21 @@ fn updateCell(
                 @intCast(render.glyph.offset_y),
             },
         });
+        // If it's a wide cell we need to underline the right half as well.
+        if (cell.gridWidth() > 1 and coord.x < self.cells.size.columns - 1) {
+            try self.cells.add(self.alloc, .underline, .{
+                .mode = .fg,
+                .grid_pos = .{ @intCast(coord.x + 1), @intCast(coord.y) },
+                .constraint_width = 1,
+                .color = .{ color.r, color.g, color.b, alpha },
+                .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y },
+                .glyph_size = .{ render.glyph.width, render.glyph.height },
+                .bearings = .{
+                    @intCast(render.glyph.offset_x),
+                    @intCast(render.glyph.offset_y),
+                },
+            });
+        }
     }
 
     // If the shaper cell has a glyph, draw it.
@@ -2611,7 +2630,7 @@ fn updateCell(
             font.sprite_index,
             @intFromEnum(font.Sprite.strikethrough),
             .{
-                .cell_width = if (cell.wide == .wide) 2 else 1,
+                .cell_width = 1,
                 .grid_metrics = self.grid_metrics,
             },
         );
@@ -2619,7 +2638,7 @@ fn updateCell(
         try self.cells.add(self.alloc, .strikethrough, .{
             .mode = .fg,
             .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
-            .constraint_width = cell.gridWidth(),
+            .constraint_width = 1,
             .color = .{ colors.fg.r, colors.fg.g, colors.fg.b, alpha },
             .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y },
             .glyph_size = .{ render.glyph.width, render.glyph.height },
@@ -2628,6 +2647,21 @@ fn updateCell(
                 @intCast(render.glyph.offset_y),
             },
         });
+        // If it's a wide cell we need to strike through the right half as well.
+        if (cell.gridWidth() > 1 and coord.x < self.cells.size.columns - 1) {
+            try self.cells.add(self.alloc, .strikethrough, .{
+                .mode = .fg,
+                .grid_pos = .{ @intCast(coord.x + 1), @intCast(coord.y) },
+                .constraint_width = 1,
+                .color = .{ colors.fg.r, colors.fg.g, colors.fg.b, alpha },
+                .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y },
+                .glyph_size = .{ render.glyph.width, render.glyph.height },
+                .bearings = .{
+                    @intCast(render.glyph.offset_x),
+                    @intCast(render.glyph.offset_y),
+                },
+            });
+        }
     }
 
     return true;
diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig
index 760721af3..7ea3f6ec8 100644
--- a/src/renderer/OpenGL.zig
+++ b/src/renderer/OpenGL.zig
@@ -1338,6 +1338,10 @@ pub fn rebuildCells(
             };
 
             for (shaper_cells) |shaper_cell| {
+                // The shaper can emit null glyphs representing the right half
+                // of wide characters, we don't need to do anything with them.
+                if (shaper_cell.glyph_index == null) continue;
+
                 // If this cell falls within our preedit range then we skip it.
                 // We do this so we don't have conflicting data on the same
                 // cell.
@@ -1779,7 +1783,7 @@ fn updateCell(
             font.sprite_index,
             @intFromEnum(sprite),
             .{
-                .cell_width = if (cell.wide == .wide) 2 else 1,
+                .cell_width = 1,
                 .grid_metrics = self.grid_metrics,
             },
         );
@@ -1790,7 +1794,7 @@ fn updateCell(
             .mode = .fg,
             .grid_col = @intCast(x),
             .grid_row = @intCast(y),
-            .grid_width = cell.gridWidth(),
+            .grid_width = 1,
             .glyph_x = render.glyph.atlas_x,
             .glyph_y = render.glyph.atlas_y,
             .glyph_width = render.glyph.width,
@@ -1806,6 +1810,29 @@ fn updateCell(
             .bg_b = bg[2],
             .bg_a = bg[3],
         });
+        // If it's a wide cell we need to underline the right half as well.
+        if (cell.gridWidth() > 1 and x < self.grid_size.columns - 1) {
+            try self.cells.append(self.alloc, .{
+                .mode = .fg,
+                .grid_col = @intCast(x + 1),
+                .grid_row = @intCast(y),
+                .grid_width = 1,
+                .glyph_x = render.glyph.atlas_x,
+                .glyph_y = render.glyph.atlas_y,
+                .glyph_width = render.glyph.width,
+                .glyph_height = render.glyph.height,
+                .glyph_offset_x = render.glyph.offset_x,
+                .glyph_offset_y = render.glyph.offset_y,
+                .r = color.r,
+                .g = color.g,
+                .b = color.b,
+                .a = alpha,
+                .bg_r = bg[0],
+                .bg_g = bg[1],
+                .bg_b = bg[2],
+                .bg_a = bg[3],
+            });
+        }
     }
 
     // If the shaper cell has a glyph, draw it.
@@ -1866,7 +1893,7 @@ fn updateCell(
             font.sprite_index,
             @intFromEnum(font.Sprite.strikethrough),
             .{
-                .cell_width = if (cell.wide == .wide) 2 else 1,
+                .cell_width = 1,
                 .grid_metrics = self.grid_metrics,
             },
         );
@@ -1875,7 +1902,7 @@ fn updateCell(
             .mode = .fg,
             .grid_col = @intCast(x),
             .grid_row = @intCast(y),
-            .grid_width = cell.gridWidth(),
+            .grid_width = 1,
             .glyph_x = render.glyph.atlas_x,
             .glyph_y = render.glyph.atlas_y,
             .glyph_width = render.glyph.width,
@@ -1891,6 +1918,29 @@ fn updateCell(
             .bg_b = bg[2],
             .bg_a = bg[3],
         });
+        // If it's a wide cell we need to strike through the right half as well.
+        if (cell.gridWidth() > 1 and x < self.grid_size.columns - 1) {
+            try self.cells.append(self.alloc, .{
+                .mode = .fg,
+                .grid_col = @intCast(x + 1),
+                .grid_row = @intCast(y),
+                .grid_width = 1,
+                .glyph_x = render.glyph.atlas_x,
+                .glyph_y = render.glyph.atlas_y,
+                .glyph_width = render.glyph.width,
+                .glyph_height = render.glyph.height,
+                .glyph_offset_x = render.glyph.offset_x,
+                .glyph_offset_y = render.glyph.offset_y,
+                .r = colors.fg.r,
+                .g = colors.fg.g,
+                .b = colors.fg.b,
+                .a = alpha,
+                .bg_r = bg[0],
+                .bg_g = bg[1],
+                .bg_b = bg[2],
+                .bg_a = bg[3],
+            });
+        }
     }
 
     return true;

From dfc0894d5dacd7cc272e45346fd7cf8ad0f58198 Mon Sep 17 00:00:00 2001
From: Qwerasd <qwerasd205@users.noreply.github.com>
Date: Wed, 2 Oct 2024 17:18:26 -0400
Subject: [PATCH 2/3] fix(renderer): make all decorations and combining marks
 visible under cursor

Metal needed to be changed to account for wide chars having decorations
on the right half and OpenGL needed to account for multiple glyphs being
under the cursor at once (decorations and combining marks) as well as
wide chars.
---
 src/renderer/Metal.zig          | 13 ++++++++++-
 src/renderer/OpenGL.zig         | 39 +++++++++++++++++++++++----------
 src/renderer/metal/shaders.zig  |  3 +++
 src/renderer/shaders/cell.metal |  7 +++++-
 4 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig
index 0fda9cbe4..0cbc1615e 100644
--- a/src/renderer/Metal.zig
+++ b/src/renderer/Metal.zig
@@ -631,6 +631,7 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal {
             .min_contrast = options.config.min_contrast,
             .cursor_pos = .{ std.math.maxInt(u16), std.math.maxInt(u16) },
             .cursor_color = undefined,
+            .wide_cursor = false,
         },
 
         // Fonts
@@ -2034,6 +2035,7 @@ pub fn setScreenSize(
         .min_contrast = old.min_contrast,
         .cursor_pos = old.cursor_pos,
         .cursor_color = old.cursor_color,
+        .wide_cursor = old.wide_cursor,
     };
 
     // Reset our cell contents if our grid size has changed.
@@ -2353,10 +2355,19 @@ fn rebuildCells(
 
         // If the cursor is visible then we set our uniforms.
         if (style == .block and screen.viewportIsBottom()) {
+            const wide = screen.cursor.page_cell.wide;
+
             self.uniforms.cursor_pos = .{
-                screen.cursor.x,
+                switch (wide) {
+                    .narrow, .spacer_head, .wide => screen.cursor.x,
+                    .spacer_tail => screen.cursor.x -| 1,
+                },
                 screen.cursor.y,
             };
+            self.uniforms.wide_cursor = switch (wide) {
+                .narrow, .spacer_head => false,
+                .wide, .spacer_tail => true,
+            };
 
             const uniform_color = if (self.cursor_invert) blk: {
                 const sty = screen.cursor.page_pin.style(screen.cursor.page_cell);
diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig
index 7ea3f6ec8..f9246aa22 100644
--- a/src/renderer/OpenGL.zig
+++ b/src/renderer/OpenGL.zig
@@ -1227,10 +1227,15 @@ pub fn rebuildCells(
         };
     } else null;
 
-    // This is the cell that has [mode == .fg] and is underneath our cursor.
-    // We keep track of it so that we can invert the colors so the character
-    // remains visible.
-    var cursor_cell: ?CellProgram.Cell = null;
+    // These are all the foreground cells underneath the cursor.
+    //
+    // We keep track of these so that we can invert the colors and move them
+    // in front of the block cursor so that the character remains visible.
+    //
+    // We init with a capacity of 4 to account for decorations such
+    // as underline and strikethrough, as well as combining chars.
+    var cursor_cells = try std.ArrayListUnmanaged(CellProgram.Cell).initCapacity(arena_alloc, 4);
+    defer cursor_cells.deinit(arena_alloc);
 
     if (rebuild) {
         switch (self.config.padding_color) {
@@ -1277,14 +1282,24 @@ pub fn rebuildCells(
         // the cell with the cursor.
         const start_i: usize = self.cells.items.len;
         defer if (cursor_row) {
-            // If we're on a wide spacer tail, then we want to look for
-            // the previous cell.
-            const screen_cell = row.cells(.all)[screen.cursor.x];
-            const x = screen.cursor.x - @intFromBool(screen_cell.wide == .spacer_tail);
+            const x = screen.cursor.x;
+            const wide = row.cells(.all)[x].wide;
+            const min_x = switch (wide) {
+                .narrow, .spacer_head, .wide => x,
+                .spacer_tail => x -| 1,
+            };
+            const max_x = switch (wide) {
+                .narrow, .spacer_head, .spacer_tail => x,
+                .wide => x +| 1,
+            };
             for (self.cells.items[start_i..]) |cell| {
-                if (cell.grid_col == x and cell.mode.isFg()) {
-                    cursor_cell = cell;
-                    break;
+                if (cell.grid_col < min_x or cell.grid_col > max_x) continue;
+                if (cell.mode.isFg()) {
+                    cursor_cells.append(arena_alloc, cell) catch {
+                        // We silently ignore if this fails because
+                        // worst case scenario some combining glyphs
+                        // aren't visible under the cursor '\_('-')_/'
+                    };
                 }
             }
         };
@@ -1422,7 +1437,7 @@ pub fn rebuildCells(
         };
 
         _ = try self.addCursor(screen, cursor_style, cursor_color);
-        if (cursor_cell) |*cell| {
+        for (cursor_cells.items) |*cell| {
             if (cell.mode.isFg() and cell.mode != .fg_color) {
                 const cell_color = if (self.cursor_invert) blk: {
                     const sty = screen.cursor.page_pin.style(screen.cursor.page_cell);
diff --git a/src/renderer/metal/shaders.zig b/src/renderer/metal/shaders.zig
index 2a202de30..c1c403848 100644
--- a/src/renderer/metal/shaders.zig
+++ b/src/renderer/metal/shaders.zig
@@ -137,6 +137,9 @@ pub const Uniforms = extern struct {
     cursor_pos: [2]u16 align(4),
     cursor_color: [4]u8 align(4),
 
+    // Whether the cursor is 2 cells wide.
+    wide_cursor: bool align(1),
+
     const PaddingExtend = packed struct(u8) {
         left: bool = false,
         right: bool = false,
diff --git a/src/renderer/shaders/cell.metal b/src/renderer/shaders/cell.metal
index b6af33824..44540e198 100644
--- a/src/renderer/shaders/cell.metal
+++ b/src/renderer/shaders/cell.metal
@@ -18,6 +18,7 @@ struct Uniforms {
   float min_contrast;
   ushort2 cursor_pos;
   uchar4 cursor_color;
+  bool wide_cursor;
 };
 
 //-------------------------------------------------------------------
@@ -293,7 +294,11 @@ vertex CellTextVertexOut cell_text_vertex(
   // If this cell is the cursor cell, then we need to change the color.
   if (
     in.mode != MODE_TEXT_CURSOR &&
-    in.grid_pos.x == uniforms.cursor_pos.x &&
+    (
+      in.grid_pos.x == uniforms.cursor_pos.x ||
+      uniforms.wide_cursor &&
+        in.grid_pos.x == uniforms.cursor_pos.x + 1
+    ) &&
     in.grid_pos.y == uniforms.cursor_pos.y
   ) {
     out.color = float4(uniforms.cursor_color) / 255.0f;

From 7aa2e2b24fa3b312750f3c4a998ab53d2c2e5d76 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <m@mitchellh.com>
Date: Wed, 2 Oct 2024 15:43:54 -0700
Subject: [PATCH 3/3] renderer: some tweaks

---
 src/renderer/Metal.zig          | 48 ++++++++++++---------------------
 src/renderer/metal/cell.zig     |  8 +++---
 src/renderer/metal/shaders.zig  |  2 +-
 src/renderer/shaders/cell.metal |  4 +--
 4 files changed, 24 insertions(+), 38 deletions(-)

diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig
index 0cbc1615e..ab64f8e4e 100644
--- a/src/renderer/Metal.zig
+++ b/src/renderer/Metal.zig
@@ -631,7 +631,7 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal {
             .min_contrast = options.config.min_contrast,
             .cursor_pos = .{ std.math.maxInt(u16), std.math.maxInt(u16) },
             .cursor_color = undefined,
-            .wide_cursor = false,
+            .cursor_wide = false,
         },
 
         // Fonts
@@ -2035,7 +2035,7 @@ pub fn setScreenSize(
         .min_contrast = old.min_contrast,
         .cursor_pos = old.cursor_pos,
         .cursor_color = old.cursor_color,
-        .wide_cursor = old.wide_cursor,
+        .cursor_wide = old.cursor_wide,
     };
 
     // Reset our cell contents if our grid size has changed.
@@ -2358,13 +2358,17 @@ fn rebuildCells(
             const wide = screen.cursor.page_cell.wide;
 
             self.uniforms.cursor_pos = .{
+                // If we are a spacer tail of a wide cell, our cursor needs
+                // to move back one cell. The saturate is to ensure we don't
+                // overflow but this shouldn't happen with well-formed input.
                 switch (wide) {
                     .narrow, .spacer_head, .wide => screen.cursor.x,
                     .spacer_tail => screen.cursor.x -| 1,
                 },
                 screen.cursor.y,
             };
-            self.uniforms.wide_cursor = switch (wide) {
+
+            self.uniforms.cursor_wide = switch (wide) {
                 .narrow, .spacer_head => false,
                 .wide, .spacer_tail => true,
             };
@@ -2563,7 +2567,7 @@ fn updateCell(
 
         const color = style.underlineColor(palette) orelse colors.fg;
 
-        try self.cells.add(self.alloc, .underline, .{
+        var gpu_cell: mtl_cell.Key.underline.CellType() = .{
             .mode = .fg,
             .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
             .constraint_width = 1,
@@ -2574,21 +2578,12 @@ fn updateCell(
                 @intCast(render.glyph.offset_x),
                 @intCast(render.glyph.offset_y),
             },
-        });
+        };
+        try self.cells.add(self.alloc, .underline, gpu_cell);
         // If it's a wide cell we need to underline the right half as well.
         if (cell.gridWidth() > 1 and coord.x < self.cells.size.columns - 1) {
-            try self.cells.add(self.alloc, .underline, .{
-                .mode = .fg,
-                .grid_pos = .{ @intCast(coord.x + 1), @intCast(coord.y) },
-                .constraint_width = 1,
-                .color = .{ color.r, color.g, color.b, alpha },
-                .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y },
-                .glyph_size = .{ render.glyph.width, render.glyph.height },
-                .bearings = .{
-                    @intCast(render.glyph.offset_x),
-                    @intCast(render.glyph.offset_y),
-                },
-            });
+            gpu_cell.grid_pos[0] = @intCast(coord.x + 1);
+            try self.cells.add(self.alloc, .underline, gpu_cell);
         }
     }
 
@@ -2646,7 +2641,7 @@ fn updateCell(
             },
         );
 
-        try self.cells.add(self.alloc, .strikethrough, .{
+        var gpu_cell: mtl_cell.Key.strikethrough.CellType() = .{
             .mode = .fg,
             .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
             .constraint_width = 1,
@@ -2657,21 +2652,12 @@ fn updateCell(
                 @intCast(render.glyph.offset_x),
                 @intCast(render.glyph.offset_y),
             },
-        });
+        };
+        try self.cells.add(self.alloc, .strikethrough, gpu_cell);
         // If it's a wide cell we need to strike through the right half as well.
         if (cell.gridWidth() > 1 and coord.x < self.cells.size.columns - 1) {
-            try self.cells.add(self.alloc, .strikethrough, .{
-                .mode = .fg,
-                .grid_pos = .{ @intCast(coord.x + 1), @intCast(coord.y) },
-                .constraint_width = 1,
-                .color = .{ colors.fg.r, colors.fg.g, colors.fg.b, alpha },
-                .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y },
-                .glyph_size = .{ render.glyph.width, render.glyph.height },
-                .bearings = .{
-                    @intCast(render.glyph.offset_x),
-                    @intCast(render.glyph.offset_y),
-                },
-            });
+            gpu_cell.grid_pos[0] = @intCast(coord.x + 1);
+            try self.cells.add(self.alloc, .strikethrough, gpu_cell);
         }
     }
 
diff --git a/src/renderer/metal/cell.zig b/src/renderer/metal/cell.zig
index 94b8b39bb..33f781ac1 100644
--- a/src/renderer/metal/cell.zig
+++ b/src/renderer/metal/cell.zig
@@ -14,7 +14,7 @@ pub const Key = enum {
     strikethrough,
 
     /// Returns the GPU vertex type for this key.
-    fn CellType(self: Key) type {
+    pub fn CellType(self: Key) type {
         return switch (self) {
             .bg => mtl_shaders.CellBg,
 
@@ -125,7 +125,7 @@ pub const Contents = struct {
         const bg_cells = try alloc.alloc(mtl_shaders.CellBg, cell_count);
         errdefer alloc.free(bg_cells);
 
-        @memset(bg_cells, .{0, 0, 0, 0});
+        @memset(bg_cells, .{ 0, 0, 0, 0 });
 
         // The foreground lists can hold 3 types of items:
         // - Glyphs
@@ -231,7 +231,7 @@ test Contents {
     for (0..rows) |y| {
         try testing.expect(c.fg_rows.lists[y + 1].items.len == 0);
         for (0..cols) |x| {
-            try testing.expectEqual(.{0, 0, 0, 0}, c.bgCell(y, x).*);
+            try testing.expectEqual(.{ 0, 0, 0, 0 }, c.bgCell(y, x).*);
         }
     }
     // And the cursor row should have a capacity of 1 and also be empty.
@@ -256,7 +256,7 @@ test Contents {
     for (0..rows) |y| {
         try testing.expect(c.fg_rows.lists[y + 1].items.len == 0);
         for (0..cols) |x| {
-            try testing.expectEqual(.{0, 0, 0, 0}, c.bgCell(y, x).*);
+            try testing.expectEqual(.{ 0, 0, 0, 0 }, c.bgCell(y, x).*);
         }
     }
 
diff --git a/src/renderer/metal/shaders.zig b/src/renderer/metal/shaders.zig
index c1c403848..b909a2f2a 100644
--- a/src/renderer/metal/shaders.zig
+++ b/src/renderer/metal/shaders.zig
@@ -138,7 +138,7 @@ pub const Uniforms = extern struct {
     cursor_color: [4]u8 align(4),
 
     // Whether the cursor is 2 cells wide.
-    wide_cursor: bool align(1),
+    cursor_wide: bool align(1),
 
     const PaddingExtend = packed struct(u8) {
         left: bool = false,
diff --git a/src/renderer/shaders/cell.metal b/src/renderer/shaders/cell.metal
index 44540e198..ced057b72 100644
--- a/src/renderer/shaders/cell.metal
+++ b/src/renderer/shaders/cell.metal
@@ -18,7 +18,7 @@ struct Uniforms {
   float min_contrast;
   ushort2 cursor_pos;
   uchar4 cursor_color;
-  bool wide_cursor;
+  bool cursor_wide;
 };
 
 //-------------------------------------------------------------------
@@ -296,7 +296,7 @@ vertex CellTextVertexOut cell_text_vertex(
     in.mode != MODE_TEXT_CURSOR &&
     (
       in.grid_pos.x == uniforms.cursor_pos.x ||
-      uniforms.wide_cursor &&
+      uniforms.cursor_wide &&
         in.grid_pos.x == uniforms.cursor_pos.x + 1
     ) &&
     in.grid_pos.y == uniforms.cursor_pos.y