Allow OSC 17 to set the selection background color #17742

DHowett · 2024-08-19T17:44:08Z

This pull request adds support for setting and querying the selection
color with OSC 17.

To make this possible, I had to move selection color down into the color
table where it always belonged. This lets us get rid of the special
SetSelectionColor method from the surface of AtlasEngine, and reunites
selection colors with the rest of the special colors.

DHowett · 2024-08-19T17:45:15Z

src/renderer/atlas/AtlasEngine.cpp

@@ -315,6 +315,15 @@ CATCH_RETURN()
        }

        _api.selectionSpans = til::point_span_subspan_within_rect(info.selectionSpans, dr);
+
+        u32 newSelectionColor{ static_cast<COLORREF>(info.selectionBackground) | 0xff000000 };


@lhecker please check me on this - I don't think there's another place for us to propagate this...

Seems good to me!

DHowett · 2024-08-19T17:45:47Z

src/terminal/parser/OutputStateMachineEngine.cpp

@@ -804,6 +804,7 @@ bool OutputStateMachineEngine::ActionOscDispatch(const size_t parameter, const s
    case OscActionCodes::SetForegroundColor:
    case OscActionCodes::SetBackgroundColor:
    case OscActionCodes::SetCursorColor:
+    case OscActionCodes::SetHighlightColor:


this is xterm's name for selection color

j4james · 2024-08-20T00:40:56Z

src/buffer/out/TextColor.h

+    static constexpr size_t SELECTION_BACKGROUND = 266;
+    static constexpr size_t TABLE_SIZE = 267;


If there's a chance we may one day support a selection foreground color like Xterm does, it may be a good idea to reserve that slot now, so we've always got our foreground and background colors paired in the same order.

Actually, I love that. That ship may have sailed for the cursor color, however... as i have always wanted to add cursor foreground.

Since we didn't have palette report support before (unreleased) 1.22¹, is there still time to make changes in our reserved color indices?

Footnotes

under the assumption that we want them stable now that people can back up and restore them :) ↩

That ship may have sailed for the cursor color, however... as i have always wanted to add cursor foreground.

Cursor color is a bit of a special case, because it can be thought of as both foreground and background depending on the style. When it's the block style, you think of the cursor color as a background attribute, and when it's the underline or bar style it's more like a foreground attribute.

But if you consider the way cursors were originally implemented, it makes the most sense as a foreground attribute, because the block cursor was really just implemented as a reverse attribute toggle. So if your default text is white on black, and your cursor color is say green, then an underline cursor is going to be green on black, and a block cursor is just green on black reversed. In both case the black background is retained, and the foreground white is replaced with green.

That said, if you wanted to introduce a second cursor color, referring to the two as foreground and background is going to be confusing no matter which way you look at it, and maybe it would be easiest to refer to the second color and something generic like "secondary cursor color".

Since we didn't have palette report support before (unreleased) 1.22, is there still time to make changes in our reserved color indices?

I don't think we want to mess with the default text and frame indices, because even though we couldn't query them, we've already been telling people they can set the colors at those specific positions. The cursor color is probably less of an issue, because I don't think we've publicaly mentioned its value anywhere. But as I explained above, I don't think we necessarily should be changing that anywav - just reserve a slot for a "secondary cursor color" if you think we're going to need that.

Oh, that makes way more sense. Thanks for explaining.

Do you not still want to reserve a spot for a SECONDARY_CURSOR_COLOR before the new selection color entries, or have you decided we aren't likely to need that?

Eh, I was going to leave it. But you're right, I might as well.

Leonard's position is more (paraphrased by me) "If these aren't a committed public API, we should be able to renumber them at our leisure later." I have not fully digested how I feel about Terminal Preview and DECCTR.

Yeah, I'm OK with that too.

src/buffer/out/TextColor.h

The base branch was changed.

This removes the AtlasEngine-specific `SetSelectionBackground` completely. Selection background is now propagated in PrepareRenderInfo (for now).

lhecker · 2024-08-21T23:27:35Z

src/renderer/atlas/AtlasEngine.cpp

+        u32 newSelectionColor{ static_cast<COLORREF>(info.selectionBackground) | 0xff000000 };
+        if (_api.s->misc->selectionColor != newSelectionColor)
+        {
+            auto misc = _api.s.write()->misc.write();


Copying from _api.s to _p.s happens in StartPaint(). Similar to AtlasEngine::PaintCursor you need to update both structs simultaneously to keep their generation count in sync.

You can test that this by setting a breakpoint into AtlasEngine::_handleSettingsUpdate. Then and emit an OSC 17. If done correctly, AtlasEngine::_handleSettingsUpdate should not get called.

Oh no, this is interesting. It didn't cause a problem because I didn't understand your architecture.

I put the selection colors on _api, and I read them out of _api. I didn't even know (remember?) that they would also be on _p.

Should PaintBufferLineHelper be reading them out of _p instead?

I made the call to read them out of _p, because that's the RenderingPayload.

DHowett changed the title ~~Add support for setting the selection background color with OSC 17~~ Allow SOC 17 to set the selection background color Aug 19, 2024

DHowett changed the title ~~Allow SOC 17 to set the selection background color~~ Allow OSC 17 to set the selection background color Aug 19, 2024

DHowett commented Aug 19, 2024

View reviewed changes

j4james reviewed Aug 20, 2024

View reviewed changes

zadjii-msft previously approved these changes Aug 21, 2024

View reviewed changes

carlos-zamora previously approved these changes Aug 21, 2024

View reviewed changes

DHowett commented Aug 21, 2024

View reviewed changes

src/buffer/out/TextColor.h Outdated Show resolved Hide resolved

Base automatically changed from dev/duhowett/osc-xterm-resources to main August 21, 2024 22:45

An error occurred while trying to automatically change base from dev/duhowett/osc-xterm-resources to main August 21, 2024 22:45

DHowett added 4 commits August 21, 2024 17:48

Push selection color down into core (as a color table entry)

687b6e6

This removes the AtlasEngine-specific `SetSelectionBackground` completely. Selection background is now propagated in PrepareRenderInfo (for now).

vt: add support for OSC 17 "set highlight color"

48d8a3e

Reserve SELECTION_FOREGROUND and keep fg/bg in the right order

018bbf8

Unreserve selection foreground for now.

d203dcb

DHowett force-pushed the dev/duhowett/selection-color-in-vt-and-core branch from be022ad to d203dcb Compare August 21, 2024 22:48

lhecker reviewed Aug 21, 2024

View reviewed changes

DHowett added 2 commits August 21, 2024 19:14

Make sure _p stays in sync with _api

0998431

Fix build and audit

c1ed74a

DHowett enabled auto-merge (squash) August 22, 2024 16:14

zadjii-msft added this to the Terminal v1.22 milestone Aug 22, 2024

carlos-zamora approved these changes Aug 22, 2024

View reviewed changes

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Aug 22, 2024

microsoft-github-policy-service bot requested a review from zadjii-msft August 22, 2024 17:58

zadjii-msft approved these changes Aug 22, 2024

View reviewed changes

DHowett merged commit cbb4a0a into main Aug 22, 2024
20 checks passed

microsoft-github-policy-service bot requested a review from PankajBhojwani August 22, 2024 17:58

DHowett deleted the dev/duhowett/selection-color-in-vt-and-core branch August 22, 2024 17:58

microsoft-github-policy-service bot requested a review from carlos-zamora August 22, 2024 17:58

microsoft-github-policy-service bot removed the Needs-Second It's a PR that needs another sign-off label Aug 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow OSC 17 to set the selection background color #17742

Allow OSC 17 to set the selection background color #17742

DHowett commented Aug 19, 2024 •

edited

Loading

DHowett Aug 19, 2024

lhecker Aug 21, 2024

DHowett Aug 19, 2024

j4james Aug 20, 2024

DHowett Aug 20, 2024

j4james Aug 20, 2024

DHowett Aug 20, 2024

j4james Aug 21, 2024

DHowett Aug 21, 2024

DHowett Aug 21, 2024

j4james Aug 22, 2024

lhecker Aug 21, 2024

DHowett Aug 21, 2024

DHowett Aug 22, 2024

		static constexpr size_t SELECTION_BACKGROUND = 266;
		static constexpr size_t TABLE_SIZE = 267;

Allow OSC 17 to set the selection background color #17742

Allow OSC 17 to set the selection background color #17742

Conversation

DHowett commented Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Aug 19, 2024 •

edited

Loading