maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Remove unneeded VT-specific control character handling (#4289)

Browse source 
## Summary of the Pull Request

This PR removes all of the VT-specific functionality from the `WriteCharsLegacy` function that dealt with control characters, since those controls are now handled in the state machine when in VT mode. It also removes most of the control character handling from the `Terminal::_WriteBuffer` method for the same reason.

## References

This is a followup to PR #4171

## PR Checklist
* [x] Closes #3971
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [x] 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: https://github.com/microsoft/terminal/issues/780#issuecomment-570287435

## Detailed Description of the Pull Request / Additional comments

There are four changes to the `WriteCharsLegacy` implementation:

1. The `TAB` character had special case handling in VT mode which is now no longer required. This fixes a bug in the Python REPL editor (when run from a cmd shell in Windows Terminal), which would prevent you tabbing past the end of the line. It also fixes #3971.

2. Following on from point 1, the `WC_NONDESTRUCTIVE_TAB` flag could also now be removed. It only ever applied in VT mode, in which case the `TAB` character isn't handled in `WriteCharsLegacy`, so there isn't a need for a non-destructive version.

3. There used to be special case handling for a `BS` character at the beginning of the line when in VT mode, and that is also no longer required. This fixes an edge-case bug which would prevent a glyph being output for code point 8 when `ENABLE_PROCESSED_OUTPUT` was disabled. 

4. There was quite a lot of special case handling for control characters in the "end-of-line wrap" implementation, which is no longer required. This fixes a bug which would prevent "low ASCII" characters from wrapping when output at the end of a line.

Then in the `Terminal::_WriteBuffer` implementation, I've simply removed all control character handling, except for `LF`. The Terminal is always in VT mode, so the control characters are always handled by the state machine. The exception for the `LF` character is simply because it doesn't have a proper implementation yet, so it still passes the character through to `_WriteBuffer`. That will get cleaned up eventually, but I thought that could wait for a later PR.

Finally, with the removal of the VT mode handling in `WriteCharsLegacy`, there was no longer a need for the `SCREEN_INFORMATION::InVTMode` method to be publicly accessible. That has now been made private.

## Validation Steps Performed

I've only tested manually, making sure the conhost and Windows Terminal still basically work, and confirming that the above-mentioned bugs are fixed by these changes.
This commit is contained in:
James Holderness 2020-01-29 19:18:46 +00:00 committed by GitHub
parent 685720a767
commit c69757ec9e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 153 additions and 216 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/cascadia/TerminalCore/ITerminalApi.hpp
View file

@ -28,6 +28,7 @@ namespace Microsoft::Terminal::Core
virtual bool SetCursorPosition(short x, short y) noexcept = 0;
virtual COORD GetCursorPosition() noexcept = 0;
virtual bool CursorLineFeed(const bool withReturn) noexcept = 0;
virtual bool DeleteCharacter(const size_t count) noexcept = 0;
virtual bool InsertCharacter(const size_t count) noexcept = 0;

158
src/cascadia/TerminalCore/Terminal.cpp
View file

@ -414,7 +414,6 @@ Viewport Terminal::_GetVisibleViewport() const noexcept
void Terminal::_WriteBuffer(const std::wstring_view& stringView)
{
auto& cursor = _buffer->GetCursor();
const Viewport bufferSize = _buffer->GetSize();
// Defer the cursor drawing while we are iterating the string, for a better performance.
// We can not waste time displaying a cursor event when we know more text is coming right behind it.
@ -425,106 +424,87 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView)
const auto wch = stringView.at(i);
const COORD cursorPosBefore = cursor.GetPosition();
COORD proposedCursorPosition = cursorPosBefore;
bool notifyScroll = false;
if (wch == UNICODE_LINEFEED)
// TODO: MSFT 21006766
// This is not great but I need it demoable. Fix by making a buffer stream writer.
//
// If wch is a surrogate character we need to read 2 code units
// from the stringView to form a single code point.
const auto isSurrogate = wch >= 0xD800 && wch <= 0xDFFF;
const auto view = stringView.substr(i, isSurrogate ? 2 : 1);
const OutputCellIterator it{ view, _buffer->GetCurrentAttributes() };
const auto end = _buffer->Write(it);
const auto cellDistance = end.GetCellDistance(it);
const auto inputDistance = end.GetInputDistance(it);
if (inputDistance > 0)
{
proposedCursorPosition.Y++;
}
else if (wch == UNICODE_CARRIAGERETURN)
{
proposedCursorPosition.X = 0;
}
else if (wch == UNICODE_BACKSPACE)
{
if (cursorPosBefore.X == 0)
{
proposedCursorPosition.X = bufferSize.Width() - 1;
proposedCursorPosition.Y--;
}
else
{
proposedCursorPosition.X--;
}
}
else if (wch == UNICODE_BEL)
{
// TODO: GitHub #1883
// For now its empty just so we don't try to write the BEL character
// If "wch" was a surrogate character, we just consumed 2 code units above.
// -> Increment "i" by 1 in that case and thus by 2 in total in this iteration.
proposedCursorPosition.X += gsl::narrow<SHORT>(cellDistance);
i += inputDistance - 1;
}
else
{
// TODO: MSFT 21006766
// This is not great but I need it demoable. Fix by making a buffer stream writer.
//
// If wch is a surrogate character we need to read 2 code units
// from the stringView to form a single code point.
const auto isSurrogate = wch >= 0xD800 && wch <= 0xDFFF;
const auto view = stringView.substr(i, isSurrogate ? 2 : 1);
const OutputCellIterator it{ view, _buffer->GetCurrentAttributes() };
const auto end = _buffer->Write(it);
const auto cellDistance = end.GetCellDistance(it);
const auto inputDistance = end.GetInputDistance(it);
if (inputDistance > 0)
{
// If "wch" was a surrogate character, we just consumed 2 code units above.
// -> Increment "i" by 1 in that case and thus by 2 in total in this iteration.
proposedCursorPosition.X += gsl::narrow<SHORT>(cellDistance);
i += inputDistance - 1;
}
else
{
// If _WriteBuffer() is called with a consecutive string longer than the viewport/buffer width
// the call to _buffer->Write() will refuse to write anything on the current line.
// GetInputDistance() thus returns 0, which would in turn cause i to be
// decremented by 1 below and force the outer loop to loop forever.
// This if() basically behaves as if "\r\n" had been encountered above and retries the write.
// With well behaving shells during normal operation this safeguard should normally not be encountered.
proposedCursorPosition.X = 0;
proposedCursorPosition.Y++;
}
// If _WriteBuffer() is called with a consecutive string longer than the viewport/buffer width
// the call to _buffer->Write() will refuse to write anything on the current line.
// GetInputDistance() thus returns 0, which would in turn cause i to be
// decremented by 1 below and force the outer loop to loop forever.
// This if() basically behaves as if "\r\n" had been encountered above and retries the write.
// With well behaving shells during normal operation this safeguard should normally not be encountered.
proposedCursorPosition.X = 0;
proposedCursorPosition.Y++;
}
// If we're about to scroll past the bottom of the buffer, instead cycle the buffer.
const auto newRows = proposedCursorPosition.Y - bufferSize.Height() + 1;
if (newRows > 0)
{
for (auto dy = 0; dy < newRows; dy++)
{
_buffer->IncrementCircularBuffer();
proposedCursorPosition.Y--;
}
notifyScroll = true;
}
// This section is essentially equivalent to `AdjustCursorPosition`
// Update Cursor Position
cursor.SetPosition(proposedCursorPosition);
const COORD cursorPosAfter = cursor.GetPosition();
// Move the viewport down if the cursor moved below the viewport.
if (cursorPosAfter.Y > _mutableViewport.BottomInclusive())
{
const auto newViewTop = std::max(0, cursorPosAfter.Y - (_mutableViewport.Height() - 1));
if (newViewTop != _mutableViewport.Top())
{
_mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow<short>(newViewTop) }, _mutableViewport.Dimensions());
notifyScroll = true;
}
}
if (notifyScroll)
{
_buffer->GetRenderTarget().TriggerRedrawAll();
_NotifyScrollEvent();
}
_AdjustCursorPosition(proposedCursorPosition);
}
cursor.EndDeferDrawing();
}
void Terminal::_AdjustCursorPosition(const COORD proposedPosition)
{
#pragma warning(suppress : 26496) // cpp core checks wants this const but it's modified below.
auto proposedCursorPosition = proposedPosition;
auto& cursor = _buffer->GetCursor();
const Viewport bufferSize = _buffer->GetSize();
bool notifyScroll = false;
// If we're about to scroll past the bottom of the buffer, instead cycle the buffer.
const auto newRows = proposedCursorPosition.Y - bufferSize.Height() + 1;
if (newRows > 0)
{
for (auto dy = 0; dy < newRows; dy++)
{
_buffer->IncrementCircularBuffer();
proposedCursorPosition.Y--;
}
notifyScroll = true;
}
// Update Cursor Position
cursor.SetPosition(proposedCursorPosition);
const COORD cursorPosAfter = cursor.GetPosition();
// Move the viewport down if the cursor moved below the viewport.
if (cursorPosAfter.Y > _mutableViewport.BottomInclusive())
{
const auto newViewTop = std::max(0, cursorPosAfter.Y - (_mutableViewport.Height() - 1));
if (newViewTop != _mutableViewport.Top())
{
_mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow<short>(newViewTop) }, _mutableViewport.Dimensions());
notifyScroll = true;
}
}
if (notifyScroll)
{
_buffer->GetRenderTarget().TriggerRedrawAll();
_NotifyScrollEvent();
}
}
void Terminal::UserScrollViewport(const int viewTop)
{
const auto clampedNewTop = std::max(0, viewTop);

3
src/cascadia/TerminalCore/Terminal.hpp
View file

@ -84,6 +84,7 @@ public:
bool ReverseText(bool reversed) noexcept override;
bool SetCursorPosition(short x, short y) noexcept override;
COORD GetCursorPosition() noexcept override;
bool CursorLineFeed(const bool withReturn) noexcept override;
bool DeleteCharacter(const size_t count) noexcept override;
bool InsertCharacter(const size_t count) noexcept override;
bool EraseCharacters(const size_t numChars) noexcept override;
@ -239,6 +240,8 @@ private:
void _WriteBuffer(const std::wstring_view& stringView);
void _AdjustCursorPosition(const COORD proposedPosition);
void _NotifyScrollEvent() noexcept;
#pragma region TextSelection

21
src/cascadia/TerminalCore/TerminalApi.cpp
View file

@ -132,6 +132,27 @@ COORD Terminal::GetCursorPosition() noexcept
return newPos;
}
// Method Description:
// - Moves the cursor down one line, and possibly also to the leftmost column.
// Arguments:
// - withReturn, set to true if a carriage return should be performed as well.
// Return value:
// - true if succeeded, false otherwise
bool Terminal::CursorLineFeed(const bool withReturn) noexcept
try
{
auto cursorPos = _buffer->GetCursor().GetPosition();
cursorPos.Y++;
if (withReturn)
{
cursorPos.X = 0;
}
_AdjustCursorPosition(cursorPos);
return true;
}
CATCH_LOG_RETURN_FALSE()
// Method Description:
// - deletes count characters starting from the cursor's current position
// - it moves over the remaining text to 'replace' the deleted text

7
src/cascadia/TerminalCore/TerminalDispatch.cpp
View file

@ -83,12 +83,9 @@ try
// There is currently no need for mode-specific line feeds in the Terminal,
// so for now we just treat them as a line feed without carriage return.
case DispatchTypes::LineFeedType::WithoutReturn:
Execute(L'\n');
return true;
return _terminalApi.CursorLineFeed(false);
case DispatchTypes::LineFeedType::WithReturn:
Execute(L'\r');
Execute(L'\n');
return true;
return _terminalApi.CursorLineFeed(true);
default:
return false;
}

148
src/host/_stream.cpp
View file

@ -330,59 +330,12 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100;
// Only act on a delayed EOL if we didn't move the cursor to a different position from where the EOL was marked.
if (coordDelayedAt.X == CursorPosition.X && coordDelayedAt.Y == CursorPosition.Y)
{
bool fDoEolWrap = false;
CursorPosition.X = 0;
CursorPosition.Y++;
if (WI_IsFlagSet(dwFlags, WC_DELAY_EOL_WRAP))
{
// Correct if it's a printable character and whoever called us still understands/wants delayed EOL wrap.
if (*lpString >= UNICODE_SPACE)
{
fDoEolWrap = true;
}
else if (*lpString == UNICODE_BACKSPACE)
{
// if we have an active wrap and a backspace comes in, process it by moving the cursor
// back one cell position unless it's already at the start of a row.
*pcb += sizeof(WCHAR);
lpString++;
pwchRealUnicode++;
if (CursorPosition.X != 0)
{
--CursorPosition.X;
Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
CursorPosition = cursor.GetPosition();
}
continue;
}
}
else
{
// Uh oh, we've hit a consumer that doesn't know about delayed end of lines. To rectify this, just quickly jump
// forward to the next line as if we had done it earlier, then let everything else play out normally.
fDoEolWrap = true;
}
Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
if (fDoEolWrap)
{
CursorPosition.X = 0;
CursorPosition.Y++;
Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
CursorPosition = cursor.GetPosition();
}
}
}
if (screenInfo.InVTMode())
{
// if we're at the beginning of a row and we get a backspace and told to limit backspacing, skip it
if (*lpString == UNICODE_BACKSPACE && CursorPosition.X == 0 && WI_IsFlagSet(dwFlags, WC_LIMIT_BACKSPACE))
{
*pcb += sizeof(wchar_t);
++lpString;
++pwchRealUnicode;
continue;
CursorPosition = cursor.GetPosition();
}
}
@ -449,28 +402,23 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100;
goto EndWhile;
break;
case UNICODE_TAB:
if (screenInfo.InVTMode())
{
const ULONG TabSize = NUMBER_OF_SPACES_IN_TAB(XPosition);
XPosition = (SHORT)(XPosition + TabSize);
if (XPosition >= coordScreenBufferSize.X)
{
goto EndWhile;
}
else
{
const ULONG TabSize = NUMBER_OF_SPACES_IN_TAB(XPosition);
XPosition = (SHORT)(XPosition + TabSize);
if (XPosition >= coordScreenBufferSize.X || WI_IsFlagSet(dwFlags, WC_NONDESTRUCTIVE_TAB))
{
goto EndWhile;
}
for (ULONG j = 0; j < TabSize && i < LOCAL_BUFFER_SIZE; j++, i++)
{
*LocalBufPtr = UNICODE_SPACE;
LocalBufPtr++;
}
for (ULONG j = 0; j < TabSize && i < LOCAL_BUFFER_SIZE; j++, i++)
{
*LocalBufPtr = UNICODE_SPACE;
LocalBufPtr++;
}
pwchBuffer++;
break;
}
case UNICODE_LINEFEED:
case UNICODE_CARRIAGERETURN:
goto EndWhile;
@ -753,52 +701,40 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100;
}
case UNICODE_TAB:
{
// if VT-style tabs are set, then handle them the VT way, including not inserting spaces.
// just move the cursor to the next tab stop.
if (screenInfo.InVTMode())
const size_t TabSize = NUMBER_OF_SPACES_IN_TAB(cursor.GetPosition().X);
CursorPosition.X = (SHORT)(cursor.GetPosition().X + TabSize);
// move cursor forward to next tab stop. fill space with blanks.
// we get here when the tab extends beyond the right edge of the
// window. if the tab goes wraps the line, set the cursor to the first
// position in the next line.
pwchBuffer++;
TempNumSpaces += TabSize;
size_t NumChars = 0;
if (CursorPosition.X >= coordScreenBufferSize.X)
{
const COORD cCursorOld = cursor.GetPosition();
CursorPosition = screenInfo.GetForwardTab(cCursorOld);
NumChars = gsl::narrow<size_t>(coordScreenBufferSize.X - cursor.GetPosition().X);
CursorPosition.X = 0;
CursorPosition.Y = cursor.GetPosition().Y + 1;
// since you just tabbed yourself past the end of the row, set the wrap
textBuffer.GetRowByOffset(cursor.GetPosition().Y).GetCharRow().SetWrapForced(true);
}
else
{
const size_t TabSize = NUMBER_OF_SPACES_IN_TAB(cursor.GetPosition().X);
CursorPosition.X = (SHORT)(cursor.GetPosition().X + TabSize);
// move cursor forward to next tab stop. fill space with blanks.
// we get here when the tab extends beyond the right edge of the
// window. if the tab goes wraps the line, set the cursor to the first
// position in the next line.
pwchBuffer++;
TempNumSpaces += TabSize;
size_t NumChars = 0;
if (CursorPosition.X >= coordScreenBufferSize.X)
{
NumChars = gsl::narrow<size_t>(coordScreenBufferSize.X - cursor.GetPosition().X);
CursorPosition.X = 0;
CursorPosition.Y = cursor.GetPosition().Y + 1;
// since you just tabbed yourself past the end of the row, set the wrap
textBuffer.GetRowByOffset(cursor.GetPosition().Y).GetCharRow().SetWrapForced(true);
}
else
{
NumChars = gsl::narrow<size_t>(CursorPosition.X - cursor.GetPosition().X);
CursorPosition.Y = cursor.GetPosition().Y;
}
if (!WI_IsFlagSet(dwFlags, WC_NONDESTRUCTIVE_TAB))
{
try
{
const OutputCellIterator it(UNICODE_SPACE, Attributes, NumChars);
const auto done = screenInfo.Write(it, cursor.GetPosition());
NumChars = done.GetCellDistance(it);
}
CATCH_LOG();
}
NumChars = gsl::narrow<size_t>(CursorPosition.X - cursor.GetPosition().X);
CursorPosition.Y = cursor.GetPosition().Y;
}
try
{
const OutputCellIterator it(UNICODE_SPACE, Attributes, NumChars);
const auto done = screenInfo.Write(it, cursor.GetPosition());
NumChars = done.GetCellDistance(it);
}
CATCH_LOG();
Status = AdjustCursorPosition(screenInfo, CursorPosition, (dwFlags & WC_KEEP_CURSOR_VISIBLE) != 0, psScrollY);
break;
}

2
src/host/cmdline.h
View file

@ -145,7 +145,7 @@ void RedrawCommandLine(COOKED_READ_DATA& cookedReadData);
//#define WC_FALSIFY_UNICODE 0x08
#define WC_LIMIT_BACKSPACE 0x10
#define WC_NONDESTRUCTIVE_TAB 0x20
//#define WC_NONDESTRUCTIVE_TAB 0x20 - This is not needed anymore, because the VT code handles tabs internally now.
//#define WC_NEWLINE_SAVE_X 0x40 - This has been replaced with an output mode flag instead as it's line discipline behavior that may not necessarily be coupled with VT.
#define WC_DELAY_EOL_WRAP 0x80

2
src/host/outputStream.cpp
View file

@ -85,7 +85,7 @@ void WriteBuffer::_DefaultStringCase(const std::wstring_view string)
&dwNumBytes,
nullptr,
_io.GetActiveOutputBuffer().GetTextBuffer().GetCursor().GetPosition().X,
WC_LIMIT_BACKSPACE | WC_NONDESTRUCTIVE_TAB | WC_DELAY_EOL_WRAP,
WC_LIMIT_BACKSPACE | WC_DELAY_EOL_WRAP,
nullptr);
}

24
src/host/screenInfo.cpp
View file

@ -127,7 +127,7 @@ SCREEN_INFORMATION::~SCREEN_INFORMATION()
const NTSTATUS status = pScreen->_InitializeOutputStateMachine();
if (pScreen->InVTMode())
if (pScreen->_IsInVTMode())
{
// microsoft/terminal#411: If VT mode is enabled, lets construct the
// VT tab stops. Without this line, if a user has
@ -191,17 +191,6 @@ StateMachine& SCREEN_INFORMATION::GetStateMachine()
return *_stateMachine;
}
// Method Description:
// - returns true if this buffer is in Virtual Terminal Output mode.
// Arguments:
// <none>
// Return Value:
// true iff this buffer is in Virtual Terminal Output mode.
bool SCREEN_INFORMATION::InVTMode() const
{
return WI_IsFlagSet(OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
}
// Routine Description:
// - This routine inserts the screen buffer pointer into the console's list of screen buffers.
// Arguments:
@ -1964,6 +1953,17 @@ bool SCREEN_INFORMATION::_IsInPtyMode() const
return _IsAltBuffer() || gci.IsInVtIoMode();
}
// Routine Description:
// - returns true if this buffer is in Virtual Terminal Output mode.
// Parameters:
// - None
// Return Value:
// - true iff this buffer is in Virtual Terminal Output mode.
bool SCREEN_INFORMATION::_IsInVTMode() const
{
return WI_IsFlagSet(OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
}
// Routine Description:
// - Sets a VT tab stop in the column sColumn. If there is already a tab there, it does nothing.
// Parameters:

3
src/host/screenInfo.hpp
View file

@ -121,8 +121,6 @@ public:
bool SendNotifyBeep() const;
bool PostUpdateWindowSize() const;
bool InVTMode() const;
// TODO: MSFT 9355062 these methods should probably be a part of construction/destruction. http://osgvsowi/9355062
static void s_InsertScreenBuffer(_In_ SCREEN_INFORMATION* const pScreenInfo);
static void s_RemoveScreenBuffer(_In_ SCREEN_INFORMATION* const pScreenInfo);
@ -281,6 +279,7 @@ private:
bool _IsAltBuffer() const;
bool _IsInPtyMode() const;
bool _IsInVTMode() const;
std::shared_ptr<Microsoft::Console::VirtualTerminal::StateMachine> _stateMachine;