Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

selectionBackground doesn't match settings.json setting? #11181

Closed
vefatica opened this issue Sep 8, 2021 · 15 comments
Closed

selectionBackground doesn't match settings.json setting? #11181

vefatica opened this issue Sep 8, 2021 · 15 comments
Labels
Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@vefatica
Copy link

vefatica commented Sep 8, 2021

The name suggests that selectionBackground will be the backGround of selected text. It often isn't. Here's Powershell and Campbell Powershell. Settings.json has bright white and I see light blue.

image

And can selectionBackground be set with a control sequence?

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 8, 2021
@DHowett
Copy link
Member

DHowett commented Sep 8, 2021

Yeah. This is sorta silly. The selection color is blended at something-percent opacity with the text because we draw it in a final pass on top of everything. We have plans to make it better.

@vefatica
Copy link
Author

vefatica commented Sep 9, 2021

I kinda like the light blue in the Campbell Powershell scheme.

@vefatica vefatica closed this as completed Sep 9, 2021
@vefatica
Copy link
Author

vefatica commented Sep 11, 2021

Actually, I also kinda like the apparent averaging of selectionBackground (#FFFFFF) with the background (#000000) in Vintage. So I'm not eagerly awaiting a change. But I wonder ... can I change selectionBackground, on-the-fly, with a control sequence? If not, how about adding OSC13; that could be "chained" with OSC10/11/12. With that, I could change the whole scheme under program control.

@vefatica vefatica reopened this Sep 11, 2021
@vefatica
Copy link
Author

With that, I could change the whole scheme under program control.

I guess that;s moot since all the builtin ones are #FFFFFF. :-)

@zadjii-msft
Copy link
Member

can I change selectionBackground, on-the-fly, with a control sequence?

Nope. selectionBackground is a uniquely Terminal thing, pretty sure there's not a VT sequence for that one. There might be an iTerm2 sequence that's similar, or a kitty one possibly, otherwise we'd have to roll our own.


iTerm2

Change the color palette
To change the current session's colors use this code:

OSC 1337 ; SetColors=[key]=[value] ST
[key] gives the color to change. The accepted values are: fg bg bold link selbg selfg curbg curfg underline tab black red green yellow blue magenta cyan white br_black br_red br_green br_yellow br_blue br_magenta br_cyan br_white

[value] gives the new color. The following formats are accepted:

  • RGB (three hex digits, like fff)
  • RRGGBB (six hex digits, like f0f0f0)
  • cs:RGB (like RGB but cs gives a color space)
  • cs:RRGGBB (like RRGGBB but cs gives a color space)

If a color space is given, it should be one of:

  • srgb (the standard sRGB color space)
  • rgb (the device-specific color space)
  • p3 (the standard P3 color space, whose gamut is supported on some newer hardware)

The following alternate schemes are also supported: * If key is preset then value should be the name of a color preset. * If key is tab then a value of default removes the tab color and restores it to the system default.

Boy that's a gross sequence though. Does take care of #6574 though, with OSC 1337 ; SetColors=tab=[whatever] ST

Tempted to convert #6574 to "implement support for iTerm2's SetColors sequence" close this as a dupe of #6574.

@j4james
Copy link
Collaborator

j4james commented Sep 13, 2021

Nope. selectionBackground is a uniquely Terminal thing, pretty sure there's not a VT sequence for that one.

XTerm uses OSC 17 for this (they call it the highlight color). It's also supported by quite a few other terminals. That seems a better choice than the iTerm2 sequence, especially since we've already implemented many of the related XTerm color sequences.

Edit: I should add that there's both a foreground and background highlight color (OSC 17 is for the background, and OSC 19 is for the foreground).

@vefatica
Copy link
Author

Heehee! I guess if you want to set 19 colors (as I do, or more) it's going to get ugly. I'm timing with the performance counter and the code below takes (on average) 90 microsecconds (@3.5GHz). I tried like heck to make it faster but I can't. Even using a BYTE rgb[N_SCHEMES][N_COLORS][3] array (which avoids all the Get*Value macros) doesn't make a measurable difference.

	for (  p = szBigSeq,color_index = 0; color_index < 16; color_index += 1 )
	{
		p += wsprintfW(p, OSC L"4;%d;rgb:%02X/%02X/%02X" ST,
				color_index,
				GetRValue(cr[scheme_index][color_index]),
				GetGValue(cr[scheme_index][color_index]),
				GetBValue(cr[scheme_index][color_index]));
	}
		
	// "chain" OSC10/11/12
	p += wsprintfW(p, OSC L"10;rgb:%02X/%02X/%02X;rgb:%02X/%02X/%02X;rgb:%02X/%02X/%02X" ST,
			GetRValue(cr[scheme_index][16]),
			GetGValue(cr[scheme_index][16]),
			GetBValue(cr[scheme_index][16]),
			GetRValue(cr[scheme_index][17]),
			GetGValue(cr[scheme_index][17]),
			GetBValue(cr[scheme_index][17]),
			GetRValue(cr[scheme_index][18]),
			GetGValue(cr[scheme_index][18]),
			GetBValue(cr[scheme_index][18]));

	WriteConsoleW(hConOut, szBigSeq, (DWORD) (p - szBigSeq), nullptr, nullptr);

Does the terminal ever see control sequences? A sequence for the "setColorScheme" action would make life easy for anyone who wanted to do this sort of thing.

@DHowett
Copy link
Member

DHowett commented Sep 13, 2021

  1. Terminal only sees control sequences.
  2. 90 microseconds doesn't seem like a terrifically high number to be concerned about.

However: you can also combine OSC 4 sequences. OSC 4;1;color;2;color;3;color;4;color;... ST. Perhaps that'll streamline things slightly.

@vefatica
Copy link
Author

However: you can also combine OSC 4 sequences. OSC 4;1;color;2;color;3;color;4;color;... ST. Perhaps that'll streamline things slightly.

Ya know, I tried exactly (I think) that at a command line and it didn't seem to work. I'll try again, a little harder.

90 microseconds doesn't seem like a terrifically high number to be concerned about.

I'm a bit obsessed with time and I have a lot of it on my hands. 😁

@vefatica
Copy link
Author

However: you can also combine OSC 4 sequences. OSC 4;1;color;2;color;3;color;4;color;... ST. Perhaps that'll streamline things slightly.

Yeah, it does work. Thanks! I'm confident I won't see a decrease in the time to construct and emit the sequence. But my guess is that it would save a lot of time on the other end, when the sequence is processed ... yes/no?

@vefatica
Copy link
Author

@DHowett said:

However: you can also combine OSC 4 sequences. OSC 4;1;color;2;color;3;color;4;color;... ST. Perhaps that'll streamline things slightly.

@vefatica said:

But my guess is that it would save a lot of time on the other end, when the sequence is processed ... yes/no?

Whoa! That made a significant difference, bringing 90 microseconds down to 75 microseconds (new code below). And I think I've answered my question in the affirmative. I doubt I saved much in constructing the sequence, so I figure the saving is in the WriteConsole. Is that right ... WriteConsole (to a "CONOUT$" handle) doesn't return until the sequence has been processed (and the sequence is being processed much faster because of the chaining)?

Here's the new code. I'm timing 100 of them and taking an average.

	p = szBigSeq;
	p += wsprintf(p, OSC L"4");
	for ( color_index = 0; color_index < 16; color_index += 1 )
	{
		p += wsprintfW(p, L";%d;rgb:%02X/%02X/%02X",
				color_index,
				GetRValue(cr[scheme_index][color_index]),
				GetGValue(cr[scheme_index][color_index]),
				GetBValue(cr[scheme_index][color_index]));
	}

	p += wsprintf(p, ST);
		
	// use OSC10, OSC11, OSC12 (fore, back, cursor) ... chain them
	p += wsprintfW(p, OSC L"10;rgb:%02X/%02X/%02X;rgb:%02X/%02X/%02X;rgb:%02X/%02X/%02X" ST,
			GetRValue(cr[scheme_index][16]),
			GetGValue(cr[scheme_index][16]),
			GetBValue(cr[scheme_index][16]),
			GetRValue(cr[scheme_index][17]),
			GetGValue(cr[scheme_index][17]),
			GetBValue(cr[scheme_index][17]),
			GetRValue(cr[scheme_index][18]),
			GetGValue(cr[scheme_index][18]),
			GetBValue(cr[scheme_index][18]));

	WriteConsoleW(hConOut, szBigSeq, (DWORD) (p - szBigSeq), nullptr, nullptr);

@DHowett
Copy link
Member

DHowett commented Sep 16, 2021

FWIW I know that this one doesn't have the same title, but we're aggregating "change how selection background works" over in /dup #3571

You're right: WriteConsole doesn't return until all console handling is complete, synchronously. That's something we're keen on changing. :)

@ghost
Copy link

ghost commented Sep 16, 2021

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@ghost ghost closed this as completed Sep 16, 2021
@ghost ghost added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 16, 2021
@vefatica
Copy link
Author

FWIW I know that this one doesn't have the same title, but we're aggregating "change how selection background works" over in /dup #3571

Did you get that right? That thread and the one it dups don't mention selectionBackground and don't otherwise seem to deal with it.

@DHowett
Copy link
Member

DHowett commented Sep 16, 2021

Whoops, that was my bad. I typoed #3561.

DHowett added a commit that referenced this issue Aug 19, 2024
…a overlay (#17725)

With the merge of #17638, selections are now accumulated early in the
rendering process. This allows Atlas, which currently makes decisions
about cell foreground/background at the time of text rendering,
awareness of the selection ranges *before* text rendering begins.

As a result, we can now paint the selection into the background and
foreground bitmaps. We no longer need to overlay a rectangle, or series
of rectangles, on top of the rendering surface and alpha blend the
selection color onto the final image.

As a reminder, "alpha selection" was always a stopgap because we didn't
have durable per-cell foreground and background customization in the
original DxEngine.

Selection foregrounds are not customizable, and will be chosen using the
same color distancing algorithm as the cursor. We can make them
customizable "easily" (once we figure out the schema for it) for #3580.

`ATLAS_DEBUG_SHOW_DIRTY` was using the `Selection` shading type to draw
colored regions. I didn't want to break that, so I elected to rename the
`Selection` shading type to `FilledRect` and keep its value. It helps
that the shader didn't have any special treatment for
`SHADING_TYPE_SELECTION`.

This fixes the entire category of issues created by selection being an
80%-opacity white rectangle. However, given that it changes the imputed
colors of the text it will reveal `SGR 8` concealed/hidden characters.

Refs #17355
Refs #14859
Refs #11181
Refs #8716
Refs #4971
Closes #3561
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
None yet
Development

No branches or pull requests

4 participants