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

Prevent the horizontal tab character wrapping at the end of a line #3197

Merged
merged 5 commits into from
Dec 4, 2019

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Oct 15, 2019

Summary of the Pull Request

When a horizontal tab ('\t') is output on the last column of the screen, the current implementation moves the cursor position to the start of the next line. However, the DEC STD 070 manual specifies that a horizontal tab shouldn't move past the last column of the active line (or the right margin, if we supported horizontal margins). This PR updates the forward tab implementation, to prevent it wrapping onto a new line when it reaches the end of a line.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Originally the SCREEN_INFORMATION::GetForwardTab method had a condition which handled a tab at the end of the line as a special case, moving the cursor to the start of the next line. I've simply removed that condition, so an end-of-line tab is handled the same way as any other position (in this case it will just leaves the cursor where it is).

While testing, though, I found that there were circumstances where you could have tab stops greater than the width of the screen, and when that happens, a tab can still end up wrapping onto the next line. To fix that I had to add an additional check to make sure the tab position was always clamped to the width of the buffer.

With these fixes in place, a tab control should now never move off the active line, so I realised that the DoPrivateTabHelper function could be optimized to calculate all of the tab movements in advance, and then only make a single call to AdjustCursorPosition with the final coordinates. This change is not strictly necessary, though, so it can easily be reverted if there are any objections.

Regarding backwards compatibility, note that the GetForwardTab method is only used in two places:

  • when handling a tab character in the WriteCharsLegacy function, but this only applies in VT mode (see here).
  • when handling the CHT escape sequence in the DoPrivateTabHelper function, and obviously an escape sequence would also only be applicable in VT mode.

So this change should have no effect on legacy console applications, which wouldn't have VT mode activated.

Validation Steps Performed

I've added another step to the TestGetForwardTab test which makes sure that a horizontal tab won't wrap at the end of a line.

I've also confirmed that this fixes the last remaining issue in the Test of autowrap in Vttest (pages 3 and 4 of the Test of cursor movements). Although I should note that this only works in conhost.

@DHowett-MSFT DHowett-MSFT added Product-Conhost For issues in the Console codebase Area-VT Virtual Terminal sequence support labels Oct 15, 2019
@zadjii-msft zadjii-msft added this to the Terminal-1912 milestone Dec 4, 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.

Yea this all seems reasonable. Thanks!

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 4, 2019
@carlos-zamora carlos-zamora merged commit 5c43f05 into microsoft:master Dec 4, 2019
@j4james j4james deleted the fix-eol-tabs branch December 15, 2019 01:50
@ghost
Copy link

ghost commented Jan 14, 2020

🎉Windows Terminal Preview v0.8.10091.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
Area-VT Virtual Terminal sequence support 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.

TAB character shouldn't move past the end of the line
4 participants