Emit lines wrapped due to spaces at the end correctly

.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

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<short>() - 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<short>(); 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());
+}

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

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;
     }