From 4bbb63ddf6763869b0f0a1559625d99b2cd0d60f Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Wed, 25 Mar 2020 14:29:37 -0700 Subject: [PATCH 01/44] Add tracing for circling and scrolling operations. Fix improper invalidation within AdjustCursorPosition routine in the subsection about scrolling down at the bottom with a set of margins enabled. --- src/host/_stream.cpp | 28 +++++++++++++++++++++++++++- src/renderer/vt/XtermEngine.cpp | 15 +++++++++------ src/renderer/vt/invalidate.cpp | 2 ++ src/renderer/vt/tracing.cpp | 14 ++++++++++++++ src/renderer/vt/tracing.hpp | 1 + 5 files changed, 53 insertions(+), 7 deletions(-) diff --git a/src/host/_stream.cpp b/src/host/_stream.cpp index 38becbe5015..6fe8be984c2 100644 --- a/src/host/_stream.cpp +++ b/src/host/_stream.cpp @@ -171,7 +171,33 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; // displays the correct text. if (newViewOrigin == viewport.Origin()) { - Viewport invalid = Viewport::FromDimensions(viewport.Origin(), { viewport.Width(), delta }); + // Inside this block, we're shifting down at the bottom. + // This means that we had something like this: + // AAAA + // BBBB + // CCCC + // DDDD + // EEEE + // + // Our margins were set for lines A-D, but not on line E. + // So we circled the whole buffer up by one: + // BBBB + // CCCC + // DDDD + // EEEE + // + // + // Then we scrolled the contents of everything OUTSIDE the margin frame down. + // BBBB + // CCCC + // DDDD + // + // EEEE + // + // And now we need to report that only the bottom line didn't "move" as we put the EEEE + // back where it started, but everything else moved. + // In this case, delta was 1. So the amount that moved is the entire viewport height minus the delta. + Viewport invalid = Viewport::FromDimensions(viewport.Origin(), { viewport.Width(), viewport.Height() - delta }); screenInfo.GetRenderTarget().TriggerRedraw(invalid); } diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 8c0420e5224..3ff05e9abcb 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -402,18 +402,20 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for safemath failure [[nodiscard]] HRESULT XtermEngine::InvalidateScroll(const COORD* const pcoordDelta) noexcept +try { - const short dx = pcoordDelta->X; - const short dy = pcoordDelta->Y; + const til::point delta{ *pcoordDelta }; - if (dx != 0 || dy != 0) + if (delta != til::point{ 0, 0 }) { + _trace.TraceInvalidateScroll(delta); + // Scroll the current offset and invalidate the revealed area - _invalidMap.translate(til::point(*pcoordDelta), true); + _invalidMap.translate(delta, true); COORD invalidScrollNew; - RETURN_IF_FAILED(ShortAdd(_scrollDelta.X, dx, &invalidScrollNew.X)); - RETURN_IF_FAILED(ShortAdd(_scrollDelta.Y, dy, &invalidScrollNew.Y)); + RETURN_IF_FAILED(ShortAdd(_scrollDelta.X, delta.x(), &invalidScrollNew.X)); + RETURN_IF_FAILED(ShortAdd(_scrollDelta.Y, delta.y(), &invalidScrollNew.Y)); // Store if safemath succeeded _scrollDelta = invalidScrollNew; @@ -421,6 +423,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, return S_OK; } +CATCH_RETURN(); // Routine Description: // - Draws one line of the buffer to the screen. Writes the characters to the diff --git a/src/renderer/vt/invalidate.cpp b/src/renderer/vt/invalidate.cpp index 597f56b0f06..7e85a81cfd4 100644 --- a/src/renderer/vt/invalidate.cpp +++ b/src/renderer/vt/invalidate.cpp @@ -121,6 +121,8 @@ CATCH_RETURN(); _circled = true; } + _trace.TraceTriggerCircling(*pForcePaint); + return S_OK; } diff --git a/src/renderer/vt/tracing.cpp b/src/renderer/vt/tracing.cpp index 023a5453f80..673336d77ca 100644 --- a/src/renderer/vt/tracing.cpp +++ b/src/renderer/vt/tracing.cpp @@ -127,6 +127,20 @@ void RenderTracing::TraceTriggerCircling(const bool newFrame) const #endif UNIT_TESTING } +void RenderTracing::TraceInvalidateScroll(const til::point scroll) const +{ +#ifndef UNIT_TESTING + const auto scrollDeltaStr = scroll.to_string(); + const auto scrollDelta = scrollDeltaStr.c_str(); + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceInvalidateScroll", + TraceLoggingWideString(scrollDelta), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); +#else + UNREFERENCED_PARAMETER(scroll); +#endif +} + void RenderTracing::TraceStartPaint(const bool quickReturn, const til::bitmap invalidMap, const til::rectangle lastViewport, diff --git a/src/renderer/vt/tracing.hpp b/src/renderer/vt/tracing.hpp index f7c18c6f835..0c36b42f05a 100644 --- a/src/renderer/vt/tracing.hpp +++ b/src/renderer/vt/tracing.hpp @@ -34,6 +34,7 @@ namespace Microsoft::Console::VirtualTerminal void TracePaintCursor(const til::point coordCursor) const; void TraceInvalidateAll(const til::rectangle view) const; void TraceTriggerCircling(const bool newFrame) const; + void TraceInvalidateScroll(const til::point scroll) const; void TraceStartPaint(const bool quickReturn, const til::bitmap invalidMap, const til::rectangle lastViewport, From a543b1ff8ac1bd2f975005d19fe7931ffe6b18ef Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Wed, 25 Mar 2020 16:21:14 -0700 Subject: [PATCH 02/44] PR feedback applied. Don't bother making string if no one is listening to ETW. Change the scroll member variable to a til::point and math that directly (and add the operators and related tests to til::point). --- src/inc/til/point.h | 24 +++++ src/renderer/vt/XtermEngine.cpp | 15 ++- src/renderer/vt/math.cpp | 2 +- src/renderer/vt/paint.cpp | 4 +- src/renderer/vt/state.cpp | 2 +- src/renderer/vt/tracing.cpp | 15 +-- src/renderer/vt/vtrenderer.hpp | 2 +- src/til/ut_til/PointTests.cpp | 161 ++++++++++++++++++++++++++++++++ 8 files changed, 205 insertions(+), 20 deletions(-) diff --git a/src/inc/til/point.h b/src/inc/til/point.h index c7643b05286..39a465f073a 100644 --- a/src/inc/til/point.h +++ b/src/inc/til/point.h @@ -107,6 +107,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" return point{ x, y }; } + point& operator+=(const point& other) + { + *this = *this + other; + return *this; + } + point operator-(const point& other) const { ptrdiff_t x; @@ -118,6 +124,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" return point{ x, y }; } + point& operator-=(const point& other) + { + *this = *this - other; + return *this; + } + point operator*(const point& other) const { ptrdiff_t x; @@ -129,6 +141,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" return point{ x, y }; } + point& operator*=(const point& other) + { + *this = *this * other; + return *this; + } + point operator/(const point& other) const { ptrdiff_t x; @@ -140,6 +158,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" return point{ x, y }; } + point& operator/=(const point& other) + { + *this = *this / other; + return *this; + } + constexpr ptrdiff_t x() const noexcept { return _x; diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 3ff05e9abcb..eaae0ad696e 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -343,19 +343,20 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. [[nodiscard]] HRESULT XtermEngine::ScrollFrame() noexcept +try { - if (_scrollDelta.X != 0) + if (_scrollDelta.x() != 0) { // No easy way to shift left-right. Everything needs repainting. return InvalidateAll(); } - if (_scrollDelta.Y == 0) + if (_scrollDelta.y() == 0) { // There's nothing to do here. Do nothing. return S_OK; } - const short dy = _scrollDelta.Y; + const short dy = _scrollDelta.y(); const short absDy = static_cast(abs(dy)); HRESULT hr = S_OK; @@ -391,6 +392,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, return hr; } +CATCH_RETURN(); // Routine Description: // - Notifies us that the console is attempting to scroll the existing screen @@ -413,12 +415,7 @@ try // Scroll the current offset and invalidate the revealed area _invalidMap.translate(delta, true); - COORD invalidScrollNew; - RETURN_IF_FAILED(ShortAdd(_scrollDelta.X, delta.x(), &invalidScrollNew.X)); - RETURN_IF_FAILED(ShortAdd(_scrollDelta.Y, delta.y(), &invalidScrollNew.Y)); - - // Store if safemath succeeded - _scrollDelta = invalidScrollNew; + _scrollDelta += delta; } return S_OK; diff --git a/src/renderer/vt/math.cpp b/src/renderer/vt/math.cpp index 1a131914e03..e0e6c703eb7 100644 --- a/src/renderer/vt/math.cpp +++ b/src/renderer/vt/math.cpp @@ -64,7 +64,7 @@ void VtEngine::_OrRect(_Inout_ SMALL_RECT* const pRectExisting, const SMALL_RECT bool VtEngine::_WillWriteSingleChar() const { // If there is scroll delta, return false. - if (til::point{ 0, 0 } != til::point{ _scrollDelta }) + if (til::point{ 0, 0 } != _scrollDelta) { return false; } diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index babc6606caa..e643f51db5f 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -27,7 +27,7 @@ using namespace Microsoft::Console::Types; // If there's nothing to do, quick return bool somethingToDo = _invalidMap.any() || - (_scrollDelta.X != 0 || _scrollDelta.Y != 0) || + _scrollDelta != til::point{ 0, 0 } || _cursorMoved || _titleChanged; @@ -52,7 +52,7 @@ using namespace Microsoft::Console::Types; _invalidMap.reset_all(); - _scrollDelta = { 0 }; + _scrollDelta = { 0, 0 }; _clearedAllThisFrame = false; _cursorMoved = false; _firstPaint = false; diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index a50179cd506..bcbaf4994fd 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -38,7 +38,7 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe, _invalidMap(initialViewport.Dimensions()), _lastRealCursor({ 0 }), _lastText({ 0 }), - _scrollDelta({ 0 }), + _scrollDelta({ 0, 0 }), _quickReturn(false), _clearedAllThisFrame(false), _cursorMoved(false), diff --git a/src/renderer/vt/tracing.cpp b/src/renderer/vt/tracing.cpp index 673336d77ca..304d15cc559 100644 --- a/src/renderer/vt/tracing.cpp +++ b/src/renderer/vt/tracing.cpp @@ -130,12 +130,15 @@ void RenderTracing::TraceTriggerCircling(const bool newFrame) const void RenderTracing::TraceInvalidateScroll(const til::point scroll) const { #ifndef UNIT_TESTING - const auto scrollDeltaStr = scroll.to_string(); - const auto scrollDelta = scrollDeltaStr.c_str(); - TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, - "VtEngine_TraceInvalidateScroll", - TraceLoggingWideString(scrollDelta), - TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + if (TraceLoggingProviderEnabled(g_hConsoleVtRendererTraceProvider, WINEVENT_LEVEL_VERBOSE, 0)) + { + const auto scrollDeltaStr = scroll.to_string(); + const auto scrollDelta = scrollDeltaStr.c_str(); + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceInvalidateScroll", + TraceLoggingWideString(scrollDelta), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + } #else UNREFERENCED_PARAMETER(scroll); #endif diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 7ebb7a71a87..2230410f4d2 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -126,7 +126,7 @@ namespace Microsoft::Console::Render COORD _lastRealCursor; COORD _lastText; - COORD _scrollDelta; + til::point _scrollDelta; bool _quickReturn; bool _clearedAllThisFrame; diff --git a/src/til/ut_til/PointTests.cpp b/src/til/ut_til/PointTests.cpp index 724d37ad7ef..bbeba7cd83c 100644 --- a/src/til/ut_til/PointTests.cpp +++ b/src/til/ut_til/PointTests.cpp @@ -207,6 +207,50 @@ class PointTests } } + TEST_METHOD(AdditionInplace) + { + Log::Comment(L"0.) Addition of two things that should be in bounds."); + { + const til::point pt{ 5, 10 }; + const til::point pt2{ 23, 47 }; + + const til::point expected{ pt.x() + pt2.x(), pt.y() + pt2.y() }; + + auto actual = pt; + actual += pt2; + + VERIFY_ARE_EQUAL(expected, actual); + } + + Log::Comment(L"1.) Addition results in value that is too large (x)."); + { + constexpr ptrdiff_t bigSize = std::numeric_limits().max(); + const til::point pt{ bigSize, static_cast(0) }; + const til::point pt2{ 1, 1 }; + + auto fn = [&]() { + auto actual = pt; + actual += pt2; + }; + + VERIFY_THROWS_SPECIFIC(fn(), wil::ResultException, [](wil::ResultException& e) { return e.GetErrorCode() == E_ABORT; }); + } + + Log::Comment(L"2.) Addition results in value that is too large (y)."); + { + constexpr ptrdiff_t bigSize = std::numeric_limits().max(); + const til::point pt{ static_cast(0), bigSize }; + const til::point pt2{ 1, 1 }; + + auto fn = [&]() { + auto actual = pt; + actual += pt2; + }; + + VERIFY_THROWS_SPECIFIC(fn(), wil::ResultException, [](wil::ResultException& e) { return e.GetErrorCode() == E_ABORT; }); + } + } + TEST_METHOD(Subtraction) { Log::Comment(L"0.) Subtraction of two things that should be in bounds."); @@ -246,6 +290,50 @@ class PointTests } } + TEST_METHOD(SubtractionInplace) + { + Log::Comment(L"0.) Subtraction of two things that should be in bounds."); + { + const til::point pt{ 5, 10 }; + const til::point pt2{ 23, 47 }; + + const til::point expected{ pt.x() - pt2.x(), pt.y() - pt2.y() }; + + auto actual = pt; + actual -= pt2; + + VERIFY_ARE_EQUAL(expected, actual); + } + + Log::Comment(L"1.) Subtraction results in value that is too small (x)."); + { + constexpr ptrdiff_t bigSize = std::numeric_limits().max(); + const til::point pt{ bigSize, static_cast(0) }; + const til::point pt2{ -2, -2 }; + + auto fn = [&]() { + auto actual = pt2; + actual -= pt; + }; + + VERIFY_THROWS_SPECIFIC(fn(), wil::ResultException, [](wil::ResultException& e) { return e.GetErrorCode() == E_ABORT; }); + } + + Log::Comment(L"2.) Subtraction results in value that is too small (y)."); + { + constexpr ptrdiff_t bigSize = std::numeric_limits().max(); + const til::point pt{ static_cast(0), bigSize }; + const til::point pt2{ -2, -2 }; + + auto fn = [&]() { + auto actual = pt2; + actual -= pt; + }; + + VERIFY_THROWS_SPECIFIC(fn(), wil::ResultException, [](wil::ResultException& e) { return e.GetErrorCode() == E_ABORT; }); + } + } + TEST_METHOD(Multiplication) { Log::Comment(L"0.) Multiplication of two things that should be in bounds."); @@ -285,6 +373,50 @@ class PointTests } } + TEST_METHOD(MultiplicationInplace) + { + Log::Comment(L"0.) Multiplication of two things that should be in bounds."); + { + const til::point pt{ 5, 10 }; + const til::point pt2{ 23, 47 }; + + const til::point expected{ pt.x() * pt2.x(), pt.y() * pt2.y() }; + + auto actual = pt; + actual *= pt2; + + VERIFY_ARE_EQUAL(expected, actual); + } + + Log::Comment(L"1.) Multiplication results in value that is too large (x)."); + { + constexpr ptrdiff_t bigSize = std::numeric_limits().max(); + const til::point pt{ bigSize, static_cast(0) }; + const til::point pt2{ 10, 10 }; + + auto fn = [&]() { + auto actual = pt; + actual *= pt2; + }; + + VERIFY_THROWS_SPECIFIC(fn(), wil::ResultException, [](wil::ResultException& e) { return e.GetErrorCode() == E_ABORT; }); + } + + Log::Comment(L"2.) Multiplication results in value that is too large (y)."); + { + constexpr ptrdiff_t bigSize = std::numeric_limits().max(); + const til::point pt{ static_cast(0), bigSize }; + const til::point pt2{ 10, 10 }; + + auto fn = [&]() { + auto actual = pt; + actual *= pt2; + }; + + VERIFY_THROWS_SPECIFIC(fn(), wil::ResultException, [](wil::ResultException& e) { return e.GetErrorCode() == E_ABORT; }); + } + } + TEST_METHOD(Division) { Log::Comment(L"0.) Division of two things that should be in bounds."); @@ -311,6 +443,35 @@ class PointTests } } + TEST_METHOD(DivisionInplace) + { + Log::Comment(L"0.) Division of two things that should be in bounds."); + { + const til::point pt{ 555, 510 }; + const til::point pt2{ 23, 47 }; + + const til::point expected{ pt.x() / pt2.x(), pt.y() / pt2.y() }; + auto actual = pt; + actual /= pt2; + + VERIFY_ARE_EQUAL(expected, actual); + } + + Log::Comment(L"1.) Division by zero"); + { + constexpr ptrdiff_t bigSize = std::numeric_limits().max(); + const til::point pt{ bigSize, static_cast(0) }; + const til::point pt2{ 1, 1 }; + + auto fn = [&]() { + auto actual = pt2; + actual /= pt; + }; + + VERIFY_THROWS_SPECIFIC(fn(), wil::ResultException, [](wil::ResultException& e) { return e.GetErrorCode() == E_ABORT; }); + } + } + TEST_METHOD(X) { const til::point pt{ 5, 10 }; From 9ba2c69417788c4512c85abcccac3ba7d56e664c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 25 Mar 2020 14:41:36 -0500 Subject: [PATCH 03/44] More tracing is always good (cherry picked from commit d972b5e07a8f5586dab988043a3ced9c03762714) --- src/renderer/vt/XtermEngine.cpp | 2 + src/renderer/vt/invalidate.cpp | 1 + src/renderer/vt/paint.cpp | 9 +++- src/renderer/vt/tracing.cpp | 83 +++++++++++++++++++++++++++++---- src/renderer/vt/tracing.hpp | 6 ++- 5 files changed, 90 insertions(+), 11 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 8c0420e5224..a926e2fa868 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -344,6 +344,8 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. [[nodiscard]] HRESULT XtermEngine::ScrollFrame() noexcept { + _trace.TraceScrollFrame(_scrollDelta); + if (_scrollDelta.X != 0) { // No easy way to shift left-right. Everything needs repainting. diff --git a/src/renderer/vt/invalidate.cpp b/src/renderer/vt/invalidate.cpp index 597f56b0f06..79fab1417a6 100644 --- a/src/renderer/vt/invalidate.cpp +++ b/src/renderer/vt/invalidate.cpp @@ -121,6 +121,7 @@ CATCH_RETURN(); _circled = true; } + _trace.TraceTriggerCircling(*pForcePaint); return S_OK; } diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index babc6606caa..5dc16b15186 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -32,7 +32,12 @@ using namespace Microsoft::Console::Types; _titleChanged; _quickReturn = !somethingToDo; - _trace.TraceStartPaint(_quickReturn, _invalidMap, _lastViewport.ToInclusive(), _scrollDelta, _cursorMoved); + _trace.TraceStartPaint(_quickReturn, + _invalidMap, + _lastViewport.ToInclusive(), + _scrollDelta, + _cursorMoved, + _wrappedRow); return _quickReturn ? S_FALSE : S_OK; } @@ -458,6 +463,7 @@ using namespace Microsoft::Console::Types; // the cursor is still waiting on that character for the next character // to follow it. _wrappedRow = std::nullopt; + _trace.TraceClearWrapped(); } // Move the cursor to the start of this run. @@ -479,6 +485,7 @@ using namespace Microsoft::Console::Types; lastWrittenChar > _lastViewport.RightInclusive()) { _wrappedRow = coord.Y; + _trace.TraceSetWrapped(coord.Y); } // Update our internal tracker of the cursor's position. diff --git a/src/renderer/vt/tracing.cpp b/src/renderer/vt/tracing.cpp index 023a5453f80..289a3aaac48 100644 --- a/src/renderer/vt/tracing.cpp +++ b/src/renderer/vt/tracing.cpp @@ -131,7 +131,8 @@ void RenderTracing::TraceStartPaint(const bool quickReturn, const til::bitmap invalidMap, const til::rectangle lastViewport, const til::point scrollDelt, - const bool cursorMoved) const + const bool cursorMoved, + std::optional wrappedRow) const { #ifndef UNIT_TESTING if (TraceLoggingProviderEnabled(g_hConsoleVtRendererTraceProvider, WINEVENT_LEVEL_VERBOSE, 0)) @@ -142,14 +143,29 @@ void RenderTracing::TraceStartPaint(const bool quickReturn, const auto lastView = lastViewStr.c_str(); const auto scrollDeltaStr = scrollDelt.to_string(); const auto scrollDelta = scrollDeltaStr.c_str(); - TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, - "VtEngine_TraceStartPaint", - TraceLoggingBool(quickReturn), - TraceLoggingWideString(invalidated), - TraceLoggingWideString(lastView), - TraceLoggingWideString(scrollDelta), - TraceLoggingBool(cursorMoved), - TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + if (wrappedRow.has_value()) + { + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceStartPaint", + TraceLoggingBool(quickReturn), + TraceLoggingWideString(invalidated), + TraceLoggingWideString(lastView), + TraceLoggingWideString(scrollDelta), + TraceLoggingBool(cursorMoved), + TraceLoggingValue(wrappedRow.value()), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + } + else + { + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceStartPaint", + TraceLoggingBool(quickReturn), + TraceLoggingWideString(invalidated), + TraceLoggingWideString(lastView), + TraceLoggingWideString(scrollDelta), + TraceLoggingBool(cursorMoved), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + } } #else UNREFERENCED_PARAMETER(quickReturn); @@ -157,6 +173,7 @@ void RenderTracing::TraceStartPaint(const bool quickReturn, UNREFERENCED_PARAMETER(lastViewport); UNREFERENCED_PARAMETER(scrollDelt); UNREFERENCED_PARAMETER(cursorMoved); + UNREFERENCED_PARAMETER(wrappedRow); #endif UNIT_TESTING } @@ -186,6 +203,24 @@ void RenderTracing::TraceLastText(const til::point lastTextPos) const UNREFERENCED_PARAMETER(lastTextPos); #endif UNIT_TESTING } + +void RenderTracing::TraceScrollFrame(const til::point scrollDeltaPos) const +{ +#ifndef UNIT_TESTING + if (TraceLoggingProviderEnabled(g_hConsoleVtRendererTraceProvider, WINEVENT_LEVEL_VERBOSE, 0)) + { + const auto scrollDeltaStr = scrollDeltaPos.to_string(); + const auto scrollDelta = scrollDeltaStr.c_str(); + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceScrollFrame", + TraceLoggingWideString(scrollDelta), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + } +#else + UNREFERENCED_PARAMETER(scrollDeltaPos); +#endif UNIT_TESTING +} + void RenderTracing::TraceMoveCursor(const til::point lastTextPos, const til::point cursor) const { #ifndef UNIT_TESTING @@ -224,6 +259,36 @@ void RenderTracing::TraceWrapped() const #endif UNIT_TESTING } +void RenderTracing::TraceSetWrapped(const short wrappedRow) const +{ +#ifndef UNIT_TESTING + if (TraceLoggingProviderEnabled(g_hConsoleVtRendererTraceProvider, WINEVENT_LEVEL_VERBOSE, 0)) + { + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceSetWrapped", + TraceLoggingValue(wrappedRow), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + } +#else + UNREFERENCED_PARAMETER(wrappedRow); +#endif UNIT_TESTING +} + +void RenderTracing::TraceClearWrapped() const +{ +#ifndef UNIT_TESTING + if (TraceLoggingProviderEnabled(g_hConsoleVtRendererTraceProvider, WINEVENT_LEVEL_VERBOSE, 0)) + { + const auto* const msg = "Cleared wrap state"; + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceClearWrapped", + TraceLoggingString(msg), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + } +#else +#endif UNIT_TESTING +} + void RenderTracing::TracePaintCursor(const til::point coordCursor) const { #ifndef UNIT_TESTING diff --git a/src/renderer/vt/tracing.hpp b/src/renderer/vt/tracing.hpp index f7c18c6f835..f667915ed24 100644 --- a/src/renderer/vt/tracing.hpp +++ b/src/renderer/vt/tracing.hpp @@ -29,7 +29,10 @@ namespace Microsoft::Console::VirtualTerminal void TraceString(const std::string_view& str) const; void TraceInvalidate(const til::rectangle view) const; void TraceLastText(const til::point lastText) const; + void TraceScrollFrame(const til::point scrollDelta) const; void TraceMoveCursor(const til::point lastText, const til::point cursor) const; + void TraceSetWrapped(const short wrappedRow) const; + void TraceClearWrapped() const; void TraceWrapped() const; void TracePaintCursor(const til::point coordCursor) const; void TraceInvalidateAll(const til::rectangle view) const; @@ -38,7 +41,8 @@ namespace Microsoft::Console::VirtualTerminal const til::bitmap invalidMap, const til::rectangle lastViewport, const til::point scrollDelta, - const bool cursorMoved) const; + const bool cursorMoved, + std::optional wrappedRow) const; void TraceEndPaint() const; }; } From d42f960949a8802901a152abede65280f333c56e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Mar 2020 10:22:13 -0500 Subject: [PATCH 04/44] So this works but I think it will break the EOL backspace test --- src/renderer/vt/XtermEngine.cpp | 81 ++++++++++++++++++++++----------- 1 file changed, 55 insertions(+), 26 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index c2d10a73f7c..8a0d7657bd3 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -214,7 +214,21 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // _nextCursorIsVisible will still be false (from when we set it during // StartPaint) _nextCursorIsVisible = true; - return VtEngine::PaintCursor(options); + + const auto stashedEOLWrap = _delayedEolWrap; + const auto stashedWappedRow = _wrappedRow; + const auto result = VtEngine::PaintCursor(options); + _delayedEolWrap = stashedEOLWrap; + _wrappedRow = stashedWappedRow; + if (_wrappedRow.has_value()) + { + _trace.TraceSetWrapped(_wrappedRow.value()); + } + else + { + _trace.TraceClearWrapped(); + } + return result; } // Routine Description: @@ -362,35 +376,50 @@ try const short absDy = static_cast(abs(dy)); HRESULT hr = S_OK; - if (dy < 0) - { - // Instead of deleting the first line (causing everything to move up) - // move to the bottom of the buffer, and newline. - // That will cause everything to move up, by moving the viewport down. - // This will let remote conhosts scroll up to see history like normal. - const short bottom = _lastViewport.ToOrigin().BottomInclusive(); - hr = _MoveCursor({ 0, bottom }); - if (SUCCEEDED(hr)) - { - std::string seq = std::string(absDy, '\n'); - hr = _Write(seq); - // Mark that the bottom line is new, so we won't spend time with an - // ECH on it. - _newBottomLine = true; - } - // We don't need to _MoveCursor the cursor again, because it's still - // at the bottom of the viewport. - } - else if (dy > 0) + + //////////////////////////////////////////////////////////////////////////// + // Experiment 1 { - // Move to the top of the buffer, and insert some lines of text, to - // cause the viewport contents to shift down. - hr = _MoveCursor({ 0, 0 }); - if (SUCCEEDED(hr)) + _lastText.Y += dy; + _trace.TraceLastText(_lastText); + if (_wrappedRow.has_value()) { - hr = _InsertLine(absDy); + _wrappedRow.value() += dy; + _trace.TraceSetWrapped(_wrappedRow.value()); } + _newBottomLine = true; } + //////////////////////////////////////////////////////////////////////////// + // if (dy < 0) + // { + // // Instead of deleting the first line (causing everything to move up) + // // move to the bottom of the buffer, and newline. + // // That will cause everything to move up, by moving the viewport down. + // // This will let remote conhosts scroll up to see history like normal. + // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); + // hr = _MoveCursor({ 0, bottom }); + // if (SUCCEEDED(hr)) + // { + // std::string seq = std::string(absDy, '\n'); + // hr = _Write(seq); + // // Mark that the bottom line is new, so we won't spend time with an + // // ECH on it. + // _newBottomLine = true; + // } + // // We don't need to _MoveCursor the cursor again, because it's still + // // at the bottom of the viewport. + // } + // else if (dy > 0) + // { + // // Move to the top of the buffer, and insert some lines of text, to + // // cause the viewport contents to shift down. + // hr = _MoveCursor({ 0, 0 }); + // if (SUCCEEDED(hr)) + // { + // hr = _InsertLine(absDy); + // } + // } + //////////////////////////////////////////////////////////////////////////// return hr; } From 22bdf9b9874cbc161a3b2ba94b0c0c8de832530f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Mar 2020 10:36:57 -0500 Subject: [PATCH 05/44] This can't possibly be right... can it? --- .../UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 2d5cb921366..dce05e20ac9 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -945,9 +945,8 @@ void ConptyRoundtripTests::PassthroughClearScrollback() else { // After we hit the bottom of the viewport, the newlines come in - // separated for whatever reason. - expectedOutput.push_back("\r"); - expectedOutput.push_back("\n"); + // separated by empty writes for whatever reason. + expectedOutput.push_back("\r\n"); expectedOutput.push_back(""); } @@ -1021,10 +1020,8 @@ void ConptyRoundtripTests::PassthroughHardReset() else { // After we hit the bottom of the viewport, the newlines come in - // separated for whatever reason. - - expectedOutput.push_back("\r"); - expectedOutput.push_back("\n"); + // separated by empty writes for whatever reason. + expectedOutput.push_back("\r\n"); expectedOutput.push_back(""); } From 466e8fc654793dd655bf5ddd408b15fe8be7b964 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Mar 2020 11:14:58 -0500 Subject: [PATCH 06/44] mysteriously the existing tests all basically pass --- src/host/ut_host/ConptyOutputTests.cpp | 2 -- src/renderer/vt/XtermEngine.cpp | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp index 07493ab4544..0b661013de5 100644 --- a/src/host/ut_host/ConptyOutputTests.cpp +++ b/src/host/ut_host/ConptyOutputTests.cpp @@ -364,7 +364,5 @@ void ConptyOutputTests::InvalidateUntilOneBeforeEnd() expectedOutput.push_back("\x1b[13X"); expectedOutput.push_back("\x1b[13C"); - expectedOutput.push_back("\x1b[?25h"); // we turn the cursor back on for good measure - VERIFY_SUCCEEDED(renderer.PaintFrame()); } diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 8a0d7657bd3..5423a061b78 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -105,7 +105,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // by prepending a cursor off. if (_lastCursorIsVisible) { - _buffer.insert(0, "\x1b[25l"); + _buffer.insert(0, "\x1b[?25l"); _lastCursorIsVisible = false; } // If the cursor was NOT previously visible, then that's fine! we don't From 248d223081f68d048f95d57caa0582e2bd344de8 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Mar 2020 11:37:04 -0500 Subject: [PATCH 07/44] this is a simple test for the case that already worked --- .../ConptyRoundtripTests.cpp | 45 +++++++++++++++++++ src/host/ut_host/ConptyOutputTests.cpp | 7 ++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index dce05e20ac9..0c1140a9327 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -171,6 +171,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(TestResizeHeight); + TEST_METHOD(OutputWrappedLines); + private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -1052,3 +1054,46 @@ void ConptyRoundtripTests::PassthroughHardReset() TestUtils::VerifyExpectedString(termTb, std::wstring(TerminalViewWidth, L' '), { 0, y }); } } + +void ConptyRoundtripTests::OutputWrappedLines() +{ + Log::Comment(L"Output various different wrapped lines, and ensure we emit them correctly"); + VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& sm = si.GetStateMachine(); + auto& hostTb = si.GetTextBuffer(); + auto& termTb = *term->_buffer; + + _flushFirstFrame(); + + const auto wrappedLineLength = TerminalViewWidth + 20; + { + Log::Comment( + L"Case 1: Write a wrapped line right at the start of the buffer, before any circling"); + + sm.ProcessString(std::wstring(wrappedLineLength, L'A')); + + auto verifyBuffer = [](const TextBuffer& tb) { + VERIFY_IS_TRUE(tb.GetRowByOffset(0).GetCharRow().WasWrapForced()); + VERIFY_IS_FALSE(tb.GetRowByOffset(1).GetCharRow().WasWrapForced()); + auto iter0 = tb.GetCellDataAt({ 0, 0 }); + TestUtils::VerifySpanOfText(L"A", iter0, 0, TerminalViewWidth); + auto iter1 = tb.GetCellDataAt({ 0, 1 }); + TestUtils::VerifySpanOfText(L"A", iter1, 0, 20); + auto iter2 = tb.GetCellDataAt({ 20, 1 }); + TestUtils::VerifySpanOfText(L" ", iter2, 0, TerminalViewWidth - 20); + }; + + verifyBuffer(hostTb); + + expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); + expectedOutput.push_back(std::string(20, 'A')); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyBuffer(termTb); + } +} diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp index 0b661013de5..cecfb890dee 100644 --- a/src/host/ut_host/ConptyOutputTests.cpp +++ b/src/host/ut_host/ConptyOutputTests.cpp @@ -27,6 +27,9 @@ using namespace Microsoft::Console::Types; class ConptyOutputTests { + static const SHORT TerminalViewWidth = 80; + static const SHORT TerminalViewHeight = 32; + BEGIN_TEST_CLASS(ConptyOutputTests) TEST_CLASS_PROPERTY(L"IsolationLevel", L"Class") END_TEST_CLASS() @@ -37,7 +40,7 @@ class ConptyOutputTests m_state->InitEvents(); m_state->PrepareGlobalFont(); - m_state->PrepareGlobalScreenBuffer(); + m_state->PrepareGlobalScreenBuffer(TerminalViewWidth, TerminalViewHeight, TerminalViewWidth, TerminalViewHeight); m_state->PrepareGlobalInputBuffer(); return true; @@ -63,7 +66,7 @@ class ConptyOutputTests gci.SetDefaultBackgroundColor(INVALID_COLOR); gci.SetFillAttribute(0x07); // DARK_WHITE on DARK_BLACK - m_state->PrepareNewTextBufferInfo(true); + m_state->PrepareNewTextBufferInfo(true, TerminalViewWidth, TerminalViewHeight); auto& currentBuffer = gci.GetActiveOutputBuffer(); // Make sure a test hasn't left us in the alt buffer on accident VERIFY_IS_FALSE(currentBuffer._IsAltBuffer()); From b94cfdb410da74baaeed5cfb8a5bb3941e682acf Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Mar 2020 11:55:05 -0500 Subject: [PATCH 08/44] This test failure helps explain that this doesn't work The test fails validating the second wrapped terminal line. I think the problem comes from us manually placing the cursor on the last line of the buffer. When we write the next 20 'A's, the first one gets written on the last cell of the first row, and that's bad. I'm going to see if I can't get rid of that call (the MoveCursor in PaintCursor) for this case. That seems like the cause of all this mess. --- .../ConptyRoundtripTests.cpp | 115 ++++++++++++++---- 1 file changed, 91 insertions(+), 24 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 0c1140a9327..d1408eeb977 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -171,7 +171,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(TestResizeHeight); - TEST_METHOD(OutputWrappedLines); + TEST_METHOD(OutputWrappedLinesAtTopOfBuffer); + TEST_METHOD(OutputWrappedLinesAtBottomOfBuffer); private: bool _writeCallback(const char* const pch, size_t const cch); @@ -1055,7 +1056,7 @@ void ConptyRoundtripTests::PassthroughHardReset() } } -void ConptyRoundtripTests::OutputWrappedLines() +void ConptyRoundtripTests::OutputWrappedLinesAtTopOfBuffer() { Log::Comment(L"Output various different wrapped lines, and ensure we emit them correctly"); VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); @@ -1071,29 +1072,95 @@ void ConptyRoundtripTests::OutputWrappedLines() _flushFirstFrame(); const auto wrappedLineLength = TerminalViewWidth + 20; + Log::Comment( + L"Case 1: Write a wrapped line right at the start of the buffer, before any circling"); + + sm.ProcessString(std::wstring(wrappedLineLength, L'A')); + + auto verifyBuffer = [](const TextBuffer& tb) { + VERIFY_IS_TRUE(tb.GetRowByOffset(0).GetCharRow().WasWrapForced()); + VERIFY_IS_FALSE(tb.GetRowByOffset(1).GetCharRow().WasWrapForced()); + auto iter0 = tb.GetCellDataAt({ 0, 0 }); + TestUtils::VerifySpanOfText(L"A", iter0, 0, TerminalViewWidth); + auto iter1 = tb.GetCellDataAt({ 0, 1 }); + TestUtils::VerifySpanOfText(L"A", iter1, 0, 20); + auto iter2 = tb.GetCellDataAt({ 20, 1 }); + TestUtils::VerifySpanOfText(L" ", iter2, 0, TerminalViewWidth - 20); + }; + + verifyBuffer(hostTb); + + expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); + expectedOutput.push_back(std::string(20, 'A')); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyBuffer(termTb); +} + +void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() +{ + Log::Comment( + L"Case 2: Write a wrapped line at the end of the buffer, once the conpty started circling"); + 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(); + + // First, fill the buffer with contents, so conpty starts circling + + const auto hostView = si.GetViewport(); + const auto end = 2 * hostView.Height(); + for (auto i = 0; i < end; i++) { - Log::Comment( - L"Case 1: Write a wrapped line right at the start of the buffer, before any circling"); - - sm.ProcessString(std::wstring(wrappedLineLength, L'A')); - - auto verifyBuffer = [](const TextBuffer& tb) { - VERIFY_IS_TRUE(tb.GetRowByOffset(0).GetCharRow().WasWrapForced()); - VERIFY_IS_FALSE(tb.GetRowByOffset(1).GetCharRow().WasWrapForced()); - auto iter0 = tb.GetCellDataAt({ 0, 0 }); - TestUtils::VerifySpanOfText(L"A", iter0, 0, TerminalViewWidth); - auto iter1 = tb.GetCellDataAt({ 0, 1 }); - TestUtils::VerifySpanOfText(L"A", iter1, 0, 20); - auto iter2 = tb.GetCellDataAt({ 20, 1 }); - TestUtils::VerifySpanOfText(L" ", iter2, 0, TerminalViewWidth - 20); - }; - - verifyBuffer(hostTb); - - expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); - expectedOutput.push_back(std::string(20, 'A')); - VERIFY_SUCCEEDED(renderer.PaintFrame()); + Log::Comment(NoThrowString().Format(L"Writing line %d/%d", i, end)); + expectedOutput.push_back("X"); + if (i < hostView.BottomInclusive()) + { + expectedOutput.push_back("\r\n"); + } + else + { + // After we hit the bottom of the viewport, the newlines come in + // separated by empty writes for whatever reason. + expectedOutput.push_back("\r\n"); + expectedOutput.push_back(""); + } + + hostSm.ProcessString(L"X\n"); - verifyBuffer(termTb); + VERIFY_SUCCEEDED(renderer.PaintFrame()); } + + const auto wrappedLineLength = TerminalViewWidth + 20; + + expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); + expectedOutput.push_back("\x1b[32;80H"); // If a future change removes the need for this, it wouldn't be the worst. + expectedOutput.push_back(std::string(20, 'A')); + + hostSm.ProcessString(std::wstring(wrappedLineLength, L'A')); + + auto verifyBuffer = [](const TextBuffer& tb, const short wrappedRow) { + VERIFY_IS_TRUE(tb.GetRowByOffset(wrappedRow).GetCharRow().WasWrapForced()); + VERIFY_IS_FALSE(tb.GetRowByOffset(wrappedRow + 1).GetCharRow().WasWrapForced()); + + auto iter0 = tb.GetCellDataAt({ 0, wrappedRow }); + TestUtils::VerifySpanOfText(L"A", iter0, 0, TerminalViewWidth); + auto iter1 = tb.GetCellDataAt({ 0, wrappedRow + 1 }); + TestUtils::VerifySpanOfText(L"A", iter1, 0, 20); + auto iter2 = tb.GetCellDataAt({ 20, wrappedRow + 1 }); + TestUtils::VerifySpanOfText(L" ", iter2, 0, TerminalViewWidth - 20); + }; + + verifyBuffer(hostTb, hostView.BottomInclusive() - 1); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyBuffer(termTb, term->_mutableViewport.BottomInclusive() - 1); } From e21101bd1380a6226f75881bb1110ab637feb5a7 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Mar 2020 15:04:19 -0500 Subject: [PATCH 09/44] This all wroks far to shockingly well --- .../ConptyRoundtripTests.cpp | 2 +- src/host/ut_host/ConptyOutputTests.cpp | 2 +- src/renderer/vt/XtermEngine.cpp | 41 +++++++++++++------ 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index d1408eeb977..798e0968d21 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1141,7 +1141,7 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() const auto wrappedLineLength = TerminalViewWidth + 20; expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); - expectedOutput.push_back("\x1b[32;80H"); // If a future change removes the need for this, it wouldn't be the worst. + // expectedOutput.push_back("\x1b[32;80H"); // If a future change removes the need for this, it wouldn't be the worst. expectedOutput.push_back(std::string(20, 'A')); hostSm.ProcessString(std::wstring(wrappedLineLength, L'A')); diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp index cecfb890dee..c771b6df143 100644 --- a/src/host/ut_host/ConptyOutputTests.cpp +++ b/src/host/ut_host/ConptyOutputTests.cpp @@ -347,7 +347,7 @@ void ConptyOutputTests::InvalidateUntilOneBeforeEnd() expectedOutput.push_back("\x1b[65C"); expectedOutput.push_back("ABCDEFGHIJKLMNO"); - expectedOutput.push_back("\x1b[1;80H"); // we move the cursor to the end of the line after paint + // expectedOutput.push_back("\x1b[1;80H"); // we move the cursor to the end of the line after paint VERIFY_SUCCEEDED(renderer.PaintFrame()); diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 5423a061b78..424ac6036df 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -215,20 +215,37 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // StartPaint) _nextCursorIsVisible = true; - const auto stashedEOLWrap = _delayedEolWrap; - const auto stashedWappedRow = _wrappedRow; - const auto result = VtEngine::PaintCursor(options); - _delayedEolWrap = stashedEOLWrap; - _wrappedRow = stashedWappedRow; - if (_wrappedRow.has_value()) - { - _trace.TraceSetWrapped(_wrappedRow.value()); - } - else + // { + // // Method.2 + // const auto stashedEOLWrap = _delayedEolWrap; + // const auto stashedWappedRow = _wrappedRow; + // const auto result = VtEngine::PaintCursor(options); + // _delayedEolWrap = stashedEOLWrap; + // _wrappedRow = stashedWappedRow; + // if (_wrappedRow.has_value()) + // { + // _trace.TraceSetWrapped(_wrappedRow.value()); + // } + // else + // { + // _trace.TraceClearWrapped(); + // } + // return result; + + // } + { - _trace.TraceClearWrapped(); + // Method.3 + if (_delayedEolWrap && _wrappedRow.has_value()) + { + } + else + { + return VtEngine::PaintCursor(options); + } + + return S_OK; } - return result; } // Routine Description: From cf1145c7aec5b63b91020733d68e61567df57fc5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Mar 2020 17:04:43 -0500 Subject: [PATCH 10/44] Add a test for this case TODO: This proves that the scrolling during a frame with other text breaks this scenario. We're going to try and special-case the scrolling thing to _only_ when * the invalid area is the bottom line, and * the line wrapped So in that case, we'll be sure that the next text will cause us to move the viewport down a line appropriately I've got a crazy theory that rendering bottom-up _might_ fix this --- .../ConptyRoundtripTests.cpp | 101 ++++++++++++++++++ src/renderer/vt/XtermEngine.cpp | 52 ++++----- src/renderer/vt/state.cpp | 1 - 3 files changed, 124 insertions(+), 30 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 798e0968d21..9c081de6a89 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -173,6 +173,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(OutputWrappedLinesAtTopOfBuffer); TEST_METHOD(OutputWrappedLinesAtBottomOfBuffer); + TEST_METHOD(ScrollWithChangesInMiddle); private: bool _writeCallback(const char* const pch, size_t const cch); @@ -1164,3 +1165,103 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() verifyBuffer(termTb, term->_mutableViewport.BottomInclusive() - 1); } + +void ConptyRoundtripTests::ScrollWithChangesInMiddle() +{ + Log::Comment(L""); + 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(); + + // First, fill the buffer with contents, so conpty starts circling + + const auto hostView = si.GetViewport(); + const auto end = 2 * hostView.Height(); + for (auto i = 0; i < end; i++) + { + Log::Comment(NoThrowString().Format(L"Writing line %d/%d", i, end)); + expectedOutput.push_back("X"); + if (i < hostView.BottomInclusive()) + { + expectedOutput.push_back("\r\n"); + } + else + { + // After we hit the bottom of the viewport, the newlines come in + // separated by empty writes for whatever reason. + expectedOutput.push_back("\r\n"); + expectedOutput.push_back(""); + } + + hostSm.ProcessString(L"X\n"); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + } + + const auto wrappedLineLength = TerminalViewWidth + 20; + + expectedOutput.push_back("\x1b[15;1H"); + expectedOutput.push_back("Y"); + expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); + // expectedOutput.push_back("\x1b[32;80H"); // If a future change removes the need for this, it wouldn't be the worst. + expectedOutput.push_back(std::string(20, 'A')); + + _checkConptyOutput = false; + + _logConpty = true; + + hostSm.ProcessString(L"\x1b" + L"7"); // Save cursor + hostSm.ProcessString(L"\x1b[15;1H"); + hostSm.ProcessString(L"Y"); + hostSm.ProcessString(L"\x1b" + L"8"); // Restore + hostSm.ProcessString(std::wstring(wrappedLineLength, L'A')); + + auto verifyBuffer = [](const TextBuffer& tb, const til::rectangle viewport) { + const short wrappedRow = viewport.bottom() - 2; + + for (short i = viewport.top(); i < wrappedRow; i++) + { + Log::Comment(NoThrowString().Format( + L"Checking row %d", i)); + TestUtils::VerifyExpectedString(tb, i == 13 ? L"Y" : L"X", { 0, i }); + } + + VERIFY_IS_TRUE(tb.GetRowByOffset(wrappedRow).GetCharRow().WasWrapForced()); + VERIFY_IS_FALSE(tb.GetRowByOffset(wrappedRow + 1).GetCharRow().WasWrapForced()); + + auto iter0 = tb.GetCellDataAt({ 0, wrappedRow }); + TestUtils::VerifySpanOfText(L"A", iter0, 0, TerminalViewWidth); + auto iter1 = tb.GetCellDataAt({ 0, wrappedRow + 1 }); + TestUtils::VerifySpanOfText(L"A", iter1, 0, 20); + auto iter2 = tb.GetCellDataAt({ 20, wrappedRow + 1 }); + TestUtils::VerifySpanOfText(L" ", iter2, 0, TerminalViewWidth - 20); + }; + + Log::Comment(NoThrowString().Format(L"Checking the host buffer...")); + verifyBuffer(hostTb, hostView.ToInclusive()); + Log::Comment(NoThrowString().Format(L"... Done")); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + // TODO: This proves that the scrolling during a frame with other text breaks this scenario. + // We're going to try and special-case the scrolling thing to _only_ when + // * the invalid area is the bottom line, and + // * the line wrapped + // So in that case, we'll be sure that the next text will cause us to move the viewport down a line appropriately + // + // I've got a crazy theory that rendering bottom-up _might_ fix this + + Log::Comment(NoThrowString().Format(L"Checking the terminal buffer...")); + verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); + Log::Comment(NoThrowString().Format(L"... Done")); +} diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 424ac6036df..37897fdfc7d 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -215,37 +215,20 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // StartPaint) _nextCursorIsVisible = true; - // { - // // Method.2 - // const auto stashedEOLWrap = _delayedEolWrap; - // const auto stashedWappedRow = _wrappedRow; - // const auto result = VtEngine::PaintCursor(options); - // _delayedEolWrap = stashedEOLWrap; - // _wrappedRow = stashedWappedRow; - // if (_wrappedRow.has_value()) - // { - // _trace.TraceSetWrapped(_wrappedRow.value()); - // } - // else - // { - // _trace.TraceClearWrapped(); - // } - // return result; - - // } - + // If we did a delayed EOL wrap because we actually wrapped the line here, + // then don't PaintCursor. When we're at the EOL because we've wrapped, our + // internal _lastText thinks the cursor is on the cell just past the right + // of the viewport (ex { 120, 0 }). However, conhost thinks the cursor is + // actually on the last cell of the row. So it'll tell us to paint the + // cursor at { 119, 0 }. If we do that movement, then we'll break line + // wrapping. + // See GH#5113, GH#1245, GH#357 + if (!(_delayedEolWrap && _wrappedRow.has_value())) { - // Method.3 - if (_delayedEolWrap && _wrappedRow.has_value()) - { - } - else - { - return VtEngine::PaintCursor(options); - } - - return S_OK; + return VtEngine::PaintCursor(options); } + + return S_OK; } // Routine Description: @@ -397,6 +380,14 @@ try //////////////////////////////////////////////////////////////////////////// // Experiment 1 { + // shift our internal tracker of the last text position according to how + // much we've scrolled. If we manually scroll the buffer right now, by + // moving the cursor to the bottom row of the viewport and newlining, + // we'll cause any wrapped lines to get broken. + // + // We'll also shift our other coordinates we're tracking - + // + // See GH#5113 _lastText.Y += dy; _trace.TraceLastText(_lastText); if (_wrappedRow.has_value()) @@ -405,6 +396,9 @@ try _trace.TraceSetWrapped(_wrappedRow.value()); } _newBottomLine = true; + + // TODO: If there's a single frame with changes in the middle of the + // frame and scrolling, how do we react? } //////////////////////////////////////////////////////////////////////////// // if (dy < 0) diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index bcbaf4994fd..8714f785561 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -36,7 +36,6 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe, _lastWasBold(false), _lastViewport(initialViewport), _invalidMap(initialViewport.Dimensions()), - _lastRealCursor({ 0 }), _lastText({ 0 }), _scrollDelta({ 0, 0 }), _quickReturn(false), From 2665b4c2b3fc35a34285f13a6fe897b87c73628f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 27 Mar 2020 09:45:18 -0500 Subject: [PATCH 11/44] this fixes this test, but maybe the test was always broken? --- .../ConptyRoundtripTests.cpp | 24 ++- src/renderer/vt/XtermEngine.cpp | 182 ++++++++++++++---- src/renderer/vt/vtrenderer.hpp | 1 - 3 files changed, 166 insertions(+), 41 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 9c081de6a89..1da1219a3d8 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1197,7 +1197,9 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() { // After we hit the bottom of the viewport, the newlines come in // separated by empty writes for whatever reason. - expectedOutput.push_back("\r\n"); + // expectedOutput.push_back("\r\n"); + expectedOutput.push_back("\n"); + expectedOutput.push_back("\r"); expectedOutput.push_back(""); } @@ -1229,11 +1231,19 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() auto verifyBuffer = [](const TextBuffer& tb, const til::rectangle viewport) { const short wrappedRow = viewport.bottom() - 2; - for (short i = viewport.top(); i < wrappedRow; i++) + const bool isTerminal = viewport.top() != 0; + + const short start = viewport.top(); + for (short i = start; i < wrappedRow; i++) { Log::Comment(NoThrowString().Format( L"Checking row %d", i)); - TestUtils::VerifyExpectedString(tb, i == 13 ? L"Y" : L"X", { 0, i }); + // TestUtils::VerifyExpectedString(tb, i == (isTerminal ? 14 : 13) ? L"Y" : L"X", { 0, i }); + TestUtils::VerifyExpectedString(tb, i == start + 13 ? L"Y" : L"X", { 0, i }); + + // auto iter = tb.GetCellDataAt({ 0, i }); + // Log::Comment(NoThrowString().Format( + // L"Found char '%s'", (iter++)->Chars().data())); } VERIFY_IS_TRUE(tb.GetRowByOffset(wrappedRow).GetCharRow().WasWrapForced()); @@ -1253,11 +1263,13 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() VERIFY_SUCCEEDED(renderer.PaintFrame()); - // TODO: This proves that the scrolling during a frame with other text breaks this scenario. - // We're going to try and special-case the scrolling thing to _only_ when + // TODO: This proves that the scrolling during a frame with other text + // breaks this scenario. We're going to try and special-case the scrolling + // thing to _only_ when // * the invalid area is the bottom line, and // * the line wrapped - // So in that case, we'll be sure that the next text will cause us to move the viewport down a line appropriately + // So in that case, we'll be sure that the next text will cause us to move + // the viewport down a line appropriately // // I've got a crazy theory that rendering bottom-up _might_ fix this diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 37897fdfc7d..8501362cd1c 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -379,15 +379,101 @@ try //////////////////////////////////////////////////////////////////////////// // Experiment 1 + // { + // // shift our internal tracker of the last text position according to how + // // much we've scrolled. If we manually scroll the buffer right now, by + // // moving the cursor to the bottom row of the viewport and newlining, + // // we'll cause any wrapped lines to get broken. + // // + // // We'll also shift our other coordinates we're tracking - + // // + // // See GH#5113 + // _lastText.Y += dy; + // _trace.TraceLastText(_lastText); + // if (_wrappedRow.has_value()) + // { + // _wrappedRow.value() += dy; + // _trace.TraceSetWrapped(_wrappedRow.value()); + // } + // _newBottomLine = true; + + // // TODO: If there's a single frame with changes in the middle of the + // // frame and scrolling, how do we react? + // } + //////////////////////////////////////////////////////////////////////////// + // //////////////////////////////////////////////////////////////////////////// + // // Experiment 2 + // { + // if (dy < 0) + // { + // // Instead of deleting the first line (causing everything to move up) + // // move to the bottom of the buffer, and newline. + // // That will cause everything to move up, by moving the viewport down. + // // This will let remote conhosts scroll up to see history like normal. + // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); + // hr = _MoveCursor({ _lastText.X, bottom }); + // if (SUCCEEDED(hr)) + // { + // std::string seq = std::string(absDy, '\n'); + // hr = _Write(seq); + // // Mark that the bottom line is new, so we won't spend time with an + // // ECH on it. + // _newBottomLine = true; + // } + // // We don't need to _MoveCursor the cursor again, because it's still + // // at the bottom of the viewport. + // } + // else if (dy > 0) + // { + // // Move to the top of the buffer, and insert some lines of text, to + // // cause the viewport contents to shift down. + // hr = _MoveCursor({ 0, 0 }); + // if (SUCCEEDED(hr)) + // { + // hr = _InsertLine(absDy); + // } + // } + // } + // //////////////////////////////////////////////////////////////////////////// + // //////////////////////////////////////////////////////////////////////////// + // // Experiment 3 + // { + // _lastText.Y += dy; + // _trace.TraceLastText(_lastText); + // if (_wrappedRow.has_value()) + // { + // _wrappedRow.value() += dy; + // _trace.TraceSetWrapped(_wrappedRow.value()); + // } + + // if (dy < 0) + // { + // // Instead of deleting the first line (causing everything to move up) + // // move to the bottom of the buffer, and newline. + // // That will cause everything to move up, by moving the viewport down. + // // This will let remote conhosts scroll up to see history like normal. + // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); + // hr = _MoveCursor({ _lastText.X, bottom }); + // _newBottomLine = true; + // // We don't need to _MoveCursor the cursor again, because it's still + // // at the bottom of the viewport. + // } + // else if (dy > 0) + // { + // // Move to the top of the buffer, and insert some lines of text, to + // // cause the viewport contents to shift down. + // hr = _MoveCursor({ 0, 0 }); + // if (SUCCEEDED(hr)) + // { + // hr = _InsertLine(absDy); + // } + // } + // } + // //////////////////////////////////////////////////////////////////////////// + + //////////////////////////////////////////////////////////////////////////// + // Experiment 4 { - // shift our internal tracker of the last text position according to how - // much we've scrolled. If we manually scroll the buffer right now, by - // moving the cursor to the bottom row of the viewport and newlining, - // we'll cause any wrapped lines to get broken. - // - // We'll also shift our other coordinates we're tracking - - // - // See GH#5113 _lastText.Y += dy; _trace.TraceLastText(_lastText); if (_wrappedRow.has_value()) @@ -395,39 +481,67 @@ try _wrappedRow.value() += dy; _trace.TraceSetWrapped(_wrappedRow.value()); } - _newBottomLine = true; - // TODO: If there's a single frame with changes in the middle of the - // frame and scrolling, how do we react? + if (dy < 0) + { + // Instead of deleting the first line (causing everything to move up) + // move to the bottom of the buffer, and newline. + // That will cause everything to move up, by moving the viewport down. + // This will let remote conhosts scroll up to see history like normal. + const short bottom = _lastViewport.ToOrigin().BottomInclusive(); + + if (!(_delayedEolWrap && _wrappedRow.has_value())) + { + hr = _MoveCursor({ _lastText.X, bottom }); + } + _newBottomLine = true; + // We don't need to _MoveCursor the cursor again, because it's still + // at the bottom of the viewport. + } + else if (dy > 0) + { + // Move to the top of the buffer, and insert some lines of text, to + // cause the viewport contents to shift down. + hr = _MoveCursor({ 0, 0 }); + if (SUCCEEDED(hr)) + { + hr = _InsertLine(absDy); + } + } } //////////////////////////////////////////////////////////////////////////// - // if (dy < 0) + + //////////////////////////////////////////////////////////////////////////// + // Original Behavior // { - // // Instead of deleting the first line (causing everything to move up) - // // move to the bottom of the buffer, and newline. - // // That will cause everything to move up, by moving the viewport down. - // // This will let remote conhosts scroll up to see history like normal. - // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); - // hr = _MoveCursor({ 0, bottom }); - // if (SUCCEEDED(hr)) + // if (dy < 0) // { - // std::string seq = std::string(absDy, '\n'); - // hr = _Write(seq); - // // Mark that the bottom line is new, so we won't spend time with an - // // ECH on it. - // _newBottomLine = true; + // // Instead of deleting the first line (causing everything to move up) + // // move to the bottom of the buffer, and newline. + // // That will cause everything to move up, by moving the viewport down. + // // This will let remote conhosts scroll up to see history like normal. + // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); + // hr = _MoveCursor({ 0, bottom }); + // if (SUCCEEDED(hr)) + // { + // std::string seq = std::string(absDy, '\n'); + // hr = _Write(seq); + // // Mark that the bottom line is new, so we won't spend time with an + // // ECH on it. + // _newBottomLine = true; + // } + // // We don't need to _MoveCursor the cursor again, because it's still + // // at the bottom of the viewport. // } - // // We don't need to _MoveCursor the cursor again, because it's still - // // at the bottom of the viewport. - // } - // else if (dy > 0) - // { - // // Move to the top of the buffer, and insert some lines of text, to - // // cause the viewport contents to shift down. - // hr = _MoveCursor({ 0, 0 }); - // if (SUCCEEDED(hr)) + // else if (dy > 0) // { - // hr = _InsertLine(absDy); + // // Move to the top of the buffer, and insert some lines of text, to + // // cause the viewport contents to shift down. + // hr = _MoveCursor({ 0, 0 }); + // if (SUCCEEDED(hr)) + // { + // hr = _InsertLine(absDy); + // } // } // } //////////////////////////////////////////////////////////////////////////// diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 2230410f4d2..3b84bc5cac8 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -124,7 +124,6 @@ namespace Microsoft::Console::Render til::bitmap _invalidMap; - COORD _lastRealCursor; COORD _lastText; til::point _scrollDelta; From 2a0cc2001b5907d814a44b0a563a108bf6c6b0d9 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 27 Mar 2020 09:58:30 -0500 Subject: [PATCH 12/44] I'm honestly shocked that this seems to work --- .../ConptyRoundtripTests.cpp | 12 +- src/renderer/vt/XtermEngine.cpp | 108 +++++++++--------- 2 files changed, 60 insertions(+), 60 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 1da1219a3d8..c8c6e127902 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1197,9 +1197,9 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() { // After we hit the bottom of the viewport, the newlines come in // separated by empty writes for whatever reason. - // expectedOutput.push_back("\r\n"); - expectedOutput.push_back("\n"); - expectedOutput.push_back("\r"); + expectedOutput.push_back("\r\n"); + // expectedOutput.push_back("\n"); + // expectedOutput.push_back("\r"); expectedOutput.push_back(""); } @@ -1212,12 +1212,12 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() expectedOutput.push_back("\x1b[15;1H"); expectedOutput.push_back("Y"); + expectedOutput.push_back("\x1b[32;1H"); expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); - // expectedOutput.push_back("\x1b[32;80H"); // If a future change removes the need for this, it wouldn't be the worst. + expectedOutput.push_back("\x1b[?25h"); expectedOutput.push_back(std::string(20, 'A')); - _checkConptyOutput = false; - + // _checkConptyOutput = false; _logConpty = true; hostSm.ProcessString(L"\x1b" diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 8501362cd1c..90bca838071 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -379,27 +379,27 @@ try //////////////////////////////////////////////////////////////////////////// // Experiment 1 - // { - // // shift our internal tracker of the last text position according to how - // // much we've scrolled. If we manually scroll the buffer right now, by - // // moving the cursor to the bottom row of the viewport and newlining, - // // we'll cause any wrapped lines to get broken. - // // - // // We'll also shift our other coordinates we're tracking - - // // - // // See GH#5113 - // _lastText.Y += dy; - // _trace.TraceLastText(_lastText); - // if (_wrappedRow.has_value()) - // { - // _wrappedRow.value() += dy; - // _trace.TraceSetWrapped(_wrappedRow.value()); - // } - // _newBottomLine = true; + { + // shift our internal tracker of the last text position according to how + // much we've scrolled. If we manually scroll the buffer right now, by + // moving the cursor to the bottom row of the viewport and newlining, + // we'll cause any wrapped lines to get broken. + // + // We'll also shift our other coordinates we're tracking - + // + // See GH#5113 + _lastText.Y += dy; + _trace.TraceLastText(_lastText); + if (_wrappedRow.has_value()) + { + _wrappedRow.value() += dy; + _trace.TraceSetWrapped(_wrappedRow.value()); + } + _newBottomLine = true; - // // TODO: If there's a single frame with changes in the middle of the - // // frame and scrolling, how do we react? - // } + // TODO: If there's a single frame with changes in the middle of the + // frame and scrolling, how do we react? + } //////////////////////////////////////////////////////////////////////////// // //////////////////////////////////////////////////////////////////////////// // // Experiment 2 @@ -473,42 +473,42 @@ try //////////////////////////////////////////////////////////////////////////// // Experiment 4 - { - _lastText.Y += dy; - _trace.TraceLastText(_lastText); - if (_wrappedRow.has_value()) - { - _wrappedRow.value() += dy; - _trace.TraceSetWrapped(_wrappedRow.value()); - } + // { + // _lastText.Y += dy; + // _trace.TraceLastText(_lastText); + // if (_wrappedRow.has_value()) + // { + // _wrappedRow.value() += dy; + // _trace.TraceSetWrapped(_wrappedRow.value()); + // } - if (dy < 0) - { - // Instead of deleting the first line (causing everything to move up) - // move to the bottom of the buffer, and newline. - // That will cause everything to move up, by moving the viewport down. - // This will let remote conhosts scroll up to see history like normal. - const short bottom = _lastViewport.ToOrigin().BottomInclusive(); + // if (dy < 0) + // { + // // Instead of deleting the first line (causing everything to move up) + // // move to the bottom of the buffer, and newline. + // // That will cause everything to move up, by moving the viewport down. + // // This will let remote conhosts scroll up to see history like normal. + // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); - if (!(_delayedEolWrap && _wrappedRow.has_value())) - { - hr = _MoveCursor({ _lastText.X, bottom }); - } - _newBottomLine = true; - // We don't need to _MoveCursor the cursor again, because it's still - // at the bottom of the viewport. - } - else if (dy > 0) - { - // Move to the top of the buffer, and insert some lines of text, to - // cause the viewport contents to shift down. - hr = _MoveCursor({ 0, 0 }); - if (SUCCEEDED(hr)) - { - hr = _InsertLine(absDy); - } - } - } + // if (!(_delayedEolWrap && _wrappedRow.has_value())) + // { + // hr = _MoveCursor({ _lastText.X, bottom }); + // } + // _newBottomLine = true; + // // We don't need to _MoveCursor the cursor again, because it's still + // // at the bottom of the viewport. + // } + // else if (dy > 0) + // { + // // Move to the top of the buffer, and insert some lines of text, to + // // cause the viewport contents to shift down. + // hr = _MoveCursor({ 0, 0 }); + // if (SUCCEEDED(hr)) + // { + // hr = _InsertLine(absDy); + // } + // } + // } //////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////// From 51f060e10024f937fad0a7b1da4618b0d061320f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 27 Mar 2020 10:41:28 -0500 Subject: [PATCH 13/44] cleanup for review, but I have to wait for #5122 to merge first --- .../ConptyRoundtripTests.cpp | 60 +++--- src/renderer/vt/XtermEngine.cpp | 188 ++---------------- 2 files changed, 42 insertions(+), 206 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index c8c6e127902..cb34944d0d0 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1059,7 +1059,8 @@ void ConptyRoundtripTests::PassthroughHardReset() void ConptyRoundtripTests::OutputWrappedLinesAtTopOfBuffer() { - Log::Comment(L"Output various different wrapped lines, and ensure we emit them correctly"); + Log::Comment( + L"Case 1: Write a wrapped line right at the start of the buffer, before any circling"); VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); auto& g = ServiceLocator::LocateGlobals(); @@ -1073,8 +1074,6 @@ void ConptyRoundtripTests::OutputWrappedLinesAtTopOfBuffer() _flushFirstFrame(); const auto wrappedLineLength = TerminalViewWidth + 20; - Log::Comment( - L"Case 1: Write a wrapped line right at the start of the buffer, before any circling"); sm.ProcessString(std::wstring(wrappedLineLength, L'A')); @@ -1142,7 +1141,6 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() const auto wrappedLineLength = TerminalViewWidth + 20; expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); - // expectedOutput.push_back("\x1b[32;80H"); // If a future change removes the need for this, it wouldn't be the worst. expectedOutput.push_back(std::string(20, 'A')); hostSm.ProcessString(std::wstring(wrappedLineLength, L'A')); @@ -1168,7 +1166,11 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() void ConptyRoundtripTests::ScrollWithChangesInMiddle() { - Log::Comment(L""); + Log::Comment(L"This test checks emitting a wrapped line at the bottom of the" + L" viewport while _also_ emitting other text elsewhere in the same frame. This" + L" output will cause us to scroll the viewport in one frame, but we need to" + L" make sure the wrapped line _stays_ wrapped, and the scrolled text appears in" + L" the right place."); VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); auto& g = ServiceLocator::LocateGlobals(); @@ -1198,8 +1200,6 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() // After we hit the bottom of the viewport, the newlines come in // separated by empty writes for whatever reason. expectedOutput.push_back("\r\n"); - // expectedOutput.push_back("\n"); - // expectedOutput.push_back("\r"); expectedOutput.push_back(""); } @@ -1210,40 +1210,36 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() const auto wrappedLineLength = TerminalViewWidth + 20; - expectedOutput.push_back("\x1b[15;1H"); - expectedOutput.push_back("Y"); - expectedOutput.push_back("\x1b[32;1H"); - expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); - expectedOutput.push_back("\x1b[?25h"); - expectedOutput.push_back(std::string(20, 'A')); + // In the Terminal, we're going to expect: + expectedOutput.push_back("\x1b[15;1H"); // Move the cursor to row 14, col 0 + expectedOutput.push_back("Y"); // Print a 'Y' + expectedOutput.push_back("\x1b[32;1H"); // Move the cursor to the last row + expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); // Print the first 80 'A's + // This is going to be the end of the first frame - b/c we moved the cursor + // in the middle of the frame, we're going to hide/show the cursor during + // this frame + expectedOutput.push_back("\x1b[?25h"); // hide the cursor + // On the subsequent frame: + expectedOutput.push_back(std::string(20, 'A')); // print the remaining 'A's - // _checkConptyOutput = false; _logConpty = true; + // To the host, we'll do something very similar: hostSm.ProcessString(L"\x1b" L"7"); // Save cursor - hostSm.ProcessString(L"\x1b[15;1H"); - hostSm.ProcessString(L"Y"); + hostSm.ProcessString(L"\x1b[15;1H"); // Move the cursor to row 14, col 0 + hostSm.ProcessString(L"Y"); // Print a 'Y' hostSm.ProcessString(L"\x1b" L"8"); // Restore - hostSm.ProcessString(std::wstring(wrappedLineLength, L'A')); + hostSm.ProcessString(std::wstring(wrappedLineLength, L'A')); // Print 100 'A's auto verifyBuffer = [](const TextBuffer& tb, const til::rectangle viewport) { const short wrappedRow = viewport.bottom() - 2; - - const bool isTerminal = viewport.top() != 0; - const short start = viewport.top(); for (short i = start; i < wrappedRow; i++) { - Log::Comment(NoThrowString().Format( - L"Checking row %d", i)); - // TestUtils::VerifyExpectedString(tb, i == (isTerminal ? 14 : 13) ? L"Y" : L"X", { 0, i }); + Log::Comment(NoThrowString().Format(L"Checking row %d", i)); TestUtils::VerifyExpectedString(tb, i == start + 13 ? L"Y" : L"X", { 0, i }); - - // auto iter = tb.GetCellDataAt({ 0, i }); - // Log::Comment(NoThrowString().Format( - // L"Found char '%s'", (iter++)->Chars().data())); } VERIFY_IS_TRUE(tb.GetRowByOffset(wrappedRow).GetCharRow().WasWrapForced()); @@ -1263,16 +1259,6 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() VERIFY_SUCCEEDED(renderer.PaintFrame()); - // TODO: This proves that the scrolling during a frame with other text - // breaks this scenario. We're going to try and special-case the scrolling - // thing to _only_ when - // * the invalid area is the bottom line, and - // * the line wrapped - // So in that case, we'll be sure that the next text will cause us to move - // the viewport down a line appropriately - // - // I've got a crazy theory that rendering bottom-up _might_ fix this - Log::Comment(NoThrowString().Format(L"Checking the terminal buffer...")); verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); Log::Comment(NoThrowString().Format(L"... Done")); diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 90bca838071..bfab27dd00d 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -375,178 +375,28 @@ try const short dy = _scrollDelta.y(); const short absDy = static_cast(abs(dy)); - HRESULT hr = S_OK; - - //////////////////////////////////////////////////////////////////////////// - // Experiment 1 + // Shift our internal tracker of the last text position according to how + // much we've scrolled. If we manually scroll the buffer right now, by + // moving the cursor to the bottom row of the viewport and emitting a + // newline, we'll cause any wrapped lines to get broken. + // + // Instead, we'll just update our internal tracker of where the buffer + // contents are. On this frame, we'll then still move the cursor correctly + // relative to the new frame contents. To do this, we'll shift our + // coordinates we're tracking, like the row that we wrapped on and the + // position we think we left the cursor. + // + // See GH#5113 + _lastText.Y += dy; + _trace.TraceLastText(_lastText); + if (_wrappedRow.has_value()) { - // shift our internal tracker of the last text position according to how - // much we've scrolled. If we manually scroll the buffer right now, by - // moving the cursor to the bottom row of the viewport and newlining, - // we'll cause any wrapped lines to get broken. - // - // We'll also shift our other coordinates we're tracking - - // - // See GH#5113 - _lastText.Y += dy; - _trace.TraceLastText(_lastText); - if (_wrappedRow.has_value()) - { - _wrappedRow.value() += dy; - _trace.TraceSetWrapped(_wrappedRow.value()); - } - _newBottomLine = true; - - // TODO: If there's a single frame with changes in the middle of the - // frame and scrolling, how do we react? + _wrappedRow.value() += dy; + _trace.TraceSetWrapped(_wrappedRow.value()); } - //////////////////////////////////////////////////////////////////////////// - // //////////////////////////////////////////////////////////////////////////// - // // Experiment 2 - // { - // if (dy < 0) - // { - // // Instead of deleting the first line (causing everything to move up) - // // move to the bottom of the buffer, and newline. - // // That will cause everything to move up, by moving the viewport down. - // // This will let remote conhosts scroll up to see history like normal. - // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); - // hr = _MoveCursor({ _lastText.X, bottom }); - // if (SUCCEEDED(hr)) - // { - // std::string seq = std::string(absDy, '\n'); - // hr = _Write(seq); - // // Mark that the bottom line is new, so we won't spend time with an - // // ECH on it. - // _newBottomLine = true; - // } - // // We don't need to _MoveCursor the cursor again, because it's still - // // at the bottom of the viewport. - // } - // else if (dy > 0) - // { - // // Move to the top of the buffer, and insert some lines of text, to - // // cause the viewport contents to shift down. - // hr = _MoveCursor({ 0, 0 }); - // if (SUCCEEDED(hr)) - // { - // hr = _InsertLine(absDy); - // } - // } - // } - // //////////////////////////////////////////////////////////////////////////// - // //////////////////////////////////////////////////////////////////////////// - // // Experiment 3 - // { - // _lastText.Y += dy; - // _trace.TraceLastText(_lastText); - // if (_wrappedRow.has_value()) - // { - // _wrappedRow.value() += dy; - // _trace.TraceSetWrapped(_wrappedRow.value()); - // } - - // if (dy < 0) - // { - // // Instead of deleting the first line (causing everything to move up) - // // move to the bottom of the buffer, and newline. - // // That will cause everything to move up, by moving the viewport down. - // // This will let remote conhosts scroll up to see history like normal. - // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); - // hr = _MoveCursor({ _lastText.X, bottom }); - // _newBottomLine = true; - // // We don't need to _MoveCursor the cursor again, because it's still - // // at the bottom of the viewport. - // } - // else if (dy > 0) - // { - // // Move to the top of the buffer, and insert some lines of text, to - // // cause the viewport contents to shift down. - // hr = _MoveCursor({ 0, 0 }); - // if (SUCCEEDED(hr)) - // { - // hr = _InsertLine(absDy); - // } - // } - // } - // //////////////////////////////////////////////////////////////////////////// - - //////////////////////////////////////////////////////////////////////////// - // Experiment 4 - // { - // _lastText.Y += dy; - // _trace.TraceLastText(_lastText); - // if (_wrappedRow.has_value()) - // { - // _wrappedRow.value() += dy; - // _trace.TraceSetWrapped(_wrappedRow.value()); - // } - - // if (dy < 0) - // { - // // Instead of deleting the first line (causing everything to move up) - // // move to the bottom of the buffer, and newline. - // // That will cause everything to move up, by moving the viewport down. - // // This will let remote conhosts scroll up to see history like normal. - // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); - - // if (!(_delayedEolWrap && _wrappedRow.has_value())) - // { - // hr = _MoveCursor({ _lastText.X, bottom }); - // } - // _newBottomLine = true; - // // We don't need to _MoveCursor the cursor again, because it's still - // // at the bottom of the viewport. - // } - // else if (dy > 0) - // { - // // Move to the top of the buffer, and insert some lines of text, to - // // cause the viewport contents to shift down. - // hr = _MoveCursor({ 0, 0 }); - // if (SUCCEEDED(hr)) - // { - // hr = _InsertLine(absDy); - // } - // } - // } - //////////////////////////////////////////////////////////////////////////// - - //////////////////////////////////////////////////////////////////////////// - // Original Behavior - // { - // if (dy < 0) - // { - // // Instead of deleting the first line (causing everything to move up) - // // move to the bottom of the buffer, and newline. - // // That will cause everything to move up, by moving the viewport down. - // // This will let remote conhosts scroll up to see history like normal. - // const short bottom = _lastViewport.ToOrigin().BottomInclusive(); - // hr = _MoveCursor({ 0, bottom }); - // if (SUCCEEDED(hr)) - // { - // std::string seq = std::string(absDy, '\n'); - // hr = _Write(seq); - // // Mark that the bottom line is new, so we won't spend time with an - // // ECH on it. - // _newBottomLine = true; - // } - // // We don't need to _MoveCursor the cursor again, because it's still - // // at the bottom of the viewport. - // } - // else if (dy > 0) - // { - // // Move to the top of the buffer, and insert some lines of text, to - // // cause the viewport contents to shift down. - // hr = _MoveCursor({ 0, 0 }); - // if (SUCCEEDED(hr)) - // { - // hr = _InsertLine(absDy); - // } - // } - // } - //////////////////////////////////////////////////////////////////////////// + _newBottomLine = true; - return hr; + return S_OK; } CATCH_RETURN(); From 436335f0ab92d4898b6f39e24d9dd1380b023425 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 30 Mar 2020 12:17:40 -0500 Subject: [PATCH 14/44] Fix the test @miniksa wrote for #5122 It was unhappy with the exact line lengths and this change --- .../ConptyRoundtripTests.cpp | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index f5db93be2a6..4a3349cbdf2 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1295,6 +1295,7 @@ void ConptyRoundtripTests::ScrollWithMargins() // The letters represent the data in the TMUX pane. // The final *** line represents the mode line which we will // attempt to hold in place and not scroll. + // Note that the last line will contain one '*' less than the width of the window. Log::Comment(L"Fill host with text pattern by feeding it into VT parser."); const auto rowsToWrite = initialTermView.Height() - 1; @@ -1311,7 +1312,7 @@ void ConptyRoundtripTests::ScrollWithMargins() } // For the last one, write out the asterisks for the mode line. - for (auto i = 0; i < initialTermView.Width(); ++i) + for (auto i = 0; i < initialTermView.Width() - 1; ++i) { hostSm.ProcessCharacter('*'); } @@ -1335,7 +1336,7 @@ void ConptyRoundtripTests::ScrollWithMargins() } // For the last row, verify we have an entire row of asterisks for the mode line. - const std::wstring expectedModeLine(initialTermView.Width(), L'*'); + const std::wstring expectedModeLine(initialTermView.Width() - 1, L'*'); const COORD expectedPos{ 0, gsl::narrow(rowsToWrite) }; TestUtils::VerifyExpectedString(tb, expectedModeLine, expectedPos); }; @@ -1348,13 +1349,13 @@ void ConptyRoundtripTests::ScrollWithMargins() expectedOutput.push_back("\r\n"); } { - const std::string expectedString(initialTermView.Width(), '*'); + const std::string expectedString(initialTermView.Width() - 1, '*'); expectedOutput.push_back(expectedString); // Cursor gets reset into bottom right corner as we're writing all the way into that corner. - std::stringstream ss; - ss << "\x1b[" << initialTermView.Height() << ";" << initialTermView.Width() << "H"; - expectedOutput.push_back(ss.str()); + // std::stringstream ss; + // ss << "\x1b[" << initialTermView.Height() << ";" << initialTermView.Width() << "H"; + // expectedOutput.push_back(ss.str()); } Log::Comment(L"Verify host buffer contains pattern."); @@ -1465,7 +1466,7 @@ void ConptyRoundtripTests::ScrollWithMargins() // For the last row, verify we have an entire row of asterisks for the mode line. { - const std::wstring expectedModeLine(initialTermView.Width(), L'*'); + const std::wstring expectedModeLine(initialTermView.Width() - 1, L'*'); const COORD modeLinePos{ 0, gsl::narrow(rowsToWrite) }; TestUtils::VerifyExpectedString(tb, expectedModeLine, modeLinePos); } @@ -1487,8 +1488,9 @@ void ConptyRoundtripTests::ScrollWithMargins() expectedOutput.push_back("\r\n"); } { - const std::string expectedString(initialTermView.Width(), '*'); - expectedOutput.push_back(expectedString); + const std::string expectedString(initialTermView.Width() - 1, '*'); + // There will be one extra blank space at the end of the line, because the + expectedOutput.push_back(expectedString + " "); } { // Cursor gets reset into second line from bottom, left most column From ac85d4353da244e21cedb5ee35dede3b0939a0af Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 Apr 2020 09:07:42 -0500 Subject: [PATCH 15/44] pr nits from dustin --- src/renderer/vt/XtermEngine.cpp | 1 - src/renderer/vt/tracing.cpp | 2 +- src/renderer/vt/tracing.hpp | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index bfab27dd00d..2e6718ad6d8 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -373,7 +373,6 @@ try } const short dy = _scrollDelta.y(); - const short absDy = static_cast(abs(dy)); // Shift our internal tracker of the last text position according to how // much we've scrolled. If we manually scroll the buffer right now, by diff --git a/src/renderer/vt/tracing.cpp b/src/renderer/vt/tracing.cpp index 5d252741514..4b7ce05ce92 100644 --- a/src/renderer/vt/tracing.cpp +++ b/src/renderer/vt/tracing.cpp @@ -149,7 +149,7 @@ void RenderTracing::TraceStartPaint(const bool quickReturn, const til::rectangle lastViewport, const til::point scrollDelt, const bool cursorMoved, - std::optional wrappedRow) const + const std::optional& wrappedRow) const { #ifndef UNIT_TESTING if (TraceLoggingProviderEnabled(g_hConsoleVtRendererTraceProvider, WINEVENT_LEVEL_VERBOSE, 0)) diff --git a/src/renderer/vt/tracing.hpp b/src/renderer/vt/tracing.hpp index d577fad28cb..f5ec8e9c5ff 100644 --- a/src/renderer/vt/tracing.hpp +++ b/src/renderer/vt/tracing.hpp @@ -43,7 +43,7 @@ namespace Microsoft::Console::VirtualTerminal const til::rectangle lastViewport, const til::point scrollDelta, const bool cursorMoved, - std::optional wrappedRow) const; + const std::optional& wrappedRow) const; void TraceEndPaint() const; }; } From d317b8b990531b391e43d21f037593b95e197e45 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 Apr 2020 14:29:27 -0500 Subject: [PATCH 16/44] pr nits from carlos --- src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 5 ----- src/host/ut_host/ConptyOutputTests.cpp | 1 - 2 files changed, 6 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 4a3349cbdf2..f002f9d4b26 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1351,11 +1351,6 @@ void ConptyRoundtripTests::ScrollWithMargins() { const std::string expectedString(initialTermView.Width() - 1, '*'); expectedOutput.push_back(expectedString); - - // Cursor gets reset into bottom right corner as we're writing all the way into that corner. - // std::stringstream ss; - // ss << "\x1b[" << initialTermView.Height() << ";" << initialTermView.Width() << "H"; - // expectedOutput.push_back(ss.str()); } Log::Comment(L"Verify host buffer contains pattern."); diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp index c9f84bda56c..20b5572d116 100644 --- a/src/host/ut_host/ConptyOutputTests.cpp +++ b/src/host/ut_host/ConptyOutputTests.cpp @@ -350,7 +350,6 @@ void ConptyOutputTests::InvalidateUntilOneBeforeEnd() expectedOutput.push_back("\x1b[65C"); expectedOutput.push_back("ABCDEFGHIJKLMNO"); - // expectedOutput.push_back("\x1b[1;80H"); // we move the cursor to the end of the line after paint VERIFY_SUCCEEDED(renderer.PaintFrame()); From 707844db88499bec2aae639b4835a046df1523a5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 Apr 2020 15:07:48 -0500 Subject: [PATCH 17/44] add the failing test for the case Dustin described --- .../ConptyRoundtripTests.cpp | 43 +++++++++++++++++++ src/renderer/vt/XtermEngine.cpp | 5 +++ 2 files changed, 48 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index f002f9d4b26..ae0f41a6aa7 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -177,6 +177,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(OutputWrappedLinesAtTopOfBuffer); TEST_METHOD(OutputWrappedLinesAtBottomOfBuffer); TEST_METHOD(ScrollWithChangesInMiddle); + TEST_METHOD(DontWrapMoveCursorInSingleFrame); TEST_METHOD(ScrollWithMargins); @@ -1507,3 +1508,45 @@ void ConptyRoundtripTests::ScrollWithMargins() // Verify the terminal side. verifyBufferAfter(termTb); } + +void ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame() +{ + // See https://github.com/microsoft/terminal/pull/5181#issuecomment-607427840 + Log::Comment(L"This is a test for when a line of text exactly wrapped, but " + L"the cursor didn't end the frame at the end of line (waiting " + L"for more wrapped text). We should still move the cursor in " + L"this case."); + 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(); + + auto verifyBuffer = [](const TextBuffer& tb) { + const COORD expectedCursor{ 8, 3 }; + VERIFY_ARE_EQUAL(expectedCursor, tb.GetCursor().GetPosition()); + VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); + }; + + hostSm.ProcessString(L"\x1b[?25l\x1b[H\x1b[75CXXXXX\x1b[4;9H\x1b[?25h"); + + Log::Comment(L"Checking the host buffer state"); + verifyBuffer(hostTb); + + expectedOutput.push_back("\x1b[75C"); + expectedOutput.push_back("XXXXX"); + expectedOutput.push_back("\x1b[4;9H"); + expectedOutput.push_back("\x1b[?25h"); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"Checking the terminal buffer state"); + verifyBuffer(termTb); +} diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 2e6718ad6d8..6e43f111177 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -225,6 +225,11 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // See GH#5113, GH#1245, GH#357 if (!(_delayedEolWrap && _wrappedRow.has_value())) { + // TODO: Only skip this when we think the cursor is in the cell + // immediately off the edge of the terminal, and the actual cursor is in + // the last cell of the row. We're in a deferred wrap, but the host + // thinks the cursor is actually in-frame. + // The test case is ^[[?25l^[[H^[[115CXXXXX^[[4;9H^[[?25h return VtEngine::PaintCursor(options); } From 8b19fcbc8d3a917502c2150b0818aa90a5694ebc Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 Apr 2020 15:23:43 -0500 Subject: [PATCH 18/44] Fix this case for @dhowett-msft --- .../ConptyRoundtripTests.cpp | 6 +++++- src/renderer/vt/XtermEngine.cpp | 21 +++++++++++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index ae0f41a6aa7..fcbed2d6484 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1529,6 +1529,9 @@ void ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame() _flushFirstFrame(); auto verifyBuffer = [](const TextBuffer& tb) { + // Simple verification: Make sure the cursor is in the correct place, + // and that it's visible. We don't care so much about the buffer + // contents in this test. const COORD expectedCursor{ 8, 3 }; VERIFY_ARE_EQUAL(expectedCursor, tb.GetCursor().GetPosition()); VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); @@ -1542,7 +1545,8 @@ void ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame() expectedOutput.push_back("\x1b[75C"); expectedOutput.push_back("XXXXX"); expectedOutput.push_back("\x1b[4;9H"); - expectedOutput.push_back("\x1b[?25h"); + // We're _not_ expecting a cursor on here, because we didn't actually hide + // the cursor during the course of this frame Log::Comment(L"Painting the frame"); VERIFY_SUCCEEDED(renderer.PaintFrame()); diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 6e43f111177..59f1c092c62 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -223,13 +223,22 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // cursor at { 119, 0 }. If we do that movement, then we'll break line // wrapping. // See GH#5113, GH#1245, GH#357 - if (!(_delayedEolWrap && _wrappedRow.has_value())) + const auto nextCursorPosition = options.coordCursor; + // Only skip this paint when we think the cursor is in the cell + // immediately off the edge of the terminal, and the actual cursor is in + // the last cell of the row. We're in a deferred wrap, but the host + // thinks the cursor is actually in-frame. + // See ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame + const bool cursorIsInDeferredWrap = (nextCursorPosition.X == _lastText.X - 1) && (nextCursorPosition.Y == _lastText.Y); + // If all three of these contitions are true, then: + // * cursorIsInDeferredWrap: The cursor is in a position where the line + // filled the last cell of the row, but the host tried to paint it in + // the past cell anyways + // * _delayedEolWrap && _wrappedRow.has_value(): We think we've deferred + // the wrap of a line. + // If they're all true, DON'T manually paint the cursor this frame. + if (!(cursorIsInDeferredWrap && _delayedEolWrap && _wrappedRow.has_value())) { - // TODO: Only skip this when we think the cursor is in the cell - // immediately off the edge of the terminal, and the actual cursor is in - // the last cell of the row. We're in a deferred wrap, but the host - // thinks the cursor is actually in-frame. - // The test case is ^[[?25l^[[H^[[115CXXXXX^[[4;9H^[[?25h return VtEngine::PaintCursor(options); } From b70aacd4be41fd4805acad0cfd3d465f7f35b299 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 Apr 2020 15:32:49 -0500 Subject: [PATCH 19/44] good bot --- .github/actions/spell-check/whitelist/alphabet.txt | 1 + .../UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 7 ++++++- src/renderer/vt/XtermEngine.cpp | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/actions/spell-check/whitelist/alphabet.txt b/.github/actions/spell-check/whitelist/alphabet.txt index 2b4d4ba8471..df9afee43af 100644 --- a/.github/actions/spell-check/whitelist/alphabet.txt +++ b/.github/actions/spell-check/whitelist/alphabet.txt @@ -21,6 +21,7 @@ QWERTYUIOP qwertyuiopasdfg TTTTTTTTTTTTTTTTTTTTTTTTTT VVVVVVVVVVVVVVVV +XXXXX yyyy ZAAZZ ZABBZ diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index fcbed2d6484..0e2a40ccdf9 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1537,7 +1537,12 @@ void ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame() VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); }; - hostSm.ProcessString(L"\x1b[?25l\x1b[H\x1b[75CXXXXX\x1b[4;9H\x1b[?25h"); + hostSm.ProcessString(L"\x1b[?25l"); + hostSm.ProcessString(L"\x1b[H"); + hostSm.ProcessString(L"\x1b[75C"); + hostSm.ProcessString(L"XXXXX"); + hostSm.ProcessString(L"\x1b[4;9H"); + hostSm.ProcessString(L"\x1b[?25h"); Log::Comment(L"Checking the host buffer state"); verifyBuffer(hostTb); diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 59f1c092c62..64217bf4b17 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -230,7 +230,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // thinks the cursor is actually in-frame. // See ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame const bool cursorIsInDeferredWrap = (nextCursorPosition.X == _lastText.X - 1) && (nextCursorPosition.Y == _lastText.Y); - // If all three of these contitions are true, then: + // If all three of these conditions are true, then: // * cursorIsInDeferredWrap: The cursor is in a position where the line // filled the last cell of the row, but the host tried to paint it in // the past cell anyways From 0cbcf1f02bcadc3873690759dcad16debbdd28c0 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 Apr 2020 16:33:22 -0500 Subject: [PATCH 20/44] fix a typo --- src/renderer/vt/XtermEngine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 64217bf4b17..40991347bbf 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -233,7 +233,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // If all three of these conditions are true, then: // * cursorIsInDeferredWrap: The cursor is in a position where the line // filled the last cell of the row, but the host tried to paint it in - // the past cell anyways + // the last cell anyways // * _delayedEolWrap && _wrappedRow.has_value(): We think we've deferred // the wrap of a line. // If they're all true, DON'T manually paint the cursor this frame. From 1c9a16f80fe0ccb5b87cc33293b727456a6e8b29 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 Apr 2020 16:33:36 -0500 Subject: [PATCH 21/44] add a test for #5039 --- .../ConptyRoundtripTests.cpp | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 0e2a40ccdf9..5b12d3438ce 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -178,6 +178,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(OutputWrappedLinesAtBottomOfBuffer); TEST_METHOD(ScrollWithChangesInMiddle); TEST_METHOD(DontWrapMoveCursorInSingleFrame); + TEST_METHOD(ClearHostTrickeryTest); TEST_METHOD(ScrollWithMargins); @@ -1559,3 +1560,73 @@ void ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame() Log::Comment(L"Checking the terminal buffer state"); verifyBuffer(termTb); } + +void ConptyRoundtripTests::ClearHostTrickeryTest() +{ + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:paintEachNewline", L"{false, true}") + END_TEST_METHOD_PROPERTIES(); + + bool paintEachNewline; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"paintEachNewline", paintEachNewline), L"TODO: Description"); + + // See https://github.com/microsoft/terminal/issues/5039#issuecomment-606833841 + Log::Comment(L"TODO: Mike, write the description"); + 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(); + + auto verifyBuffer = [](const TextBuffer& tb) { + // We _would_ expect the Terminal's cursor to be on { 8, 0 }, but this + // is currently broken due to #381/#4676. So we'll only check the X + // position, since the Y will be 1 in the Terminal till the above is + // fixed. + VERIFY_ARE_EQUAL(8, tb.GetCursor().GetPosition().X); + VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); + auto iter = TestUtils::VerifyExpectedString(tb, L" ", { 0, 0 }); + TestUtils::VerifyExpectedString(L"XXXX", iter); + }; + + _logConpty = true; + // TODO: Check the conpty output to make sure it's sensible + _checkConptyOutput = false; + + gci.LockConsole(); // Lock must be taken to manipulate buffer. + auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); + + hostSm.ProcessString(L" "); + hostSm.ProcessString(L"XXXX"); + hostSm.ProcessString(L"\x1b[?1049h"); + hostSm.ProcessString(L"\x1b#8"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + for (auto i = 0; i < si.GetViewport().Height(); i++) + { + hostSm.ProcessString(L"\n"); + if (paintEachNewline) + { + VERIFY_SUCCEEDED(renderer.PaintFrame()); + } + } + if (!paintEachNewline) + { + VERIFY_SUCCEEDED(renderer.PaintFrame()); + } + hostSm.ProcessString(L"\x1b[?1049l"); + + Log::Comment(L"Checking the host buffer state"); + verifyBuffer(hostTb); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"Checking the terminal buffer state"); + verifyBuffer(termTb); +} From c7e5a45291804470eea31fae1413fee19f9b8502 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 2 Apr 2020 09:51:13 -0500 Subject: [PATCH 22/44] This is the test case that's actaully broken in #5039 --- .../ConptyRoundtripTests.cpp | 74 +++++++++++++++---- 1 file changed, 60 insertions(+), 14 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 5b12d3438ce..683c8f9a1c3 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1564,12 +1564,29 @@ void ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame() void ConptyRoundtripTests::ClearHostTrickeryTest() { BEGIN_TEST_METHOD_PROPERTIES() - TEST_METHOD_PROPERTY(L"Data:paintEachNewline", L"{false, true}") + // TEST_METHOD_PROPERTY(L"Data:paintEachNewline", L"{0, 1, 2}") + TEST_METHOD_PROPERTY(L"Data:paintEachNewline", L"{2}") + // TEST_METHOD_PROPERTY(L"Data:cursorOnNextLine", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:cursorOnNextLine", L"{false}") + TEST_METHOD_PROPERTY(L"Data:paintAfterDECALN", L"{false, true}") + // TEST_METHOD_PROPERTY(L"Data:changeAttributes", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:changeAttributes", L"{true}") END_TEST_METHOD_PROPERTIES(); - - bool paintEachNewline; + constexpr int PaintEveryNewline = 0; + constexpr int PaintAfterAllNewlines = 1; + constexpr int DontPaintAfterNewlines = 2; + int paintEachNewline; VERIFY_SUCCEEDED(TestData::TryGetValue(L"paintEachNewline", paintEachNewline), L"TODO: Description"); + bool cursorOnNextLine; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"cursorOnNextLine", cursorOnNextLine), L"TODO: Description"); + + bool paintAfterDECALN; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"paintAfterDECALN", paintAfterDECALN), L"TODO: Description"); + + bool changeAttributes; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"changeAttributes", changeAttributes), L"TODO: Description"); + // See https://github.com/microsoft/terminal/issues/5039#issuecomment-606833841 Log::Comment(L"TODO: Mike, write the description"); VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); @@ -1584,15 +1601,26 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() _flushFirstFrame(); - auto verifyBuffer = [](const TextBuffer& tb) { + auto verifyBuffer = [&cursorOnNextLine](const TextBuffer& tb, const til::rectangle viewport) { // We _would_ expect the Terminal's cursor to be on { 8, 0 }, but this // is currently broken due to #381/#4676. So we'll only check the X // position, since the Y will be 1 in the Terminal till the above is // fixed. - VERIFY_ARE_EQUAL(8, tb.GetCursor().GetPosition().X); + const short viewTop = viewport.origin().y(); + const short cursorRow = viewTop + (cursorOnNextLine ? 1 : 0); + const short cursorCol = (cursorOnNextLine ? 5 : 15); + const COORD expectedCursor{ cursorCol, cursorRow }; + // VERIFY_ARE_EQUAL(8, tb.GetCursor().GetPosition().X); + VERIFY_ARE_EQUAL(expectedCursor, tb.GetCursor().GetPosition()); VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); - auto iter = TestUtils::VerifyExpectedString(tb, L" ", { 0, 0 }); - TestUtils::VerifyExpectedString(L"XXXX", iter); + auto iter = TestUtils::VerifyExpectedString(tb, L"AAAAA", { 0, viewTop }); + TestUtils::VerifyExpectedString(L" ", iter); + TestUtils::VerifyExpectedString(L"ZZZZZ", iter); + + if (cursorOnNextLine) + { + TestUtils::VerifyExpectedString(tb, L"BBBBB", { 0, cursorRow }); + } }; _logConpty = true; @@ -1602,31 +1630,49 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() gci.LockConsole(); // Lock must be taken to manipulate buffer. auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); - hostSm.ProcessString(L" "); - hostSm.ProcessString(L"XXXX"); + hostSm.ProcessString(L"AAAAA"); + hostSm.ProcessString(L" "); + if (changeAttributes) + { + hostSm.ProcessString(L"\x1b[44m"); + } + hostSm.ProcessString(L"ZZZZZ"); + hostSm.ProcessString(L"\x1b[0m"); + + if (cursorOnNextLine) + { + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"BBBBB"); + } + hostSm.ProcessString(L"\x1b[?1049h"); hostSm.ProcessString(L"\x1b#8"); - VERIFY_SUCCEEDED(renderer.PaintFrame()); + if (paintAfterDECALN) + { + VERIFY_SUCCEEDED(renderer.PaintFrame()); + } + for (auto i = 0; i < si.GetViewport().Height(); i++) { hostSm.ProcessString(L"\n"); - if (paintEachNewline) + if (paintEachNewline == PaintEveryNewline) { VERIFY_SUCCEEDED(renderer.PaintFrame()); } } - if (!paintEachNewline) + if (paintEachNewline == PaintAfterAllNewlines) { VERIFY_SUCCEEDED(renderer.PaintFrame()); } hostSm.ProcessString(L"\x1b[?1049l"); Log::Comment(L"Checking the host buffer state"); - verifyBuffer(hostTb); + verifyBuffer(hostTb, si.GetViewport().ToInclusive()); Log::Comment(L"Painting the frame"); + // DebugBreak(); VERIFY_SUCCEEDED(renderer.PaintFrame()); Log::Comment(L"Checking the terminal buffer state"); - verifyBuffer(termTb); + verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); } From 015844b24e43efec0424d6387434ed2ded0d6f19 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 2 Apr 2020 10:52:33 -0500 Subject: [PATCH 23/44] finish the test for #5039 --- .../ConptyRoundtripTests.cpp | 84 ++++++++++++------- src/renderer/vt/XtermEngine.cpp | 5 +- 2 files changed, 60 insertions(+), 29 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 683c8f9a1c3..1955d2ade87 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -48,6 +48,11 @@ namespace TerminalCoreUnitTests }; using namespace TerminalCoreUnitTests; +// Helper for declaring a variable to store a TEST_METHOD_PROPERTY and get it's value from the test metadata +#define INIT_TEST_PROPERTY(type, identifer, description) \ + type identifer; \ + VERIFY_SUCCEEDED(TestData::TryGetValue(L#identifer, identifer), description); + class TerminalCoreUnitTests::ConptyRoundtripTests final { static const SHORT TerminalViewWidth = 80; @@ -1564,31 +1569,49 @@ void ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame() void ConptyRoundtripTests::ClearHostTrickeryTest() { BEGIN_TEST_METHOD_PROPERTIES() - // TEST_METHOD_PROPERTY(L"Data:paintEachNewline", L"{0, 1, 2}") - TEST_METHOD_PROPERTY(L"Data:paintEachNewline", L"{2}") - // TEST_METHOD_PROPERTY(L"Data:cursorOnNextLine", L"{false, true}") - TEST_METHOD_PROPERTY(L"Data:cursorOnNextLine", L"{false}") + TEST_METHOD_PROPERTY(L"Data:paintEachNewline", L"{0, 1, 2}") + TEST_METHOD_PROPERTY(L"Data:cursorOnNextLine", L"{false, true}") TEST_METHOD_PROPERTY(L"Data:paintAfterDECALN", L"{false, true}") - // TEST_METHOD_PROPERTY(L"Data:changeAttributes", L"{false, true}") - TEST_METHOD_PROPERTY(L"Data:changeAttributes", L"{true}") + TEST_METHOD_PROPERTY(L"Data:changeAttributes", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:useLongSpaces", L"{false, true}") END_TEST_METHOD_PROPERTIES(); constexpr int PaintEveryNewline = 0; constexpr int PaintAfterAllNewlines = 1; constexpr int DontPaintAfterNewlines = 2; - int paintEachNewline; - VERIFY_SUCCEEDED(TestData::TryGetValue(L"paintEachNewline", paintEachNewline), L"TODO: Description"); - - bool cursorOnNextLine; - VERIFY_SUCCEEDED(TestData::TryGetValue(L"cursorOnNextLine", cursorOnNextLine), L"TODO: Description"); - bool paintAfterDECALN; - VERIFY_SUCCEEDED(TestData::TryGetValue(L"paintAfterDECALN", paintAfterDECALN), L"TODO: Description"); - - bool changeAttributes; - VERIFY_SUCCEEDED(TestData::TryGetValue(L"changeAttributes", changeAttributes), L"TODO: Description"); + INIT_TEST_PROPERTY(int, paintEachNewline, L"Ene of manually PaintFrame after each newline is emitted, once at the end of all newlines, or not at all"); + INIT_TEST_PROPERTY(bool, cursorOnNextLine, L"Either leave the cursor on the first line, or place it on the second line of the buffer"); + INIT_TEST_PROPERTY(bool, paintAfterDECALN, L"Controls whether we manually paint a frame after the DECALN sequence is emitted."); + INIT_TEST_PROPERTY(bool, changeAttributes, L"If true, change the text attributes after the 'A's and spaces"); + INIT_TEST_PROPERTY(bool, useLongSpaces, L"If true, print 10 spaces instead of 5, longer than a CUF sequence."); // See https://github.com/microsoft/terminal/issues/5039#issuecomment-606833841 - Log::Comment(L"TODO: Mike, write the description"); + Log::Comment(L"This is a more than comprehensive test for GH#5039. We're " + L"going to print some text to the buffer, then fill the alt-" + L"buffer with text, then switch back to the main buffer. The " + L"text from the alt buffer should not pollute the main buffer."); + + // The text we're printing will look like one of the following, with the + // cursor on the _ + // * cursorOnNextLine=false, useLongSpaces=false: + // AAAAA ZZZZZ_ + // * cursorOnNextLine=false, useLongSpaces=true: + // AAAAA ZZZZZ_ + // * cursorOnNextLine=true, useLongSpaces=false: + // AAAAA ZZZZZ + // BBBBB_ + // * cursorOnNextLine=true, useLongSpaces=true: + // AAAAA ZZZZZ + // BBBBB_ + // + // The interesting case that repros the bug in GH#5039 is + // - paintEachNewline=DontPaintAfterNewlines (2) + // - cursorOnNextLine=false + // - paintAfterDECALN= + // - changeAttributes=true + // - useLongSpaces= + // + // All the possible cases are left here though, to catch potential future regressions. VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); auto& g = ServiceLocator::LocateGlobals(); @@ -1601,20 +1624,21 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() _flushFirstFrame(); - auto verifyBuffer = [&cursorOnNextLine](const TextBuffer& tb, const til::rectangle viewport) { + auto verifyBuffer = [&cursorOnNextLine, &useLongSpaces](const TextBuffer& tb, + const til::rectangle viewport) { // We _would_ expect the Terminal's cursor to be on { 8, 0 }, but this - // is currently broken due to #381/#4676. So we'll only check the X - // position, since the Y will be 1 in the Terminal till the above is - // fixed. + // is currently broken due to #381/#4676. So we'll use the viewport + // provided to find the actual Y position of the cursor. const short viewTop = viewport.origin().y(); const short cursorRow = viewTop + (cursorOnNextLine ? 1 : 0); - const short cursorCol = (cursorOnNextLine ? 5 : 15); + const short cursorCol = (cursorOnNextLine ? 5 : + (15 + (useLongSpaces ? 5 : 0))); const COORD expectedCursor{ cursorCol, cursorRow }; - // VERIFY_ARE_EQUAL(8, tb.GetCursor().GetPosition().X); + VERIFY_ARE_EQUAL(expectedCursor, tb.GetCursor().GetPosition()); VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); auto iter = TestUtils::VerifyExpectedString(tb, L"AAAAA", { 0, viewTop }); - TestUtils::VerifyExpectedString(L" ", iter); + TestUtils::VerifyExpectedString(useLongSpaces ? L" " : L" ", iter); TestUtils::VerifyExpectedString(L"ZZZZZ", iter); if (cursorOnNextLine) @@ -1624,14 +1648,15 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() }; _logConpty = true; - // TODO: Check the conpty output to make sure it's sensible + // We're _not_ checking the conpty output during this test, only the side effects. _checkConptyOutput = false; - gci.LockConsole(); // Lock must be taken to manipulate buffer. + gci.LockConsole(); // Lock must be taken to manipulate alt/main buffer state. auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); + Log::Comment(L"Setting up the host buffer..."); hostSm.ProcessString(L"AAAAA"); - hostSm.ProcessString(L" "); + hostSm.ProcessString(useLongSpaces ? L" " : L" "); if (changeAttributes) { hostSm.ProcessString(L"\x1b[44m"); @@ -1644,7 +1669,10 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() hostSm.ProcessString(L"\n"); hostSm.ProcessString(L"BBBBB"); } + Log::Comment(L"Painting after the initial setup."); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + Log::Comment(L"Switching to the alt buffer and using DECALN to fill it with 'E's"); hostSm.ProcessString(L"\x1b[?1049h"); hostSm.ProcessString(L"\x1b#8"); if (paintAfterDECALN) @@ -1664,13 +1692,13 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() { VERIFY_SUCCEEDED(renderer.PaintFrame()); } + Log::Comment(L"Returning to the main buffer."); hostSm.ProcessString(L"\x1b[?1049l"); Log::Comment(L"Checking the host buffer state"); verifyBuffer(hostTb, si.GetViewport().ToInclusive()); Log::Comment(L"Painting the frame"); - // DebugBreak(); VERIFY_SUCCEEDED(renderer.PaintFrame()); Log::Comment(L"Checking the terminal buffer state"); diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 40991347bbf..203a3aeb792 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -407,7 +407,10 @@ try _wrappedRow.value() += dy; _trace.TraceSetWrapped(_wrappedRow.value()); } - _newBottomLine = true; + + const bool allInvalidated = _invalidMap.all(); + + _newBottomLine = !allInvalidated; return S_OK; } From a939f7e2125a422f5391304799554e3a03c12255 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 2 Apr 2020 11:28:47 -0500 Subject: [PATCH 24/44] Update this test with more info from the discussion thread --- .../ConptyRoundtripTests.cpp | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 1955d2ade87..f75126586a2 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1574,6 +1574,7 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() TEST_METHOD_PROPERTY(L"Data:paintAfterDECALN", L"{false, true}") TEST_METHOD_PROPERTY(L"Data:changeAttributes", L"{false, true}") TEST_METHOD_PROPERTY(L"Data:useLongSpaces", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:printTextAfterSpaces", L"{false, true}") END_TEST_METHOD_PROPERTIES(); constexpr int PaintEveryNewline = 0; constexpr int PaintAfterAllNewlines = 1; @@ -1584,6 +1585,7 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() INIT_TEST_PROPERTY(bool, paintAfterDECALN, L"Controls whether we manually paint a frame after the DECALN sequence is emitted."); INIT_TEST_PROPERTY(bool, changeAttributes, L"If true, change the text attributes after the 'A's and spaces"); INIT_TEST_PROPERTY(bool, useLongSpaces, L"If true, print 10 spaces instead of 5, longer than a CUF sequence."); + INIT_TEST_PROPERTY(bool, printTextAfterSpaces, L"If true, print \"ZZZZZ\" after the spaces on the first line."); // See https://github.com/microsoft/terminal/issues/5039#issuecomment-606833841 Log::Comment(L"This is a more than comprehensive test for GH#5039. We're " @@ -1604,12 +1606,15 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() // AAAAA ZZZZZ // BBBBB_ // + // If printTextAfterSpaces=false, then we won't print the "ZZZZZ" + // // The interesting case that repros the bug in GH#5039 is // - paintEachNewline=DontPaintAfterNewlines (2) // - cursorOnNextLine=false // - paintAfterDECALN= // - changeAttributes=true // - useLongSpaces= + // - printTextAfterSpaces= // // All the possible cases are left here though, to catch potential future regressions. VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); @@ -1624,22 +1629,30 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() _flushFirstFrame(); - auto verifyBuffer = [&cursorOnNextLine, &useLongSpaces](const TextBuffer& tb, - const til::rectangle viewport) { + auto verifyBuffer = [&cursorOnNextLine, &useLongSpaces, &printTextAfterSpaces](const TextBuffer& tb, + const til::rectangle viewport) { // We _would_ expect the Terminal's cursor to be on { 8, 0 }, but this // is currently broken due to #381/#4676. So we'll use the viewport // provided to find the actual Y position of the cursor. const short viewTop = viewport.origin().y(); const short cursorRow = viewTop + (cursorOnNextLine ? 1 : 0); const short cursorCol = (cursorOnNextLine ? 5 : - (15 + (useLongSpaces ? 5 : 0))); + (10 + (useLongSpaces ? 5 : 0) + (printTextAfterSpaces ? 5 : 0))); const COORD expectedCursor{ cursorCol, cursorRow }; VERIFY_ARE_EQUAL(expectedCursor, tb.GetCursor().GetPosition()); VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); auto iter = TestUtils::VerifyExpectedString(tb, L"AAAAA", { 0, viewTop }); TestUtils::VerifyExpectedString(useLongSpaces ? L" " : L" ", iter); - TestUtils::VerifyExpectedString(L"ZZZZZ", iter); + if (printTextAfterSpaces) + { + TestUtils::VerifyExpectedString(L"ZZZZZ", iter); + } + else + { + TestUtils::VerifyExpectedString(L" ", iter); + } + TestUtils::VerifyExpectedString(L" ", iter); if (cursorOnNextLine) { @@ -1647,7 +1660,6 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() } }; - _logConpty = true; // We're _not_ checking the conpty output during this test, only the side effects. _checkConptyOutput = false; @@ -1661,7 +1673,10 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() { hostSm.ProcessString(L"\x1b[44m"); } - hostSm.ProcessString(L"ZZZZZ"); + if (printTextAfterSpaces) + { + hostSm.ProcessString(L"ZZZZZ"); + } hostSm.ProcessString(L"\x1b[0m"); if (cursorOnNextLine) From a1cd2c517aa1ec9397d7214d434c777b541d3cc4 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 2 Apr 2020 12:01:50 -0500 Subject: [PATCH 25/44] This is the latest testcase dustin found for me --- .../ConptyRoundtripTests.cpp | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index f75126586a2..cda7cf0798e 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -184,6 +184,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(ScrollWithChangesInMiddle); TEST_METHOD(DontWrapMoveCursorInSingleFrame); TEST_METHOD(ClearHostTrickeryTest); + TEST_METHOD(OverstrikeAtBottomOfBuffer); TEST_METHOD(ScrollWithMargins); @@ -1719,3 +1720,75 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() Log::Comment(L"Checking the terminal buffer state"); verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); } + +void ConptyRoundtripTests::OverstrikeAtBottomOfBuffer() +{ + // See https://github.com/microsoft/terminal/pull/5181#issuecomment-607545241 + Log::Comment(L"TODO: description"); + 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(); + + auto verifyBuffer = [](const TextBuffer& tb, + const til::rectangle viewport) { + const auto lastRow = viewport.bottom() - 1; + const til::point expectedCursor{ 0, lastRow - 1 }; + VERIFY_ARE_EQUAL(expectedCursor, til::point{ tb.GetCursor().GetPosition() }); + VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); + + TestUtils::VerifyExpectedString(tb, L"AAAAAAAAAA DDDDDDDDDD", til::point{ 0, lastRow - 2 }); + TestUtils::VerifyExpectedString(tb, L"BBBBBBBBBB", til::point{ 0, lastRow - 1 }); + TestUtils::VerifyExpectedString(tb, L"FFFFFFFFFE", til::point{ 0, lastRow }); + }; + + _logConpty = true; + // We're _not_ checking the conpty output during this test, only the side effects. + _checkConptyOutput = false; + + hostSm.ProcessString(L"\x1b#8"); + + hostSm.ProcessString(L"\x1b[32;1H"); + + hostSm.ProcessString(L"\x1b[J"); + hostSm.ProcessString(L"AAAAAAAAAA"); + hostSm.ProcessString(L"\x1b[K"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"BBBBBBBBBB"); + hostSm.ProcessString(L"\x1b[K"); + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"CCCCCCCCCC"); + hostSm.ProcessString(L"\x1b[2A"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"\x1b[23C"); + hostSm.ProcessString(L"DDDDDDDDDD"); + hostSm.ProcessString(L"\x1b[K"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"\x1b[1B"); + hostSm.ProcessString(L"EEEEEEEEEE"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"FFFFFFFFF"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"\x1b[A"); + hostSm.ProcessString(L"\x1b[A"); + hostSm.ProcessString(L"\n"); + + Log::Comment(L"========== Checking the host buffer state =========="); + verifyBuffer(hostTb, si.GetViewport().ToInclusive()); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Checking the terminal buffer state =========="); + // DebugBreak(); + verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); +} From c39871b4e7c16cd805e8e422d68a2fa38105a942 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 3 Apr 2020 09:58:44 -0500 Subject: [PATCH 26/44] Fix the bug that Dustin reported in the PR --- .../ConptyRoundtripTests.cpp | 31 +++++++--- src/renderer/vt/XtermEngine.cpp | 58 ++++++++++++++++--- src/renderer/vt/paint.cpp | 11 ++-- 3 files changed, 79 insertions(+), 21 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index cda7cf0798e..1ec372cd275 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -963,7 +963,8 @@ void ConptyRoundtripTests::PassthroughClearScrollback() { // After we hit the bottom of the viewport, the newlines come in // separated by empty writes for whatever reason. - expectedOutput.push_back("\r\n"); + expectedOutput.push_back("\r"); + expectedOutput.push_back("\n"); expectedOutput.push_back(""); } @@ -1038,7 +1039,8 @@ void ConptyRoundtripTests::PassthroughHardReset() { // After we hit the bottom of the viewport, the newlines come in // separated by empty writes for whatever reason. - expectedOutput.push_back("\r\n"); + expectedOutput.push_back("\r"); + expectedOutput.push_back("\n"); expectedOutput.push_back(""); } @@ -1142,7 +1144,8 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() { // After we hit the bottom of the viewport, the newlines come in // separated by empty writes for whatever reason. - expectedOutput.push_back("\r\n"); + expectedOutput.push_back("\r"); + expectedOutput.push_back("\n"); expectedOutput.push_back(""); } @@ -1154,7 +1157,12 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() const auto wrappedLineLength = TerminalViewWidth + 20; expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); - expectedOutput.push_back(std::string(20, 'A')); + // TODO GH#5228 might break the "newline & repaint the wrapped char" checks here, that's okay. + expectedOutput.push_back("\r"); // This \r\n is emitted by ScrollFrame to + expectedOutput.push_back("\n"); // add a newline to the bottom of the buffer + expectedOutput.push_back("\x1b[31;80H"); // Move the cursor BACK to the wrapped row + expectedOutput.push_back(std::string(1, 'A')); // Reprint the last character of the wrapped row + expectedOutput.push_back(std::string(20, 'A')); // Print the second line. hostSm.ProcessString(std::wstring(wrappedLineLength, L'A')); @@ -1172,6 +1180,7 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() verifyBuffer(hostTb, hostView.BottomInclusive() - 1); + // DebugBreak(); VERIFY_SUCCEEDED(renderer.PaintFrame()); verifyBuffer(termTb, term->_mutableViewport.BottomInclusive() - 1); @@ -1212,7 +1221,8 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() { // After we hit the bottom of the viewport, the newlines come in // separated by empty writes for whatever reason. - expectedOutput.push_back("\r\n"); + expectedOutput.push_back("\r"); + expectedOutput.push_back("\n"); expectedOutput.push_back(""); } @@ -1233,7 +1243,12 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() // this frame expectedOutput.push_back("\x1b[?25h"); // hide the cursor // On the subsequent frame: - expectedOutput.push_back(std::string(20, 'A')); // print the remaining 'A's + // TODO GH#5228 might break the "newline & repaint the wrapped char" checks here, that's okay. + expectedOutput.push_back("\r"); // This \r\n is emitted by ScrollFrame to + expectedOutput.push_back("\n"); // add a newline to the bottom of the buffer + expectedOutput.push_back("\x1b[31;80H"); // Move the cursor BACK to the wrapped row + expectedOutput.push_back(std::string(1, 'A')); // Reprint the last character of the wrapped row + expectedOutput.push_back(std::string(20, 'A')); // Print the second line. _logConpty = true; @@ -1744,7 +1759,7 @@ void ConptyRoundtripTests::OverstrikeAtBottomOfBuffer() VERIFY_ARE_EQUAL(expectedCursor, til::point{ tb.GetCursor().GetPosition() }); VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); - TestUtils::VerifyExpectedString(tb, L"AAAAAAAAAA DDDDDDDDDD", til::point{ 0, lastRow - 2 }); + TestUtils::VerifyExpectedString(tb, L"AAAAAAAAAA DDDDDDDDDD", til::point{ 0, lastRow - 2 }); TestUtils::VerifyExpectedString(tb, L"BBBBBBBBBB", til::point{ 0, lastRow - 1 }); TestUtils::VerifyExpectedString(tb, L"FFFFFFFFFE", til::point{ 0, lastRow }); }; @@ -1768,7 +1783,7 @@ void ConptyRoundtripTests::OverstrikeAtBottomOfBuffer() hostSm.ProcessString(L"CCCCCCCCCC"); hostSm.ProcessString(L"\x1b[2A"); hostSm.ProcessString(L"\r"); - hostSm.ProcessString(L"\x1b[23C"); + hostSm.ProcessString(L"\x1b[20C"); hostSm.ProcessString(L"DDDDDDDDDD"); hostSm.ProcessString(L"\x1b[K"); hostSm.ProcessString(L"\r"); diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 203a3aeb792..75f1051424a 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -260,9 +260,9 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, [[nodiscard]] HRESULT XtermEngine::_MoveCursor(COORD const coord) noexcept { HRESULT hr = S_OK; - + const auto originalPos = _lastText; _trace.TraceMoveCursor(_lastText, coord); - + bool performedSoftWrap = false; if (coord.X != _lastText.X || coord.Y != _lastText.Y) { if (coord.X == 0 && coord.Y == 0) @@ -288,6 +288,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, if (previousLineWrapped) { + performedSoftWrap = true; _trace.TraceWrapped(); hr = S_OK; } @@ -346,10 +347,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _lastText = coord; } } - if (_lastText.Y != _lastViewport.ToOrigin().BottomInclusive()) - { - _newBottomLine = false; - } + _deferredCursorPos = INVALID_COORDS; _wrappedRow = std::nullopt; @@ -386,7 +384,38 @@ try return S_OK; } - const short dy = _scrollDelta.y(); + const short dy = _scrollDelta.y(); + const short absDy = static_cast(abs(dy)); + + if (dy < 0) + { + // Save the old wrap state here. We're gonig to clear it so that + // _MoveCursor will definitely move us to the right position. We'll + // restore the state afterwards. + const auto oldWrappedRow = _wrappedRow; + const auto oldDelayedEolWrap = _delayedEolWrap; + _delayedEolWrap = false; + _wrappedRow = std::nullopt; + + // TODO GH#5228 - We could optimize this by only doing this newline work + // when there's more invalid than just the bottom line. If only the + // bottom line is invalid, then the next thing the Renderer is going to + // tell us to do is print the new line at the bottom of the viewport, + // and _MoveCursor will automatically give us the newline we want. + // When that's implemented, we'll probably want to make sure to add a + // _lastText.Y += dy; + // statement here. + + // Move the cursor to the bottom of the current viewport + const short bottom = _lastViewport.BottomInclusive(); + RETURN_IF_FAILED(_MoveCursor({ 0, bottom })); + // Emit some number of newlines to create space in the buffer. + RETURN_IF_FAILED(_Write(std::string(absDy, '\n'))); + + // Restore our wrap state. + _wrappedRow = oldWrappedRow; + _delayedEolWrap = oldDelayedEolWrap; + } // Shift our internal tracker of the last text position according to how // much we've scrolled. If we manually scroll the buffer right now, by @@ -400,7 +429,6 @@ try // position we think we left the cursor. // // See GH#5113 - _lastText.Y += dy; _trace.TraceLastText(_lastText); if (_wrappedRow.has_value()) { @@ -408,8 +436,20 @@ try _trace.TraceSetWrapped(_wrappedRow.value()); } - const bool allInvalidated = _invalidMap.all(); + if (_delayedEolWrap && _wrappedRow.has_value()) + { + const til::rectangle lastCellOfWrappedRow{ + til::point{ _lastViewport.RightInclusive(), _wrappedRow.value() }, + til::size{ 1, 1 } + }; + _trace.TraceInvalidate(lastCellOfWrappedRow); + _invalidMap.set(lastCellOfWrappedRow); + } + // If the entire viewport was invalidated this frame, don't mark the bottom + // line as new. There are cases where this can cause visual artifacts - see + // GH#5039 and ConptyRoundtripTests::ClearHostTrickeryTest + const bool allInvalidated = _invalidMap.all(); _newBottomLine = !allInvalidated; return S_OK; diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 0954ca437ef..095fe6334ed 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -553,7 +553,7 @@ using namespace Microsoft::Console::Types; { _deferredCursorPos = { _lastText.X + sNumSpaces, _lastText.Y }; } - else + else if (numSpaces > 0) { std::wstring spaces = std::wstring(numSpaces, L' '); RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(spaces)); @@ -562,9 +562,12 @@ using namespace Microsoft::Console::Types; } } - // If we previously though that this was a new bottom line, it certainly - // isn't new any longer. - _newBottomLine = false; + // If we printed to the bottom line, and we previously thought that this was + // a new bottom line, it certainly isn't new any longer. + if (coord.Y == _lastViewport.BottomInclusive()) + { + _newBottomLine = false; + } return S_OK; } From 746a1c830b3b0cb5473020a5a846d4269750117e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 3 Apr 2020 10:04:31 -0500 Subject: [PATCH 27/44] Frick forgot to save --- src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 1ec372cd275..70954033258 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1180,7 +1180,6 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() verifyBuffer(hostTb, hostView.BottomInclusive() - 1); - // DebugBreak(); VERIFY_SUCCEEDED(renderer.PaintFrame()); verifyBuffer(termTb, term->_mutableViewport.BottomInclusive() - 1); From 9ea3960ac3cc80380afbee80e26702f30a121a6f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 3 Apr 2020 10:23:54 -0500 Subject: [PATCH 28/44] Spellcheck fixes, thanks to @jsoref --- .github/actions/spell-check/patterns/patterns.txt | 2 ++ src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 2 +- src/renderer/vt/XtermEngine.cpp | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/actions/spell-check/patterns/patterns.txt b/.github/actions/spell-check/patterns/patterns.txt index 157451cf94c..ded17943941 100644 --- a/.github/actions/spell-check/patterns/patterns.txt +++ b/.github/actions/spell-check/patterns/patterns.txt @@ -5,3 +5,5 @@ https://(?:(?:www\.|)youtube\.com|youtu.be)/[-a-zA-Z0-9?&=]* Scro\&ll # selectionInput.cpp :\\windows\\syste\b +TestUtils::VerifyExpectedString\(tb, L"[^"]+" +hostSm.ProcessString(L"[^"]+" diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 70954033258..8efea60523b 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1595,7 +1595,7 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() constexpr int PaintAfterAllNewlines = 1; constexpr int DontPaintAfterNewlines = 2; - INIT_TEST_PROPERTY(int, paintEachNewline, L"Ene of manually PaintFrame after each newline is emitted, once at the end of all newlines, or not at all"); + INIT_TEST_PROPERTY(int, paintEachNewline, L"Any of: manually PaintFrame after each newline is emitted, once at the end of all newlines, or not at all"); INIT_TEST_PROPERTY(bool, cursorOnNextLine, L"Either leave the cursor on the first line, or place it on the second line of the buffer"); INIT_TEST_PROPERTY(bool, paintAfterDECALN, L"Controls whether we manually paint a frame after the DECALN sequence is emitted."); INIT_TEST_PROPERTY(bool, changeAttributes, L"If true, change the text attributes after the 'A's and spaces"); diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 75f1051424a..e95047c6f98 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -389,7 +389,7 @@ try if (dy < 0) { - // Save the old wrap state here. We're gonig to clear it so that + // Save the old wrap state here. We're going to clear it so that // _MoveCursor will definitely move us to the right position. We'll // restore the state afterwards. const auto oldWrappedRow = _wrappedRow; From 12f9c827bb114097dd529d489e3d7364bc648161 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 3 Apr 2020 14:50:34 -0500 Subject: [PATCH 29/44] Add more comments --- .github/actions/spell-check/patterns/patterns.txt | 2 +- .../UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 5 +++-- src/renderer/vt/XtermEngine.cpp | 10 ++++++++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/.github/actions/spell-check/patterns/patterns.txt b/.github/actions/spell-check/patterns/patterns.txt index ded17943941..07d7d1881ba 100644 --- a/.github/actions/spell-check/patterns/patterns.txt +++ b/.github/actions/spell-check/patterns/patterns.txt @@ -6,4 +6,4 @@ Scro\&ll # selectionInput.cpp :\\windows\\syste\b TestUtils::VerifyExpectedString\(tb, L"[^"]+" -hostSm.ProcessString(L"[^"]+" +hostSm\.ProcessString\(L"[^"]+" diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 8efea60523b..49d518e8a6a 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1738,7 +1738,9 @@ void ConptyRoundtripTests::ClearHostTrickeryTest() void ConptyRoundtripTests::OverstrikeAtBottomOfBuffer() { // See https://github.com/microsoft/terminal/pull/5181#issuecomment-607545241 - Log::Comment(L"TODO: description"); + Log::Comment(L"This test replicates the zsh menu-complete functionality. In" + L" the course of a single frame, we're going to both scroll " + L"the frame and print multiple lines of text above the bottom line."); VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); auto& g = ServiceLocator::LocateGlobals(); @@ -1803,6 +1805,5 @@ void ConptyRoundtripTests::OverstrikeAtBottomOfBuffer() VERIFY_SUCCEEDED(renderer.PaintFrame()); Log::Comment(L"========== Checking the terminal buffer state =========="); - // DebugBreak(); verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); } diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index e95047c6f98..b0687235b72 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -438,6 +438,16 @@ try if (_delayedEolWrap && _wrappedRow.has_value()) { + // If we wrapped the last line, and we're in the middle of painting it, + // then the newline we did above just manually broke the line. What + // we're doing here is a hack: we're going to manually re-invalidate the + // last character of the wrapped row. When the PaintBufferLine calls + // come back through, we'll paint this last character again, causing us + // to get into the wrapped state once again. This is the only way to + // ensure that if a line was wrapped, and we painted the first line in + // one frame, and the second line in another frame that included other + // changes _above_ the wrapped line, that we maintain the wrap state in + // the Terminal. const til::rectangle lastCellOfWrappedRow{ til::point{ _lastViewport.RightInclusive(), _wrappedRow.value() }, til::size{ 1, 1 } From 3495cad437600296d4c8733670c8d4ad756ad29a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 3 Apr 2020 14:52:05 -0500 Subject: [PATCH 30/44] Starting on working on fixing this bug, but I can't get a minimal repro, so I'm stashing this. re: #5161 --- .../ConptyRoundtripTests.cpp | 75 +++++++++++++++++++ src/renderer/vt/paint.cpp | 2 + 2 files changed, 77 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 49d518e8a6a..97ce5df98bb 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -185,6 +185,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(DontWrapMoveCursorInSingleFrame); TEST_METHOD(ClearHostTrickeryTest); TEST_METHOD(OverstrikeAtBottomOfBuffer); + TEST_METHOD(MarginsWithStatusLine); TEST_METHOD(ScrollWithMargins); @@ -1805,5 +1806,79 @@ void ConptyRoundtripTests::OverstrikeAtBottomOfBuffer() VERIFY_SUCCEEDED(renderer.PaintFrame()); Log::Comment(L"========== Checking the terminal buffer state =========="); + + verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); +} + +void ConptyRoundtripTests::MarginsWithStatusLine() +{ + // See https://github.com/microsoft/terminal/issues/5161 + Log::Comment(L"TODO: description"); + 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(); + + auto verifyBuffer = [](const TextBuffer& tb, + const til::rectangle viewport) { + const auto lastRow = viewport.bottom() - 1; + const til::point expectedCursor{ 0, lastRow - 1 }; + VERIFY_ARE_EQUAL(expectedCursor, til::point{ tb.GetCursor().GetPosition() }); + VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); + + TestUtils::VerifyExpectedString(tb, L"AAAAAAAAAA DDDDDDDDDD", til::point{ 0, lastRow - 2 }); + TestUtils::VerifyExpectedString(tb, L"BBBBBBBBBB", til::point{ 0, lastRow - 1 }); + TestUtils::VerifyExpectedString(tb, L"FFFFFFFFFE", til::point{ 0, lastRow }); + }; + + _logConpty = true; + // We're _not_ checking the conpty output during this test, only the side effects. + _checkConptyOutput = false; + + hostSm.ProcessString(L"\x1b#8"); + + hostSm.ProcessString(L"\x1b[1;30r"); + hostSm.ProcessString(L"\x1b[30;1H"); + + hostSm.ProcessString(L"\x1b[J"); + hostSm.ProcessString(L"AAAAAAAAAA"); + hostSm.ProcessString(L"\x1b[K"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"BBBBBBBBBB"); + hostSm.ProcessString(L"\x1b[K"); + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"CCCCCCCCCC"); + hostSm.ProcessString(L"\x1b[2A"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"\x1b[20C"); + hostSm.ProcessString(L"DDDDDDDDDD"); + hostSm.ProcessString(L"\x1b[K"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"\x1b[1B"); + hostSm.ProcessString(L"EEEEEEEEEE"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"FFFFFFFFF"); + hostSm.ProcessString(L"\r"); + hostSm.ProcessString(L"\x1b[A"); + hostSm.ProcessString(L"\x1b[A"); + hostSm.ProcessString(L"\n"); + + Log::Comment(L"========== Checking the host buffer state =========="); + verifyBuffer(hostTb, si.GetViewport().ToInclusive()); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Checking the terminal buffer state =========="); + verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); } diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 095fe6334ed..82269130469 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -447,6 +447,8 @@ using namespace Microsoft::Console::Types; // If we're not using erase char, but we did erase all at the start of the // frame, don't add spaces at the end. const bool removeSpaces = (useEraseChar || (_clearedAllThisFrame) || (_newBottomLine)); + // This fixes the bug, but why? + // const bool removeSpaces = (useEraseChar || (_clearedAllThisFrame) || (_newBottomLine && coord.Y == _lastViewport.BottomInclusive())); const size_t cchActual = removeSpaces ? (cchLine - numSpaces) : cchLine; From c0fb26f31a494e8c8c40b721f76a65027128bfd7 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 3 Apr 2020 16:16:47 -0500 Subject: [PATCH 31/44] This scratch application repros a similar bug, so I'm treating this as the minimal repro --- src/tools/scratch/main.cpp | 123 +++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/src/tools/scratch/main.cpp b/src/tools/scratch/main.cpp index c6f9cc901a2..c53225407ca 100644 --- a/src/tools/scratch/main.cpp +++ b/src/tools/scratch/main.cpp @@ -2,9 +2,132 @@ // Licensed under the MIT license. #include +#include +#include +#include +#include +#include +#include /* srand, rand */ +#include /* time */ + +#include +#include +#include +#include +#include +#include // This wmain exists for help in writing scratch programs while debugging. int __cdecl wmain(int /*argc*/, WCHAR* /*argv[]*/) { + auto hOut = GetStdHandle(STD_OUTPUT_HANDLE); + + { + DWORD dwMode = 0; + THROW_LAST_ERROR_IF(!GetConsoleMode(hOut, &dwMode)); + const auto originalMode = dwMode; + dwMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING; + // dwMode |= DISABLE_NEWLINE_AUTO_RETURN; + THROW_LAST_ERROR_IF(!SetConsoleMode(hOut, dwMode)); + wprintf(L"\x1b[2J"); + wprintf(L"\x1b[H"); + THROW_LAST_ERROR_IF(!SetConsoleMode(hOut, originalMode)); + } + + CONSOLE_SCREEN_BUFFER_INFOEX csbiex = { 0 }; + csbiex.cbSize = sizeof(CONSOLE_SCREEN_BUFFER_INFOEX); + bool fSuccess = !!GetConsoleScreenBufferInfoEx(hOut, &csbiex); + if (!fSuccess) + return -1; + + SMALL_RECT srViewport = csbiex.srWindow; + + const unsigned short width = srViewport.Right - srViewport.Left + 1; + const unsigned short height = srViewport.Bottom - srViewport.Top + 1; + + // CONSOLE_CURSOR_INFO cursorInfo{}; + // fSuccess = GetConsoleCursorInfo(hOut, &cursorInfo); + // if (!fSuccess) return -1; + + auto restoreCursor = wil::scope_exit([&]() { + const COORD adjusted{ 0, csbiex.dwCursorPosition.Y + 1 }; + SetConsoleCursorPosition(hOut, adjusted); + }); + + const auto originalBottom = srViewport.Bottom; + + // { + + // COORD nearBottom{ 0, height - 3 }; + // SetConsoleCursorPosition(hOut, nearBottom); + // wprintf(L"AAAAAAAAAAAAAAAAAAAA"); + // wprintf(L"\n"); + // wprintf(L"BBBBBBBBBBBBBBBBBBBB"); + // wprintf(L"\n"); + // wprintf(L"CCCCCCCCCCCCCCCCCCCC"); + // Sleep(1000); + + // CHAR_INFO clear; + // clear.Char.UnicodeChar = L' '; + // clear.Attributes = csbiex.wAttributes; + + // SMALL_RECT src; + // src.Top = 1; + // src.Left = 0; + // src.Right = width; + // src.Bottom = height - 3; + // COORD tgt = { 0, 0 }; + // ScrollConsoleScreenBuffer(hOut, &src, nullptr, tgt, &clear); + + // // Sleep(1000); + + // COORD statusLine{ 0, height - 2 }; + // SetConsoleCursorPosition(hOut, statusLine); + + // wprintf(L"D---"); + // wprintf(L"\n"); + // wprintf(L"E---"); + + // Sleep(1000); + + // } + + { + COORD nearBottom{ 0, originalBottom - 2 }; + SetConsoleCursorPosition(hOut, nearBottom); + wprintf(L"AAAAAAAAAAAAAAAAAAAA"); + wprintf(L"\n"); + wprintf(L"BBBBBBBBBBBBBBBBBBBB"); + wprintf(L"\n"); + wprintf(L"CCCCCCCCCCCCCCCCCCCC"); + Sleep(1000); + // COORD atBottom{ 0, height }; + // SetConsoleCursorPosition(hOut, nearBottom); + wprintf(L"\n"); + const short newBottom = originalBottom + 1; + Sleep(1000); + + CHAR_INFO clear; + clear.Char.UnicodeChar = L' '; + clear.Attributes = csbiex.wAttributes; + + SMALL_RECT src; + src.Top = originalBottom - 1; + src.Left = 0; + src.Right = width; + src.Bottom = originalBottom; + COORD tgt = { 0, newBottom - 1 }; + ScrollConsoleScreenBuffer(hOut, &src, nullptr, tgt, &clear); + + Sleep(1000); + COORD statusLine{ 0, newBottom - 1 }; + SetConsoleCursorPosition(hOut, statusLine); + + wprintf(L"D---"); + wprintf(L"\n"); + wprintf(L"E---"); + Sleep(1000); + } + return 0; } From 812dc86b9dc806828ee5c5f1162547e2038dda27 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 3 Apr 2020 16:56:09 -0500 Subject: [PATCH 32/44] wait this test does work for the host --- .../ConptyRoundtripTests.cpp | 92 ++++++++++++++----- src/tools/scratch/main.cpp | 21 +++-- 2 files changed, 84 insertions(+), 29 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 97ce5df98bb..b9751c0a49f 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -23,6 +23,7 @@ class InputBuffer; // This for some reason needs to be fwd-decl'd #include "../host/inputBuffer.hpp" #include "../host/readDataCooked.hpp" +#include "../host/output.h" #include "test/CommonState.hpp" #include "../cascadia/TerminalCore/Terminal.hpp" @@ -1829,13 +1830,15 @@ void ConptyRoundtripTests::MarginsWithStatusLine() auto verifyBuffer = [](const TextBuffer& tb, const til::rectangle viewport) { const auto lastRow = viewport.bottom() - 1; - const til::point expectedCursor{ 0, lastRow - 1 }; + const til::point expectedCursor{ 1, lastRow }; VERIFY_ARE_EQUAL(expectedCursor, til::point{ tb.GetCursor().GetPosition() }); VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); - TestUtils::VerifyExpectedString(tb, L"AAAAAAAAAA DDDDDDDDDD", til::point{ 0, lastRow - 2 }); - TestUtils::VerifyExpectedString(tb, L"BBBBBBBBBB", til::point{ 0, lastRow - 1 }); - TestUtils::VerifyExpectedString(tb, L"FFFFFFFFFE", til::point{ 0, lastRow }); + TestUtils::VerifyExpectedString(tb, L"EEEEEEEEEE", til::point{ 0, lastRow - 4 }); + TestUtils::VerifyExpectedString(tb, L"AAAAAAAAAA", til::point{ 0, lastRow - 3 }); + TestUtils::VerifyExpectedString(tb, L" ", til::point{ 0, lastRow - 2 }); + TestUtils::VerifyExpectedString(tb, L"XBBBBBBBBB", til::point{ 0, lastRow - 1 }); + TestUtils::VerifyExpectedString(tb, L"YCCCCCCCCC", til::point{ 0, lastRow }); }; _logConpty = true; @@ -1844,33 +1847,76 @@ void ConptyRoundtripTests::MarginsWithStatusLine() hostSm.ProcessString(L"\x1b#8"); - hostSm.ProcessString(L"\x1b[1;30r"); + const short originalBottom = si.GetViewport().BottomInclusive(); hostSm.ProcessString(L"\x1b[30;1H"); - - hostSm.ProcessString(L"\x1b[J"); + // COORD nearBottom{ 0, originalBottom - 2 }; + // SetConsoleCursorPosition(hOut, nearBottom); hostSm.ProcessString(L"AAAAAAAAAA"); - hostSm.ProcessString(L"\x1b[K"); - hostSm.ProcessString(L"\r"); hostSm.ProcessString(L"\n"); hostSm.ProcessString(L"BBBBBBBBBB"); - hostSm.ProcessString(L"\x1b[K"); hostSm.ProcessString(L"\n"); hostSm.ProcessString(L"CCCCCCCCCC"); - hostSm.ProcessString(L"\x1b[2A"); - hostSm.ProcessString(L"\r"); - hostSm.ProcessString(L"\x1b[20C"); - hostSm.ProcessString(L"DDDDDDDDDD"); - hostSm.ProcessString(L"\x1b[K"); - hostSm.ProcessString(L"\r"); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + // Sleep(1000); + // COORD atBottom{ 0, height }; + // SetConsoleCursorPosition(hOut, nearBottom); hostSm.ProcessString(L"\n"); - hostSm.ProcessString(L"\x1b[1B"); - hostSm.ProcessString(L"EEEEEEEEEE"); - hostSm.ProcessString(L"\r"); - hostSm.ProcessString(L"FFFFFFFFF"); - hostSm.ProcessString(L"\r"); - hostSm.ProcessString(L"\x1b[A"); - hostSm.ProcessString(L"\x1b[A"); + const short newBottom = si.GetViewport().BottomInclusive(); + // Sleep(1000); + + // CHAR_INFO clear; + // clear.Char.UnicodeChar = L' '; + // clear.Attributes = csbiex.wAttributes; + DebugBreak(); + SMALL_RECT src; + src.Top = newBottom - 2; + src.Left = 0; + src.Right = si.GetViewport().Width(); + src.Bottom = originalBottom; + COORD tgt = { 0, newBottom - 1 }; + // ScrollConsoleScreenBuffer(hOut, &src, nullptr, tgt, &clear); + TextAttribute useThisAttr(0x07); // We don't terribly care about the attributes so this is arbitrary + ScrollRegion(si, src, std::nullopt, tgt, L' ', useThisAttr); + + // Sleep(1000); + // COORD statusLine{ 0, newBottom - 1 }; + // SetConsoleCursorPosition(hOut, statusLine); + hostSm.ProcessString(L"\x1b[31;1H"); + + hostSm.ProcessString(L"X"); hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"Y"); + + // hostSm.ProcessString(L"\x1b[1;30r"); + // hostSm.ProcessString(L"\x1b[30;1H"); + + // hostSm.ProcessString(L"\x1b[J"); + // hostSm.ProcessString(L"AAAAAAAAAA"); + // hostSm.ProcessString(L"\x1b[K"); + // hostSm.ProcessString(L"\r"); + // hostSm.ProcessString(L"\n"); + // hostSm.ProcessString(L"BBBBBBBBBB"); + // hostSm.ProcessString(L"\x1b[K"); + // hostSm.ProcessString(L"\n"); + // hostSm.ProcessString(L"CCCCCCCCCC"); + // hostSm.ProcessString(L"\x1b[2A"); + // hostSm.ProcessString(L"\r"); + // hostSm.ProcessString(L"\x1b[20C"); + // hostSm.ProcessString(L"DDDDDDDDDD"); + // hostSm.ProcessString(L"\x1b[K"); + // hostSm.ProcessString(L"\r"); + // hostSm.ProcessString(L"\n"); + // hostSm.ProcessString(L"\x1b[1B"); + // hostSm.ProcessString(L"EEEEEEEEEE"); + // hostSm.ProcessString(L"\r"); + // hostSm.ProcessString(L"FFFFFFFFF"); + // hostSm.ProcessString(L"\r"); + // hostSm.ProcessString(L"\x1b[A"); + // hostSm.ProcessString(L"\x1b[A"); + // hostSm.ProcessString(L"\n"); Log::Comment(L"========== Checking the host buffer state =========="); verifyBuffer(hostTb, si.GetViewport().ToInclusive()); diff --git a/src/tools/scratch/main.cpp b/src/tools/scratch/main.cpp index c53225407ca..751e46c5bac 100644 --- a/src/tools/scratch/main.cpp +++ b/src/tools/scratch/main.cpp @@ -50,7 +50,8 @@ int __cdecl wmain(int /*argc*/, WCHAR* /*argv[]*/) // if (!fSuccess) return -1; auto restoreCursor = wil::scope_exit([&]() { - const COORD adjusted{ 0, csbiex.dwCursorPosition.Y + 1 }; + // const COORD adjusted{ 0, csbiex.dwCursorPosition.Y + 1 }; + const COORD adjusted{ srViewport.Left, srViewport.Top }; SetConsoleCursorPosition(hOut, adjusted); }); @@ -104,29 +105,37 @@ int __cdecl wmain(int /*argc*/, WCHAR* /*argv[]*/) // COORD atBottom{ 0, height }; // SetConsoleCursorPosition(hOut, nearBottom); wprintf(L"\n"); - const short newBottom = originalBottom + 1; - Sleep(1000); + + fSuccess = !!GetConsoleScreenBufferInfoEx(hOut, &csbiex); + if (!fSuccess) + return -1; + + srViewport = csbiex.srWindow; + + // const short newBottom = originalBottom + 1; + const short newBottom = srViewport.Bottom; + // Sleep(1000); CHAR_INFO clear; clear.Char.UnicodeChar = L' '; clear.Attributes = csbiex.wAttributes; SMALL_RECT src; - src.Top = originalBottom - 1; + src.Top = newBottom - 2; src.Left = 0; src.Right = width; src.Bottom = originalBottom; COORD tgt = { 0, newBottom - 1 }; ScrollConsoleScreenBuffer(hOut, &src, nullptr, tgt, &clear); - Sleep(1000); + // Sleep(1000); COORD statusLine{ 0, newBottom - 1 }; SetConsoleCursorPosition(hOut, statusLine); wprintf(L"D---"); wprintf(L"\n"); wprintf(L"E---"); - Sleep(1000); + // Sleep(1000); } return 0; From 87a1486733ade3e908e804c140be5468138a3d16 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 6 Apr 2020 10:04:00 -0500 Subject: [PATCH 33/44] Add a real test for #5161 and fix the associated bug --- .../spell-check/whitelist/whitelist.txt | 1 + .../ConptyRoundtripTests.cpp | 88 +++++++------------ src/renderer/vt/paint.cpp | 11 ++- 3 files changed, 42 insertions(+), 58 deletions(-) diff --git a/.github/actions/spell-check/whitelist/whitelist.txt b/.github/actions/spell-check/whitelist/whitelist.txt index c73f7cf90f6..746caf63c7d 100644 --- a/.github/actions/spell-check/whitelist/whitelist.txt +++ b/.github/actions/spell-check/whitelist/whitelist.txt @@ -2895,6 +2895,7 @@ ZCtrl zd zh ZM +zsh zu zxcvbnm zy diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index b9751c0a49f..d024d0db722 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1814,7 +1814,14 @@ void ConptyRoundtripTests::OverstrikeAtBottomOfBuffer() void ConptyRoundtripTests::MarginsWithStatusLine() { // See https://github.com/microsoft/terminal/issues/5161 - Log::Comment(L"TODO: description"); + // + // This test reproduces a case from the MSYS/cygwin vim. From what I can + // tell, they implement scrolling by emitting a newline at the bototm of the + // buffer (to create a new blank line), then they use + // ScrollConsoleScreenBuffer to shift the status line(s) down a line, and + // then they repring the status line. + Log::Comment(L"Newline, and scroll the bottom lines of the buffer down with" + L" ScrollConsoleScreenBuffer to emulate how cygwin VIM works"); VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); auto& g = ServiceLocator::LocateGlobals(); @@ -1841,16 +1848,21 @@ void ConptyRoundtripTests::MarginsWithStatusLine() TestUtils::VerifyExpectedString(tb, L"YCCCCCCCCC", til::point{ 0, lastRow }); }; - _logConpty = true; // We're _not_ checking the conpty output during this test, only the side effects. _checkConptyOutput = false; + // Use DECALN to fill the buffer with 'E's. hostSm.ProcessString(L"\x1b#8"); const short originalBottom = si.GetViewport().BottomInclusive(); + // Print 3 lines into the bottom of the buffer: + // AAAAAAAAAA + // BBBBBBBBBB + // CCCCCCCCCC + // In this test, the 'B' and 'C' lines represent the status lines at the + // bottom of vim, and the 'A' line is a buffer line. hostSm.ProcessString(L"\x1b[30;1H"); - // COORD nearBottom{ 0, originalBottom - 2 }; - // SetConsoleCursorPosition(hOut, nearBottom); + hostSm.ProcessString(L"AAAAAAAAAA"); hostSm.ProcessString(L"\n"); hostSm.ProcessString(L"BBBBBBBBBB"); @@ -1860,64 +1872,32 @@ void ConptyRoundtripTests::MarginsWithStatusLine() Log::Comment(L"Painting the frame"); VERIFY_SUCCEEDED(renderer.PaintFrame()); - // Sleep(1000); - // COORD atBottom{ 0, height }; - // SetConsoleCursorPosition(hOut, nearBottom); + // After printing the 'C' line, the cursor is on the bottom line of the viewport. + // Emit a newline here to get a new line at the bottom of the viewport. hostSm.ProcessString(L"\n"); const short newBottom = si.GetViewport().BottomInclusive(); - // Sleep(1000); - - // CHAR_INFO clear; - // clear.Char.UnicodeChar = L' '; - // clear.Attributes = csbiex.wAttributes; - DebugBreak(); - SMALL_RECT src; - src.Top = newBottom - 2; - src.Left = 0; - src.Right = si.GetViewport().Width(); - src.Bottom = originalBottom; - COORD tgt = { 0, newBottom - 1 }; - // ScrollConsoleScreenBuffer(hOut, &src, nullptr, tgt, &clear); - TextAttribute useThisAttr(0x07); // We don't terribly care about the attributes so this is arbitrary - ScrollRegion(si, src, std::nullopt, tgt, L' ', useThisAttr); - - // Sleep(1000); - // COORD statusLine{ 0, newBottom - 1 }; - // SetConsoleCursorPosition(hOut, statusLine); + + { + // Emulate calling ScrollConsoleScreenBuffer to scroll the B and C lines + // down one line. + SMALL_RECT src; + src.Top = newBottom - 2; + src.Left = 0; + src.Right = si.GetViewport().Width(); + src.Bottom = originalBottom; + COORD tgt = { 0, newBottom - 1 }; + TextAttribute useThisAttr(0x07); // We don't terribly care about the attributes so this is arbitrary + ScrollRegion(si, src, std::nullopt, tgt, L' ', useThisAttr); + } + + // Move the cursor to the location of the B line hostSm.ProcessString(L"\x1b[31;1H"); + // Print an 'X' on the 'B' line, and a 'Y' on the 'C' line. hostSm.ProcessString(L"X"); hostSm.ProcessString(L"\n"); hostSm.ProcessString(L"Y"); - // hostSm.ProcessString(L"\x1b[1;30r"); - // hostSm.ProcessString(L"\x1b[30;1H"); - - // hostSm.ProcessString(L"\x1b[J"); - // hostSm.ProcessString(L"AAAAAAAAAA"); - // hostSm.ProcessString(L"\x1b[K"); - // hostSm.ProcessString(L"\r"); - // hostSm.ProcessString(L"\n"); - // hostSm.ProcessString(L"BBBBBBBBBB"); - // hostSm.ProcessString(L"\x1b[K"); - // hostSm.ProcessString(L"\n"); - // hostSm.ProcessString(L"CCCCCCCCCC"); - // hostSm.ProcessString(L"\x1b[2A"); - // hostSm.ProcessString(L"\r"); - // hostSm.ProcessString(L"\x1b[20C"); - // hostSm.ProcessString(L"DDDDDDDDDD"); - // hostSm.ProcessString(L"\x1b[K"); - // hostSm.ProcessString(L"\r"); - // hostSm.ProcessString(L"\n"); - // hostSm.ProcessString(L"\x1b[1B"); - // hostSm.ProcessString(L"EEEEEEEEEE"); - // hostSm.ProcessString(L"\r"); - // hostSm.ProcessString(L"FFFFFFFFF"); - // hostSm.ProcessString(L"\r"); - // hostSm.ProcessString(L"\x1b[A"); - // hostSm.ProcessString(L"\x1b[A"); - // hostSm.ProcessString(L"\n"); - Log::Comment(L"========== Checking the host buffer state =========="); verifyBuffer(hostTb, si.GetViewport().ToInclusive()); diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 82269130469..371d950c322 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -445,10 +445,13 @@ using namespace Microsoft::Console::Types; (!_clearedAllThisFrame); // If we're not using erase char, but we did erase all at the start of the - // frame, don't add spaces at the end. - const bool removeSpaces = (useEraseChar || (_clearedAllThisFrame) || (_newBottomLine)); - // This fixes the bug, but why? - // const bool removeSpaces = (useEraseChar || (_clearedAllThisFrame) || (_newBottomLine && coord.Y == _lastViewport.BottomInclusive())); + // frame, don't add spaces at the end. + // + // GH#5161: Only removeSpaces when we're in the _newBottomLine state and the + // line we're trying to print right now _actually is the bottom line_ + const bool removeSpaces = useEraseChar || + _clearedAllThisFrame || + (_newBottomLine && coord.Y == _lastViewport.BottomInclusive()); const size_t cchActual = removeSpaces ? (cchLine - numSpaces) : cchLine; From c7fd127ae2b2eb56d903fb50dc10707abdb35e7e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 6 Apr 2020 11:07:58 -0500 Subject: [PATCH 34/44] Fix the Xterm*Invalidate tests --- src/renderer/vt/XtermEngine.cpp | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index b0687235b72..dba15950b46 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -387,16 +387,16 @@ try const short dy = _scrollDelta.y(); const short absDy = static_cast(abs(dy)); + // Save the old wrap state here. We're going to clear it so that + // _MoveCursor will definitely move us to the right position. We'll + // restore the state afterwards. + const auto oldWrappedRow = _wrappedRow; + const auto oldDelayedEolWrap = _delayedEolWrap; + _delayedEolWrap = false; + _wrappedRow = std::nullopt; + if (dy < 0) { - // Save the old wrap state here. We're going to clear it so that - // _MoveCursor will definitely move us to the right position. We'll - // restore the state afterwards. - const auto oldWrappedRow = _wrappedRow; - const auto oldDelayedEolWrap = _delayedEolWrap; - _delayedEolWrap = false; - _wrappedRow = std::nullopt; - // TODO GH#5228 - We could optimize this by only doing this newline work // when there's more invalid than just the bottom line. If only the // bottom line is invalid, then the next thing the Renderer is going to @@ -411,11 +411,18 @@ try RETURN_IF_FAILED(_MoveCursor({ 0, bottom })); // Emit some number of newlines to create space in the buffer. RETURN_IF_FAILED(_Write(std::string(absDy, '\n'))); - - // Restore our wrap state. - _wrappedRow = oldWrappedRow; - _delayedEolWrap = oldDelayedEolWrap; } + else if (dy > 0) + { + // If we've scrolled _down_, then move the cursor to the top of the + // buffer, and insert some newlines using the InsertLines VT sequence + RETURN_IF_FAILED(_MoveCursor({ 0, 0 })); + RETURN_IF_FAILED(_InsertLine(absDy)); + } + + // Restore our wrap state. + _wrappedRow = oldWrappedRow; + _delayedEolWrap = oldDelayedEolWrap; // Shift our internal tracker of the last text position according to how // much we've scrolled. If we manually scroll the buffer right now, by From 55f81bc185b6985dcc268f2b1b2dcad9e3753db3 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 6 Apr 2020 11:08:35 -0500 Subject: [PATCH 35/44] I really wish I could run this bot locally on save or _before_ pushing --- .github/actions/spell-check/whitelist/alphabet.txt | 3 +++ src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/actions/spell-check/whitelist/alphabet.txt b/.github/actions/spell-check/whitelist/alphabet.txt index df9afee43af..4e1d82e14b1 100644 --- a/.github/actions/spell-check/whitelist/alphabet.txt +++ b/.github/actions/spell-check/whitelist/alphabet.txt @@ -1,3 +1,4 @@ +AAAAAAAAAA abcd abcde abcdef @@ -8,6 +9,8 @@ abcdefghijk abcdefghijklmnop ABCDEFGHIJKLMNOPQRST abcdefghijklmnopqrstuvwxyz +BBBBBBBBBB +CCCCCCCCCC QQQQQ QQQQQQQQQ QQQQQQQQQQ diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index d024d0db722..b66ede742d3 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1816,10 +1816,10 @@ void ConptyRoundtripTests::MarginsWithStatusLine() // See https://github.com/microsoft/terminal/issues/5161 // // This test reproduces a case from the MSYS/cygwin vim. From what I can - // tell, they implement scrolling by emitting a newline at the bototm of the + // tell, they implement scrolling by emitting a newline at the bottom of the // buffer (to create a new blank line), then they use // ScrollConsoleScreenBuffer to shift the status line(s) down a line, and - // then they repring the status line. + // then they re-printing the status line. Log::Comment(L"Newline, and scroll the bottom lines of the buffer down with" L" ScrollConsoleScreenBuffer to emulate how cygwin VIM works"); VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); From 3137a3bd3bc985e8a4580840cd53faa29b6c5747 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 6 Apr 2020 12:37:50 -0500 Subject: [PATCH 36/44] Revert scratch.exe --- .../spell-check/whitelist/alphabet.txt | 1 + src/tools/scratch/main.cpp | 132 ------------------ 2 files changed, 1 insertion(+), 132 deletions(-) diff --git a/.github/actions/spell-check/whitelist/alphabet.txt b/.github/actions/spell-check/whitelist/alphabet.txt index 4e1d82e14b1..2dd829c826f 100644 --- a/.github/actions/spell-check/whitelist/alphabet.txt +++ b/.github/actions/spell-check/whitelist/alphabet.txt @@ -9,6 +9,7 @@ abcdefghijk abcdefghijklmnop ABCDEFGHIJKLMNOPQRST abcdefghijklmnopqrstuvwxyz +BBBBB BBBBBBBBBB CCCCCCCCCC QQQQQ diff --git a/src/tools/scratch/main.cpp b/src/tools/scratch/main.cpp index 751e46c5bac..c6f9cc901a2 100644 --- a/src/tools/scratch/main.cpp +++ b/src/tools/scratch/main.cpp @@ -2,141 +2,9 @@ // Licensed under the MIT license. #include -#include -#include -#include -#include -#include -#include /* srand, rand */ -#include /* time */ - -#include -#include -#include -#include -#include -#include // This wmain exists for help in writing scratch programs while debugging. int __cdecl wmain(int /*argc*/, WCHAR* /*argv[]*/) { - auto hOut = GetStdHandle(STD_OUTPUT_HANDLE); - - { - DWORD dwMode = 0; - THROW_LAST_ERROR_IF(!GetConsoleMode(hOut, &dwMode)); - const auto originalMode = dwMode; - dwMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING; - // dwMode |= DISABLE_NEWLINE_AUTO_RETURN; - THROW_LAST_ERROR_IF(!SetConsoleMode(hOut, dwMode)); - wprintf(L"\x1b[2J"); - wprintf(L"\x1b[H"); - THROW_LAST_ERROR_IF(!SetConsoleMode(hOut, originalMode)); - } - - CONSOLE_SCREEN_BUFFER_INFOEX csbiex = { 0 }; - csbiex.cbSize = sizeof(CONSOLE_SCREEN_BUFFER_INFOEX); - bool fSuccess = !!GetConsoleScreenBufferInfoEx(hOut, &csbiex); - if (!fSuccess) - return -1; - - SMALL_RECT srViewport = csbiex.srWindow; - - const unsigned short width = srViewport.Right - srViewport.Left + 1; - const unsigned short height = srViewport.Bottom - srViewport.Top + 1; - - // CONSOLE_CURSOR_INFO cursorInfo{}; - // fSuccess = GetConsoleCursorInfo(hOut, &cursorInfo); - // if (!fSuccess) return -1; - - auto restoreCursor = wil::scope_exit([&]() { - // const COORD adjusted{ 0, csbiex.dwCursorPosition.Y + 1 }; - const COORD adjusted{ srViewport.Left, srViewport.Top }; - SetConsoleCursorPosition(hOut, adjusted); - }); - - const auto originalBottom = srViewport.Bottom; - - // { - - // COORD nearBottom{ 0, height - 3 }; - // SetConsoleCursorPosition(hOut, nearBottom); - // wprintf(L"AAAAAAAAAAAAAAAAAAAA"); - // wprintf(L"\n"); - // wprintf(L"BBBBBBBBBBBBBBBBBBBB"); - // wprintf(L"\n"); - // wprintf(L"CCCCCCCCCCCCCCCCCCCC"); - // Sleep(1000); - - // CHAR_INFO clear; - // clear.Char.UnicodeChar = L' '; - // clear.Attributes = csbiex.wAttributes; - - // SMALL_RECT src; - // src.Top = 1; - // src.Left = 0; - // src.Right = width; - // src.Bottom = height - 3; - // COORD tgt = { 0, 0 }; - // ScrollConsoleScreenBuffer(hOut, &src, nullptr, tgt, &clear); - - // // Sleep(1000); - - // COORD statusLine{ 0, height - 2 }; - // SetConsoleCursorPosition(hOut, statusLine); - - // wprintf(L"D---"); - // wprintf(L"\n"); - // wprintf(L"E---"); - - // Sleep(1000); - - // } - - { - COORD nearBottom{ 0, originalBottom - 2 }; - SetConsoleCursorPosition(hOut, nearBottom); - wprintf(L"AAAAAAAAAAAAAAAAAAAA"); - wprintf(L"\n"); - wprintf(L"BBBBBBBBBBBBBBBBBBBB"); - wprintf(L"\n"); - wprintf(L"CCCCCCCCCCCCCCCCCCCC"); - Sleep(1000); - // COORD atBottom{ 0, height }; - // SetConsoleCursorPosition(hOut, nearBottom); - wprintf(L"\n"); - - fSuccess = !!GetConsoleScreenBufferInfoEx(hOut, &csbiex); - if (!fSuccess) - return -1; - - srViewport = csbiex.srWindow; - - // const short newBottom = originalBottom + 1; - const short newBottom = srViewport.Bottom; - // Sleep(1000); - - CHAR_INFO clear; - clear.Char.UnicodeChar = L' '; - clear.Attributes = csbiex.wAttributes; - - SMALL_RECT src; - src.Top = newBottom - 2; - src.Left = 0; - src.Right = width; - src.Bottom = originalBottom; - COORD tgt = { 0, newBottom - 1 }; - ScrollConsoleScreenBuffer(hOut, &src, nullptr, tgt, &clear); - - // Sleep(1000); - COORD statusLine{ 0, newBottom - 1 }; - SetConsoleCursorPosition(hOut, statusLine); - - wprintf(L"D---"); - wprintf(L"\n"); - wprintf(L"E---"); - // Sleep(1000); - } - return 0; } From b1886297e4f7ec4ec8ac093c6c3593d6749f1fb5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 6 Apr 2020 16:04:32 -0500 Subject: [PATCH 37/44] Some nits from Dustin --- .../UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index b66ede742d3..9ab0e9ce7d4 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1508,7 +1508,7 @@ void ConptyRoundtripTests::ScrollWithMargins() } { const std::string expectedString(initialTermView.Width() - 1, '*'); - // There will be one extra blank space at the end of the line, because the + // There will be one extra blank space at the end of the line, to prevent delayed EOL wrapping expectedOutput.push_back(expectedString + " "); } { @@ -1815,9 +1815,9 @@ void ConptyRoundtripTests::MarginsWithStatusLine() { // See https://github.com/microsoft/terminal/issues/5161 // - // This test reproduces a case from the MSYS/cygwin vim. From what I can - // tell, they implement scrolling by emitting a newline at the bottom of the - // buffer (to create a new blank line), then they use + // This test reproduces a case from the MSYS/cygwin (runtime < 3.1) vim. + // From what I can tell, they implement scrolling by emitting a newline at + // the bottom of the buffer (to create a new blank line), then they use // ScrollConsoleScreenBuffer to shift the status line(s) down a line, and // then they re-printing the status line. Log::Comment(L"Newline, and scroll the bottom lines of the buffer down with" From 6f9bd9a84f372baeb8e14ee0f4e921594f989f1a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Apr 2020 10:07:56 -0500 Subject: [PATCH 38/44] test commit please ignore --- .github/actions/spell-check/patterns/patterns.txt | 1 + .github/actions/spell-check/whitelist/alphabet.txt | 12 ------------ 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/.github/actions/spell-check/patterns/patterns.txt b/.github/actions/spell-check/patterns/patterns.txt index 07d7d1881ba..f0c899e49de 100644 --- a/.github/actions/spell-check/patterns/patterns.txt +++ b/.github/actions/spell-check/patterns/patterns.txt @@ -7,3 +7,4 @@ Scro\&ll :\\windows\\syste\b TestUtils::VerifyExpectedString\(tb, L"[^"]+" hostSm\.ProcessString\(L"[^"]+" +([A-Za-z])\1{3,} diff --git a/.github/actions/spell-check/whitelist/alphabet.txt b/.github/actions/spell-check/whitelist/alphabet.txt index 2dd829c826f..2590c9142f5 100644 --- a/.github/actions/spell-check/whitelist/alphabet.txt +++ b/.github/actions/spell-check/whitelist/alphabet.txt @@ -1,4 +1,3 @@ -AAAAAAAAAA abcd abcde abcdef @@ -9,12 +8,6 @@ abcdefghijk abcdefghijklmnop ABCDEFGHIJKLMNOPQRST abcdefghijklmnopqrstuvwxyz -BBBBB -BBBBBBBBBB -CCCCCCCCCC -QQQQQ -QQQQQQQQQ -QQQQQQQQQQ QQQQQQQQQQABCDEFGHIJ QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQ QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQQ @@ -23,10 +16,6 @@ qrstuvwxyz qwerty QWERTYUIOP qwertyuiopasdfg -TTTTTTTTTTTTTTTTTTTTTTTTTT -VVVVVVVVVVVVVVVV -XXXXX -yyyy ZAAZZ ZABBZ ZBAZZ @@ -36,4 +25,3 @@ ZYXWVUT ZZBBZ ZZZBB ZZZBZ -ZZZZZ From 1cc103571568bfd5c641c3e101061c8dad048738 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Apr 2020 10:23:43 -0500 Subject: [PATCH 39/44] some PR feedback --- .github/actions/spell-check/whitelist/alphabet.txt | 6 ++---- .../UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 8 +++----- src/host/ut_host/ConptyOutputTests.cpp | 3 +++ src/inc/consoletaeftemplates.hpp | 5 +++++ 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/.github/actions/spell-check/whitelist/alphabet.txt b/.github/actions/spell-check/whitelist/alphabet.txt index 2590c9142f5..fb5811c4f43 100644 --- a/.github/actions/spell-check/whitelist/alphabet.txt +++ b/.github/actions/spell-check/whitelist/alphabet.txt @@ -7,11 +7,9 @@ ABCDEFGHIJ abcdefghijk abcdefghijklmnop ABCDEFGHIJKLMNOPQRST +ABCDEFGHIJKLMNOPQRSTQ abcdefghijklmnopqrstuvwxyz -QQQQQQQQQQABCDEFGHIJ -QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQ -QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQQ -QQQQQQQQQQABCDEFGHIJPQRSTQQQQQQQQQQ +ABCDEFGHIJPQRSTQ qrstuvwxyz qwerty QWERTYUIOP diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 9ab0e9ce7d4..f2609858b63 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -49,13 +49,11 @@ namespace TerminalCoreUnitTests }; using namespace TerminalCoreUnitTests; -// Helper for declaring a variable to store a TEST_METHOD_PROPERTY and get it's value from the test metadata -#define INIT_TEST_PROPERTY(type, identifer, description) \ - type identifer; \ - VERIFY_SUCCEEDED(TestData::TryGetValue(L#identifer, identifer), description); - class TerminalCoreUnitTests::ConptyRoundtripTests final { + // !!! DANGER: Many tests in this class expect the Terminal and Host buffers + // to be 80x32. If you change these, you'll probably inadvertently break a + // bunch of tests !!! static const SHORT TerminalViewWidth = 80; static const SHORT TerminalViewHeight = 32; diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp index 20b5572d116..6b438ab9d07 100644 --- a/src/host/ut_host/ConptyOutputTests.cpp +++ b/src/host/ut_host/ConptyOutputTests.cpp @@ -27,6 +27,9 @@ using namespace Microsoft::Console::Types; class ConptyOutputTests { + // !!! DANGER: Many tests in this class expect the Terminal and Host buffers + // to be 80x32. If you change these, you'll probably inadvertently break a + // bunch of tests !!! static const SHORT TerminalViewWidth = 80; static const SHORT TerminalViewHeight = 32; diff --git a/src/inc/consoletaeftemplates.hpp b/src/inc/consoletaeftemplates.hpp index aa169ad4e7a..592b9d09b7d 100644 --- a/src/inc/consoletaeftemplates.hpp +++ b/src/inc/consoletaeftemplates.hpp @@ -17,6 +17,11 @@ Revision History: #pragma once +// Helper for declaring a variable to store a TEST_METHOD_PROPERTY and get it's value from the test metadata +#define INIT_TEST_PROPERTY(type, identifer, description) \ + type identifer; \ + VERIFY_SUCCEEDED(TestData::TryGetValue(L#identifer, identifer), description); + namespace WEX::TestExecution { template<> From c1119618e672f8e764bc023500cd8473ad730c5c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Apr 2020 10:48:04 -0500 Subject: [PATCH 40/44] I guess I need this one too --- .github/actions/spell-check/whitelist/alphabet.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spell-check/whitelist/alphabet.txt b/.github/actions/spell-check/whitelist/alphabet.txt index fb5811c4f43..beda2721886 100644 --- a/.github/actions/spell-check/whitelist/alphabet.txt +++ b/.github/actions/spell-check/whitelist/alphabet.txt @@ -9,6 +9,7 @@ abcdefghijklmnop ABCDEFGHIJKLMNOPQRST ABCDEFGHIJKLMNOPQRSTQ abcdefghijklmnopqrstuvwxyz +ABCDEFGHIJPQRST ABCDEFGHIJPQRSTQ qrstuvwxyz qwerty From 15a47d37e3b53ac70355f07213106f4bc920fd3f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Apr 2020 11:03:56 -0500 Subject: [PATCH 41/44] Add a test to cover the new case Dustin found --- .../ConptyRoundtripTests.cpp | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index f2609858b63..f25c5f3fc91 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -185,6 +185,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(ClearHostTrickeryTest); TEST_METHOD(OverstrikeAtBottomOfBuffer); TEST_METHOD(MarginsWithStatusLine); + TEST_METHOD(OutputWrappedLineWithSpace); TEST_METHOD(ScrollWithMargins); @@ -1906,3 +1907,56 @@ void ConptyRoundtripTests::MarginsWithStatusLine() verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); } + +void ConptyRoundtripTests::OutputWrappedLineWithSpace() +{ + // See https://github.com/microsoft/terminal/pull/5181#issuecomment-610110348 + Log::Comment(L"Ensures that a buffer line in conhost that wrapped _on a " + L"space_ will still be emitted as wrapped."); + VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& sm = si.GetStateMachine(); + auto& hostTb = si.GetTextBuffer(); + auto& termTb = *term->_buffer; + + _flushFirstFrame(); + + const auto firstTextLength = TerminalViewWidth - 2; + const auto spacesLength = 3; + const auto secondTextLength = 1; + + sm.ProcessString(std::wstring(firstTextLength, L'A')); + sm.ProcessString(std::wstring(spacesLength, L' ')); + sm.ProcessString(std::wstring(secondTextLength, L'B')); + + auto verifyBuffer = [&](const TextBuffer& tb) { + VERIFY_IS_TRUE(tb.GetRowByOffset(0).GetCharRow().WasWrapForced()); + VERIFY_IS_FALSE(tb.GetRowByOffset(1).GetCharRow().WasWrapForced()); + + auto iter0 = tb.GetCellDataAt({ 0, 0 }); + TestUtils::VerifySpanOfText(L"A", iter0, 0, firstTextLength); + TestUtils::VerifySpanOfText(L" ", iter0, 0, 2); + auto iter1 = tb.GetCellDataAt({ 0, 1 }); + TestUtils::VerifySpanOfText(L" ", iter1, 0, 1); + auto iter2 = tb.GetCellDataAt({ 1, 1 }); + TestUtils::VerifySpanOfText(L"B", iter2, 0, secondTextLength); + }; + + Log::Comment(L"========== Checking the host buffer state =========="); + verifyBuffer(hostTb); + + // TODO: Actually check the conpty output here. + _checkConptyOutput = false; + _logConpty = true; + // expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); + // expectedOutput.push_back(std::string(20, 'A')); + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Checking the terminal buffer state =========="); + verifyBuffer(termTb); +} From 5e167cde77ece24ef7d69de5700c74f27ca5fd45 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Apr 2020 12:51:39 -0500 Subject: [PATCH 42/44] Fix the bug that dustin found. 0% change spellcheck bot loves me --- .../actions/spell-check/patterns/patterns.txt | 2 +- .../ConptyRoundtripTests.cpp | 236 +++++++++++++++++- src/renderer/vt/paint.cpp | 11 +- 3 files changed, 236 insertions(+), 13 deletions(-) diff --git a/.github/actions/spell-check/patterns/patterns.txt b/.github/actions/spell-check/patterns/patterns.txt index f0c899e49de..11068abd8e4 100644 --- a/.github/actions/spell-check/patterns/patterns.txt +++ b/.github/actions/spell-check/patterns/patterns.txt @@ -7,4 +7,4 @@ Scro\&ll :\\windows\\syste\b TestUtils::VerifyExpectedString\(tb, L"[^"]+" hostSm\.ProcessString\(L"[^"]+" -([A-Za-z])\1{3,} +\b([A-Za-z])\1{3,}\b diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index f25c5f3fc91..e85fd243ddd 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -186,6 +186,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(OverstrikeAtBottomOfBuffer); TEST_METHOD(MarginsWithStatusLine); TEST_METHOD(OutputWrappedLineWithSpace); + TEST_METHOD(OutputWrappedLineWithSpaceAtBottomOfBuffer); TEST_METHOD(ScrollWithMargins); @@ -1094,6 +1095,15 @@ void ConptyRoundtripTests::OutputWrappedLinesAtTopOfBuffer() sm.ProcessString(std::wstring(wrappedLineLength, L'A')); auto verifyBuffer = [](const TextBuffer& tb) { + // Buffer contents should look like the following: (80 wide) + // (w) means we hard wrapped the line + // (b) means the line is _not_ wrapped (it's broken, the default state.) + // cursor is on the '_' + // + // |AAAAAAAA...AAAA| (w) + // |AAAAA_ ... | (b) (There are 20 'A's on this line.) + // | ... | (b) + VERIFY_IS_TRUE(tb.GetRowByOffset(0).GetCharRow().WasWrapForced()); VERIFY_IS_FALSE(tb.GetRowByOffset(1).GetCharRow().WasWrapForced()); auto iter0 = tb.GetCellDataAt({ 0, 0 }); @@ -1157,17 +1167,75 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer() const auto wrappedLineLength = TerminalViewWidth + 20; + // The following diagrams show the buffer contents after each string emitted + // from conpty. For each of these diagrams: + // (w) means we hard wrapped the line + // (b) means the line is _not_ wrapped (it's broken, the default state.) + // cursor is on the '_' + + // Initial state: + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |_ | (b) + expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AAAA|_ (w) The cursor is actually on the last A here + // TODO GH#5228 might break the "newline & repaint the wrapped char" checks here, that's okay. expectedOutput.push_back("\r"); // This \r\n is emitted by ScrollFrame to expectedOutput.push_back("\n"); // add a newline to the bottom of the buffer + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AAAA| (b) + // |_ | (b) + expectedOutput.push_back("\x1b[31;80H"); // Move the cursor BACK to the wrapped row + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AAAA| (b) The cursor is actually on the last A here + // | | (b) + expectedOutput.push_back(std::string(1, 'A')); // Reprint the last character of the wrapped row + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AAAA|_ (w) The cursor is actually on the last A here + // | | (b) + expectedOutput.push_back(std::string(20, 'A')); // Print the second line. + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AAAA| (w) + // |AAAAA_ | (b) There are 20 'A's on this line. hostSm.ProcessString(std::wstring(wrappedLineLength, L'A')); auto verifyBuffer = [](const TextBuffer& tb, const short wrappedRow) { + // Buffer contents should look like the following: (80 wide) + // (w) means we hard wrapped the line + // (b) means the line is _not_ wrapped (it's broken, the default state.) + // cursor is on the '_' + // + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AAAA| (w) + // |AAAAA_ ... | (b) (There are 20 'A's on this line.) + VERIFY_IS_TRUE(tb.GetRowByOffset(wrappedRow).GetCharRow().WasWrapForced()); VERIFY_IS_FALSE(tb.GetRowByOffset(wrappedRow + 1).GetCharRow().WasWrapForced()); @@ -1275,6 +1343,7 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle() auto iter0 = tb.GetCellDataAt({ 0, wrappedRow }); TestUtils::VerifySpanOfText(L"A", iter0, 0, TerminalViewWidth); + auto iter1 = tb.GetCellDataAt({ 0, wrappedRow + 1 }); TestUtils::VerifySpanOfText(L"A", iter1, 0, 20); auto iter2 = tb.GetCellDataAt({ 20, wrappedRow + 1 }); @@ -1934,12 +2003,23 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpace() sm.ProcessString(std::wstring(secondTextLength, L'B')); auto verifyBuffer = [&](const TextBuffer& tb) { + // Buffer contents should look like the following: (80 wide) + // (w) means we hard wrapped the line + // (b) means the line is _not_ wrapped (it's broken, the default state.) + // + // |AAAA...AA | (w) + // | B_ ... | (b) (cursor is on the '_') + // | ... | (b) + VERIFY_IS_TRUE(tb.GetRowByOffset(0).GetCharRow().WasWrapForced()); VERIFY_IS_FALSE(tb.GetRowByOffset(1).GetCharRow().WasWrapForced()); + // First row auto iter0 = tb.GetCellDataAt({ 0, 0 }); TestUtils::VerifySpanOfText(L"A", iter0, 0, firstTextLength); TestUtils::VerifySpanOfText(L" ", iter0, 0, 2); + + // Second row auto iter1 = tb.GetCellDataAt({ 0, 1 }); TestUtils::VerifySpanOfText(L" ", iter1, 0, 1); auto iter2 = tb.GetCellDataAt({ 1, 1 }); @@ -1949,14 +2029,160 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpace() Log::Comment(L"========== Checking the host buffer state =========="); verifyBuffer(hostTb); - // TODO: Actually check the conpty output here. - _checkConptyOutput = false; - _logConpty = true; - // expectedOutput.push_back(std::string(TerminalViewWidth, 'A')); - // expectedOutput.push_back(std::string(20, 'A')); + std::string firstLine = std::string(firstTextLength, 'A'); + firstLine += " "; + std::string secondLine{ " B" }; + + expectedOutput.push_back(firstLine); + expectedOutput.push_back(secondLine); Log::Comment(L"Painting the frame"); VERIFY_SUCCEEDED(renderer.PaintFrame()); Log::Comment(L"========== Checking the terminal buffer state =========="); verifyBuffer(termTb); } + +void ConptyRoundtripTests::OutputWrappedLineWithSpaceAtBottomOfBuffer() +{ + // See https://github.com/microsoft/terminal/pull/5181#issuecomment-610110348 + // This is the same test as OutputWrappedLineWithSpace, but at the bottom of + // the buffer, so we get scrolling behavior as well. + Log::Comment(L"Ensures that a buffer line in conhost that wrapped _on a " + L"space_ will still be emitted as wrapped."); + VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& sm = si.GetStateMachine(); + auto& hostTb = si.GetTextBuffer(); + auto& termTb = *term->_buffer; + + _flushFirstFrame(); + + // First, fill the buffer with contents, so conpty starts circling + const auto hostView = si.GetViewport(); + const auto end = 2 * hostView.Height(); + for (auto i = 0; i < end; i++) + { + Log::Comment(NoThrowString().Format(L"Writing line %d/%d", i, end)); + expectedOutput.push_back("X"); + if (i < hostView.BottomInclusive()) + { + expectedOutput.push_back("\r\n"); + } + else + { + // After we hit the bottom of the viewport, the newlines come in + // separated by empty writes for whatever reason. + expectedOutput.push_back("\r"); + expectedOutput.push_back("\n"); + expectedOutput.push_back(""); + } + + sm.ProcessString(L"X\n"); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + } + + const auto firstTextLength = TerminalViewWidth - 2; + const auto spacesLength = 3; + const auto secondTextLength = 1; + + std::string firstLine = std::string(firstTextLength, 'A'); + firstLine += " "; + std::string secondLine{ " B" }; + + // The following diagrams show the buffer contents after each string emitted + // from conpty. For each of these diagrams: + // (w) means we hard wrapped the line + // (b) means the line is _not_ wrapped (it's broken, the default state.) + // cursor is on the '_' + + // Initial state: + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |_ | (b) + + expectedOutput.push_back(firstLine); + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AA _| (w) The cursor is actually on the last ' ' here + + // TODO GH#5228 might break the "newline & repaint the wrapped char" checks here, that's okay. + expectedOutput.push_back("\r"); // This \r\n is emitted by ScrollFrame to + expectedOutput.push_back("\n"); // add a newline to the bottom of the buffer + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AA | (b) + // |_ | (b) + + expectedOutput.push_back("\x1b[31;80H"); // Move the cursor BACK to the wrapped row + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AA _| (b) The cursor is actually on the last ' ' here + // | | (b) + + expectedOutput.push_back(std::string(1, ' ')); // Reprint the last character of the wrapped row + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AA |_ (w) The cursor is actually on the last ' ' here + // | | (b) + + expectedOutput.push_back(secondLine); + // |X | (b) + // |X | (b) + // ... + // |X | (b) + // |AAAAAAAA...AA | (w) + // | B_ | (b) + + sm.ProcessString(std::wstring(firstTextLength, L'A')); + sm.ProcessString(std::wstring(spacesLength, L' ')); + sm.ProcessString(std::wstring(secondTextLength, L'B')); + + auto verifyBuffer = [&](const TextBuffer& tb, const til::rectangle viewport) { + // Buffer contents should look like the following: (80 wide) + // (w) means we hard wrapped the line + // (b) means the line is _not_ wrapped (it's broken, the default state.) + // + // |AAAA...AA | (w) + // | B_ ... | (b) (cursor is on the '_') + // | ... | (b) + + const short wrappedRow = viewport.bottom() - 2; + VERIFY_IS_TRUE(tb.GetRowByOffset(wrappedRow).GetCharRow().WasWrapForced()); + VERIFY_IS_FALSE(tb.GetRowByOffset(wrappedRow + 1).GetCharRow().WasWrapForced()); + + // First row + auto iter0 = tb.GetCellDataAt({ 0, wrappedRow }); + TestUtils::VerifySpanOfText(L"A", iter0, 0, firstTextLength); + TestUtils::VerifySpanOfText(L" ", iter0, 0, 2); + + // Second row + auto iter1 = tb.GetCellDataAt({ 0, wrappedRow + 1 }); + TestUtils::VerifySpanOfText(L" ", iter1, 0, 1); + auto iter2 = tb.GetCellDataAt({ 1, wrappedRow + 1 }); + TestUtils::VerifySpanOfText(L"B", iter2, 0, secondTextLength); + }; + + Log::Comment(L"========== Checking the host buffer state =========="); + verifyBuffer(hostTb, hostView.ToInclusive()); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Checking the terminal buffer state =========="); + verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); +} diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 371d950c322..42916cbc0e9 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -478,16 +478,13 @@ using namespace Microsoft::Console::Types; std::wstring wstr = std::wstring(unclusteredString.data(), cchActual); RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(wstr)); - // If we've written text to the last column of the viewport, then mark + // GH#4415, GH#5181 + // If the renderer told us that this was a wrapped line, then mark // that we've wrapped this line. The next time we attempt to move the // cursor, if we're trying to move it to the start of the next line, // we'll remember that this line was wrapped, and not manually break the // line. - // Don't do this if the last character we're writing is a space - The last - // char will always be a space, but if we see that, we shouldn't wrap. - const short lastWrittenChar = base::ClampAdd(_lastText.X, base::ClampSub(totalWidth, numSpaces)); - if (lineWrapped && - lastWrittenChar > _lastViewport.RightInclusive()) + if (lineWrapped) { _wrappedRow = coord.Y; _trace.TraceSetWrapped(coord.Y); @@ -558,7 +555,7 @@ using namespace Microsoft::Console::Types; { _deferredCursorPos = { _lastText.X + sNumSpaces, _lastText.Y }; } - else if (numSpaces > 0) + else if (removeSpaces && numSpaces > 0) { std::wstring spaces = std::wstring(numSpaces, L' '); RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(spaces)); From efe4aaf66cb75eade3c0a81103dbdc0849a2930d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Apr 2020 13:00:00 -0500 Subject: [PATCH 43/44] love me --- .github/actions/spell-check/whitelist/alphabet.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/actions/spell-check/whitelist/alphabet.txt b/.github/actions/spell-check/whitelist/alphabet.txt index beda2721886..596715059ea 100644 --- a/.github/actions/spell-check/whitelist/alphabet.txt +++ b/.github/actions/spell-check/whitelist/alphabet.txt @@ -11,6 +11,10 @@ ABCDEFGHIJKLMNOPQRSTQ abcdefghijklmnopqrstuvwxyz ABCDEFGHIJPQRST ABCDEFGHIJPQRSTQ +QQQQQQQQQQABCDEFGHIJ +QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQ +QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQQ +QQQQQQQQQQABCDEFGHIJPQRSTQQQQQQQQQQ qrstuvwxyz qwerty QWERTYUIOP From d5af8c623c86f38fcb5a66607b27d984227b5988 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Apr 2020 14:48:43 -0500 Subject: [PATCH 44/44] This makes more sense --- src/renderer/vt/paint.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 42916cbc0e9..8ece3076288 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -547,7 +547,7 @@ using namespace Microsoft::Console::Types; RETURN_IF_FAILED(_EraseLine()); } } - else if (_newBottomLine) + else if (_newBottomLine && coord.Y == _lastViewport.BottomInclusive()) { // If we're on a new line, then we don't need to erase the line. The // line is already empty. @@ -555,7 +555,7 @@ using namespace Microsoft::Console::Types; { _deferredCursorPos = { _lastText.X + sNumSpaces, _lastText.Y }; } - else if (removeSpaces && numSpaces > 0) + else if (numSpaces > 0) { std::wstring spaces = std::wstring(numSpaces, L' '); RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(spaces));