Add support for resetting the color scheme with RIS#17879
Add support for resetting the color scheme with RIS#17879DHowett merged 2 commits intomicrosoft:mainfrom
Conversation
| // Save the changes made above and in UpdateAppearance as the new default render settings. | ||
| GetRenderSettings().SaveDefaultSettings(); |
There was a problem hiding this comment.
If the code above calls SetColorTableEntry, won't this overwrite the modified colors set by the terminal application (as if a RIS was received)?
There was a problem hiding this comment.
Yeah. That's typically what you want if you're actually using the settings to change the color scheme, but not so much when you're editing an unrelated setting. In fact the colors can even end up being reset when when you haven't edited the settings at all! This is a known problem that is tracked in issue #11522.
There was a problem hiding this comment.
Yeah. That's typically what you want if you're actually using the settings to change the color scheme
I'm somewhat surprised about that - I expected the opposite. But that answered my question. It was the only concern I had, and otherwise the PR looks great. :)
There was a problem hiding this comment.
Just to be clear, I don't feel strongly about it either way, but that's the way it's always worked, and it seemed to me a reasonable behavior. If you open up a shell with a particular color scheme, decide you don't like it, so you change the scheme in the settings, wouldn't you expect that shell to update straight away?
Of course if you were busy using an application that set its own palette, having it overwritten would be annoying, but it's much less likely you'd be trying to switch your color scheme in that situation (this assumes that #11522 is fixed, so changing other settings wouldn't trigger a color scheme refresh).
Either way, if we want to reconsider that behavior, I think it's best raised as a separate issue.
## Summary of the Pull Request This improves our `RIS` (hard reset) implementation, so it now also resets any changes that are made to the color table and color aliases, which is one of the things it's supposed to be doing. ## References and Relevant Issues This is also a small step towards implementing the `OSC` sequences that reset individual color table entries (issue #3719). ## Detailed Description of the Pull Request / Additional comments The way this works is by having a second copy of the color table and alias indices to hold the default values in the `RenderSettings` class. This default set is initially populated at startup with the user's chosen color scheme, but can also potentially be updated if the user changes their settings while a session is already in progress. When we receive an `RIS` request, we just copy the default values back over the active settings, and refresh the renderer. ## Validation Steps Performed I've manually tested both OpenConsole and Windows Terminal by changing my color scheme programmatically, and then confirming that the original colors are restored when an `RIS` sequence is received. I've also added some basic unit tests that check both the color aliases and color table are restored by `RIS`. ## PR Checklist - [x] Tests added/passed (cherry picked from commit 5e8e10f) Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgTTOyY Service-Version: 1.22
This pull request adds support for resetting the various color table entries and xterm resource values back to their defaults. Building on the default color table James introduced in #17879, it was relatively straightforward to add support for resetting specific entries. This implementation cleaves tightly to observed behavior in xterm(379) rather than observed behavior in libvte(0.70.6). They differ in the following ways: - xterm rejects any OSC [110..119] with any number of parameters; libvte accepts it but only resets the first color. - When passed a list of color indices to reset in 104, xterm resets any colors up until the first one which fails to parse as an integer and does _not_ reset the rest; libvte resets all parseable color indices. I was unable to verify how these reset commands interact with colors set via `DECAC Assign Color` so I went with the implementation that made the most sense: - Resetting the background color with `110` also restores the background color alias entry to its pre-`DECAC` value; this results in the perceived background color returning to e.g. index 0 in conhost and the `background` color in Terminal. - _ibid._ for the foreground color Refs #18695 Refs #17879 Closes #3719
This pull request adds support for resetting the various color table entries and xterm resource values back to their defaults. Building on the default color table James introduced in #17879, it was relatively straightforward to add support for resetting specific entries. This implementation cleaves tightly to observed behavior in xterm(379) rather than observed behavior in libvte(0.70.6). They differ in the following ways: - xterm rejects any OSC [110..119] with any number of parameters; libvte accepts it but only resets the first color. - When passed a list of color indices to reset in 104, xterm resets any colors up until the first one which fails to parse as an integer and does _not_ reset the rest; libvte resets all parseable color indices. I was unable to verify how these reset commands interact with colors set via `DECAC Assign Color` so I went with the implementation that made the most sense: - Resetting the background color with `110` also restores the background color alias entry to its pre-`DECAC` value; this results in the perceived background color returning to e.g. index 0 in conhost and the `background` color in Terminal. - _ibid._ for the foreground color Refs #18695 Refs #17879 Closes #3719 (cherry picked from commit 5f31150) Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgZOWsU Service-Version: 1.22
This pull request adds support for resetting the various color table entries and xterm resource values back to their defaults. Building on the default color table James introduced in #17879, it was relatively straightforward to add support for resetting specific entries. This implementation cleaves tightly to observed behavior in xterm(379) rather than observed behavior in libvte(0.70.6). They differ in the following ways: - xterm rejects any OSC [110..119] with any number of parameters; libvte accepts it but only resets the first color. - When passed a list of color indices to reset in 104, xterm resets any colors up until the first one which fails to parse as an integer and does _not_ reset the rest; libvte resets all parseable color indices. I was unable to verify how these reset commands interact with colors set via `DECAC Assign Color` so I went with the implementation that made the most sense: - Resetting the background color with `110` also restores the background color alias entry to its pre-`DECAC` value; this results in the perceived background color returning to e.g. index 0 in conhost and the `background` color in Terminal. - _ibid._ for the foreground color Refs #18695 Refs #17879 Closes #3719 (cherry picked from commit 5f31150) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgZOWsQ Service-Version: 1.23
Summary of the Pull Request
This improves our
RIS(hard reset) implementation, so it now alsoresets any changes that are made to the color table and color aliases,
which is one of the things it's supposed to be doing.
References and Relevant Issues
This is also a small step towards implementing the
OSCsequences thatreset individual color table entries (issue #3719).
Detailed Description of the Pull Request / Additional comments
The way this works is by having a second copy of the color table and
alias indices to hold the default values in the
RenderSettingsclass.This default set is initially populated at startup with the user's
chosen color scheme, but can also potentially be updated if the user
changes their settings while a session is already in progress.
When we receive an
RISrequest, we just copy the default values backover the active settings, and refresh the renderer.
Validation Steps Performed
I've manually tested both OpenConsole and Windows Terminal by changing
my color scheme programmatically, and then confirming that the original
colors are restored when an
RISsequence is received.I've also added some basic unit tests that check both the color aliases
and color table are restored by
RIS.PR Checklist