feat: support selection foreground being cell foreground #5219

dal-liu · 2025-01-19T07:14:52Z

This resolves #2685.

Changes

Created SelectionColor tagged union that can take a Color, cell-foreground, or cell-background
Used the new union to implement the feature for Metal and OpenGL
Removed the use of invert_selection_fg_bg during rendering as suggested in the issue

Demo

macOS:

demo.mov

Ubuntu:

demo2.mov

Any feedback would be helpful, I'm sure there's a lot of room for improvement here.

kjmph · 2025-01-20T15:19:17Z

I like this patch. A couple of minor things, when there are inverted cells, this patch shows the original foreground, rather than the inverted foreground. If you compare to Terminal.app, it will always show the inverted foreground when selecting inverted cells. Additionally, very oddly, the theme's cursor-text seems to not be honored with this patch, unless this is a main vs 1.0.1 change?

If any example code could help, here is my personal patch (from the 1.0.1 tag) that demonstrates equivalence with Terminal.app selection & cursor behavior, with the exception that I have a custom theme with no selection-foreground specified. I also made a change to cursor rendering so that the cell-foreground is used. Can we support cell-foreground with cursor-text as well?

diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig
index b37f440f..a2ef6c16 100644
--- a/src/renderer/Metal.zig
+++ b/src/renderer/Metal.zig
@@ -2435,8 +2435,17 @@ fn rebuildCells(
                 if (selected and !self.config.invert_selection_fg_bg) {
                     // If we don't have invert selection fg/bg set
                     // then we just use the selection foreground if
-                    // set, otherwise the default bg color.
-                    break :fg self.config.selection_foreground orelse self.background_color orelse self.default_background_color;
+                    // set, otherwise the existing fg color.
+                    if (style.flags.inverse) {
+                        break :fg self.config.selection_foreground orelse bg_style orelse self.background_color orelse self.default_background_color;
+                    } else {
+                        break :fg self.config.selection_foreground orelse fg_style;
+                    }
+                    // Except, all themes set selection_foreground! We
+                    // want a theme that doesn't set
+                    // selection_foreground. We could always force
+                    // fg_style, but that isn't a general solution
+                    //break :fg fg_style;
                 }
 
                 // Whether we need to use the bg color as our fg color:
@@ -2671,8 +2680,14 @@ fn rebuildCells(
                 break :blk sty.bg(screen.cursor.page_cell, color_palette) orelse self.background_color orelse self.default_background_color;
             } else if (self.config.cursor_text) |txt|
                 txt
-            else
-                self.background_color orelse self.default_background_color;
+            else base: {
+                const sty = screen.cursor.page_pin.style(screen.cursor.page_cell);
+                if (sty.flags.inverse) {
+                    break :base sty.bg(screen.cursor.page_cell, color_palette) orelse self.background_color orelse self.default_background_color;
+                } else {
+                    break :base sty.fg(color_palette, self.config.bold_is_bright) orelse self.foreground_color orelse self.default_foreground_color;
+                }
+            };
 
             self.uniforms.cursor_color = .{
                 uniform_color.r,

…rt-fg-bg during rendering

dal-liu · 2025-01-22T02:12:26Z

@kjmph Thanks for catching the inverted cell issue, it should hopefully be fixed now. I've also made cursor-text and cursor-color accept cell-foreground and cell-background the same way that the selection colors do.

kjmph · 2025-01-22T03:46:54Z

Haha, great improvement! Except for two things. 1) If the cursor-color is cell-foreground then it wipes out the cell's text. If cursor-color is cell-background then it can't be seen because it blends in. I think it wise to remove the support from cursor-color since it will lead people astray into odd configuration territory. 2) The cursor foreground is wiped out with inverted cells. I don't understand why that happens with your PR, unless it exists in the upstream main branch? I'm personally working on the 1.0.1 tag.

Here is an image showing this patch (and cursor-text = cell-foreground) with inverted cells:

Here is an image showing the patch I shared against 1.0.1 with inverted cells:

I'll give a test against main.

kjmph · 2025-01-22T04:05:58Z

Oh!!! The cursor wiping out the foreground is a problem upstream, it isn't a regression with this patch. I bet most people allow the shell integration for cursors and because it is a bar, don't notice the color mishap. Will have to open a ticket unless you want to fix it in this patch too?

dal-liu · 2025-01-22T06:50:27Z

To me, changing cursor-color to accept cell colors makes sense since we should have more control over the colors, plus it was suggested in the original issue as well. That being said, if a 3rd opinion disagrees then I'll gladly revert the changes, so I will leave it for now.

As for the cursor-text thing, it's definitely worth opening a discussion about it.

gpanders

Nice work for a first time contributor :) This is imo a nice simplification of some of this code which was starting to get a bit hairy.

This PR makes selection-invert-fg-bg and cursor-invert-fg-bg no-ops, which is a breaking change. I'll leave it to Mitchell to determine how to approach that, but at the least the doc strings for those options should be updated to indicate that they no longer do anything.

gpanders · 2025-01-22T14:08:55Z

src/renderer/Metal.zig

+        // Otherwise, use the foreground color.
+        self.foreground_color orelse self.default_foreground_color;


Indentation looks off here, run this through zig fmt if you haven't already.

zig fmt didn't change anything so I just moved the comment somewhere else, hopefully it still makes sense

gpanders · 2025-01-22T14:12:09Z

src/renderer/Metal.zig

+            const sty = screen.cursor.page_pin.style(screen.cursor.page_cell);
+            const fg_style = sty.fg(color_palette, self.config.bold_is_bright) orelse self.foreground_color orelse self.default_foreground_color;
+            const bg_style = sty.bg(screen.cursor.page_cell, color_palette) orelse self.background_color orelse self.default_background_color;


It's probably not a big deal, but we are computing these styles even if they're not needed when the cursor color is set to an explicit style. I don't think there is a significant runtime cost though. If we wanted to avoid that we could first check to see if the enum has the .color tag and only compute the styles if it does not (this would be a perfect use case for ziglang/zig#12863)

Added a check before these calls which should skip them if the union has the .color tag

gpanders · 2025-01-22T14:12:48Z

src/config/Config.zig

+/// Specified as either hex (`#RRGGBB` or `RRGGBB`), a named X11 color,
+/// `cell-foreground` to match the cell foreground color, or `cell-background`
+/// to match the cell background color.
+@"cursor-color": ?DynamicColor = null,


Probably not for this PR (and maybe never), but I wonder if we should unify the terminology for cursor and selection, so that instead of cursor-text and cursor-color we use cursor-foreground and cursor-background.

…mputation to avoid unnecssary invocations

cwrau · 2025-01-27T13:00:31Z

Hello,

just wondering, does this allow me to configure ghostty to behave like gnome-terminal?/

2025-01-27T13_55_47.mp4

Left ist gnome-terminal and right is ghostty. What I specifically want, is that the blinking cursor block has the same background color as the foreground of the text and the foreground to be the inverted foreground-color of the text (at least that's what I can figure is what gnome-terminal does)

kjmph · 2025-01-28T20:24:31Z

@cwrau The PR looks like it could match gnome-terminal, but I don't have your precise setup. Here is the best replication I can draw:

But again, I'm using block_hollow to show this. Otherwise, the block will wipe out the foreground because it matches the inverted foreground color in my example. (As it won't invert the text color like gnome-terminal.)

I think you would want a cursor-invert-fg-bg = true corollary to still be to get the same behavior as gnome-terminal (although it will have to be blink aware). I notice my patch also has this problem, and I get away with it via a non-standard cursor color by default. I fixed it in my personal patch, which I attached the diff from the 1.0.1 tag. Note, if you wanted to try it you would have to be on a Mac, and you would need to remove selection-foreground from your theme in: Ghostty.app/Contents/Resources/ghostty/themes/<your_theme>

Then, set the config:

shell-integration-features = no-cursor
cursor-style = block
cursor-style-blink = true
cursor-invert-fg-bg = true

That should match gnome terminal in your example. See attached video:

cursor_test.mp4

Patch:

diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig
index b37f440f..db41d413 100644
--- a/src/renderer/Metal.zig
+++ b/src/renderer/Metal.zig
@@ -2435,8 +2435,17 @@ fn rebuildCells(
                 if (selected and !self.config.invert_selection_fg_bg) {
                     // If we don't have invert selection fg/bg set
                     // then we just use the selection foreground if
-                    // set, otherwise the default bg color.
-                    break :fg self.config.selection_foreground orelse self.background_color orelse self.default_background_color;
+                    // set, otherwise the existing fg color.
+                    if (style.flags.inverse) {
+                        break :fg self.config.selection_foreground orelse bg_style orelse self.background_color orelse self.default_background_color;
+                    } else {
+                        break :fg self.config.selection_foreground orelse fg_style;
+                    }
+                    // Except, all themes set selection_foreground! We
+                    // want a theme that doesn't set
+                    // selection_foreground. We could always force
+                    // fg_style, but that isn't a general solution
+                    //break :fg fg_style;
                 }
 
                 // Whether we need to use the bg color as our fg color:
@@ -2638,7 +2647,11 @@ fn rebuildCells(
         const cursor_color = self.cursor_color orelse self.default_cursor_color orelse color: {
             if (self.cursor_invert) {
                 const sty = screen.cursor.page_pin.style(screen.cursor.page_cell);
-                break :color sty.fg(color_palette, self.config.bold_is_bright) orelse self.foreground_color orelse self.default_foreground_color;
+                if (sty.flags.inverse) {
+                    break :color sty.bg(screen.cursor.page_cell, color_palette) orelse self.background_color orelse self.default_background_color;
+                } else {
+                    break :color sty.fg(color_palette, self.config.bold_is_bright) orelse self.foreground_color orelse self.default_foreground_color;
+                }
             } else {
                 break :color self.foreground_color orelse self.default_foreground_color;
             }
@@ -2668,11 +2681,21 @@ fn rebuildCells(
 
             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 orelse self.default_background_color;
+                if (sty.flags.inverse) {
+                    break :blk sty.fg(color_palette, self.config.bold_is_bright) orelse self.foreground_color orelse self.default_foreground_color;
+                } else {
+                    break :blk sty.bg(screen.cursor.page_cell, color_palette) orelse self.background_color orelse self.default_background_color;
+                }
             } else if (self.config.cursor_text) |txt|
                 txt
-            else
-                self.background_color orelse self.default_background_color;
+            else base: {
+                const sty = screen.cursor.page_pin.style(screen.cursor.page_cell);
+                if (sty.flags.inverse) {
+                    break :base sty.bg(screen.cursor.page_cell, color_palette) orelse self.background_color orelse self.default_background_color;
+                } else {
+                    break :base sty.fg(color_palette, self.config.bold_is_bright) orelse self.foreground_color orelse self.default_foreground_color;
+                }
+            };
 
             self.uniforms.cursor_color = .{
                 uniform_color.r,

kjmph · 2025-01-28T20:37:16Z

Actually, I stand corrected, with this PR, I can get close to gnome-terminal @cwrau:

shell-integration-features = no-cursor
cursor-style = block
cursor-text = cell-background
cursor-style-blink = true
cursor-color = cell-foreground

But the foreground colors look saturated incorrectly? (see the S and the E here:)

cwrau · 2025-01-29T09:02:59Z

Actually, I stand corrected, with this PR, I can get close to gnome-terminal @cwrau:
shell-integration-features = no-cursor
cursor-style = block
cursor-text = cell-background
cursor-style-blink = true
cursor-color = cell-foreground
But the foreground colors look saturated incorrectly? (see the S and the E here:)

Amazing! Looking forward to this PR 😁

dal-liu added 5 commits January 18, 2025 20:47

Created tagged union for selection colors, enabled parsing

671a12a

Implemented cell color for Metal

8fe5904

Removed use of selection-invert-fg-bg

ccb5f00

Mirrored feature to OpenGL

861a6c2

Added tests for SelectionColor

686e08d

dal-liu added 4 commits January 20, 2025 22:14

Fixed selection on inverted cell behavior

4d75871

Implemented cell colors for cursor-text

3b500f1

Implemented cell colors for cursor-color, removed uses of cursor-inve…

0e1f21d

…rt-fg-bg during rendering

Updated docs for dynamically colored options

0c05ff5

gpanders reviewed Jan 22, 2025

View reviewed changes

Updated docstrings, cleaned up awkward formatting, and moved style co…

6444db4

…mputation to avoid unnecssary invocations

Bump version in docstrings

168d013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support selection foreground being cell foreground #5219

feat: support selection foreground being cell foreground #5219

dal-liu commented Jan 19, 2025 •

edited

Loading

kjmph commented Jan 20, 2025 •

edited

Loading

dal-liu commented Jan 22, 2025

kjmph commented Jan 22, 2025

kjmph commented Jan 22, 2025 •

edited

Loading

dal-liu commented Jan 22, 2025

gpanders left a comment

gpanders Jan 22, 2025

dal-liu Jan 23, 2025

gpanders Jan 22, 2025

dal-liu Jan 23, 2025

gpanders Jan 22, 2025

cwrau commented Jan 27, 2025

kjmph commented Jan 28, 2025 •

edited

Loading

kjmph commented Jan 28, 2025

cwrau commented Jan 29, 2025

		// Otherwise, use the foreground color.
		self.foreground_color orelse self.default_foreground_color;

feat: support selection foreground being cell foreground #5219

Are you sure you want to change the base?

feat: support selection foreground being cell foreground #5219

Conversation

dal-liu commented Jan 19, 2025 • edited Loading

Changes

Demo

kjmph commented Jan 20, 2025 • edited Loading

dal-liu commented Jan 22, 2025

kjmph commented Jan 22, 2025

kjmph commented Jan 22, 2025 • edited Loading

dal-liu commented Jan 22, 2025

gpanders left a comment

Choose a reason for hiding this comment

gpanders Jan 22, 2025

Choose a reason for hiding this comment

dal-liu Jan 23, 2025

Choose a reason for hiding this comment

gpanders Jan 22, 2025

Choose a reason for hiding this comment

dal-liu Jan 23, 2025

Choose a reason for hiding this comment

gpanders Jan 22, 2025

Choose a reason for hiding this comment

cwrau commented Jan 27, 2025

kjmph commented Jan 28, 2025 • edited Loading

kjmph commented Jan 28, 2025

cwrau commented Jan 29, 2025

dal-liu commented Jan 19, 2025 •

edited

Loading

kjmph commented Jan 20, 2025 •

edited

Loading

kjmph commented Jan 22, 2025 •

edited

Loading

kjmph commented Jan 28, 2025 •

edited

Loading