diff --git a/.github/actions/spell-check/dictionary/dictionary.txt b/.github/actions/spell-check/dictionary/dictionary.txt index 10d5738d482..ca528f2f7df 100644 --- a/.github/actions/spell-check/dictionary/dictionary.txt +++ b/.github/actions/spell-check/dictionary/dictionary.txt @@ -384723,23 +384723,7 @@ spadonic spadonism spadrone spadroon -spae -spaebook -spaecraft -spaed -spaedom -spaeing -spaeings -spae-man -spaeman -spaer -Spaerobee -spaes spaetzle -spaewife -spaewoman -spaework -spaewright SPAG spag spagetti diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index e85fd243ddd..be39bd199b8 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -188,6 +188,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(OutputWrappedLineWithSpace); TEST_METHOD(OutputWrappedLineWithSpaceAtBottomOfBuffer); + TEST_METHOD(BreakLinesOnCursorMovement); + TEST_METHOD(ScrollWithMargins); private: @@ -2186,3 +2188,174 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpaceAtBottomOfBuffer() Log::Comment(L"========== Checking the terminal buffer state =========="); verifyBuffer(termTb, term->_mutableViewport.ToInclusive()); } + +void ConptyRoundtripTests::BreakLinesOnCursorMovement() +{ + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1, 2, 3, 4, 5, 6}") + END_TEST_METHOD_PROPERTIES(); + constexpr int MoveCursorWithCUP = 0; + constexpr int MoveCursorWithCR_LF = 1; + constexpr int MoveCursorWithLF_CR = 2; + constexpr int MoveCursorWithVPR_CR = 3; + constexpr int MoveCursorWithCUB_LF = 4; + constexpr int MoveCursorWithCUD_CR = 5; + constexpr int MoveCursorWithNothing = 6; + INIT_TEST_PROPERTY(int, cursorMovementMode, L"Controls how we move the cursor, either with CUP, newline/carriage-return, or some other VT sequence"); + + Log::Comment(L"This is a test for GH#5291. WSL vim uses spaces to clear the" + L" ends of blank lines, not EL. This test ensures we emit text" + L" from conpty such that the terminal re-creates the state of" + L" the host, which includes wrapped lines of lots of spaces."); + 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& termTb = *term->_buffer; + + _flushFirstFrame(); + + // Any of the cursor movements that use a LF will actually hard break the + // line - everything else will leave it marked as wrapped. + const bool expectHardBreak = (cursorMovementMode == MoveCursorWithLF_CR) || + (cursorMovementMode == MoveCursorWithCR_LF) || + (cursorMovementMode == MoveCursorWithCUB_LF); + + auto verifyBuffer = [&](const TextBuffer& tb, + const til::rectangle viewport) { + const auto lastRow = viewport.bottom() - 1; + const til::point expectedCursor{ 5, lastRow }; + VERIFY_ARE_EQUAL(expectedCursor, til::point{ tb.GetCursor().GetPosition() }); + VERIFY_IS_TRUE(tb.GetCursor().IsVisible()); + + for (auto y = viewport.top(); y < lastRow; y++) + { + // We're using CUP to move onto the status line _always_, so the + // second-last row will always be marked as wrapped. + const auto rowWrapped = (!expectHardBreak) || (y == lastRow - 1); + VERIFY_ARE_EQUAL(rowWrapped, tb.GetRowByOffset(y).GetCharRow().WasWrapForced()); + TestUtils::VerifyExpectedString(tb, L"~ ", til::point{ 0, y }); + } + + TestUtils::VerifyExpectedString(tb, L"AAAAA", til::point{ 0, lastRow }); + }; + + // We're _not_ checking the conpty output during this test, only the side effects. + _logConpty = true; + _checkConptyOutput = false; + + // Lock must be taken to manipulate alt/main buffer state. + gci.LockConsole(); + auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); + + // Use DECALN to fill the buffer with 'E's. + hostSm.ProcessString(L"\x1b#8"); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"Switching to the alt buffer"); + hostSm.ProcessString(L"\x1b[?1049h"); + auto restoreBuffer = wil::scope_exit([&] { hostSm.ProcessString(L"\x1b[?1049l"); }); + auto& altBuffer = gci.GetActiveOutputBuffer(); + auto& altTextBuffer = altBuffer.GetTextBuffer(); + + // Go home and clear the screen. + hostSm.ProcessString(L"\x1b[H"); + hostSm.ProcessString(L"\x1b[2J"); + + // Write out lines of '~' followed by enough spaces to fill the line. + hostSm.ProcessString(L"\x1b[94m"); + for (auto y = 0; y < altBuffer.GetViewport().BottomInclusive(); y++) + { + // Vim uses CUP to position the cursor on the first cell of each row, every row. + if (cursorMovementMode == MoveCursorWithCUP) + { + std::wstringstream ss; + ss << L"\x1b["; + ss << y + 1; + ss << L";1H"; + hostSm.ProcessString(ss.str()); + } + // As an additional test, try breaking lines manually with \r\n + else if (cursorMovementMode == MoveCursorWithCR_LF) + { + // Don't need to newline on the 0'th row + if (y > 0) + { + hostSm.ProcessString(L"\r\n"); + } + } + // As an additional test, try breaking lines manually with \n\r + else if (cursorMovementMode == MoveCursorWithLF_CR) + { + // Don't need to newline on the 0'th row + if (y > 0) + { + hostSm.ProcessString(L"\n\r"); + } + } + // As an additional test, move the cursor down with VPR, then to the start of the line with CR + else if (cursorMovementMode == MoveCursorWithVPR_CR) + { + // Don't need to newline on the 0'th row + if (y > 0) + { + hostSm.ProcessString(L"\x1b[1e"); + hostSm.ProcessString(L"\r"); + } + } + // As an additional test, move the cursor back with CUB, then down with LF + else if (cursorMovementMode == MoveCursorWithCUB_LF) + { + // Don't need to newline on the 0'th row + if (y > 0) + { + hostSm.ProcessString(L"\x1b[80D"); + hostSm.ProcessString(L"\n"); + } + } + // As an additional test, move the cursor down with CUD, then to the start of the line with CR + else if (cursorMovementMode == MoveCursorWithCUD_CR) + { + // Don't need to newline on the 0'th row + if (y > 0) + { + hostSm.ProcessString(L"\x1b[B"); + hostSm.ProcessString(L"\r"); + } + } + // Win32 vim.exe will simply do _nothing_ in this scenario. It'll just + // print the lines one after the other, without moving the cursor, + // relying on us auto moving to the following line. + else if (cursorMovementMode == MoveCursorWithNothing) + { + } + + // IMPORTANT! The way vim writes these blank lines is as '~' followed by + // enough spaces to fill the line. + // This bug (GH#5291 won't repro if you don't have the spaces). + std::wstring line{ L"~" }; + line += std::wstring(79, L' '); + hostSm.ProcessString(line); + } + + // Print the "Status Line" + hostSm.ProcessString(L"\x1b[32;1H"); + hostSm.ProcessString(L"\x1b[m"); + hostSm.ProcessString(L"AAAAA"); + + Log::Comment(L"========== Checking the host buffer state =========="); + verifyBuffer(altTextBuffer, altBuffer.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/host/screenInfo.cpp b/src/host/screenInfo.cpp index b893c1962cd..c9348270cf2 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1663,6 +1663,19 @@ void SCREEN_INFORMATION::SetCursorDBMode(const bool DoubleCursor) return STATUS_INVALID_PARAMETER; } + // In GH#5291, we experimented with manually breaking the line on all cursor + // movements here. As we print lines into the buffer, we mark lines as + // wrapped when we print the last cell of the row, not the first cell of the + // subsequent row (the row the first line wrapped onto). + // + // Logically, we thought that manually breaking lines when we move the + // cursor was a good idea. We however, did not have the time to fully + // validate that this was the correct answer, and a simpler solution for the + // bug on hand was found. Furthermore, we thought it would be a more + // comprehensive solution to only mark lines as wrapped when we print the + // first cell of the second row, which would require some WriteCharsLegacy + // work. + cursor.SetPosition(Position); // if we have the focus, adjust the cursor state diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 8ece3076288..dd704a582bf 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -443,15 +443,23 @@ using namespace Microsoft::Console::Types; const bool useEraseChar = (optimalToUseECH) && (!_newBottomLine) && (!_clearedAllThisFrame); + const bool printingBottomLine = coord.Y == _lastViewport.BottomInclusive(); // 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. // // 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()); + // + // GH#5291: DON'T remove spaces when the row wrapped. We might need those + // spaces to preserve the wrap state of this line, or the cursor position. + // For example, vim.exe uses "~ "... to clear the line, and then leaves + // the lines _wrapped_. It doesn't care to manually break the lines, but if + // we trimmed the spaces off here, we'd print all the "~"s one after another + // on the same line. + const bool removeSpaces = !lineWrapped && (useEraseChar || + _clearedAllThisFrame || + (_newBottomLine && printingBottomLine)); const size_t cchActual = removeSpaces ? (cchLine - numSpaces) : cchLine; @@ -547,7 +555,7 @@ using namespace Microsoft::Console::Types; RETURN_IF_FAILED(_EraseLine()); } } - else if (_newBottomLine && coord.Y == _lastViewport.BottomInclusive()) + else if (_newBottomLine && printingBottomLine) { // If we're on a new line, then we don't need to erase the line. The // line is already empty. @@ -566,7 +574,7 @@ using namespace Microsoft::Console::Types; // 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()) + if (printingBottomLine) { _newBottomLine = false; }