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

Add support for resetting the color scheme with RIS #17879

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Sep 8, 2024

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

  • Tests added/passed

Comment on lines +104 to +105
// Save the changes made above and in UpdateAppearance as the new default render settings.
GetRenderSettings().SaveDefaultSettings();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code above calls SetColorTableEntry, won't this overwrite the modified colors set by the terminal application (as if a RIS was received)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@j4james j4james marked this pull request as ready for review September 10, 2024 13:29
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent, thank you

@DHowett DHowett merged commit 5e8e10f into microsoft:main Sep 16, 2024
15 checks passed
DHowett pushed a commit that referenced this pull request Sep 26, 2024
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants