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 chaining OSC 10-12 #8999

Merged
3 commits merged into from Feb 4, 2021

Conversation

skyline75489
Copy link
Collaborator

@skyline75489 skyline75489 commented Feb 2, 2021

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

@@ -1441,6 +1443,13 @@ class StatefulDispatch final : public TermDispatch
return true;
}

bool SetCursorColor(const DWORD color) noexcept override
Copy link
Collaborator Author

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++;
Copy link
Collaborator Author

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.

@skyline75489
Copy link
Collaborator Author

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.

@skyline75489 skyline75489 added Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Feb 2, 2021
Copy link
Member

@zadjii-msft zadjii-msft left a 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.

Comment on lines +16 to +17
// the console uses 0xffffffff as an "invalid color" value
constexpr COLORREF INVALID_COLOR = 0xffffffff;
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 console uses this as an invalid color, I really hope there's a constant for it somewhere else !

Copy link
Collaborator Author

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.

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 looks great to me. Thank you!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 4, 2021
@ghost
Copy link

ghost commented Feb 4, 2021

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 779354d into microsoft:main Feb 4, 2021
@ghost
Copy link

ghost commented Mar 1, 2021

🎉Windows Terminal Preview v1.7.572.0 has been released which incorporates this pull request.:tada:

Handy links:

@skyline75489 skyline75489 deleted the chesterliu/dev/osc-chaining-10-12 branch March 16, 2021 05:51
@DHowett
Copy link
Member

DHowett commented Mar 17, 2021

This is available in conhost as of Windows Insider Preview build 21337. Congrats! 😄

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants