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 the HPR and VPR escape sequences #4297

Merged
3 commits merged into from
Jan 21, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jan 19, 2020

Summary of the Pull Request

This PR adds support for the HPR and VPR escape sequences from the VT510 terminal. HPR moves the cursor position forward by a given number of columns, and VPR moves the cursor position downward by a given number of rows. They're similar in function to the CUF and CUD escape sequences, except that they're not constrained by the scrolling margins.

References

#3628 provided the new _CursorMovePosition method that made these operations possible

PR Checklist

  • Closes Add support for the VPR and HPR escape sequences #3428
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Most of the implementation is in the new _CursorMovePosition method that was created in PR #3628, so all we're really doing here is hooking up the escape sequences to call that method with the appropriate parameters.

Validation Steps Performed

I've extended the existing state machine tests for CSI cursor movement to confirm that the HPR and VPR sequences are dispatched correctly, and also added screen buffer tests to make sure the movement is clamped by the screen boundaries and not the scrolling margins (we don't yet support horizontal margins, but the test is at least in place for when we do eventually add that support).

I've also checked the HPR and VPR tests in Vttest (under Test non-VT100 / ISO-6429 cursor-movement) and confirmed that they are now working as expected.

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.

Wow, so straightforward. Great work!

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Conhost For issues in the Console codebase labels Jan 20, 2020
Copy link
Contributor

@DHowett-MSFT DHowett-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 is great, as always. Thanks!

@DHowett-MSFT
Copy link
Contributor

@msftbot merge this in 24 hours

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

ghost commented Jan 20, 2020

Hello @DHowett-MSFT!

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 won't merge this pull request until after the UTC date Tue, 21 Jan 2020 22:39:06 GMT, which is in 1 day

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".

@ghost ghost merged commit cbb87b9 into microsoft:master Jan 21, 2020
@j4james j4james deleted the feature-hpr-vpr branch January 26, 2020 23:36
@ghost
Copy link

ghost commented Feb 13, 2020

🎉Windows Terminal Preview v0.9.433.0 has been released which incorporates this pull request.:tada:

Handy links:

@DHowett-MSFT
Copy link
Contributor

🎉 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 19603.

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 Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for the VPR and HPR escape sequences
3 participants