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

Enable DECCOLM support via a private mode escape sequence #1709

Merged
merged 3 commits into from
Jul 2, 2019

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jun 28, 2019

Summary of the Pull Request

While the DECCOLM escape sequence is technically implemented, it's disabled by a flag that is not user-accessible. This PR adds support for an escape sequence (XTerm's private mode 40) which essentially enables that flag, so users can turn on DECCOLM support in an XTerm-compatible way. Tested manually, with Vttest, and with unit tests.

PR Checklist

Detailed Description of the Pull Request / Additional comments

My initial plan was to build this on top of the existing _fIsSetColumnsEnabled flag, but after further investigation that proved not to be practical. In order to be compatible with XTerm, the flag needs to be checked in the _DoDECCOLMHelper method rather than in SetColumns, since it needs to prevent all facets of the DECCOLM operation taking effect - not just the column sizing.

And in the future, if the SetColumns method is also going to be used to support the DECSCPP escape sequence, that will need a flag of its own - it's not controlled by the same option as DECCOLMN.

For those reasons, I've removed the existing _fIsSetColumnsEnabled flag and added a new _fIsDECCOLMAllowed flag which is checked in the _DoDECCOLMHelper method. I've then added a EnableDECCOLMSupport method to toggle that flag, and added a case in _PrivateModeParamsHelper to call that method in response to the private mode 40 sequence.

While I did mention my initial plans on issue #171, I never actually got feedback from core contributors, so I completely understand if this PR is rejected in favour of a different approach. I'm just offering it as one possible solution.

Validation Steps Performed

I've added an output engine unit test which validates that the private mode escape sequences trigger the EnableDECCOLMSupport call, and some screen buffer unit tests which check that the DECCOLM sequence does take effect when allowed, and doesn't do anything when disallowed.

I've also done a fair amount of manual testing, and run several of the Vttest suites which toggle between 80 and 132 column modes, and which now work (at least in terms of column width) where previously they didn't.

@zadjii-msft zadjii-msft requested a review from miniksa July 1, 2019 17:46
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 is great. Thanks!

@DHowett-MSFT
Copy link
Contributor

Curious: What happens in xterm if you DECCOLM and then enter the alternate screen buffer? Or, enable private mode 40, then enter the alt buffer? Or enter the alt buffer, then enable DECCOLM?

@j4james
Copy link
Collaborator Author

j4james commented Jul 2, 2019

Curious: What happens in xterm if you DECCOLM and then enter the alternate screen buffer? Or, enable private mode 40, then enter the alt buffer? Or enter the alt buffer, then enable DECCOLM?

Good questions. First off, private mode 40 doesn't seem to be affected by anything other than the SM/RM mode sequences themselves. Not even an RIS hard reset will change that.

As for DECCOLM itself, changing back and forth to the alt buffer doesn't seem to affect it. If you made the screen size smaller while you're in the alt buffer (by whatever means), once you restore the original buffer, you can end up losing content that is now cut off by the smaller screen size. That makes sense when you consider that the resize could be a manual user choice, which they're unlikely to want reset when the buffer is restored.

In the Windows terminal it seems the full original buffer is restored, but not the original size, so you'll get a horizontal scroll bar. This doesn't seem unreasonable either, but it's not exactly compatible with XTerm. So that may well be something you want to fix, but I think it's probably orthogonal to the DECCOLM functionality - it's already an issue for the other resizing escape sequence (CSI 8 ; h ; w t).

Another difference I noticed in testing now is that XTerm resets DECCOLM (i.e. the screen width goes back to 80) after an RIS hard reset (it doesn't reset other resizing operations though). If we wanted to match that behaviour, I think we'd have to add an additional state variable to track the fact that DECCOLM had been set, so we know to reset it. I'm not sure if that is really worth the effort.

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 awesome. Thanks for working on it!

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.

I'm good with this.

@miniksa miniksa merged commit 2e0e962 into microsoft:master Jul 2, 2019
@j4james j4james deleted the feature-mode-40 branch July 2, 2019 19:06
@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.

DECCOLM escape sequence doesn't work
4 participants