Merge pull request #2365 from qwerasd205/underline-fixes

Underline Fixes

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,
+            .cursor_wide = false,
         },
 
         // Fonts
@@ -2034,6 +2035,7 @@ pub fn setScreenSize(
         .min_contrast = old.min_contrast,
         .cursor_pos = old.cursor_pos,
         .cursor_color = old.cursor_color,
+        .cursor_wide = old.cursor_wide,
     };
 
     // Reset our cell contents if our grid size has changed.
@@ -2274,6 +2276,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,
@@ -2349,11 +2355,24 @@ 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,
+                // 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.cursor_wide = 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);
                 break :blk sty.bg(screen.cursor.page_cell, color_palette) orelse self.background_color;
@@ -2541,17 +2560,17 @@ fn updateCell(
             font.sprite_index,
             @intFromEnum(sprite),
             .{
-                .cell_width = if (cell.wide == .wide) 2 else 1,
+                .cell_width = 1,
                 .grid_metrics = self.grid_metrics,
             },
         );
 
         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 = 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 },
@@ -2559,7 +2578,13 @@ 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) {
+            gpu_cell.grid_pos[0] = @intCast(coord.x + 1);
+            try self.cells.add(self.alloc, .underline, gpu_cell);
+        }
     }
 
     // If the shaper cell has a glyph, draw it.
@@ -2611,15 +2636,15 @@ 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,
             },
         );
 
-        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 = 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 },
@@ -2627,7 +2652,13 @@ 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) {
+            gpu_cell.grid_pos[0] = @intCast(coord.x + 1);
+            try self.cells.add(self.alloc, .strikethrough, gpu_cell);
+        }
     }
 
     return true;

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.
+    // These are all the foreground cells underneath the cursor.
-    // We keep track of it so that we can invert the colors so the character
+    //
-    // remains visible.
+    // We keep track of these so that we can invert the colors and move them
-    var cursor_cell: ?CellProgram.Cell = null;
+    // 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
+            const x = screen.cursor.x;
-            // the previous cell.
+            const wide = row.cells(.all)[x].wide;
-            const screen_cell = row.cells(.all)[screen.cursor.x];
+            const min_x = switch (wide) {
-            const x = screen.cursor.x - @intFromBool(screen_cell.wide == .spacer_tail);
+                .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()) {
+                if (cell.grid_col < min_x or cell.grid_col > max_x) continue;
-                    cursor_cell = cell;
+                if (cell.mode.isFg()) {
-                    break;
+                    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 '\_('-')_/'
+                    };
                 }
             }
         };
@@ -1338,6 +1353,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.
@@ -1418,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);
@@ -1779,7 +1798,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 +1809,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 +1825,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 +1908,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 +1917,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 +1933,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;

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).*);
         }
     }
 

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.
+    cursor_wide: bool align(1),
+
     const PaddingExtend = packed struct(u8) {
         left: bool = false,
         right: bool = false,

src/renderer/shaders/cell.metal

@@ -18,6 +18,7 @@ struct Uniforms {
   float min_contrast;
   ushort2 cursor_pos;
   uchar4 cursor_color;
+  bool cursor_wide;
 };
 
 //-------------------------------------------------------------------
@@ -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.cursor_wide &&
+        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;