From edea9a335c1e826dcd2027e3f0124cd5fdc0c3b1 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Jan 2020 15:27:34 -0600 Subject: [PATCH] Cleanup for review - this is a _great_ fix for #3490 as well as #1465 --- src/buffer/out/textBuffer.cpp | 7 +- src/cascadia/TerminalCore/Terminal.cpp | 124 +++------- .../ConptyRoundtripTests.cpp | 214 ++++++++++++++++++ 3 files changed, 249 insertions(+), 96 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 04941fc2448..0a6b06ad72d 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1554,9 +1554,14 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi // Arguments: // - oldBuffer - the text buffer to copy the contents FROM // - newBuffer - the text buffer to copy the contents TO +// - lastCharacterViewport - Optional. If the caller knows that the last +// nonspace character is in a particular Viewport, the caller can provide this +// parameter as an optimization, as opposed to searching the entire buffer. // Return Value: // - S_OK if we successfully copied the contents to the new buffer, otherwise an appropriate HRESULT. -HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport) +HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, + TextBuffer& newBuffer, + const std::optional lastCharacterViewport) { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index fcda670e784..e6e2bf87a04 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -187,115 +187,49 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting { newTextBuffer = std::make_unique(bufferSize, _buffer->GetCurrentAttributes(), - 0, - _buffer->GetRenderTarget()); // temporarily set size to 0 so it won't render. + 0, // temporarily set size to 0 so it won't render. + _buffer->GetRenderTarget()); } CATCH_RETURN(); - // RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize)); RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), *newTextBuffer.get(), _mutableViewport)); - // OLD WAY OF FINDING NEW TERMINAL VIEWPORT - { - // auto proposedTop = oldTop; - // const auto newView = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); - // const auto proposedBottom = newView.BottomExclusive(); - // // If the new bottom would be below the bottom of the buffer, then slide the - // // top up so that we'll still fit within the buffer. - // if (proposedBottom > bufferSize.Y) - // { - // proposedTop -= (proposedBottom - bufferSize.Y); - // } - // _mutableViewport = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); - } - - // SCREENINFO WAY OF FINDING NEW TERMINAL VIEWPORT + // However conpty resizes a little oddly - if the height decreased, and + // there were blank lines at the bottom, those lines will get trimmed. + // If there's not blank lines, then the top will get "shifted down", + // moving the top line into scrollback. + // See GH#3490 for more details. + + // If the final position in the buffer is on the bottom row of the new + // viewport, then we're going to need to move the top down. Otherwise, + // move the bottom up. + const auto dy = viewportSize.Y - oldDimensions.Y; + const COORD oldCursorPos = _buffer->GetCursor().GetPosition(); + COORD oldLastChar = oldCursorPos; + try { - // // Save cursor's relative height versus the viewport - // SHORT const sCursorHeightInViewportBefore = _buffer->GetCursor().GetPosition().Y - _mutableViewport.Top(); - // auto& newCursor = newTextBuffer->GetCursor(); - // // Adjust the viewport so the cursor doesn't wildly fly off up or down. - // const short cursorHeightInViewportAfter = newCursor.GetPosition().Y - _mutableViewport.Top(); - // const short cursorHeightDiff = cursorHeightInViewportAfter - sCursorHeightInViewportBefore; - // // LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, true)); - - // auto proposedTop = static_cast(oldTop + cursorHeightDiff); - // const auto newView = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); - // const auto proposedBottom = newView.BottomExclusive(); - // // If the new bottom would be below the bottom of the buffer, then slide the - // // top up so that we'll still fit within the buffer. - // if (proposedBottom > bufferSize.Y) - // { - // proposedTop -= (proposedBottom - bufferSize.Y); - // } - - // _mutableViewport = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); - + oldLastChar = _buffer->GetLastNonSpaceCharacter(_mutableViewport); } + CATCH_LOG(); - // #4354 way of doing things - { + const auto maxRow = std::max(oldLastChar.Y, oldCursorPos.Y); - // const auto dy = viewportSize.Y - oldDimensions.Y; - // const COORD cOldCursorPos = _buffer->GetCursor().GetPosition(); - // COORD cOldLastChar = cOldCursorPos; - // try - // { - // cOldLastChar = _buffer->GetLastNonSpaceCharacter(); - // } - // CATCH_LOG(); + const bool beforeLastRowOfView = maxRow < _mutableViewport.BottomInclusive(); + const auto adjustment = beforeLastRowOfView ? 0 : std::max(0, -dy); - // const auto maxRow = std::max(cOldLastChar.Y, cOldCursorPos.Y); - - // const bool beforeLastRow = maxRow < bufferSize.Y - 1; - // const auto adjustment = beforeLastRow ? 0 : std::max(0, -dy); - - // auto proposedTop = oldTop + adjustment; - - // const auto newView = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); - // const auto proposedBottom = newView.BottomExclusive(); - // // If the new bottom would be below the bottom of the buffer, then slide the - // // top up so that we'll still fit within the buffer. - // if (proposedBottom > bufferSize.Y) - // { - // proposedTop -= (proposedBottom - bufferSize.Y); - // } - - // _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); - } + auto proposedTop = oldTop + adjustment; - // Attempt No. 4 + const auto newView = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); + const auto proposedBottom = newView.BottomExclusive(); + // If the new bottom would be below the bottom of the buffer, then slide the + // top up so that we'll still fit within the buffer. + if (proposedBottom > bufferSize.Y) { - const auto dy = viewportSize.Y - oldDimensions.Y; - const COORD cOldCursorPos = _buffer->GetCursor().GetPosition(); - COORD cOldLastChar = cOldCursorPos; - try - { - cOldLastChar = _buffer->GetLastNonSpaceCharacter(_mutableViewport); - } - CATCH_LOG(); - - const auto maxRow = std::max(cOldLastChar.Y, cOldCursorPos.Y); - - // const bool beforeLastRow = maxRow < bufferSize.Y - 1; - const bool beforeLastRowOfView = maxRow < _mutableViewport.BottomInclusive(); - // const auto adjustment = beforeLastRow ? 0 : std::max(0, -dy); - const auto adjustment = beforeLastRowOfView ? 0 : std::max(0, -dy); - - auto proposedTop = oldTop + adjustment; - - const auto newView = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); - const auto proposedBottom = newView.BottomExclusive(); - // If the new bottom would be below the bottom of the buffer, then slide the - // top up so that we'll still fit within the buffer. - if (proposedBottom > bufferSize.Y) - { - proposedTop -= (proposedBottom - bufferSize.Y); - } - - _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); + proposedTop -= (proposedBottom - bufferSize.Y); } + _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); + _buffer.swap(newTextBuffer); _scrollOffset = 0; diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index b6dac60bb92..e3bbdd9be42 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -156,6 +156,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(TestExactWrappingWithoutSpaces); TEST_METHOD(TestExactWrappingWithSpaces); + TEST_METHOD(TestResizeHeight); + private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -598,3 +600,215 @@ void ConptyRoundtripTests::TestExactWrappingWithSpaces() verifyBuffer(termTb); } +void ConptyRoundtripTests::TestResizeHeight() +{ + // This test class is _60_ tests to ensure that resizing the terminal works + // with conpty correctly. There's a lot of min/maxing in expressions here, + // to account for the sheer number of cases here, and that we have to handle + // both resizing larger and smaller all in one test. + + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method") + TEST_METHOD_PROPERTY(L"Data:dx", L"{-1, 0, 1}") + TEST_METHOD_PROPERTY(L"Data:dy", L"{-10, -1, 0, 1, 10}") + TEST_METHOD_PROPERTY(L"Data:printedRows", L"{1, 10, 50, 200}") + END_TEST_METHOD_PROPERTIES() + int dx, dy; + int printedRows; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"dx", dx), L"change in width of buffer"); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"dy", dy), L"change in height of buffer"); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"printedRows", printedRows), L"Number of rows of text to print"); + + _checkConptyOutput = false; + + 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.get(); + const auto initialHostView = si.GetViewport(); + const auto initialTermView = term->GetViewport(); + const auto initialTerminalBufferHeight = term->GetTextBuffer().GetSize().Height(); + + VERIFY_ARE_EQUAL(0, initialHostView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight, initialHostView.BottomExclusive()); + VERIFY_ARE_EQUAL(0, initialTermView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight, initialTermView.BottomExclusive()); + + Log::Comment(NoThrowString().Format( + L"Print %d lines of output, which will scroll the viewport", printedRows)); + + for (auto i = 0; i < printedRows; i++) + { + // This looks insane, but this expression is carefully crafted to give + // us only printable characters, starting with `!` (0n33). + // Similar statements are used elsewhere throughout this test. + auto wstr = std::wstring(1, static_cast((i) % 93) + 33); + hostSm.ProcessString(wstr); + hostSm.ProcessString(L"\r\n"); + } + + // Conpty doesn't have a scrollback, it's view's origin is always 0,0 + const auto secondHostView = si.GetViewport(); + VERIFY_ARE_EQUAL(0, secondHostView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight, secondHostView.BottomExclusive()); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + const auto secondTermView = term->GetViewport(); + // If we've printed more lines than the height of the buffer, then we're + // expecting the viewport to have moved down. Otherwise, the terminal's + // viewport will stay at 0,0. + const auto expectedTerminalViewBottom = std::max(std::min(gsl::narrow_cast(printedRows + 1), + term->GetBufferHeight()), + term->GetViewport().Height()); + + VERIFY_ARE_EQUAL(expectedTerminalViewBottom, secondTermView.BottomExclusive()); + VERIFY_ARE_EQUAL(expectedTerminalViewBottom - initialTermView.Height(), secondTermView.Top()); + + auto verifyTermData = [&expectedTerminalViewBottom, &printedRows, this, &initialTerminalBufferHeight](TextBuffer& termTb, const int resizeDy = 0) { + // Some number of lines of text were lost from the scrollback. The + // number of lines lost will be determined by whichever of the initial + // or current buffer is smaller. + const auto numLostRows = std::max(0, + printedRows - std::min(term->GetTextBuffer().GetSize().Height(), initialTerminalBufferHeight) + 1); + + const auto rowsWithText = std::min(gsl::narrow_cast(printedRows), + expectedTerminalViewBottom) - + 1 + std::min(resizeDy, 0); + + for (short row = 0; row < rowsWithText; row++) + { + SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures); + auto iter = termTb.GetCellDataAt({ 0, row }); + const wchar_t expectedChar = static_cast((row + numLostRows) % 93) + 33; + + auto expectedString = std::wstring(1, expectedChar); + + if (iter->Chars() != expectedString) + { + Log::Comment(NoThrowString().Format(L"row [%d] was mismatched", row)); + } + VERIFY_ARE_EQUAL(expectedString, (iter++)->Chars()); + VERIFY_ARE_EQUAL(L" ", (iter)->Chars()); + } + }; + auto verifyHostData = [&si, &initialHostView, &printedRows](TextBuffer& hostTb, const int resizeDy = 0) { + const auto hostView = si.GetViewport(); + + // In the host, there are two regions we're interested in: + + // 1. the first section of the buffer with the output in it. Before + // we're resized, this will be filled with one character on each row. + // 2. The second area below the first that's empty (filled with spaces). + // Initially, this is only one row. + // After we resize, different things will happen. + // * If we decrease the height of the buffer, the characters in the + // buffer will all move _up_ the same number of rows. We'll want to + // only check the first initialView+dy rows for characters. + // * If we increase the height, rows will be added at the bottom. We'll + // want to check the initial viewport height for the original + // characters, but then we'll want to look for more blank rows at the + // bottom. The characters in the initial viewport won't have moved. + + const short originalViewHeight = gsl::narrow_cast(resizeDy < 0 ? + initialHostView.Height() + resizeDy : + initialHostView.Height()); + const auto rowsWithText = std::min(originalViewHeight - 1, printedRows); + const bool scrolled = printedRows > initialHostView.Height(); + // The last row of the viewport should be empty + // The second last row will have '0'+50 + // The third last row will have '0'+49 + // ... + // The last row will have '0'+(50-height+1) + const auto firstChar = static_cast(scrolled ? + (printedRows - originalViewHeight + 1) : + 0); + + short row = 0; + // Don't include the last row of the viewport in this check, since it'll + // be blank. We'll check it in the below loop. + for (; row < rowsWithText; row++) + { + SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures); + auto iter = hostTb.GetCellDataAt({ 0, row }); + + const auto expectedChar = static_cast(((firstChar + row) % 93) + 33); + auto expectedString = std::wstring(1, static_cast(expectedChar)); + + if (iter->Chars() != expectedString) + { + Log::Comment(NoThrowString().Format(L"row [%d] was mismatched", row)); + } + VERIFY_ARE_EQUAL(expectedString, (iter++)->Chars(), NoThrowString().Format(L"%s", expectedString.data())); + VERIFY_ARE_EQUAL(L" ", (iter)->Chars()); + } + + // Check that the remaining rows in the viewport are empty. + for (; row < hostView.Height(); row++) + { + SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures); + auto iter = hostTb.GetCellDataAt({ 0, row }); + VERIFY_ARE_EQUAL(L" ", (iter)->Chars()); + } + }; + + verifyHostData(*hostTb); + verifyTermData(*termTb); + + const COORD newViewportSize{ + gsl::narrow_cast(TerminalViewWidth + dx), + gsl::narrow_cast(TerminalViewHeight + dy) + }; + + Log::Comment(NoThrowString().Format(L"Resize the Terminal and conpty here")); + auto resizeResult = term->UserResize(newViewportSize); + VERIFY_SUCCEEDED(resizeResult); + _resizeConpty(newViewportSize.X, newViewportSize.Y); + + // After we resize, make sure to get the new textBuffers + hostTb = &si.GetTextBuffer(); + termTb = term->_buffer.get(); + + // Conpty's doesn't have a scrollback, it's view's origin is always 0,0 + const auto thirdHostView = si.GetViewport(); + VERIFY_ARE_EQUAL(0, thirdHostView.Top()); + VERIFY_ARE_EQUAL(newViewportSize.Y, thirdHostView.BottomExclusive()); + + // The Terminal should be stuck to the top of the viewport. + const auto thirdTermView = term->GetViewport(); + + // TODO: For dy<0, rows=50, this is not true. In that set of cases, we + // _didn't_ pin the top of the Terminal to the old top, we actually shifted + // it down (because the output was at the bottom of the window, not empty + // lines). + // TODO: VERIFY_ARE_EQUAL(secondTermView.Top(), thirdTermView.Top()); + // TODO: VERIFY_ARE_EQUAL(expectedTerminalViewBottom + dy, thirdTermView.BottomExclusive()); + + verifyHostData(*hostTb, dy); + // Note that at this point, nothing should have changed with the Terminal. + verifyTermData(*termTb, dy); + + Log::Comment(NoThrowString().Format(L"Paint a frame to update the Terminal")); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + // Conpty's doesn't have a scrollback, it's view's origin is always 0,0 + const auto fourthHostView = si.GetViewport(); + VERIFY_ARE_EQUAL(0, fourthHostView.Top()); + VERIFY_ARE_EQUAL(newViewportSize.Y, fourthHostView.BottomExclusive()); + + // The Terminal should be stuck to the top of the viewport. + const auto fourthTermView = term->GetViewport(); + + // TODO: For dy<0, rows=50, this is not true. In that set of cases, we + // _didn't_ pin the top of the Terminal to the old top, we actually shifted + // it down (because the output was at the bottom of the window, not empty + // lines). + // TODO: VERIFY_ARE_EQUAL(secondTermView.Top(), fourthTermView.Top()); + // TODO: VERIFY_ARE_EQUAL(expectedTerminalViewBottom + dy, fourthTermView.BottomExclusive()); + + verifyHostData(*hostTb, dy); + verifyTermData(*termTb, dy); +}