Support resetting the colors via OSC 104, 110, 111... #3719
Comments
egmontkob
added
the
Issue-Feature
ghost
added
Needs-Triage
zadjii-msft
added
Product-Conhost
zadjii-msft
removed
the
Issue-Feature
DHowett-MSFT
removed
the
Needs-Triage
|
I've just bounced off this. I'm using a script with OSC 10 and 11 to switch from Solarized Dark to Solarized Light in Administrator PowerShell sessions. However, editing my config then causes all such sessions to have their colours reset, except for a bunch of stuff that seems to keep the colour scheme from the time it was written (under PowerShell, I guess this is PS-Readline at work). I'm particularly interested in the config layering behaviour, but without OSC 104/110/111, being unable to apply config changes is probably a UX regression. Are there any particular concerns with the proposed behaviour, e.g. standarisation/commonality? Or is it just a case of implementing the relevant code in this repo? Or (based on the milestone being a Windows release) does it actually require MS-internal work first to support resetting the colours? |
|
Just speaking for myself, this is quite low down my priority list, because I think it's going to be a bit of a pain. It needs to be implemented twice, because the color palette is one of those things that has to be handled separately by conhost and Windows Terminal. And on the conhost side there really isn't a concept of "default palette" to reset to, because the initial colors can come from a variety of sources (e.g. shortcut links and registry entries), and the source can change at run time. I also have plans to refactor some of the palette management code, and I'm not keen to add any more palette functionality until that refactoring is out of the way. Not that the refactoring is necessarily a prerequisite for this feature, but I'd personally prefer to get that cleaned up first. So basically it seems like a lot of work for a fairly obscure feature, and one which I suspect few people will care about. But that's just my point of view. It's possible someone else may consider this more of a priority and decide to take it on sooner. |
|
Makes sense. It's not in any way a blocker for me, so I'm happy to wait for any planned refactoring to land (and perhaps #942), before I start trying to pick-off low hanging fruit. |
## 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
## 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
|
Well, it looks like I've accidentally licked this cookie. @j4james thanks for adding the default color entry stuff to RenderSettings and calling out this issue in the description. I never would have found it otherwise. :) |
Description of the new feature/enhancement
These are the counterparts of OSC 4, 10, 11 to reset the default color. (And continue the line with 117, 119 if 17, 19 get implemented, etc.)
Proposed technical implementation details (optional)
In VTE + GNOME Terminal we figured out the best is if the OSC 4, 10, 11 sequences have precedence over the UI / config file settings. That is, for each color slot, if its value is defined via OSC 4, 10, 11 then that one is used and the one in the settings is ignored. If a value hasn't been defined via OSC 4, 10, 11, or has been reset via OSC 104, 110, 111 then the value specified in the terminal emulator's settings is used. This way re-applying the same settings is an idempotent operation. (Previously the two sources were fighting with each other, both overwriting the value in the same slot. That way re-applying the user config (e.g. "altering" a color to the same value) would invalidate a previous OSC, which was bad.)
The text was updated successfully, but these errors were encountered: