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

Support VT100 DECOM Origin Mode #1331

Merged
merged 8 commits into from Jul 2, 2019

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jun 19, 2019

Summary of the Pull Request

This adds support for the DECOM (origin mode) escape sequence, which controls whether cursor positioning is relative to the margins of the scrolling region, or independent of the scrolling region (the original behaviour). Tested manually, with Vttest, and with some unit tests.

PR Checklist

Detailed Description of the Pull Request / Additional comments

This adds two state variables to the AdaptDispatch class to track the origin mode and a saved copy of the origin mode for use with the Save/Restore Cursor commands (DECSC/DECRC). When the origin mode is relative, adjustments are then made to the line number in the _CursorMovePosition and _CursorPositionReport methods to account for the margin offset (this is a slight change from my initial proposal, which was to make those adjustments in the methods that called _CursorMovePosition).

It's a bit complicated when it comes to restoring the saved origin mode in the CursorRestorePosition method, because the saved cursor position is always absolute, so we have to temporarily set the origin mode to the absolute mode before restoring the cursor, and then afterwards restore the actual saved origin mode. But I think that complication is justified if it allows us to keep the origin adjustments centralized in the _CursorMovePosition method (which may one day need to support left/right margins too).

Other changes include updating the SoftReset method to reset the origin mode and the saved origin mode, and reordering the HardReset method to call the SoftReset before resetting the cursor position to home (this makes sure that the home position is absolute, and not relative to the previous margins).

I also updated _DoDECCOLMHelper to reset the origin mode when changing between 80 and 132 column modes. I couldn't find a spec reference for this, but I thought it made sense given that the margins are being reset there. And, as far as I can tell, that's what Xterm is doing too.

Validation Steps Performed

I've added a basic "state machine" unit test which validates that the set/reset escape sequences trigger the SetOriginMode call, and some "screen buffer" units tests which validate various interactions between the origin mode, the margins, and the cursor positioning. I've also done quite a lot of manual testing, not all of which I've been able to replicate in the unit tests (e.g. cursor position reporting).

I've run a couple of the Vttest suites that are dependent on origin mode support and noted that they were at least improved, if not fully working yet (because of other VT features not yet implemented). This includes screens 3 and 4 of the Test of cursor movements, and screens 11 and 12 of the Test of screen features.

@j4james j4james closed this Jun 20, 2019
@j4james
Copy link
Collaborator Author

j4james commented Jun 20, 2019

I've just realised that the bottom margin constraint isn't going to work correctly when the margins are reset to "full screen", since that results in the bottom margin value being zero.

@j4james j4james reopened this Jun 20, 2019
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.

Looks good with my minor comments. But I want to get @zadjii-msft on this as required before merging.

src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
@miniksa miniksa requested a review from zadjii-msft June 24, 2019 18:02
@miniksa
Copy link
Member

miniksa commented Jun 24, 2019

@msftbot make sure @zadjii-msft signs off on this

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 24, 2019
@ghost
Copy link

ghost commented Jun 24, 2019

Hello @miniksa!

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 @zadjii-msft

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 removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 24, 2019
@j4james j4james changed the title Support VT100 DECOM Orgin Mode Support VT100 DECOM Origin Mode Jun 24, 2019
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.

Great work here, thanks for the contribution!

@miniksa
Copy link
Member

miniksa commented Jul 2, 2019

I fixed the merge conflict. When the CI passes, we'll get it merged.

@miniksa miniksa merged commit fe7fd33 into microsoft:master Jul 2, 2019
@j4james j4james deleted the feature-origin-mode branch July 2, 2019 19:07
@ghost
Copy link

ghost commented Aug 3, 2019

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

Handy links:

@ghost ghost mentioned this pull request Aug 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for VT100 DECOM Orgin Mode
4 participants