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

Colors palette changes using SetConsoleScreenBufferInfoEx should trigger equivalent OSC 4 sequences #7888

Closed
PhMajerus opened this issue Oct 11, 2020 · 7 comments
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@PhMajerus
Copy link

PhMajerus commented Oct 11, 2020

Win32 CUI apps typically use SetConsoleScreenBufferInfoEx to change the 16 colors palette used by the console.
Now that conhost can be used to connect these apps to a terminal, as much as possible of the original Console API needs to get translated to VT control sequences.

In the current implementation, conhost records the palette changes, as GetConsoleScreenBufferInfoEx will show the colors have been updated, but it will be kept purely as an internal state and the connected terminal app will not change its colors palette.

For Win32 CUI apps that change their colors to work properly with terminals, for any color that has been changed in the CONSOLE_SCREEN_BUFFER_INFOEX structure, we need conhost to generate and send the equivalent inband OSC 4 control sequences (ESC ] 4) to the terminal app.
(https://docs.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences#screen-colors)

The following screenshot shows before and after changing the console colors, note how the right shows the new palette as it requests the actual conhost values through API and shows them using RGB/24-bit colors control sequences, while the colors wheel isn't updated because it uses 16-colors control sequences relying on the terminal's colors palette.
image

Same after a function has been used to get the console colors and generate and send equivalent OSC 4 control sequences.
image

My applyConsoleColorsToTerminal function is a workaround that simply gets the 16 colors using GetConsoleScreenBufferInfoEx and generates the 16 equivalent OSC 4 sequences to apply them to the attached terminal.

@PhMajerus PhMajerus added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Oct 11, 2020
@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 Oct 11, 2020
@PhMajerus
Copy link
Author

PhMajerus commented Oct 11, 2020

Note it seems doing this could also fix #399, as the legacy console properly refreshes its whole client area when the background color gets changed through OSC 4.

@j4james
Copy link
Collaborator

j4james commented Oct 11, 2020

Isn't this just a duplicate of #2985?

@PhMajerus
Copy link
Author

Ah, sorry, didn't spot that one when looking for existing colors issues. Indeed, it is the same core issue, we need to propagate palette changes with OSC 4 to fix this and hopefully #399 as well.

@j4james
Copy link
Collaborator

j4james commented Oct 11, 2020

#399 is a conhost bug though. It's got nothing to do with conpty. I've actually hacked together fixes for both of these issues in the past, just as a proof of concept. Fixing them properly is more complicated though, which is why I never got around to submitting PRs.

@PhMajerus
Copy link
Author

Could be, as the console definitely should refresh its whole client area when the colors are changed, however, it seems it does it properly when an OSC 4 command is processed, so it finally updates on that second pass.
While not fixing the original bug, it is still better than the bug we've been living with for over a year and a half by now. I really hope we can get all those colors issues fixed for 21H1 release.

@DHowett
Copy link
Member

DHowett commented Oct 11, 2020

Thanks all! /dup #2985

@ghost
Copy link

ghost commented Oct 11, 2020

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 Oct 11, 2020
@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 Oct 11, 2020
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. 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

3 participants