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 VT52 emulation #4789

Merged
9 commits merged into from
Jun 1, 2020
Merged

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Mar 3, 2020

Summary of the Pull Request

This PR adds support for the core VT52 commands, and implements the DECANM private mode sequence, which switches the terminal between ANSI mode and VT52-compatible mode.

References

PR #2017 defined the initial specification for VT52 support.
PR #4044 removed the original VT52 cursor ops that conflicted with VT100 sequences.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Most of the work involves updates to the parsing state machine, which behaves differently in VT52 mode. CSI, OSC, and SS3 sequences are not applicable, and there is one special-case escape sequence (Direct Cursor Address), which requires an additional state to handle parameters that come after the final character.

Once the parsing is handled though, it's mostly just a matter of dispatching the commands to existing methods in the ITermDispatch interface. Only one new method was required in the interface to handle the Identify command.

The only real new functionality is in the TerminalInput class, which needs to generate different escape sequences for certain keys in VT52 mode. This does not yet support all of the VT52 key sequences, because the VT100 support is itself not yet complete. But the basics are in place, and I think the rest is best left for a follow-up issue, and potentially a refactor of the TerminalInput class.

I should point out that the original spec called for a new Graphic Mode character set, but I've since discovered that the VT terminals that emulate VT52 just use the existing VT100 Special Graphics set, so that is really what we should be doing too. We can always consider adding the VT52 graphic set as a option later, if there is demand for strict VT52 compatibility.

Validation Steps Performed

I've added state machine and adapter tests to confirm that the DECANM mode changing sequences are correctly dispatched and forwarded to the ConGetSet handler. I've also added state machine tests that confirm the VT52 escape sequences are dispatched correctly when the ANSI mode is reset.

For fuzzing support, I've extended the VT command fuzzer to generate the different kinds of VT52 sequences, as well as mode change sequences to switch between the ANSI and VT52 modes.

In terms of manual testing, I've confirmed that the Test of VT52 mode in Vttest now works as expected.

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase labels Mar 10, 2020
@j4james j4james changed the title Add support for a VT52 emulation Add support for VT52 emulation Mar 17, 2020
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.

This seems fine to me, and there's a lot of good work in here. Sorry we took so long :P

src/terminal/input/terminalInput.cpp Show resolved Hide resolved
@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label May 29, 2020
@ghost ghost requested review from miniksa, carlos-zamora and leonMSFT May 29, 2020 20:27
@DHowett
Copy link
Member

DHowett commented May 29, 2020

@msftbot make sure @miniksa signs off on this

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 29, 2020
@ghost
Copy link

ghost commented May 29, 2020

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@DHowett DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label May 29, 2020
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Excellent, @j4james. Very glad to have this. Again and as usual lately, sorry for the delay.

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 1, 2020
@ghost
Copy link

ghost commented Jun 1, 2020

Hello @zadjii-msft!

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 d92c829 into microsoft:master Jun 1, 2020
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.

I read this post-merge and just wanted to say it's stellar as always.

@ghost ghost removed the Needs-Second It's a PR that needs another sign-off label Jun 1, 2020
@j4james j4james deleted the feature-vt52-support branch June 4, 2020 23:02
@ghost
Copy link

ghost commented Jun 18, 2020

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

Handy links:

@ghost ghost mentioned this pull request Jun 18, 2020
@DHowett
Copy link
Member

DHowett commented Jul 2, 2020

🎉 Once again, thanks for the contribution!

This pull request was included in a set of conhost changes that was just
released with Windows Insider Build 20161.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate ESC D defined as IND vs CUB
5 participants