microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
94.53k stars 8.18k forks source link

GetConsoleScreenBufferInfoEx doesn't return the actual palette #10639

Open alabuzhev opened 3 years ago

alabuzhev commented 3 years ago

Windows Terminal version (or Windows build number)

1.9.1523.0

Other Software

No response

Steps to reproduce

  1. Compile the following code:
    
    #include <iomanip>
    #include <iostream>

include

int main() { CONSOLE_SCREEN_BUFFER_INFOEX Info{ sizeof(Info) }; if (!GetConsoleScreenBufferInfoEx(GetStdHandle(STD_OUTPUT_HANDLE), &Info)) return 1;

for (const auto& i: Info.ColorTable)
{
    std::cout << std::hex << std::setw(8) << std::setfill('0') << i << '\n';
}

std::cout.flush();

}

2. Set WT palette to "Vintage" in settings
3. Run the compiled code

### Expected Behavior

The same colour codes as in the corresponding section of `C:\Program Files\WindowsApps\Microsoft.WindowsTerminalPreview_1.9.1523.0_x64__8wekyb3d8bbwe\defaults.json`:

000000 800000 008000 808000 000080 800080 008080 C0C0C0 808080 FF0000 00FF00 FFFF00 0000FF FF00FF 00FFFF FFFFFF


### Actual Behavior

It looks like the default Windows 10 palette is returned instead:

000c0c0c 00da3700 000ea113 00dd963a 001f0fc5 00981788 00009cc1 00cccccc 00767676 00ff783b 000cc616 00d6d661 005648e7 009e00b4 00a5f1f9 00f2f2f2

chrisant996 commented 1 year ago

@zadjii-msft This is problematic because it makes it impossible for a console mode application to detect whether the current terminal theme is Light or Dark. For example, clink cannot automatically choose colors with correct contrast ratio to be visible. Instead I have to force users to manually configure their colors. Users are unhappy about this.

It would be much appreciated if this API could get fixed to return the actual current terminal theme colors.

(I understand that GCSBIE() doesn't have any way to return the default foreground or default background colors. At least for my usage cases, that's easily worked around by printing ESC [ m and then calling GCSBIE() and checking the .wAttributes value.)

j4james commented 1 year ago

The reason we can't easily do this is because conhost (which handles GCSBIE) doesn't know what the terminal's colors are. To find that out, it would need to send an escape sequence to the connected conpty client, and then block while it waits for a response, which isn't great for performance.

This is problematic, because some applications call GCSBIE a lot. Imagine a number that you think is a ridiculous amount, and it's likely more than that. And to further complicate the matter, there's no guarantee that the connected conpty client even supports palette queries.

I did play around with querying the palette once at startup, but that still introduces the complexity of having to wait for a response which may never arrive. An easier version of that would be to have the client pass in the palette via the initial command line. But both of those solutions fall apart if a user updates their palette settings at run-time.

So while I would love to get this working somehow, I just don't see how. But if anyone has any bright ideas, feel free to share them here.

alabuzhev commented 1 year ago

To find that out, it would need to send an escape sequence to the connected conpty client, and then block while it waits for a response

Why? I have a very basic idea of how all this is organised, but this sounds unnecessarily inverted. Why can't conhost return its internal local array, exactly as it does currently, but update that array whenever the connected conpty client updates its palette? There should be some notification mechanism. After all, you somehow do know client's window and buffer dimensions, and I don't think they are queried every time in a blocking way.


Our usage of this API is mostly related to colours mixing, e.g. transparency and shadows. To properly mix index colours we have to know to which exactly RGB values they are mapped.

chrisant996 commented 1 year ago

@j4james @zadjii-msft Here are some brainstorming thoughts. Maybe some of them will be viable, or maybe some of them with spark further ideas.

conhost (which handles GCSBIE) doesn't know what the terminal's colors are. To find that out, it would need to send an escape sequence to the connected conpty client, and then block while it waits for a response, which isn't great for performance.

What if conhost didn't need to wait?

This is problematic, because some applications call GCSBIE a lot. Imagine a number that you think is a ridiculous amount, and it's likely more than that.

Yes, GCSBIE can get called a ton. What if the work weren't done every time?

And to further complicate the matter, there's no guarantee that the connected conpty client even supports palette queries.

Do you mean that there is a single two-way channel of communication, i.e. all data communication must be through input and output streams, so the two sides can only communicate by passing escape codes to each other? And so if one side doesn't support responding to a query sequence then the other side can hang?

I did play around with querying the palette once at startup, but that still introduces the complexity of having to wait for a response which may never arrive. An easier version of that would be to have the client pass in the palette via the initial command line. But both of those solutions fall apart if a user updates their palette settings at run-time.

I agree that supporting palette changes at runtime should be considered a requirement for any solution. Making runtime palette changes instantly be noticed is not necessarily a requirement, though.

So while I would love to get this working somehow, I just don't see how. But if anyone has any bright ideas, feel free to share them here.

I'm not suggesting it would be trivial to implement, nor that it should be perfect with no caveats. But this is currently a pretty significant regression from the perspective of console mode programs.

j4james commented 1 year ago

My understanding is that conpty is designed to work in a similar fashion to the Linux pty system, so a Linux terminal could theoretically be ported to Windows without too much effort. I don't know all the details, but I think there's an input stream, an output stream, and a signal stream, and it's the latter that is responsible for notifying conhost of window size changes, similar to the SIGWINCH signal on Linux

There isn't an equivalent signal for palette changes (as far as I'm aware), so that would be something we'd have to invent. Whereas conhost querying the palette from the conpty client just relies on standard escape sequences that a lot of terminals should already support out of the box. That's why I was considering a query as my initial approach to the problem.

But as you're both proposing, having the conpty client notify conhost of palette changes does seem like a much better idea. And if some don't support it, they'd be no worse off than they are now. I'm not very familiar with this part of the code, so there may be technical reasons why this isn't feasible, but if the core devs don't raise any objections, I'd be willing to give it a try some time.

alabuzhev commented 1 year ago

conhost querying the palette from the conpty client just relies on standard escape sequences

By the way, are these standard escape sequences already supported by WT?

chrisant996 commented 1 year ago

Btw, not all terminals support the same escape sequences. So I don't think it's safe for conhost to make assumptions about which codes it can send internally. If the conpty client indicated its capabilities, then it could be considered safe.

j4james commented 1 year ago

By the way, are these standard escape sequences already supported by WT?

@alabuzhev No, for the same reason we don't return the palette in GCSBIE. That's actually the main reason I want to fix this. I don't particular care about GCSBIE, but I would like to get the VT palette queries working (and a number of other VT queries that we can't currently handle).

Btw, not all terminals support the same escape sequences.

@chrisant996 Yeah, but we only care about terminals that are using conpty, and it also doesn't really matter if they don't support the sequence - it should just be ignored. But that's not something I'm planning to do now anyway. I much prefer the approach that you guys were suggesting, i.e. the conpty client pushing the palette to conhost.

alabuzhev commented 5 months ago

But as you're both proposing, having the conpty client notify conhost of palette changes does seem like a much better idea

Thinking about it again, do we even need a separate notification here? Can the frontend just build an OSC 4 string and push it to the stream a) on startup b) whenever the user changes the palette?

DHowett commented 5 months ago

Well, yes, that would rather constitute a notification.

chrisant996 commented 5 months ago

By the way, another note: The implementations of GetConsoleScreenBufferInfoEx and SetConsoleScreenBufferInfoEx are swapping Red and Blue bytes in the COLORREF. Currently an app has to memcmp the ColorTable against a well-known copy of the "default palette" with Red and Blue bytes swapped, to detect that the API is malfunctioning and discard its results.

Please fix the Red / Blue byte swapping with or before fixing GCSBIEx to return the real ColorTable, otherwise it will become extremely difficult for apps to detect whether GCSBIEx/SCSBIEx have a working implementation.

Here's a sample program that demonstates the R/B swapping in GCSBIEx and SCSBIEx: repro_colortable.zip

alabuzhev commented 5 months ago

@DHowett I mean that in a more conventional sense a "notification" is rather something like: Frontend to backend: "hey we have a change here, do something" Backend: extracts the palette from the frontend using some internal (and not yet existing) magic

Whereas "build and shove OSC 4 to the stream" is more like a direct request. Notably it should not require any extra changes in the backend, it can do it already.

DHowett commented 5 months ago
Byte swapping discussion (hidden as off-topic) > The implementations of GetConsoleScreenBufferInfoEx and SetConsoleScreenBufferInfoEx are swapping Red and Blue bytes in the COLORREF. This is the first we've heard of GCSBIEx returning invalid data in the color palette! Do you have a test app that exhibits this? It sounds (sorry, based on "... _detect_ that the API is malfunctioning") like it isn't consistent. (EDIT: Oh, I see you added one. Thanks!)

it can do it already

Alas, it still will. ConPTY's VT input state machine is vastly different from the output one.

chrisant996 commented 5 months ago

This is the first we've heard of GCSBIEx returning invalid data in the color palette! Do you have a test app that exhibits this? It sounds (sorry, based on "... detect that the API is malfunctioning") like it isn't consistent.

(EDIT: Oh, I see you added one. Thanks!)

An app running in a ConPTY based terminal experiences R/B swapping in GCSBIEx/SCSBIEx, in addition to a canned "default palette" instead of the real palette.

An app running in a legacy ConHost based terminal experiences correct R/B values, and receives the real palette.

So, an app has to use detection heuristics to figure out whether GCSBIEx is malfunctioning.

DHowett commented 5 months ago

(For thread tending purposes, I moved the byte swapping discussion over to #16795)

j4james commented 5 months ago

FYI, I came across a discussion recently on the issue tracker of another terminal (most likely VTE), where somebody was asking for similar functionality. The use case was for multiplexers like tmux and zellij, which are in a similar situation to us with conpty, i.e. needing to know the palette of the connected terminal.

I think one of the proposals was to trigger SIGWINCH whenever the color changes, with the assumption that the app could then request the current palette whenever that happens (personally I don't think that's a great idea though).

The other suggestion was something like a mode that the app would set letting the terminal know that it wanted to receive automatic updates. This seemed a more promising approach to me, but it would need to cover more than just palette changes in my opinion (I made some notes on this subject in https://github.com/microsoft/terminal/issues/16672#issuecomment-1930711349).

Anyway, the main reason I wanted to bring this up was because it would be good if we could agree with other terminals on a standard for this if they are going to be doing the same sort of thing.

bash commented 5 months ago

I think one of the proposals was to trigger SIGWINCH whenever the color changes [...]

That was my proposal / patch you came across :)

Here's the link for reference: https://gitlab.gnome.org/GNOME/vte/-/issues/2740