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

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

# 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.
This commit is contained in:
James Holderness 2019-12-04 18:48:09 +00:00 committed by Carlos Zamora
parent 2f0abc202a
commit 5c43f055c5
4 changed files with 21 additions and 17 deletions

View file

@ -746,7 +746,6 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100;
if (screenInfo.InVTMode())
{
const COORD cCursorOld = cursor.GetPosition();
// Get Forward tab handles tabbing past the end of the buffer
CursorPosition = screenInfo.GetForwardTab(cCursorOld);
}
else

View file

@ -1567,19 +1567,15 @@ void DoSrvPrivateUseMainScreenBuffer(SCREEN_INFORMATION& screenInfo)
{
CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
SCREEN_INFORMATION& _screenBuffer = gci.GetActiveOutputBuffer().GetActiveBuffer();
COORD cursorPos = _screenBuffer.GetTextBuffer().GetCursor().GetPosition();
NTSTATUS Status = STATUS_SUCCESS;
FAIL_FAST_IF(!(sNumTabs >= 0));
for (SHORT sTabsExecuted = 0; sTabsExecuted < sNumTabs && NT_SUCCESS(Status); sTabsExecuted++)
for (SHORT sTabsExecuted = 0; sTabsExecuted < sNumTabs; sTabsExecuted++)
{
const COORD cursorPos = _screenBuffer.GetTextBuffer().GetCursor().GetPosition();
COORD cNewPos = (fForward) ? _screenBuffer.GetForwardTab(cursorPos) : _screenBuffer.GetReverseTab(cursorPos);
// GetForwardTab is smart enough to move the cursor to the next line if
// it's at the end of the current one already. AdjustCursorPos shouldn't
// to be doing anything funny, just moving the cursor to the location GetForwardTab returns
Status = AdjustCursorPosition(_screenBuffer, cNewPos, TRUE, nullptr);
cursorPos = (fForward) ? _screenBuffer.GetForwardTab(cursorPos) : _screenBuffer.GetReverseTab(cursorPos);
}
return Status;
return AdjustCursorPosition(_screenBuffer, cursorPos, TRUE, nullptr);
}
// Routine Description:

View file

@ -2211,12 +2211,7 @@ COORD SCREEN_INFORMATION::GetForwardTab(const COORD cCurrCursorPos) const noexce
{
COORD cNewCursorPos = cCurrCursorPos;
SHORT sWidth = GetBufferSize().RightInclusive();
if (cCurrCursorPos.X == sWidth)
{
cNewCursorPos.X = 0;
cNewCursorPos.Y += 1;
}
else if (_tabStops.empty() || cCurrCursorPos.X >= _tabStops.back())
if (_tabStops.empty() || cCurrCursorPos.X >= _tabStops.back())
{
cNewCursorPos.X = sWidth;
}
@ -2227,7 +2222,8 @@ COORD SCREEN_INFORMATION::GetForwardTab(const COORD cCurrCursorPos) const noexce
{
if (*it > cCurrCursorPos.X)
{
cNewCursorPos.X = *it;
// make sure we don't exceed the width of the buffer
cNewCursorPos.X = std::min(*it, sWidth);
break;
}
}

View file

@ -599,6 +599,19 @@ void ScreenBufferTests::TestGetForwardTab()
L"Cursor advanced to end of screen buffer.");
}
Log::Comment(L"Find next tab from rightmost column.");
{
coordCursor.X = coordScreenBufferSize.X - 1;
COORD coordCursorExpected;
coordCursorExpected = coordCursor;
COORD const coordCursorResult = si.GetForwardTab(coordCursor);
VERIFY_ARE_EQUAL(coordCursorExpected,
coordCursorResult,
L"Cursor remains in rightmost column.");
}
si._tabStops.clear();
}