-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 chaining OSC 10-12 #8999
Add support for chaining OSC 10-12 #8999
Conversation
@@ -1441,6 +1443,13 @@ class StatefulDispatch final : public TermDispatch | |||
return true; | |||
} | |||
|
|||
bool SetCursorColor(const DWORD color) noexcept override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why this was not added before. I need to test this so I add one for my own selfish purpose.
} | ||
TermTelemetry::Instance().Log(TermTelemetry::Codes::OSCSCC); | ||
commandIndex++; | ||
colorIndex++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full range is OSC 10-17. I don't think we'll be adding 13-17 in the near future. But just in case.
Although setting the 10+ range was rarely supported as suggested in #942 (comment), this PR will make WT a more faithful follower of xterm. Personally I like the idea to chain multi OSC commands. So here it is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I wasn't familiar with how OSC chaining like this worked, but this looks right. That's a confusing spec:
Each successive parameter changes the next color in the list. The value of
Ps
tells the starting point in the list.
But this looks like it's implemented correctly.
// the console uses 0xffffffff as an "invalid color" value | ||
constexpr COLORREF INVALID_COLOR = 0xffffffff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the console uses this as an invalid color, I really hope there's a constant for it somewhere else !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh this line originally for ResetCursorColor and was accidentally removed in #7578. I didn’t write it. Just add it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me. Thank you!
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
🎉 Handy links: |
This is available in conhost as of Windows Insider Preview build 21337. Congrats! 😄 |
This adds the support for chaining OSC 10-12, allowing users to set all
of them at once.
BREAKING CHANGE
Before this PR, the OSC 10/11/12 command will only be dispatched iff the
first color is valid. This is no longer true. The new implementation
strictly follows xterm's behavior. Each color is treated independently.
For example,
\e]10;invalid;white\e\\
is effectively\e]11;white\e\\
.Validation Steps Performed
Tests added. Manually tested.
Main OSC color tracking issue: #942
OSC 4 & Initial OSC 10-12 PR: #7578
Closes one item in #942