From 70cb335fd1ffafa24908338e6c5d889410ff06b3 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 12 May 2020 17:02:15 -0500 Subject: [PATCH] Fix an accidental regression from #5771 (#5870) This PR reverts a relatively minor change that was made incorrectly to ConPTY in #5771. In that PR, I authored two tests. One of them actually caught the bug that was supposed to be fixed by #5771. The other test was simply authored during the investigation. I believed at the time that the test revealed a bug in conpty that was fixed by _removing_ this block of code. However, an investigation itno #5839 revealed that this code was actually fairly critical. So, I'm also _skipping_ this buggy test for now. I'm also adding a specific test case to this bug. The problem in the bugged case of `WrapNewLineAtBottom` is that `WriteCharsLegacy` is wrapping the bottom row of the ConPTY buffer, which is causing the cursor to automatically move to the next line in the buffer. This is because `WriteCharsLegacy` isn't being called with the `WC_DELAY_EOL_WRAP` flag. So, in that test case, * The client emits a wrapped line to conpty * conpty fills the bottom line with that text, then dutifully increments the buffer to make space for the cursor on a _new_ bottom line. * Conpty reprints the last `~` of the wrapped line * Then it gets to the next line, which is being painted _before_ the client emits the rest of the line of text to fill that row. * Conpty thinks this row is empty, (it is) and manually breaks the row. However, the test expects this row to be emitted as wrapped. The problem comes from the torn state in the middle of these frames - the original line probably _should_ remain wrapped, but this is a sufficiently rare case that the fix is being punted into the next release. It's possible that improving how we handle line wrapping might also fix this case - currently we're only marking a row as wrapped when we print the last cell of a row, but we should probably mark it as wrapped instead when we print the first char of the _following_ row. That work is being tracked in #5800 ### The real bug in this PR The problem in the `DeleteWrappedWord` test is that the first line is still being marked as wrapped. So when we get to painting the line below it, we'll see that there are no characters to be printed (only spaces), we emit a `^[20X^[20C`, but the cursor is still at the end of the first line. Because it's there, we don't actually clear the text we want to clear. So DeleteWrappedWord, #5839 needs the `_wrappedRow = std::nullopt;` statement here. ## References * I guess just look at #5800, I put everything in there. ## Validation Steps Performed * Tested manually that this was fixed for the Terminal * ran tests Closes #5839 (cherry picked from commit b4c33dd84260e3a4883ca179be97a94420fedab2) --- .../ConptyRoundtripTests.cpp | 103 ++++++++++++++++++ src/renderer/vt/paint.cpp | 18 +++ 2 files changed, 121 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index d60efda4250..c3c84ca1ceb 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -218,6 +218,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(WrapNewLineAtBottom); TEST_METHOD(WrapNewLineAtBottomLikeMSYS); + TEST_METHOD(DeleteWrappedWord); + private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -3026,6 +3028,17 @@ void ConptyRoundtripTests::WrapNewLineAtBottom() INIT_TEST_PROPERTY(int, circledRows, L"Controls the number of lines we output."); INIT_TEST_PROPERTY(int, paintEachNewline, L"Controls whether we should call PaintFrame every line of text or not."); + // GH#5839 - + // This test does expose a real bug when using WriteCharsLegacy to emit + // wrapped lines in conpty without WC_DELAY_EOL_WRAP. However, this fix has + // not yet been made, so for now, we need to just skip the cases that cause + // this. + if (writingMethod == PrintWithWriteCharsLegacy && paintEachNewline == PaintEveryLine) + { + Log::Result(WEX::Logging::TestResults::Skipped); + return; + } + // I've tested this with 0x0, 0x4, 0x80, 0x84, and 0-8, and none of these // flags seem to make a difference. So we're just assuming 0 here, so we // don't test a bunch of redundant cases. @@ -3374,3 +3387,93 @@ void ConptyRoundtripTests::WrapNewLineAtBottomLikeMSYS() VERIFY_ARE_EQUAL(circledRows + 1, term->_mutableViewport.Top()); verifyBuffer(*termTb, term->_mutableViewport.ToInclusive()); } + +void ConptyRoundtripTests::DeleteWrappedWord() +{ + // See https://github.com/microsoft/terminal/issues/5839 + Log::Comment(L"This test ensures that when we print a empty row beneath a " + L"wrapped row, that we _actually_ clear it."); + 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.get(); + + _flushFirstFrame(); + + _checkConptyOutput = false; + _logConpty = true; + + // Print two lines of text: + // |AAAAAAAAAAAAA BBBBBB| + // |BBBBBBBB_ | + // (cursor on the '_') + + sm.ProcessString(L"\x1b[?25l"); + sm.ProcessString(std::wstring(50, L'A')); + sm.ProcessString(L" "); + sm.ProcessString(std::wstring(50, L'B')); + sm.ProcessString(L"\x1b[?25h"); + + auto verifyBuffer = [&](const TextBuffer& tb, const til::rectangle viewport, const bool after) { + const auto width = viewport.width(); + + auto iter1 = tb.GetCellDataAt({ 0, 0 }); + TestUtils::VerifySpanOfText(L"A", iter1, 0, 50); + TestUtils::VerifySpanOfText(L" ", iter1, 0, 1); + if (after) + { + TestUtils::VerifySpanOfText(L" ", iter1, 0, 50); + + auto iter2 = tb.GetCellDataAt({ 0, 1 }); + TestUtils::VerifySpanOfText(L" ", iter2, 0, width); + } + else + { + TestUtils::VerifySpanOfText(L"B", iter1, 0, 50); + + auto iter2 = tb.GetCellDataAt({ 0, 1 }); + TestUtils::VerifySpanOfText(L"B", iter2, 0, 50 - (width - 51)); + TestUtils::VerifySpanOfText(L" ", iter2, 0, width); + } + }; + + Log::Comment(L"========== Checking the host buffer state (before) =========="); + verifyBuffer(*hostTb, si.GetViewport().ToInclusive(), false); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + Log::Comment(L"========== Checking the terminal buffer state (before) =========="); + verifyBuffer(*termTb, term->_mutableViewport.ToInclusive(), false); + + // Now, go back and erase all the 'B's, as if the user executed a + // backward-kill-word in PowerShell. Afterwards, the buffer will look like: + // + // |AAAAAAAAAAAAA_ | + // | | + // + // We're doing this the same way PsReadline redraws the prompt - by just + // reprinting all of it. + + sm.ProcessString(L"\x1b[?25l"); + sm.ProcessString(L"\x1b[H"); + sm.ProcessString(std::wstring(50, L'A')); + sm.ProcessString(L" "); + + sm.ProcessString(std::wstring(TerminalViewWidth - 51, L' ')); + + sm.ProcessString(L"\x1b[2;1H"); + sm.ProcessString(std::wstring(50 - (TerminalViewWidth - 51), L' ')); + sm.ProcessString(L"\x1b[1;50H"); + sm.ProcessString(L"\x1b[?25h"); + + Log::Comment(L"========== Checking the host buffer state (after) =========="); + verifyBuffer(*hostTb, si.GetViewport().ToInclusive(), true); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + Log::Comment(L"========== Checking the terminal buffer state (after) =========="); + verifyBuffer(*termTb, term->_mutableViewport.ToInclusive(), true); +} diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index b539950efb4..0294fe20b57 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -474,6 +474,24 @@ using namespace Microsoft::Console::Types; (totalWidth - numSpaces) : totalWidth; + if (cchActual == 0) + { + // If the previous row wrapped, but this line is empty, then we actually + // do want to move the cursor down. Otherwise, we'll possibly end up + // accidentally erasing the last character from the previous line, as + // the cursor is still waiting on that character for the next character + // to follow it. + // + // GH#5839 - If we've emitted a wrapped row, because the cursor is + // sitting just past the last cell of the previous row, if we execute a + // EraseCharacter or EraseLine here, then the row won't actually get + // cleared here. This logic is important to make sure that the cursor is + // in the right position before we do that. + + _wrappedRow = std::nullopt; + _trace.TraceClearWrapped(); + } + // Move the cursor to the start of this run. RETURN_IF_FAILED(_MoveCursor(coord));