Skip to content

Commit

Permalink
Add a test for this case
Browse files Browse the repository at this point in the history
    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
  • Loading branch information
zadjii-msft committed Mar 26, 2020
1 parent e21101b commit cf1145c
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 30 deletions.
101 changes: 101 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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<short>() - 2;

for (short i = viewport.top<short>(); 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"));
}
52 changes: 23 additions & 29 deletions src/renderer/vt/XtermEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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())
Expand All @@ -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)
Expand Down
1 change: 0 additions & 1 deletion src/renderer/vt/state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down

1 comment on commit cf1145c

@github-actions

This comment was marked as resolved.

Please sign in to comment.