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 HPA escape sequence #3368

Merged
merged 2 commits into from
Oct 31, 2019

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Oct 29, 2019

Summary of the Pull Request

HPA (horizontal position absolute) is just an alias for the existing CHA (cursor horizontal absolute) escape sequence, which moves the cursor position to the specified column on the current line. This is needed in order to pass the HPA test in Vttest.

PR Checklist

Detailed Description of the Pull Request / Additional comments

As discussed in issue #3348, the general consensus is that this can be considered an alias for the CHA escape sequence. So I've just added a new entry in the VTActionCodes enum for the HPA final character, and then updated the switch statements in OutputStateMachineEngine::ActionCsiDispatch to share the same code path as the existing CHA action.

I haven't bothered with giving it a unique telemetry code, since we haven't done that for the CUP and HVP codes either, so I assumed that would be OK.

Validation Steps Performed

I've extended the existing output engine tests for CSI cursor movement to confirm that HPA is dispatched in the same way as CHA. I've also checked the HPA test in Vttest (under Test non-VT100 / ISO-6429 cursor-movement) and confirmed that it is now working as expected.

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.

Looks good to me!

@DHowett-MSFT DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Oct 30, 2019
@ghost ghost requested review from zadjii-msft, miniksa and carlos-zamora October 30, 2019 20:23
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.

Looks like a convenient easy change. Thanks!

@zadjii-msft zadjii-msft added the Product-Conhost For issues in the Console codebase label Oct 31, 2019
@zadjii-msft zadjii-msft merged commit 46ac191 into microsoft:master Oct 31, 2019
@j4james j4james deleted the feature-hpa branch November 2, 2019 12:06
@ghost
Copy link

ghost commented Nov 26, 2019

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 HPA escape sequence
3 participants