Add support for OSC 10 and 11 to set the default colors #891
Conversation
|
i don't think this correctly handles chaining to following OSC sequences. if you have at a glance, looks like OSC 4 isn't quite handled well either, but i think that's a known bug. |
Jaykul
force-pushed
the
dev/jaykul/SettableDefaultColors
branch
from
873e4c4
to
436a4d7
Compare
|
@Jaykul hey, congrats on being the first external person to spelunk through terminal, conpty and conhost to add a new feature! Thanks, this is really cool. I'm requesting review from the two people who know the area best. 😄 |
There was a problem hiding this comment.
I'd also make sure to add a test for these guys. The right place to do this might be in ScreenBufferTests.cpp - there are a lot of tests in there that use VT to set the colors and check them again.
I have a few minor notes about making sure there are doc comments, but otherwise great job 👍
ghost
added
Needs-Author-Feedback
| StateMachine& stateMachine = mainBuffer.GetStateMachine(); | ||
|
|
||
| const COLORREF originalColor = gci.GetDefaultBackgroundColor(); | ||
| const COLORREF testColor = RGB(0x33, 0x66, 0x99); |
There was a problem hiding this comment.
just on a lark: can we test things that go outside the 0-255 or 0-ff (?) range? some negative cases and some cases for longer inputs might be interesting to see
There was a problem hiding this comment.
Sure, but the parser for that isn't my code (I re-used the existing color parser).
There was a problem hiding this comment.
Added just a couple. How deep do you want to go? They're all just going to silently do nothing unless we find a bug in the parsers.
There should probably be (maybe there are) unit tests for the color parser already?
Jaykul
force-pushed
the
dev/jaykul/SettableDefaultColors
branch
from
94ec7f9
to
6481719
Compare
|
@vapier wrote:
That's correct. It doesn't handle chaining at all. It also doesn't handle color names. Since this was my first PR, I was trying not to rewrite, but to just add a little on top of what was there and learn how things work -- I even reused the color parser that was written for OSC 4, though it won't support color names, meaning you can only set RGB values, and not just reuse colors from your base 16 for foreground and background. The structure of the OSC handler in the State Machine seems designed on the assumption that they are single-valued (meaning one value per escape sequence), which makes chaining basically impossible. Basically, still to do:
Those things are definitely needed, but didn't seem necessary for a first pass. Somewhere along the way someone will need to decide if we're going to do special colors for bold/italic/underline/blink/reverse ... and which window properties should be settable by OSC 3 😉 @oising? |
|
This latest build is failing |
|
I'm not sure if missing support for multiple parameters is worse than not supporting it at all. Not saying you need to fix the world before making any progress, but certainly both multi value sequences (OSC 4 & OSC 10+) need fixing at some point before the VT parser could be considered reliable. Is there a document somewhere in the code base noting known deficiencies (i.e. known issues)? That'd be useful for users too. I'm just making observations from the sidelines ... I'm not a developer for this project ;). |
I will say as a developer for this project and say that missing support for multiple parameters is not worse than not supporting it at all. While we don't technically have a complete parser without support for multi-param OSC sequences, we definitely have a good enough parser without them. While we won't reliably work with 100% of scenarios, we do work well enough with 99% of scenarios that are actually used. We've been working on the VT implementation iteratively, and getting better with each release. Someday soon we may even have 100% compatibility. I've moved this discussion to #942, so we can track adding support there. @Jaykul That's one of those tests that's just a little flaky. I'll re-run the CI, it should go away. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Overall, very good. Great job following the existing style/pattern as you implemented it. I fully agree that it's important to work incrementally here and get something going and then improve.
I mostly had nitpicks for a quick fix before commit and a few things that probably need to be turned into future work Issues.
Send a little cleanup revision and I'll swap to Approve and we can get this merged.
Thank you very much for your contribution!
ghost
added
Needs-Author-Feedback
Also fix a few outdated comments and whitespace
Jaykul
force-pushed
the
dev/jaykul/SettableDefaultColors
branch
from
cf57467
to
ea978ed
Compare
|
Rebased on current master (to handle merge issues) |
|
Sorry if this is the wrong place to ask, but how do I use this? |
|
The color format that the xterm color OSCs take is certainly strange 😄 it's You'll want to try |
Add support for OSC 10 and 11 to set the default colors
Supports setting the rgb value for the default foreground and background (but no support for querying yet, same as OSC 4)
PR Checklist
This adds support for OSC 10 to set the default foreground color and OSC 11 to set the default background color (just as if they were set in the profiles.json, except that the settings are not changes, so they can be reloaded).
I have not implemented querying (which is also mentioned in #317), so this only addresses one item in the list.
I have not really added any unit tests -- I'm happy to do so, but unsure where they should go, or what they look like. I found some mocks for the other OSC codes, and implemented those, but didn't spot tests.