Add support for VT100 Auto Wrap Mode (DECAWM) #3943

j4james · 2019-12-13T01:19:18Z

Summary of the Pull Request

This adds support for the DECAWM private mode escape sequence, which controls whether or not the output wraps to the next line when the cursor reaches the right edge of the screen. Tested manually, with Vttest, and with some new unit tests.

PR Checklist

Closes Add support for VT100 Auto Wrap Mode (DECAWM) #3826
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: Add support for VT100 Auto Wrap Mode (DECAWM) #3826

Detailed Description of the Pull Request / Additional comments

The idea was to repurpose the existing ENABLE_WRAP_AT_EOL_OUTPUT mode, but the problem with that was it didn't work in VT mode - specifically, disabling it didn't prevent the wrapping from happening. This was because in VT mode the WC_DELAY_EOL_WRAP behaviour takes affect, and that bypasses the usual codepath where ENABLE_WRAP_AT_EOL_OUTPUT is checked,

To fix this, I had to add additional checks in the WriteCharsLegacy function (7dbefe0) to make sure the WC_DELAY_EOL_WRAP mode is only activated when ENABLE_WRAP_AT_EOL_OUTPUT is also set.

Once that was fixed, though, another issue came to light: the ENABLE_WRAP_AT_EOL_OUTPUT mode doesn't actually work as documented. According to the docs, "if this mode is disabled, the last character in the row is overwritten with any subsequent characters". What actually happens is the cursor jumps back to the position at the start of the write, which could be anywhere on the line.

This seems completely broken to me, but I've checked in the Windows XP, and it has the same behaviour, so it looks like that's the way it has always been. So I've added a fix for this (9df9849), but it is only applied in VT mode.

Once that basic functionality was in place, though, we just needed a private API in the ConGetSet interface to toggle the mode, and then that API could be called from the AdaptDispatch class when the DECAWM escape sequence was received.

One last thing was to reenable the mode in reponse to a DECSTR soft reset. Technically the auto wrap mode was disabled by default on many of the DEC terminals, and some documentation suggests that DECSTR should reset it to that state, But most modern terminals (including XTerm) expect the wrapping to be enabled by default, and DECSTR reenables that state, so that's the behaviour I've copied.

Validation Steps Performed

I've add a state machine test to confirm the DECAWM escape is dispatched correctly, and a screen buffer test to make sure the output is wrapped or clamped as appropriate for the two states.

I've also confirmed that the "wrap around" test is now working correctly in the Test of screen features in Vttest.

miniksa

Just one minor request comment before I'll sign.
Overall, I think this is fine. Thank you.

I want to get this in before I work on the boogaloo stream-writing fixes (#780) that are intended to replace WriteCharsLegacy. I'm a bit nervous about changing WriteCharsLegacy and AdjustCursorPosition here because they've historically been bug factories (which is why #780 is a thing). But I accept your justifications and mitigation of risk by scoping it to VT here.

src/host/getset.cpp

j4james · 2020-01-02T15:39:42Z

I want to get this in before I work on the boogaloo stream-writing fixes (#780) that are intended to replace WriteCharsLegacy.

If #780 is likely to happen soonish, I'm actually wondering now if we're better off leaving this PR for the time being, and then later reimplement it on top of the new stream writer? At that point, none of this code would be needed in WriteCharsLegacy and AdjustCursorPosition, so we'd just be pulling it all out again. I don't mind either way.

miniksa · 2020-01-02T18:26:58Z

I want to get this in before I work on the boogaloo stream-writing fixes (#780) that are intended to replace WriteCharsLegacy.

If #780 is likely to happen soonish, I'm actually wondering now if we're better off leaving this PR for the time being, and then later reimplement it on top of the new stream writer? At that point, none of this code would be needed in WriteCharsLegacy and AdjustCursorPosition, so we'd just be pulling it all out again. I don't mind either way.

Let's bring it in because it will guide the way I work. And I think these in conjunction with your C0 proposal will make #780 more self-evident.

miniksa

K looks good.

j4james · 2020-01-16T01:32:20Z

(╯‵□′)╯︵┻━┻

miniksa · 2020-01-16T22:37:40Z

(╯‵□′)╯︵┻━┻

Going to try to re-run/merge until success.

miniksa · 2020-01-16T23:00:00Z

OK after I busted #4171 by clicking update branch, I don't really want to do it here. It looks like the build cannot pull the merge result branch. Can you update your branch, @j4james?

j4james · 2020-01-17T01:51:43Z

I thought I had it working, but it looks like I'm going to need to merge again or rebase. I have to go to sleep now, and I have work tomorrow, but will try again tomorrow evening or over the weekend.

miniksa · 2020-01-22T23:26:44Z

The merge looked super easy so I did it in the web editor. Marking AutoMerge. If build succeeds, we're good to go.

ghost · 2020-01-22T23:26:58Z

Hello @miniksa!

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.

j4james · 2020-01-22T23:47:58Z

The merge looked super easy so I did it in the web editor. Marking AutoMerge. If build succeeds, we're good to go.

I think the web editor screws up line endings, which is something the code formatter now checks, and I suspect that is why it's failing.

j4james · 2020-01-23T00:24:15Z

@miniksa Is it OK if I just force-push a rebase? I don't know how else to fix things at this point.

DHowett-MSFT · 2020-01-23T00:24:46Z

@j4james that'll be fine. Sorry about that 😄

miniksa · 2020-01-23T00:26:30Z

@j4james that'll be fine. Sorry about that 😄

Yes, it's fine. Sorry, everyone stood in my office for like 45 minutes BSing so I didn't see this as it came in.

Also, apologies that I keep learning bad merging lessons on your branches.

…BLE_WRAP_AT_EOL_OUTPUT mode is set.

…P_AT_EOL_OUTPUT mode.

…OL_OUTPUT mode.

…patched correctly.

…ing is disabled in VT mode.

…mped appropriately for the mode.

…ew audit mode.

j4james · 2020-01-23T13:34:47Z

FYI, the current build failure is just the unit tests timing out, which seems to happen quite often in the CI build. I don't think there's actually anything wrong with the code itself.

DHowett-MSFT · 2020-01-23T16:53:08Z

/azp run

azure-pipelines · 2020-01-23T16:53:31Z

Azure Pipelines successfully started running 1 pipeline(s).

miniksa · 2020-01-28T23:10:29Z

FYI, the current build failure is just the unit tests timing out, which seems to happen quite often in the CI build. I don't think there's actually anything wrong with the code itself.

Yeah , looking at this in #4384

miniksa · 2020-02-04T00:19:46Z

WELP.

ghost · 2020-02-13T18:02:08Z

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

Handy links:

DHowett-MSFT · 2020-04-08T19:59:42Z

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

zadjii-msft added Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase labels Dec 16, 2019

j4james force-pushed the feature-decawm branch from 1039a3e to 555e773 Compare December 21, 2019 16:42

miniksa reviewed Dec 31, 2019

View reviewed changes

src/host/getset.cpp Outdated Show resolved Hide resolved

miniksa approved these changes Jan 2, 2020

View reviewed changes

miniksa added the Needs-Second It's a PR that needs another sign-off label Jan 2, 2020

ghost requested review from zadjii-msft, carlos-zamora and DHowett-MSFT January 2, 2020 18:30

j4james force-pushed the feature-decawm branch from 1e9d39c to febbbf8 Compare January 5, 2020 22:33

carlos-zamora approved these changes Jan 13, 2020

View reviewed changes

j4james force-pushed the feature-decawm branch from febbbf8 to 0d8eb11 Compare January 16, 2020 00:49

ghost requested a review from leonMSFT January 16, 2020 00:49

j4james force-pushed the feature-decawm branch from dc2bcdd to 0c6944f Compare January 17, 2020 23:38

miniksa removed the Needs-Second It's a PR that needs another sign-off label Jan 22, 2020

miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 22, 2020

j4james added 2 commits January 23, 2020 00:27

Make sure the "delay EOL wrap" behaviour is only applied when the ENA…

295b0e1

…BLE_WRAP_AT_EOL_OUTPUT mode is set.

Add a private API in the ConGetSet interface to toggle the ENABLE_WRA…

49d899f

…P_AT_EOL_OUTPUT mode.

j4james added 7 commits January 23, 2020 00:41

Add support for DECAWM escape sequence to toggle the ENABLE_WRAP_AT_E…

16b0d8a

…OL_OUTPUT mode.

Reenable the auto wrap mode when a DECSTR soft reset is triggered.

036d2d1

Add a state machine test to confirm the DECAWM escape sequence is dis…

552d37b

…patched correctly.

Make sure the cursor is left in the last column of the row when wrapp…

47bddbc

…ing is disabled in VT mode.

Add a screen buffer test to confirm that the output is wrapped or cla…

7d5193d

…mped appropriately for the mode.

Use WI_UpdateFlag instead of a conditional SetFlag and ClearFlag.

1a2bca4

Make the TermDispatch::SetAutoWrapMode method noexcept, to pass the n…

dc9a4b6

…ew audit mode.

j4james force-pushed the feature-decawm branch from 680bd61 to dc9a4b6 Compare January 23, 2020 01:07

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 23, 2020

zadjii-msft mentioned this pull request Jan 23, 2020

Some of the unittests are flaky in CI #4344

Closed

miniksa added 2 commits January 30, 2020 13:56

Merge branch 'master' into feature-decawm

a4c22cc

Merge branch 'master' into feature-decawm

432ce50

miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 4, 2020

ghost merged commit 0d92f71 into microsoft:master Feb 4, 2020

ghost mentioned this pull request Feb 13, 2020

Add support for VT100 Auto Wrap Mode (DECAWM) #3826

Closed

j4james deleted the feature-decawm branch February 29, 2020 23:55

DHowett mentioned this pull request Nov 25, 2020

ANSI control code for line wrapping not supported? #8404

Closed

chrisant996 mentioned this pull request Dec 21, 2021

Pasting long lines does not wrap the text correctly Maximus5/ConEmu#2404

Open

na-na-hi mentioned this pull request Jun 24, 2024

console.lua: fix the max width calculation mpv-player/mpv#14419

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for VT100 Auto Wrap Mode (DECAWM) #3943

Add support for VT100 Auto Wrap Mode (DECAWM) #3943

j4james commented Dec 13, 2019 •

edited

Loading

miniksa left a comment

j4james commented Jan 2, 2020

miniksa commented Jan 2, 2020

miniksa left a comment

j4james commented Jan 16, 2020

miniksa commented Jan 16, 2020

miniksa commented Jan 16, 2020

j4james commented Jan 17, 2020

miniksa commented Jan 22, 2020

ghost commented Jan 22, 2020

j4james commented Jan 22, 2020

j4james commented Jan 23, 2020

DHowett-MSFT commented Jan 23, 2020

miniksa commented Jan 23, 2020

j4james commented Jan 23, 2020

DHowett-MSFT commented Jan 23, 2020

azure-pipelines bot commented Jan 23, 2020

miniksa commented Jan 28, 2020

miniksa commented Feb 4, 2020

ghost commented Feb 13, 2020

DHowett-MSFT commented Apr 8, 2020

Add support for VT100 Auto Wrap Mode (DECAWM) #3943

Add support for VT100 Auto Wrap Mode (DECAWM) #3943

Conversation

j4james commented Dec 13, 2019 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

miniksa left a comment

Choose a reason for hiding this comment

j4james commented Jan 2, 2020

miniksa commented Jan 2, 2020

miniksa left a comment

Choose a reason for hiding this comment

j4james commented Jan 16, 2020

miniksa commented Jan 16, 2020

miniksa commented Jan 16, 2020

j4james commented Jan 17, 2020

miniksa commented Jan 22, 2020

ghost commented Jan 22, 2020

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.

j4james commented Jan 22, 2020

j4james commented Jan 23, 2020

DHowett-MSFT commented Jan 23, 2020

miniksa commented Jan 23, 2020

j4james commented Jan 23, 2020

DHowett-MSFT commented Jan 23, 2020

azure-pipelines bot commented Jan 23, 2020

miniksa commented Jan 28, 2020

miniksa commented Feb 4, 2020

ghost commented Feb 13, 2020

DHowett-MSFT commented Apr 8, 2020

j4james commented Dec 13, 2019 •

edited

Loading

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.