From 841a064401d1f97b45ea5c37670f12f0c0356071 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 26 Feb 2023 19:53:10 +0000 Subject: [PATCH 1/6] Reset the delayed wrap flag when required. --- src/buffer/out/textBuffer.cpp | 5 +++++ src/terminal/adapter/adaptDispatch.cpp | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 2bbc2e20721..2da129ac544 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -901,6 +901,11 @@ void TextBuffer::SetCurrentLineRendition(const LineRendition lineRendition) } TriggerRedraw(Viewport::FromDimensions({ 0, rowIndex }, { GetSize().Width(), 1 })); } + // There is some variation in how this was handled by the different DEC + // terminals, but the STD 070 reference (on page D-13) makes it clear that + // the delayed wrap (aka the Last Column Flag) was expected to be reset when + // line rendition controls were executed. + GetCursor().ResetDelayEOLWrap(); } void TextBuffer::ResetLineRenditionRange(const til::CoordType startRow, const til::CoordType endRow) noexcept diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 3990fbdccaa..8e957c7535a 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -600,6 +600,8 @@ void AdaptDispatch::_InsertDeleteCharacterHelper(const VTInt delta) const auto startCol = textBuffer.GetCursor().GetPosition().x; const auto endCol = textBuffer.GetLineWidth(row); _ScrollRectHorizontally(textBuffer, { startCol, row, endCol, row + 1 }, delta); + // The ICH and DCH controls are expected to reset the delayed wrap flag. + textBuffer.GetCursor().ResetDelayEOLWrap(); } // Routine Description: @@ -668,6 +670,9 @@ bool AdaptDispatch::EraseCharacters(const VTInt numChars) const auto startCol = textBuffer.GetCursor().GetPosition().x; const auto endCol = std::min(startCol + numChars, textBuffer.GetLineWidth(row)); + // The ECH control is expected to reset the delayed wrap flag. + textBuffer.GetCursor().ResetDelayEOLWrap(); + auto eraseAttributes = textBuffer.GetCurrentAttributes(); eraseAttributes.SetStandardErase(); _FillRect(textBuffer, { startCol, row, endCol, row + 1 }, L' ', eraseAttributes); @@ -721,6 +726,11 @@ bool AdaptDispatch::EraseInDisplay(const DispatchTypes::EraseType eraseType) const auto row = textBuffer.GetCursor().GetPosition().y; const auto col = textBuffer.GetCursor().GetPosition().x; + // The ED control is expected to reset the delayed wrap flag. + // The special case variants above ("erase all" and "erase scrollback") + // take care of that themselves when they set the cursor position. + textBuffer.GetCursor().ResetDelayEOLWrap(); + auto eraseAttributes = textBuffer.GetCurrentAttributes(); eraseAttributes.SetStandardErase(); @@ -758,6 +768,9 @@ bool AdaptDispatch::EraseInLine(const DispatchTypes::EraseType eraseType) const auto row = textBuffer.GetCursor().GetPosition().y; const auto col = textBuffer.GetCursor().GetPosition().x; + // The EL control is expected to reset the delayed wrap flag. + textBuffer.GetCursor().ResetDelayEOLWrap(); + auto eraseAttributes = textBuffer.GetCurrentAttributes(); eraseAttributes.SetStandardErase(); switch (eraseType) @@ -822,6 +835,9 @@ bool AdaptDispatch::SelectiveEraseInDisplay(const DispatchTypes::EraseType erase const auto row = textBuffer.GetCursor().GetPosition().y; const auto col = textBuffer.GetCursor().GetPosition().x; + // The DECSED control is expected to reset the delayed wrap flag. + textBuffer.GetCursor().ResetDelayEOLWrap(); + switch (eraseType) { case DispatchTypes::EraseType::FromBeginning: @@ -855,6 +871,9 @@ bool AdaptDispatch::SelectiveEraseInLine(const DispatchTypes::EraseType eraseTyp const auto row = textBuffer.GetCursor().GetPosition().y; const auto col = textBuffer.GetCursor().GetPosition().x; + // The DECSEL control is expected to reset the delayed wrap flag. + textBuffer.GetCursor().ResetDelayEOLWrap(); + switch (eraseType) { case DispatchTypes::EraseType::FromBeginning: @@ -1606,6 +1625,11 @@ bool AdaptDispatch::_ModeParamsHelper(const DispatchTypes::ModeParams param, con return true; case DispatchTypes::ModeParams::DECAWM_AutoWrapMode: _api.SetAutoWrapMode(enable); + // Resetting DECAWM should also reset the delayed wrap flag. + if (!enable) + { + _api.GetTextBuffer().GetCursor().ResetDelayEOLWrap(); + } return true; case DispatchTypes::ModeParams::DECARM_AutoRepeatMode: _terminalInput.SetInputMode(TerminalInput::Mode::AutoRepeat, enable); From 83a323d7ea54c17b2a182348f4bf72e061ae690a Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 26 Feb 2023 19:57:39 +0000 Subject: [PATCH 2/6] Horizontal tabs should not reset the delayed wrap flag. --- src/buffer/out/cursor.cpp | 4 ++-- src/buffer/out/cursor.h | 2 +- src/terminal/adapter/adaptDispatch.cpp | 14 +++++++++++++- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/buffer/out/cursor.cpp b/src/buffer/out/cursor.cpp index 485f2dbc829..9084a60003d 100644 --- a/src/buffer/out/cursor.cpp +++ b/src/buffer/out/cursor.cpp @@ -286,9 +286,9 @@ void Cursor::CopyProperties(const Cursor& OtherCursor) noexcept _cursorType = OtherCursor._cursorType; } -void Cursor::DelayEOLWrap(const til::point coordDelayedAt) noexcept +void Cursor::DelayEOLWrap() noexcept { - _coordDelayedAt = coordDelayedAt; + _coordDelayedAt = _cPosition; _fDelayedEolWrap = true; } diff --git a/src/buffer/out/cursor.h b/src/buffer/out/cursor.h index bbd908eb3e7..3c377e61b2a 100644 --- a/src/buffer/out/cursor.h +++ b/src/buffer/out/cursor.h @@ -76,7 +76,7 @@ class Cursor final void CopyProperties(const Cursor& OtherCursor) noexcept; - void DelayEOLWrap(const til::point coordDelayedAt) noexcept; + void DelayEOLWrap() noexcept; void ResetDelayEOLWrap() noexcept; til::point GetDelayedAtPosition() const noexcept; bool IsDelayedEOLWrap() const noexcept; diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 8e957c7535a..450b4aaeaf0 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -149,7 +149,7 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string) cursor.SetPosition(cursorPosition); if (wrapAtEOL) { - cursor.DelayEOLWrap(cursorPosition); + cursor.DelayEOLWrap(); } } else @@ -2121,8 +2121,20 @@ bool AdaptDispatch::ForwardTab(const VTInt numTabs) } } + // While the STD 070 reference suggests that horizontal tabs should reset + // the delayed wrap, almost none of the DEC terminals actually worked that + // way, and most modern terminal emulators appear to have taken the same + // approach (i.e. they don't reset). For us this is a bit messy, since all + // cursor movement resets the flag automatically, so we need to save the + // original state here, and potentially reapply it after the move. + const auto delayedWrapOriginallySet = cursor.IsDelayedEOLWrap(); cursor.SetXPosition(column); _ApplyCursorMovementFlags(cursor); + if (delayedWrapOriginallySet) + { + cursor.DelayEOLWrap(); + } + return true; } From bdac748b8afc93c9a0ec9b57b6989d65745209f6 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 26 Feb 2023 20:00:38 +0000 Subject: [PATCH 3/6] Save and restore the delayed wrap flag with DECSC/DECRC. --- src/terminal/adapter/adaptDispatch.cpp | 7 +++++++ src/terminal/adapter/adaptDispatch.hpp | 1 + 2 files changed, 8 insertions(+) diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 450b4aaeaf0..49316ddc4fe 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -446,6 +446,7 @@ bool AdaptDispatch::CursorSaveState() auto& savedCursorState = _savedCursorState.at(_usingAltBuffer); savedCursorState.Column = cursorPosition.x + 1; savedCursorState.Row = cursorPosition.y + 1; + savedCursorState.IsDelayedEOLWrap = textBuffer.GetCursor().IsDelayedEOLWrap(); savedCursorState.IsOriginModeRelative = _modes.test(Mode::Origin); savedCursorState.Attributes = attributes; savedCursorState.TermOutput = _termOutput; @@ -484,6 +485,12 @@ bool AdaptDispatch::CursorRestoreState() _modes.reset(Mode::Origin); CursorPosition(row, col); + // If the delayed wrap flag was set when the cursor was saved, we need to restore than now. + if (savedCursorState.IsDelayedEOLWrap) + { + _api.GetTextBuffer().GetCursor().DelayEOLWrap(); + } + // Once the cursor position is restored, we can then restore the actual origin mode. _modes.set(Mode::Origin, savedCursorState.IsOriginModeRelative); diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index 1a283a1c987..5eff7b40d17 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -170,6 +170,7 @@ namespace Microsoft::Console::VirtualTerminal { VTInt Row = 1; VTInt Column = 1; + bool IsDelayedEOLWrap = false; bool IsOriginModeRelative = false; TextAttribute Attributes = {}; TerminalOutput TermOutput = {}; From a811441e9dc24d2f2a987ce600a38ddc527b1526 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 28 Feb 2023 01:24:14 +0000 Subject: [PATCH 4/6] Don't output EL in the delayed wrap state. --- src/renderer/vt/paint.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 16ba7caf548..32a0a9949c0 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -604,7 +604,11 @@ using namespace Microsoft::Console::Types; { RETURN_IF_FAILED(_EraseCharacter(numSpaces)); } - else + // If we're past the end of the row (i.e. in the "delayed EOL wrap" + // state), then there is no need to erase the rest of line. In fact + // if we did output an EL sequence at this point, it could reset the + // "delayed EOL wrap" state, breaking subsequent output. + else if (_lastText.x <= _lastViewport.RightInclusive()) { RETURN_IF_FAILED(_EraseLine()); } From a94bf69c302383fa669bb607229d29747e7a926f Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 28 Feb 2023 15:01:36 +0000 Subject: [PATCH 5/6] Add unit test for controls resetting delayed wrap. --- src/host/ut_host/ScreenBufferTests.cpp | 98 ++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 33d9eacd78b..f61d1fe9738 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -252,6 +252,8 @@ class ScreenBufferTests TEST_METHOD(TestDeferredMainBufferResize); TEST_METHOD(RectangularAreaOperations); + + TEST_METHOD(DelayedWrapReset); }; void ScreenBufferTests::SingleAlternateBufferCreationTest() @@ -7538,3 +7540,99 @@ void ScreenBufferTests::RectangularAreaOperations() VERIFY_IS_TRUE(_ValidateLinesContain(targetArea.top, targetArea.bottom, bufferChars, bufferAttr)); VERIFY_IS_TRUE(_ValidateLinesContain(targetArea.right, targetArea.top, targetArea.bottom, bufferChar, bufferAttr)); } + +void ScreenBufferTests::DelayedWrapReset() +{ + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method") + TEST_METHOD_PROPERTY(L"Data:op", L"{0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34}") + END_TEST_METHOD_PROPERTIES(); + + int opIndex; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"op", opIndex)); + + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer(); + auto& stateMachine = si.GetStateMachine(); + const auto width = si.GetTextBuffer().GetSize().Width(); + const auto halfWidth = width / 2; + WI_SetFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING); + WI_SetFlag(si.OutputMode, DISABLE_NEWLINE_AUTO_RETURN); + stateMachine.ProcessString(L"\033[?40h"); // Make sure DECCOLM is allowed + + const auto startRow = 5; + const auto startCol = width - 1; + + // The operations below are all those that the DEC STD 070 reference has + // documented as needing to reset the Last Column Flag (see page D-13). The + // only controls that we haven't included are HT and SUB, because most DEC + // terminals did *not* trigger a reset after executing those sequences, and + // most modern terminals also seem to have agreed that that is the correct + // approach to take. + const struct + { + std::wstring_view name; + std::wstring_view sequence; + til::point expectedPos = {}; + bool absolutePos = false; + } ops[] = { + { L"DECSTBM", L"\033[1;10r", { 0, 0 }, true }, + { L"DECSWL", L"\033#5" }, + { L"DECDWL", L"\033#6", { halfWidth - 1, startRow }, true }, + { L"DECDHL (top)", L"\033#3", { halfWidth - 1, startRow }, true }, + { L"DECDHL (bottom)", L"\033#4", { halfWidth - 1, startRow }, true }, + { L"DECCOLM set", L"\033[?3h", { 0, 0 }, true }, + { L"DECOM set", L"\033[?6h", { 0, 0 }, true }, + { L"DECCOLM set", L"\033[?3l", { 0, 0 }, true }, + { L"DECOM reset", L"\033[?6l", { 0, 0 }, true }, + { L"DECAWM reset", L"\033[?7l" }, + { L"CUU", L"\033[A", { 0, -1 } }, + { L"CUD", L"\033[B", { 0, 1 } }, + { L"CUF", L"\033[C" }, + { L"CUB", L"\033[D", { -1, 0 } }, + { L"CUP", L"\033[3;7H", { 6, 2 }, true }, + { L"HVP", L"\033[3;7f", { 6, 2 }, true }, + { L"BS", L"\b", { -1, 0 } }, + { L"LF", L"\n", { 0, 1 } }, + { L"VT", L"\v", { 0, 1 } }, + { L"FF", L"\f", { 0, 1 } }, + { L"CR", L"\r", { 0, startRow }, true }, + { L"IND", L"\033D", { 0, 1 } }, + { L"RI", L"\033M", { 0, -1 } }, + { L"NEL", L"\033E", { 0, startRow + 1 }, true }, + { L"ECH", L"\033[X" }, + { L"DCH", L"\033[P" }, + { L"ICH", L"\033[@" }, + { L"EL", L"\033[K" }, + { L"DECSEL", L"\033[?K" }, + { L"DL", L"\033[M", { 0, startRow }, true }, + { L"IL", L"\033[L", { 0, startRow }, true }, + { L"ED", L"\033[J" }, + { L"ED (all)", L"\033[2J" }, + { L"ED (scrollback)", L"\033[3J" }, + { L"DECSED", L"\033[?J" }, + }; + const auto& op = gsl::at(ops, opIndex); + + Log::Comment(L"Writing a character at the end of the line should set delayed EOL wrap"); + const auto startPos = til::point{ startCol, startRow }; + si.GetTextBuffer().GetCursor().SetPosition(startPos); + stateMachine.ProcessCharacter(L'X'); + { + auto& cursor = si.GetTextBuffer().GetCursor(); + VERIFY_IS_TRUE(cursor.IsDelayedEOLWrap()); + VERIFY_ARE_EQUAL(startPos, cursor.GetPosition()); + } + + Log::Comment(NoThrowString().Format( + L"Executing a %s control should reset delayed EOL wrap", + op.name.data())); + const auto expectedPos = op.absolutePos ? op.expectedPos : startPos + op.expectedPos; + stateMachine.ProcessString(op.sequence); + { + auto& cursor = si.GetTextBuffer().GetCursor(); + const auto actualPos = cursor.GetPosition() - si.GetViewport().Origin(); + VERIFY_IS_FALSE(cursor.IsDelayedEOLWrap()); + VERIFY_ARE_EQUAL(expectedPos, actualPos); + } +} From 4eada0471621c053e60446d3c74a8cbeecef744e Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 28 Feb 2023 17:04:19 +0000 Subject: [PATCH 6/6] Extend CursorSaveRestore test to cover delayed wrap. --- src/host/ut_host/ScreenBufferTests.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index f61d1fe9738..92593b013f2 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -6268,33 +6268,38 @@ void ScreenBufferTests::CursorSaveRestore() VERIFY_SUCCEEDED(si.SetViewportOrigin(true, til::point(0, 0), true)); Log::Comment(L"Restore after save."); - // Set the cursor position, attributes, and character set. + // Set the cursor position, delayed wrap, attributes, and character set. cursor.SetPosition({ 20, 10 }); + cursor.DelayEOLWrap(); si.SetAttributes(colorAttrs); stateMachine.ProcessString(selectGraphicsChars); // Save state. stateMachine.ProcessString(saveCursor); - // Reset the cursor position, attributes, and character set. + // Reset the cursor position, delayed wrap, attributes, and character set. cursor.SetPosition({ 0, 0 }); si.SetAttributes(defaultAttrs); stateMachine.ProcessString(selectAsciiChars); // Restore state. stateMachine.ProcessString(restoreCursor); - // Verify initial position, colors, and graphic character set. + // Verify initial position, delayed wrap, colors, and graphic character set. VERIFY_ARE_EQUAL(til::point(20, 10), cursor.GetPosition()); + VERIFY_IS_TRUE(cursor.IsDelayedEOLWrap()); + cursor.ResetDelayEOLWrap(); VERIFY_ARE_EQUAL(colorAttrs, si.GetAttributes()); stateMachine.ProcessString(asciiText); VERIFY_IS_TRUE(_ValidateLineContains({ 20, 10 }, graphicText, colorAttrs)); Log::Comment(L"Restore again without save."); - // Reset the cursor position, attributes, and character set. + // Reset the cursor position, delayed wrap, attributes, and character set. cursor.SetPosition({ 0, 0 }); si.SetAttributes(defaultAttrs); stateMachine.ProcessString(selectAsciiChars); // Restore state. stateMachine.ProcessString(restoreCursor); - // Verify initial saved position, colors, and graphic character set. + // Verify initial saved position, delayed wrap, colors, and graphic character set. VERIFY_ARE_EQUAL(til::point(20, 10), cursor.GetPosition()); + VERIFY_IS_TRUE(cursor.IsDelayedEOLWrap()); + cursor.ResetDelayEOLWrap(); VERIFY_ARE_EQUAL(colorAttrs, si.GetAttributes()); stateMachine.ProcessString(asciiText); VERIFY_IS_TRUE(_ValidateLineContains({ 20, 10 }, graphicText, colorAttrs)); @@ -6302,14 +6307,16 @@ void ScreenBufferTests::CursorSaveRestore() Log::Comment(L"Restore after reset."); // Soft reset. stateMachine.ProcessString(L"\x1b[!p"); - // Set the cursor position, attributes, and character set. + // Set the cursor position, delayed wrap, attributes, and character set. cursor.SetPosition({ 20, 10 }); + cursor.DelayEOLWrap(); si.SetAttributes(colorAttrs); stateMachine.ProcessString(selectGraphicsChars); // Restore state. stateMachine.ProcessString(restoreCursor); - // Verify home position, default attributes, and ascii character set. + // Verify home position, no delayed wrap, default attributes, and ascii character set. VERIFY_ARE_EQUAL(til::point(0, 0), cursor.GetPosition()); + VERIFY_IS_FALSE(cursor.IsDelayedEOLWrap()); VERIFY_ARE_EQUAL(defaultAttrs, si.GetAttributes()); stateMachine.ProcessString(asciiText); VERIFY_IS_TRUE(_ValidateLineContains(til::point(0, 0), asciiText, defaultAttrs));