From 26a16d4d49a2144b88bc66ea3e2fdeb198e81ee5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 24 Jan 2020 09:13:06 -0600 Subject: [PATCH] some cleanup for the terminal core --- src/cascadia/TerminalCore/Terminal.cpp | 91 +++++++------------ .../ConptyRoundtripTests.cpp | 56 ++++++------ 2 files changed, 62 insertions(+), 85 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index e1d40859c8..e5958c10eb 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -175,67 +175,44 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting { return S_FALSE; } - // const short dy = gsl::narrow_cast(viewportSize.Y - oldDimensions.Y); + const auto dy = viewportSize.Y - oldDimensions.Y; - // const bool resizeDown = viewportSize.Y < oldDimensions.Y; - // const bool changedWidth = viewportSize.X != oldDimensions.X; - const bool resizeDown = false; - if (resizeDown) - // if (resizeDown && !changedWidth) + + // We're going to attempt to "stick to the top" of where the old viewport was. + const auto oldTop = _mutableViewport.Top(); + + const short newBufferHeight = viewportSize.Y + _scrollbackLines; + COORD bufferSize{ viewportSize.X, newBufferHeight }; + RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize)); + + // 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. + + // 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 COORD cOldCursorPos = _buffer->GetCursor().GetPosition(); + const COORD cOldLastChar = _buffer->GetLastNonSpaceCharacter(); + + const auto maxRow = std::max(cOldLastChar.Y, cOldCursorPos.Y); + + const bool beforeLastRow = maxRow < bufferSize.Y - 1; // -1? 0? + 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) { - // stick to bottom - const auto oldTop = _mutableViewport.Top(); - const auto oldBottom = _mutableViewport.BottomExclusive(); - - const short newBufferHeight = viewportSize.Y + _scrollbackLines; - COORD bufferSize{ viewportSize.X, newBufferHeight }; - RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize)); - - // Attempt to "stick" to the bottom of the current viewport. Lines will be - // moved into scrollback. - const auto newViewSize = Viewport::FromDimensions({ 0, 0 }, viewportSize); - - auto proposedBottom = oldBottom; - if (proposedBottom > bufferSize.Y) - { - proposedBottom = bufferSize.Y; - } - short proposedTop = proposedBottom - newViewSize.Height(); - - _mutableViewport = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); + proposedTop -= (proposedBottom - bufferSize.Y); } - else // <---- We're always doing this for now - { - // stick to top - const auto oldTop = _mutableViewport.Top(); - const short newBufferHeight = viewportSize.Y + _scrollbackLines; - COORD bufferSize{ viewportSize.X, newBufferHeight }; - RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize)); - - const COORD cOldCursorPos = _buffer->GetCursor().GetPosition(); - const COORD cOldLastChar = _buffer->GetLastNonSpaceCharacter(); - const auto maxRow = std::max(cOldLastChar.Y, cOldCursorPos.Y); - const bool beforeLastRow = maxRow < bufferSize.Y - 1; // -1? 0? - const auto adjustment = beforeLastRow ? 0 : std::max(0, -dy); - - // auto proposedTop = oldTop; - // auto proposedTop = oldTop + 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); - // _scrollOffset = 0; - // _NotifyScrollEvent(); - } + _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); _scrollOffset = 0; _NotifyScrollEvent(); diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index c86cc5972a..1a4144ca35 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -476,6 +476,21 @@ void ConptyRoundtripTests::TestResizeHeight() auto verifyHostData = [&si, &initialHostView](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()); @@ -485,19 +500,11 @@ void ConptyRoundtripTests::TestResizeHeight() // The third last row will have '0'+49 // ... // The last row will have '0'+(50-height+1) - // const auto firstChar = static_cast(L'0' + (50 - hostView.Height() + 1)); - // const auto firstChar = static_cast(L'0' + (50 - initialHostView.Height() + 1)); const auto firstChar = static_cast(L'0' + (50 - originalViewHeight + 1)); - // If we increased the height of the buffer, then we're going to insert - // pad the top of the buffer with blank lines, to keep the real content - // we had at the bottom of the buffer. - // In that case, check for the first lines to be filled with spaces. - // const short firstRowWithChars = gsl::narrow_cast(resizeDy > 0 ? resizeDy : 0); - const short firstRowWithChars = gsl::narrow_cast(0); - - // Don't include the last row of the viewport in this check, since it'll be blank - short row = firstRowWithChars; + 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 < originalViewHeight - 1; row++) { auto iter = hostTb.GetCellDataAt({ 0, row }); @@ -512,17 +519,11 @@ void ConptyRoundtripTests::TestResizeHeight() VERIFY_ARE_EQUAL(L" ", (iter)->Chars()); } - // // Check the last row of the viewport here - // auto iter = hostTb.GetCellDataAt({ 0, hostView.Height() - 1 }); - // VERIFY_ARE_EQUAL(L" ", (iter)->Chars()); - - // if (resizeDy > 0) + // Check that the remaining rows in the viewport are empty. + for (; row < hostView.Height(); row++) { - for (; row < hostView.Height(); row++) - { - auto iter = hostTb.GetCellDataAt({ 0, row }); - VERIFY_ARE_EQUAL(L" ", (iter)->Chars()); - } + auto iter = hostTb.GetCellDataAt({ 0, row }); + VERIFY_ARE_EQUAL(L" ", (iter)->Chars()); } }; verifyHostData(*hostTb); @@ -533,10 +534,11 @@ void ConptyRoundtripTests::TestResizeHeight() gsl::narrow_cast(TerminalViewHeight + dy) }; + Log::Comment(NoThrowString().Format(L"Resize the Terminal and conpty here")); auto resizeResult = term->UserResize(newViewportSize); VERIFY_SUCCEEDED(resizeResult); - // DebugBreak(); _resizeConpty(newViewportSize.X, newViewportSize.Y); + // After we resize, make sure to get the new textBuffers hostTb = &si.GetTextBuffer(); termTb = term->_buffer.get(); @@ -546,17 +548,17 @@ void ConptyRoundtripTests::TestResizeHeight() VERIFY_ARE_EQUAL(0, thirdHostView.Top()); VERIFY_ARE_EQUAL(newViewportSize.Y, thirdHostView.BottomExclusive()); - // The Terminal should be stuck on the bottom of the viewport + // The Terminal should be stuck to the top of the viewport. const auto thirdTermView = term->GetViewport(); - // VERIFY_ARE_EQUAL(50 - thirdTermView.Height() + 1, thirdTermView.Top()); - // VERIFY_ARE_EQUAL(50, thirdTermView.BottomInclusive()); VERIFY_ARE_EQUAL(secondTermView.Top(), thirdTermView.Top()); VERIFY_ARE_EQUAL(50 + dy, thirdTermView.BottomInclusive()); verifyHostData(*hostTb, dy); + // Note that at this point, nothing should have changed with the Terminal. verifyTermData(*termTb); + 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 @@ -564,11 +566,9 @@ void ConptyRoundtripTests::TestResizeHeight() VERIFY_ARE_EQUAL(0, fourthHostView.Top()); VERIFY_ARE_EQUAL(newViewportSize.Y, fourthHostView.BottomExclusive()); - // The Terminal should be stuck on the bottom of the viewport + // The Terminal should be stuck to the top of the viewport. const auto fourthTermView = term->GetViewport(); - // VERIFY_ARE_EQUAL(50 - fourthTermView.Height() + 1, fourthTermView.Top()); - // VERIFY_ARE_EQUAL(50, fourthTermView.BottomInclusive()); VERIFY_ARE_EQUAL(secondTermView.Top(), fourthTermView.Top()); VERIFY_ARE_EQUAL(50 + dy, fourthTermView.BottomInclusive()); }