Add support for querying Xterm dynamic colors and palette #17729

DHowett · 2024-08-16T20:54:03Z

This pull request adds support for querying all of the "dynamic
resource" colors (foreground, background, cursor) as well as the entire
color palette using OSC 4, 10, 11 and 12 with the ? color specifier.

To ease integration and to make it easier to extend later, I have
consolidated SetDefaultForeground, SetDefaultBackground and
SetCursorColor into one function SetXtermColorResource, plus its
analog RequestXtermColorResource.

Those functions will map xterm resource OSC numbers to color table
entries and optionally color alias entries using a constant table. The
alias mappings are required to support reassigning the default
foreground and background to their indexed entries after a DECAC.

While there are only three real entries in the mapping table right now,
I have designs on bringing in selection background (xterm "highlight")
and foreground (xterm "highlightText").

We can also extend this to support resetting via OSC 110-119. However,
at the adapter layer we do not have the requisite information to restore
any of the colors (even the cursor color!) to the user's defaults.

OSC 10 and OSC 11 queries report the final values of
DECAC-reassigned entries, under the assumption that an application
asking for them wants to make a determination regardless of their
internal meaning to us (that is: they read through the aliased color to
its final destination in the color palette.)

I've tested this with lsix, which detects the background color before
generating sixel previews. It works great!

ConPTY does not currently pass OSC sequences received on the input
handle, so work was required to make it do so.

Closes #3718

src/terminal/adapter/adaptDispatch.cpp

src/terminal/adapter/adaptDispatch.hpp

DHowett · 2024-08-16T20:55:35Z

src/terminal/adapter/adaptDispatch.cpp

+    /* 13 */ { -1, -1 },
+    /* 14 */ { -1, -1 },
+    /* 15 */ { -1, -1 },
+    /* 16 */ { -1, -1 },
+    /* 17 */ { -1, -1 },
+    /* 18 */ { -1, -1 },
+    /* 19 */ { -1, -1 },


I will not be filling all of these in, but are technically part of Xterm's supported color complement. We could put TEKTRONIX colors in here if we ever support it (OSC 15, 16, 18). We may never support pointer colors (OSC 13, 14). I intend to support selection colors (OSC 17, 19).

DHowett · 2024-08-16T20:56:18Z

src/terminal/adapter/adaptDispatch.cpp

 {
-    _renderSettings.SetColorAliasIndex(ColorAlias::DefaultForeground, TextColor::DEFAULT_FOREGROUND);
-    return SetColorTableEntry(TextColor::DEFAULT_FOREGROUND, dwColor);
+    assert(static_cast<size_t>(resource) >= 10);


This protects the developer. {Set,Request}XtermColorResource are amply guarded by the switch in OutputStateMachineEngine.

DHowett · 2024-08-16T20:56:42Z

src/terminal/parser/OutputStateMachineEngine.cpp

@@ -14,6 +14,8 @@
 using namespace Microsoft::Console;
 using namespace Microsoft::Console::VirtualTerminal;

+constexpr COLORREF COLOR_INQUIRY_COLOR = 0xfeffffff; // It's like INVALID_COLOR but special


I DUNNO ABOUT THIS YALL

It's like INVALID_COLOR but special

LMAO

DHowett · 2024-08-17T00:57:38Z

I reread the issue I linked and learned that I need to report 16-bit colors. I also figured, in halfway there - why not report OSC 4 colors as well?

I confirmed with xterm that each reply comes in as a separate OSC, even if they were bundled together in the input

j4james · 2024-08-17T01:24:02Z

Should OSC 10 and OSC 11 queries report on the values of DECAC-reassigned entries?

@DHowett I think so, yes. If an app is querying OSC 10 and OSC 11 it's most likely because they want to know the actual RGB rendering they're going to get for SGR 0 (like for figuring out whether the user has a light or dark color scheme).

Also, I'm fairly certain that's what other terminals do that support both DECAC and OSC 10/11. You can test on MLTerm and/or RLogin.

j4james · 2024-08-17T10:24:43Z

@DHowett As far as I can tell this doesn't work in Windows Terminal (I'm testing with printf "\e]10;?\e\\"; read in a WSL bash shell). I suspect this is because the InputStateMachine isn't setup to handle OSC input sequences. Technically it's not setup to handle DCS sequences either, but they're different, and kind of work accidentally.

DHowett · 2024-08-17T14:49:29Z

As far as I can tell this doesn't work in Windows Terminal

Argh, good catch! Predictably, I've been testing with conhost 😅

This pull request consolidates `SetDefaultForeground`, `SetDefaultBackground` and `SetCursorColor` into one function `SetXtermColorResource`. `SetXtermColorResource` maps xterm resource OSC numbers to color table entries and, optionally, color _alias_ entries. The alias mappings are required to support reassigning the default foreground and background to their indexed entries after a `DECAC`. While there are only three real entries in the mapping table right now, I have designs on bringing in selection background (xterm "highlight") and foreground (xterm "highlightText"). Having this mapping will also make color inquiry easier.

src/terminal/adapter/ut_adapter/adapterTest.cpp

j4james

LGTM. Just nits.

src/terminal/parser/OutputStateMachineEngine.cpp

src/terminal/adapter/DispatchTypes.hpp

src/terminal/parser/OutputStateMachineEngine.cpp

src/terminal/adapter/adaptDispatch.cpp

src/terminal/adapter/ut_adapter/adapterTest.cpp

alabuzhev · 2024-08-18T18:04:53Z

the only requirement is that you have ENABLE_VIRTUAL_TERMINAL_INPUT on

Thanks, it works, but why such a requirement though?
It works without it in conhost and the documentation explicitly says that

The ENABLE_VIRTUAL_TERMINAL_INPUT flag does not apply to query commands as it is assumed that an application making the query will always want to receive the reply.

DHowett · 2024-08-18T18:31:17Z

why such a requirement though?

Ah, I see. That’s one of the decisions we made for ConPTY (and only ConPTY). When the connected terminal sends an escape sequence to the input side of the pty, we need to decide whether the application on the receiving end is expecting it. There is no signal between the connected terminal and the pty host that indicates it is in response to a command (and we could detect it, but we would need to get back into parsing the VT output to figure that out and there would probably be a race condition (since cross-process synchronization is one of our bugbears) and we just got out of that whole frying pan.)

Unfortunately, OSC 4/10/11/12 isn’t the first casualty of this implementation detail.

DHowett · 2024-08-18T18:34:56Z

This was initially done to make sure that applications that weren’t expecting VT mouse input didn’t get it. Lack of ability to differentiate those inputs from replies is at the heart of it, though.

@j4james do you think passing OSCs replies over would be fine? By and large, nobody will be generating OSC input in 2024 (gods willing)… so the risk is much smaller of passing through something that is not a reply.

DHowett · 2024-08-18T19:44:52Z

~~Hm, this is more nuanced than all that. We need to get it into the input record encoder if we go this route.~~ No, this should work. ConEchoKey reported it properly.

j4james · 2024-08-18T19:54:48Z

do you think passing OSCs replies over would be fine?

I'm not sure I fully understand what the issue is here, but I was just about to report what I suspect is a similar bug. Usually I could type something like echo ^[[c at the cmd prompt (where ^[ is Esc), and that would show me the DA report, but that doesn't work anymore. I found similar problems with some of my python scripts that test VT queries, and which no longer work from the cmd shell.

And from what you've just said, I suspect that those cases are also likely failing because ENABLE_VIRTUAL_TERMINAL_INPUT hasn't been set. If that's the case, it's more than just OSC replies that we aren't passing through anymore. It's not a big deal for me personally, but that seems like it could be a serious regression. Was that really intentional?

DHowett · 2024-08-18T20:37:53Z

Oh no, you're totally right.

conhost was replying in these cases!

Now that Terminal is in charge of replying, we need to pass everything through. @lhecker this could be a big conpty v2 regression.

…m-resources

src/terminal/adapter/adaptDispatch.cpp

zadjii-msft

as the original author of INVALID_COLOR, i approve of COLOR_INQUIRY_COLOR

alabuzhev · 2024-08-21T16:05:01Z

@DHowett after 43e8883 WT replies without ENABLE_VIRTUAL_TERMINAL_INPUT, but the reply is mangled, is it expected?

Code

#include <windows.h>

void test(bool VtInput)
{
	const auto
		In = GetStdHandle(STD_INPUT_HANDLE),
		Out = GetStdHandle(STD_OUTPUT_HANDLE);

	DWORD InMode;
	GetConsoleMode(In, &InMode);
	SetConsoleMode(In, (InMode & ~ENABLE_LINE_INPUT) | (VtInput? ENABLE_VIRTUAL_TERMINAL_INPUT : 0));

	DWORD OutMode;
	GetConsoleMode(Out, &OutMode);
	SetConsoleMode(Out, OutMode | ENABLE_PROCESSED_OUTPUT | ENABLE_VIRTUAL_TERMINAL_PROCESSING);

	const wchar_t Command[] = L"\x1b]4;1;?\x1b\\";

	DWORD Written;
	WriteConsole(Out, Command, ARRAYSIZE(Command) - 1, &Written, {});

	wchar_t Reply[1024]{};
	DWORD Read;
	ReadConsole(In, Reply, ARRAYSIZE(Reply), &Read, {});

	SetConsoleMode(In, InMode);

	for (size_t i = 0; i != Read; ++i)
	{
		if (Reply[i] < 0x20)
			Reply[i] += 0x2400;
	}

	if (VtInput)
		WriteConsole(Out, L"VT_INPUT=1: ", 12, &Written, {});
	else
		WriteConsole(Out, L"VT_INPUT=0: ", 12, &Written, {});

	WriteConsole(Out, Reply, Read, &Written, {});
	WriteConsole(Out, L"\n", 1, &Written, {});
}

int main()
{
	test(true);
	test(false);
}

Expected:

VT_INPUT=1: ␛]4;1;rgb:8080/0000/0000␛\
VT_INPUT=0: ␛]4;1;rgb:8080/0000/0000␛\

Actual:

VT_INPUT=1: ␛]4;1;rgb:8080/0000/0000␛\
VT_INPUT=0: ]4;1;rgb:8080/0000/0000\

I.e. both ESC are missing.
Only WT, OpenConsole is ok.

DHowett · 2024-08-21T17:58:33Z

@alabuzhev how strange! I got the expected response. I did have to recompile it with -DUNICODE because I got a very confused output the first time.

In the boxes, incoming requests are in gray and responses are in red. The debug tap pre-converts all control characters to control pictures.

alabuzhev · 2024-08-21T19:01:57Z

I did have to recompile it with -DUNICODE because I got a very confused output the first time

Right, sorry, it's on by default in Visual Studio so I didn't bother to define it manually.

how strange!

I debugged it and looks like it's not entering this block:

terminal/src/host/stream.cpp

Lines 207 to 215 in 10c97a9

    
           // Ignore Escape and Newline chars 
        
           if (Event.Event.KeyEvent.bKeyDown && 
        
               (WI_IsFlagSet(pInputBuffer->InputMode, ENABLE_VIRTUAL_TERMINAL_INPUT) || 
        
                (Event.Event.KeyEvent.wVirtualKeyCode != VK_ESCAPE && 
        
                 Event.Event.KeyEvent.uChar.UnicodeChar != UNICODE_LINEFEED))) 
        
           { 
        
               *pwchOut = Event.Event.KeyEvent.uChar.UnicodeChar; 
        
               return STATUS_SUCCESS; 
        
           }

DHowett · 2024-08-21T20:06:52Z

Alright. We're dropping the ESC because ~ ~ input modes ~ ~. This is an issue with the new ConPTY infrastructure. We will need to fix it before 1.22 exits preview.

alabuzhev · 2024-08-21T22:02:32Z

We will need to fix it before 1.22 exits preview

Thanks. Should I create a new issue or you have it tracked already?

Requiring VT input is not only potentially a big conpty v2 regression, but a potential source of new bugs: when I'm querying something, I expect a more or less sensible reply, but when VT input is enabled the user can screw everything up by simply pressing some keys.
To reproduce just add Sleep(1000) to my code above before test(true) and hold a key while it's sleeping:

VT_INPUT=1: dddddddddddddddddddd

Pushing these responses to the front of the input queue might help, but I guess you've been there already:

terminal/src/host/outputStream.cpp

Lines 44 to 48 in 0a7c258

    
           // TODO GH#4954 During the input refactor we may want to add a "priority" input list 
        
           // to make sure that "response" input is spooled directly into the application. 
        
           // We switched this to an append (vs. a prepend) to fix GH#1637, a bug where two CPR 
        
           // could collide with each other. 
        
           _io.GetActiveInputBuffer()->WriteString(response);

alabuzhev · 2024-08-21T22:07:22Z

the user can screw everything up by simply pressing some keys

Ugh, it's the same without VT.

DHowett · 2024-08-21T22:15:40Z

To be fair, I think that is true of any terminal emulator that supports reporting. By its nature as an in-band signaling protocol, it is within some other band ;P

j4james · 2024-08-22T00:20:49Z

the user can screw everything up by simply pressing some keys

Just on this point, it's worth noting that there's a mode (KAM) that you can set to temporarily lock the keyboard if you're running a bunch of queries, and are concerned that the user might type something in the middle of the responses. We don't currently support that mode, but we could if there was a demand for it.

juliannoble · 2024-09-06T03:47:44Z

I believe this is related - in my project (tcl based)
Version 1.22 is no longer useable (in line mode) for various reports like DECTABSR (\x1b[2$w) DECCIR (\x1b[1$w) or even the common CPR (cursor position report) \x1b[6n
The beginning of reports read from stdin are truncated in some cases by the leading CSI (ESC [) - but in other cases almost the entire report, leaving only a trailing backslash (seems to only return data after the final esc)
My project aside - from a standard tclsh shell one could previously emit various requests and see the responses - making development significantly easier in previous versions of windows terminal.

All seems to work fine again in canary - sorry for the noise

DHowett · 2024-09-07T00:29:38Z

All seems to work fine again in canary - sorry for the noise

Yay, you beat me to it!

Thanks for testing Canary!

github-advanced-security bot found potential problems Aug 16, 2024

View reviewed changes

src/terminal/adapter/adaptDispatch.cpp Fixed Show fixed Hide fixed

src/terminal/adapter/adaptDispatch.hpp Fixed Show fixed Hide fixed

DHowett requested review from j4james and lhecker August 16, 2024 20:54

DHowett commented Aug 16, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

DHowett commented Aug 16, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase labels Aug 17, 2024

This comment has been minimized.

Sign in to view

DHowett added 4 commits August 17, 2024 10:54

Add support for querying the color values of OSC 10, 11 and 12.

b5fd8f3

Add support for reporting colors via OSC 4

d53b1b4

Flush OSC through InputStateMachineEngine for inquiries over conpty

99d1979

DHowett force-pushed the dev/duhowett/osc-xterm-resources branch from ccb10a3 to 99d1979 Compare August 17, 2024 15:55

github-advanced-security bot found potential problems Aug 17, 2024

View reviewed changes

src/terminal/adapter/ut_adapter/adapterTest.cpp Fixed Show resolved Hide resolved

src/terminal/adapter/ut_adapter/adapterTest.cpp Fixed Show fixed Hide fixed

src/terminal/adapter/ut_adapter/adapterTest.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

j4james approved these changes Aug 17, 2024

View reviewed changes

DHowett added 7 commits August 17, 2024 19:40

Dispense with the XtermColorResource enumeration

19c301e

It's fine for AdaptDispatch to except

0fe2b89

Add another adapter test for INVALID_COLOR reporting

da99629

Add an output engine test for Xterm reporting

9d371d0

Add an adapter test for OSC 4 reporting

88bc529

Add an output engine test for OSC 4 reporting

fafe99f

Fix the VT names in the comments

250a924

Pass through all OSCs always(?)

43e8883

Merge remote-tracking branch 'origin/main' into dev/duhowett/osc-xter…

df6b466

…m-resources

DHowett mentioned this pull request Aug 19, 2024

Allow OSC 17 to set the selection background color #17742

Merged

DHowett commented Aug 19, 2024

View reviewed changes

src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved

Update src/terminal/adapter/adaptDispatch.cpp

10c97a9

zadjii-msft approved these changes Aug 21, 2024

View reviewed changes

DHowett enabled auto-merge (squash) August 21, 2024 12:51

DHowett disabled auto-merge August 21, 2024 13:43

lhecker approved these changes Aug 21, 2024

View reviewed changes

DHowett merged commit 3b4ee83 into main Aug 21, 2024
20 checks passed

DHowett deleted the dev/duhowett/osc-xterm-resources branch August 21, 2024 22:45

bash mentioned this pull request Aug 22, 2024

Windows Support bash/terminal-colorsaurus#19

Closed

alabuzhev mentioned this pull request Aug 22, 2024

Low reliability of VT replies #17775

Closed

DHowett mentioned this pull request Aug 28, 2024

It seems like DA1 is broken in 1.22 (the response doesn't include a valid CSI (\x1b)) #17813

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for querying Xterm dynamic colors and palette #17729

Add support for querying Xterm dynamic colors and palette #17729

DHowett commented Aug 16, 2024 •

edited

Loading

DHowett Aug 16, 2024

This comment has been minimized.

DHowett Aug 16, 2024

DHowett Aug 16, 2024

lhecker Aug 17, 2024

This comment has been minimized.

This comment has been minimized.

DHowett commented Aug 17, 2024

j4james commented Aug 17, 2024

j4james commented Aug 17, 2024

DHowett commented Aug 17, 2024

This comment has been minimized.

j4james left a comment

alabuzhev commented Aug 18, 2024

DHowett commented Aug 18, 2024 •

edited

Loading

DHowett commented Aug 18, 2024

DHowett commented Aug 18, 2024 •

edited

Loading

j4james commented Aug 18, 2024

DHowett commented Aug 18, 2024

zadjii-msft left a comment

alabuzhev commented Aug 21, 2024

DHowett commented Aug 21, 2024

alabuzhev commented Aug 21, 2024

DHowett commented Aug 21, 2024

alabuzhev commented Aug 21, 2024

alabuzhev commented Aug 21, 2024

DHowett commented Aug 21, 2024

j4james commented Aug 22, 2024

juliannoble commented Sep 6, 2024 •

edited

Loading

DHowett commented Sep 7, 2024

Add support for querying Xterm dynamic colors and palette #17729

Add support for querying Xterm dynamic colors and palette #17729

Conversation

DHowett commented Aug 16, 2024 • edited Loading

DHowett Aug 16, 2024

Choose a reason for hiding this comment

This comment has been minimized.

DHowett Aug 16, 2024

Choose a reason for hiding this comment

DHowett Aug 16, 2024

Choose a reason for hiding this comment

lhecker Aug 17, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

DHowett commented Aug 17, 2024

j4james commented Aug 17, 2024

j4james commented Aug 17, 2024

DHowett commented Aug 17, 2024

This comment has been minimized.

j4james left a comment

Choose a reason for hiding this comment

alabuzhev commented Aug 18, 2024

DHowett commented Aug 18, 2024 • edited Loading

DHowett commented Aug 18, 2024

DHowett commented Aug 18, 2024 • edited Loading

j4james commented Aug 18, 2024

DHowett commented Aug 18, 2024

zadjii-msft left a comment

Choose a reason for hiding this comment

alabuzhev commented Aug 21, 2024

DHowett commented Aug 21, 2024

alabuzhev commented Aug 21, 2024

DHowett commented Aug 21, 2024

alabuzhev commented Aug 21, 2024

alabuzhev commented Aug 21, 2024

DHowett commented Aug 21, 2024

j4james commented Aug 22, 2024

juliannoble commented Sep 6, 2024 • edited Loading

DHowett commented Sep 7, 2024

DHowett commented Aug 16, 2024 •

edited

Loading

DHowett commented Aug 18, 2024 •

edited

Loading

DHowett commented Aug 18, 2024 •

edited

Loading

juliannoble commented Sep 6, 2024 •

edited

Loading