Skip to content

Commit

Permalink
Fix an accidental regression from #5771 (#5870)
Browse files Browse the repository at this point in the history 
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 b4c33dd)
  • Loading branch information
@zadjii-msft @DHowett
zadjii-msft authored and DHowett committed May 12, 2020
1 parent b9483a9 commit 70cb335
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 0 deletions.
103 changes: 103 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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| <wrap>
// |BBBBBBBB_ | <break>
// (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<short>();

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);
}
18 changes: 18 additions & 0 deletions src/renderer/vt/paint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down

0 comments on commit 70cb335

Please sign in to comment.