From cf1145c7aec5b63b91020733d68e61567df57fc5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Mar 2020 17:04:43 -0500 Subject: [PATCH] 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),