From 313c7f4cf1e7ef55807abbee7e9a85d3bafa4ff5 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Tue, 30 Apr 2024 11:08:13 -0700
Subject: [PATCH] font: runs do not split on bg color change

We previously split text runs for shaping on bg color changes. As
pointed out in Discord, this is not necessary, since we can always color
cells according to their desired background even if the text in the cell
shapes to something else.
---
 src/font/shaper/harfbuzz.zig | 14 ++++----------
 src/font/shaper/run.zig      | 25 +++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/src/font/shaper/harfbuzz.zig b/src/font/shaper/harfbuzz.zig
index 13988ecf3..38ac7c104 100644
--- a/src/font/shaper/harfbuzz.zig
+++ b/src/font/shaper/harfbuzz.zig
@@ -356,15 +356,9 @@ test "run iterator: empty cells with background set" {
         );
         {
             const run = (try it.next(alloc)).?;
-            try testing.expectEqual(@as(u32, 1), shaper.hb_buf.getLength());
+            try testing.expectEqual(@as(u32, 3), shaper.hb_buf.getLength());
             const cells = try shaper.shape(run);
-            try testing.expectEqual(@as(usize, 1), cells.len);
-        }
-        {
-            const run = (try it.next(alloc)).?;
-            try testing.expectEqual(@as(u32, 2), shaper.hb_buf.getLength());
-            const cells = try shaper.shape(run);
-            try testing.expectEqual(@as(usize, 2), cells.len);
+            try testing.expectEqual(@as(usize, 3), cells.len);
         }
         try testing.expect(try it.next(alloc) == null);
     }
@@ -1124,7 +1118,7 @@ test "shape cell attribute change" {
         try testing.expectEqual(@as(usize, 2), count);
     }
 
-    // Changing bg color should split
+    // Changing bg color should not split
     {
         var screen = try terminal.Screen.init(alloc, 3, 10, 0);
         defer screen.deinit();
@@ -1146,7 +1140,7 @@ test "shape cell attribute change" {
             count += 1;
             _ = try shaper.shape(run);
         }
-        try testing.expectEqual(@as(usize, 2), count);
+        try testing.expectEqual(@as(usize, 1), count);
     }
 
     // Same bg color should not split
diff --git a/src/font/shaper/run.zig b/src/font/shaper/run.zig
index 36e7ef2ab..9441fae22 100644
--- a/src/font/shaper/run.zig
+++ b/src/font/shaper/run.zig
@@ -88,9 +88,17 @@ pub const RunIterator = struct {
             // If our cell attributes are changing, then we split the run.
             // This prevents a single glyph for ">=" to be rendered with
             // one color when the two components have different styling.
-            if (j > self.i) {
+            if (j > self.i) style: {
                 const prev_cell = cells[j - 1];
-                if (prev_cell.style_id != cell.style_id) break;
+
+                // If the style is exactly the change then fast path out.
+                if (prev_cell.style_id == cell.style_id) break :style;
+
+                // The style is different. We allow differing background
+                // styles but any other change results in a new run.
+                const c1 = comparableStyle(style);
+                const c2 = comparableStyle(self.row.style(&cells[j]));
+                if (!c1.eql(c2)) break;
             }
 
             // Text runs break when font styles change so we need to get
@@ -301,3 +309,16 @@ pub const RunIterator = struct {
         return null;
     }
 };
+
+/// Returns a style that when compared must be identical for a run to
+/// continue.
+fn comparableStyle(style: terminal.Style) terminal.Style {
+    var s = style;
+
+    // We allow background colors to differ because we'll just paint the
+    // cell background whatever the style is, and wherever the glyph
+    // lands on top of it will be the color of the glyph.
+    s.bg_color = .none;
+
+    return s;
+}