From 7fd5d515b28dfb857732ac2faa65bdd41dd8e026 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 29 Jan 2020 12:52:19 -0600 Subject: [PATCH 1/6] This actually fixes this bug - different terminals EOL wrap differently, esp conhost v wt v gnome-terminal. Remove ambiguity - just hardcore move the cursor in this scenario. --- src/renderer/vt/XtermEngine.cpp | 5 +++++ src/renderer/vt/paint.cpp | 7 ++++++- src/renderer/vt/vtrenderer.hpp | 2 ++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 2c1d8a08ed1..4281dcf1c67 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -242,6 +242,10 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _needToDisableCursor = true; hr = _CursorHome(); } + else if (_delayedEolWrap) + { + hr = _CursorPosition(coord); + } else if (coord.X == 0 && coord.Y == (_lastText.Y + 1)) { // Down one line, at the start of the line. @@ -298,6 +302,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _newBottomLine = false; } _deferredCursorPos = INVALID_COORDS; + _delayedEolWrap = false; return hr; } diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 920dc86aeb2..3839c6423ba 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -461,11 +461,16 @@ using namespace Microsoft::Console::Types; // we'll determine that we need to emit a \b to put the cursor in the // right position. This is wrong, and will cause us to move the cursor // back one character more than we wanted. - if (_lastText.X < _lastViewport.RightInclusive()) + if (_lastText.X < _lastViewport.RightExclusive()) { _lastText.X += static_cast(columnsActual); } + if (_lastText.X >= _lastViewport.RightInclusive()) + { + _delayedEolWrap = true; + } + short sNumSpaces; try { diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 27658e7f9c8..5e9cdb1e716 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -144,6 +144,8 @@ namespace Microsoft::Console::Render Microsoft::Console::VirtualTerminal::RenderTracing _trace; bool _inResizeRequest{ false }; + bool _delayedEolWrap{ false }; + [[nodiscard]] HRESULT _Write(std::string_view const str) noexcept; [[nodiscard]] HRESULT _WriteFormattedString(const std::string* const pFormat, ...) noexcept; [[nodiscard]] HRESULT _Flush() noexcept; From 9b6554b10f3f6f679c90097547bd72ad0e06ffb7 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 29 Jan 2020 16:05:53 -0600 Subject: [PATCH 2/6] Add some comments for PR --- src/renderer/vt/XtermEngine.cpp | 18 ++++++++++++++---- src/renderer/vt/paint.cpp | 13 +++++++++++-- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 4281dcf1c67..dfbe434f2d2 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -242,10 +242,6 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _needToDisableCursor = true; hr = _CursorHome(); } - else if (_delayedEolWrap) - { - hr = _CursorPosition(coord); - } else if (coord.X == 0 && coord.Y == (_lastText.Y + 1)) { // Down one line, at the start of the line. @@ -262,6 +258,20 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, hr = _Write(seq); } } + else if (_delayedEolWrap) + { + // GH#1245, GH#357 - If we were in the delayed EOL wrap state, make + // sure to _manually_ position the cursor now, with a full CUP + // sequence, don't try and be clever with \b or \r or other control + // sequences. Different terminals (conhost, gnome-terminal, wt) all + // behave differently with how the cursor behaves at an end of line. + // This is the only solution that works in all of them, and also + // works wrapped lines emitted by conpty. + // + // Make sure to do this _after_ the possible \r\n branch above, + // otherwise we might accidentally break wrapped lines (GH#405) + hr = _CursorPosition(coord); + } else if (coord.X == 0 && coord.Y == _lastText.Y) { // Start of this line diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 3839c6423ba..35b0bd66d99 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -450,7 +450,7 @@ using namespace Microsoft::Console::Types; RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(wstr)); // Update our internal tracker of the cursor's position. - // See MSFT:20266233 + // See MSFT:20266233 (which is also GH#357) // If the cursor is at the rightmost column of the terminal, and we write a // space, the cursor won't actually move to the next cell (which would // be {0, _lastText.Y++}). The cursor will stay visibly in that last @@ -461,11 +461,20 @@ using namespace Microsoft::Console::Types; // we'll determine that we need to emit a \b to put the cursor in the // right position. This is wrong, and will cause us to move the cursor // back one character more than we wanted. + // + // GH#1245: This needs to be RightExclusive, _not_ inclusive. Otherwise, we + // won't update our internal cursor position tracker correctly at the last + // character of the row. if (_lastText.X < _lastViewport.RightExclusive()) { _lastText.X += static_cast(columnsActual); } - + // GH#1245: If we wrote the exactly last char of the row, then we're + // probably in the "delayed EOL wrap" state. Different terminals (conhost, + // gnome-terminal, wt) all behave differently with how the cursor behaves at + // an end of line. Marke that we're in the delayed EOL wrap state - we don't + // want to be clever about how we move the cursor in this state, since + // different terminals will handle a backspace differently in this state. if (_lastText.X >= _lastViewport.RightInclusive()) { _delayedEolWrap = true; From 0a98cceddb79d2b41dd1e45473e30f3a48c7330c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 29 Jan 2020 16:06:25 -0600 Subject: [PATCH 3/6] Try adding a test, but I can't get the test to repro the original behavior quite right --- .../ConptyRoundtripTests.cpp | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 3f9b7563cce..52e9053d071 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -149,6 +149,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(WriteTwoLinesUsesNewline); TEST_METHOD(WriteAFewSimpleLines); + TEST_METHOD(DelayEOLWrap); + private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -340,3 +342,56 @@ void ConptyRoundtripTests::WriteAFewSimpleLines() verifyData(termTb); } + +void ConptyRoundtripTests::DelayEOLWrap() +{ + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& hostSm = si.GetStateMachine(); + auto& hostTb = si.GetTextBuffer(); + auto& termTb = *term->_buffer; + + _flushFirstFrame(); + + for (auto i = 0; i < TerminalViewWidth; i++) + { + hostSm.ProcessString(L"A"); + } + + auto verifyData0 = [](TextBuffer& tb) { + auto iter = tb.GetCellDataAt({ 0, 0 }); + TestUtils::VerifySpanOfText(L"A", iter, 0, 80); + }; + + verifyData0(hostTb); + + expectedOutput.push_back(std::string(80, 'A')); + + // TODO:GH#405 - When conpty wrapped lines lands, this will behave + // differently, but this has been verified to test it's current behavior, + // and this change works even with conpty wrapped lines. + + expectedOutput.push_back("\x1b[1;80H"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyData0(termTb); + + hostSm.ProcessString(L"\b"); + hostSm.ProcessString(L" "); + + auto verifyData1 = [](TextBuffer& tb) { + auto iter = tb.GetCellDataAt({ 0, 0 }); + TestUtils::VerifySpanOfText(L"A", iter, 0, 79); + }; + verifyData1(hostTb); + + // expectedOutput.push_back("\x1b[1;79H"); + expectedOutput.push_back("\b"); + expectedOutput.push_back(" "); + // expectedOutput.push_back("\x1b[1;80H"); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + verifyData1(termTb); +} From 40b4966782d78ab89b0f40f772c16765fe84b0ef Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 29 Jan 2020 16:06:34 -0600 Subject: [PATCH 4/6] Revert "Try adding a test, but I can't get the test to repro the original behavior quite right" This reverts commit 0a98cceddb79d2b41dd1e45473e30f3a48c7330c. --- .../ConptyRoundtripTests.cpp | 55 ------------------- 1 file changed, 55 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 52e9053d071..3f9b7563cce 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -149,8 +149,6 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(WriteTwoLinesUsesNewline); TEST_METHOD(WriteAFewSimpleLines); - TEST_METHOD(DelayEOLWrap); - private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -342,56 +340,3 @@ void ConptyRoundtripTests::WriteAFewSimpleLines() verifyData(termTb); } - -void ConptyRoundtripTests::DelayEOLWrap() -{ - auto& g = ServiceLocator::LocateGlobals(); - auto& renderer = *g.pRender; - auto& gci = g.getConsoleInformation(); - auto& si = gci.GetActiveOutputBuffer(); - auto& hostSm = si.GetStateMachine(); - auto& hostTb = si.GetTextBuffer(); - auto& termTb = *term->_buffer; - - _flushFirstFrame(); - - for (auto i = 0; i < TerminalViewWidth; i++) - { - hostSm.ProcessString(L"A"); - } - - auto verifyData0 = [](TextBuffer& tb) { - auto iter = tb.GetCellDataAt({ 0, 0 }); - TestUtils::VerifySpanOfText(L"A", iter, 0, 80); - }; - - verifyData0(hostTb); - - expectedOutput.push_back(std::string(80, 'A')); - - // TODO:GH#405 - When conpty wrapped lines lands, this will behave - // differently, but this has been verified to test it's current behavior, - // and this change works even with conpty wrapped lines. - - expectedOutput.push_back("\x1b[1;80H"); - VERIFY_SUCCEEDED(renderer.PaintFrame()); - - verifyData0(termTb); - - hostSm.ProcessString(L"\b"); - hostSm.ProcessString(L" "); - - auto verifyData1 = [](TextBuffer& tb) { - auto iter = tb.GetCellDataAt({ 0, 0 }); - TestUtils::VerifySpanOfText(L"A", iter, 0, 79); - }; - verifyData1(hostTb); - - // expectedOutput.push_back("\x1b[1;79H"); - expectedOutput.push_back("\b"); - expectedOutput.push_back(" "); - // expectedOutput.push_back("\x1b[1;80H"); - - VERIFY_SUCCEEDED(renderer.PaintFrame()); - verifyData1(termTb); -} From 774b23b2cfd8662d09c54850e46cdbdf1c8a4b7f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 31 Jan 2020 14:25:17 -0600 Subject: [PATCH 5/6] add a test for this particular case --- .../ConptyRoundtripTests.cpp | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 3f9b7563cce..0ed0763049d 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -149,6 +149,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(WriteTwoLinesUsesNewline); TEST_METHOD(WriteAFewSimpleLines); + TEST_METHOD(MoveCursorAtEOL); + private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -340,3 +342,67 @@ void ConptyRoundtripTests::WriteAFewSimpleLines() verifyData(termTb); } + +void ConptyRoundtripTests::MoveCursorAtEOL() +{ + // This is a test for GH#1245 + + VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& hostSm = si.GetStateMachine(); + auto& hostTb = si.GetTextBuffer(); + auto& termTb = *term->_buffer; + + _flushFirstFrame(); + + Log::Comment(NoThrowString().Format( + L"Write exactly a full line of text")); + hostSm.ProcessString(std::wstring(TerminalViewWidth, L'A')); + + auto verifyData0 = [](TextBuffer& tb) { + auto iter = tb.GetCellDataAt({ 0, 0 }); + TestUtils::VerifySpanOfText(L"A", iter, 0, TerminalViewWidth); + TestUtils::VerifySpanOfText(L" ", iter, 0, TerminalViewWidth); + }; + + verifyData0(hostTb); + + // TODO: GH#405/#4415 - Before #405 merges, the VT sequences conpty emits + // might change, but the buffer contents shouldn't. + // If they do change and these tests break, that's to be expected. + expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); + expectedOutput.push_back("\x1b[1;80H"); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyData0(termTb); + + Log::Comment(NoThrowString().Format( + L"Emulate backspacing at a bash prompt when the previous line wrapped.\n" + L"We'll move the cursor up to the last char of the prev line, and erase it.")); + hostSm.ProcessString(L"\x1b[1;80H"); + hostSm.ProcessString(L"\x1b[K"); + + auto verifyData1 = [](TextBuffer& tb) { + auto iter = tb.GetCellDataAt({ 0, 0 }); + // There should be 79 'A's, followed by a space, and the following line should be blank. + TestUtils::VerifySpanOfText(L"A", iter, 0, TerminalViewWidth - 1); + TestUtils::VerifySpanOfText(L" ", iter, 0, 1); + TestUtils::VerifySpanOfText(L" ", iter, 0, TerminalViewWidth); + + auto& cursor = tb.GetCursor(); + VERIFY_ARE_EQUAL(TerminalViewWidth - 1, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y); + }; + + verifyData1(hostTb); + + expectedOutput.push_back(" "); + expectedOutput.push_back("\x1b[1;80H"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + verifyData1(termTb); +} From 6fcc52cc3b5d9e16ceb42b518ae17fad3f4f704a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 31 Jan 2020 15:44:06 -0600 Subject: [PATCH 6/6] update comments --- src/renderer/vt/paint.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 35b0bd66d99..3f9d3017e7d 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -469,12 +469,12 @@ using namespace Microsoft::Console::Types; { _lastText.X += static_cast(columnsActual); } - // GH#1245: If we wrote the exactly last char of the row, then we're - // probably in the "delayed EOL wrap" state. Different terminals (conhost, - // gnome-terminal, wt) all behave differently with how the cursor behaves at - // an end of line. Marke that we're in the delayed EOL wrap state - we don't - // want to be clever about how we move the cursor in this state, since - // different terminals will handle a backspace differently in this state. + // GH#1245: If we wrote the exactly last char of the row, then we're in the + // "delayed EOL wrap" state. Different terminals (conhost, gnome-terminal, + // wt) all behave differently with how the cursor behaves at an end of line. + // Mark that we're in the delayed EOL wrap state - we don't want to be + // clever about how we move the cursor in this state, since different + // terminals will handle a backspace differently in this state. if (_lastText.X >= _lastViewport.RightInclusive()) { _delayedEolWrap = true;