From 715014d10f4617fd9de4e4be0e4a518529b26787 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 24 Jan 2020 14:15:40 -0600 Subject: [PATCH] Outstanding cleanup for PR --- src/buffer/out/textBuffer.cpp | 23 +---------- src/buffer/out/textBuffer.hpp | 2 +- src/cascadia/TerminalCore/Terminal.cpp | 1 + .../ConptyRoundtripTests.cpp | 4 +- src/host/VtIo.cpp | 8 ---- src/host/VtIo.hpp | 1 - src/host/screenInfo.cpp | 37 ++++++------------ src/renderer/vt/state.cpp | 38 +++---------------- src/renderer/vt/vtrenderer.hpp | 1 - 9 files changed, 24 insertions(+), 91 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 9724fb900b..99cdbc78e5 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1554,13 +1554,9 @@ 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 -// - padTop - If true, and we're increasing the height of the buffer, we should -// start by inserting new lines at the top of the buffer, so the contents stay -// in the relatively same position. This is only ever set to `true` in the -// console by conpty mode. // 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 bool padTop, const bool preserveSpaceAtBottom) +HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer) { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); @@ -1572,29 +1568,14 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const b // place the new cursor back on the equivalent character in // the new buffer. const COORD cOldCursorPos = oldCursor.GetPosition(); - // const COORD cOldLastChar = preserveSpaceAtBottom ? oldBuffer.GetSize().Dimensions() : oldBuffer.GetLastNonSpaceCharacter(); const COORD cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(); - // short const cOldRowsTotal = cOldLastChar.Y + 1; - short const cOldRowsTotal = preserveSpaceAtBottom ? - oldBuffer.GetSize().Dimensions().Y : - cOldLastChar.Y + 1; + short const cOldRowsTotal = cOldLastChar.Y + 1; short const cOldColsTotal = oldBuffer.GetSize().Width(); COORD cNewCursorPos = { 0 }; bool fFoundCursorPos = false; - // If we increased the height of the buffer, pad the top of the new buffer - // with newlines, so that the contents stay in the same relative location. - if (padTop && oldBuffer.GetSize().Height() < newBuffer.GetSize().Height()) - { - const auto diff = newBuffer.GetSize().Height() - oldBuffer.GetSize().Height(); - for (int i = 0; i < diff; i++) - { - newBuffer.NewlineCursor(); - } - } - HRESULT hr = S_OK; // Loop through all the rows of the old buffer and reprint them into the new buffer for (short iOldRow = 0; iOldRow < cOldRowsTotal; iOldRow++) diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 8fce63ed1a..4d70bb4830 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -158,7 +158,7 @@ public: const std::wstring_view fontFaceName, const COLORREF backgroundColor); - static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const bool padTop, const bool preserveSpaceAtBottom); + static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer); private: std::deque _storage; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 67b5948fed..127411f133 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -189,6 +189,7 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting // 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, diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index d7f587b849..31872c4a37 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -449,7 +449,7 @@ void ConptyRoundtripTests::TestResizeHeight() VERIFY_ARE_EQUAL(TerminalViewHeight, initialTermView.BottomExclusive()); Log::Comment(NoThrowString().Format( - L"Print 50 lines of output, which will scroll the viewport")); + L"Print %d lines of output, which will scroll the viewport", printedRows)); for (auto i = 0; i < printedRows; i++) { @@ -492,7 +492,7 @@ void ConptyRoundtripTests::TestResizeHeight() for (short row = 0; row < rowsWithText; row++) { - // SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures); + SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures); auto iter = termTb.GetCellDataAt({ 0, row }); const wchar_t expectedChar = static_cast((row + numLostRows) % 93) + 33; diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index ed20503c3d..3c446e6c92 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -446,11 +446,3 @@ void VtIo::EndResize() _pVtRenderEngine->EndResizeRequest(); } } - -void VtIo::SetVirtualTop(const short virtualTop) noexcept -{ - if (_pVtRenderEngine) - { - _pVtRenderEngine->SetVirtualTop(virtualTop); - } -} diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index 73b066edc7..b2600c1027 100644 --- a/src/host/VtIo.hpp +++ b/src/host/VtIo.hpp @@ -38,7 +38,6 @@ namespace Microsoft::Console::VirtualTerminal void BeginResize(); void EndResize(); - void SetVirtualTop(const short virtualTop) noexcept; #ifdef UNIT_TESTING void EnableConptyModeForTests(); diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index 815e1f808c..0471c2bda0 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1427,29 +1427,11 @@ bool SCREEN_INFORMATION::IsMaximizedY() const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); const bool isConpty = gci.IsInVtIoMode(); - HRESULT hr = TextBuffer::Reflow(*_textBuffer.get(), *newTextBuffer.get(), false, false); + HRESULT hr = TextBuffer::Reflow(*_textBuffer.get(), *newTextBuffer.get()); + const bool widthChanged = coordNewScreenSize.X != _textBuffer->GetSize().Width(); if (SUCCEEDED(hr)) { - // GH#3490 - In conpty mode, we want to pad new lines into to the top of - // the buffer when we increase the buffer height. Terminals typically - // keep the buffer contents "stuck" to the bottom of the viewport when - // they increase in height, moving lines from the scrollback into the - // viewport. We don't have a scrollback at all in conpty mode, but we - // can still keep the content we have at the bottom by inserting new - // lines at the top. - // - // When that happens, make sure to move our relative cursor position - // down, so that it stays in the same position relative to the bottom - // that it was before. - - // if (isConpty && _textBuffer->GetSize().Height() < newTextBuffer->GetSize().Height()) - // { - // const short diff = newTextBuffer->GetSize().Height() - _textBuffer->GetSize().Height(); - // cursorHeightInViewportBefore += diff; - // gci.GetVtIo()->SetVirtualTop(diff); - // } - Cursor& newCursor = newTextBuffer->GetCursor(); // Adjust the viewport so the cursor doesn't wildly fly off up or down. const short cursorHeightInViewportAfter = newCursor.GetPosition().Y - _viewport.Top(); @@ -1460,13 +1442,18 @@ bool SCREEN_INFORMATION::IsMaximizedY() const _textBuffer.swap(newTextBuffer); } - if (isConpty) + // GH#3490 - In conpty mode, We want to invalidate all of the viewport that + // might have been below any wrapped lines, up until the last character of + // the buffer. Lines that were wrapped may have been re-wrapped during this + // resize, so we want them repainted to the terminal. We don't want to just + // invalidate everything though - if there were blank lines at the bottom, + // those can just be ignored. + if (isConpty && widthChanged) { // Loop through all the rows of the old buffer and reprint them into the new buffer - // for (short iOldRow = 0; iOldRow < cOldRowsTotal; iOldRow++) - auto bottom = std::max(_textBuffer->GetCursor().GetPosition().Y, - std::min(_viewport.BottomInclusive(), - _textBuffer->GetLastNonSpaceCharacter().Y)); + const auto bottom = std::max(_textBuffer->GetCursor().GetPosition().Y, + std::min(_viewport.BottomInclusive(), + _textBuffer->GetLastNonSpaceCharacter().Y)); bool foundWrappedLine = false; for (short y = _viewport.Top(); y <= bottom; y++) { diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index a8624122df..771102c7e4 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -277,33 +277,12 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe, // lead to the first _actual_ resize being suppressed. _suppressResizeRepaint = false; - if (SUCCEEDED(hr)) - { - // if (oldView.Width() != newView.Width()) - // { - // // Viewport is a different width now - just update it all. We may - // // have re-wrapped the buffer contents. - // hr = InvalidateAll(); - // } - // else if (oldView.Height() < newView.Height()) - // { - // // // We grew in height. We inserted empty lines at the top of the - // // // buffer. The cursor is now _actually_ lower (larger Y/row value) - // // // than it was before. - // // _lastText.Y += gsl::narrow_cast(newView.Height() - oldView.Height()); - // // // The text content will try and stay "stuck" at the bottom of the - // // // viewport of the terminal, and invalidating the bottom here can - // // // cause unnecessary lines to get written to the terminal. See - // // // GH#3490. - // } - // else if (oldView.Height() > newView.Height()) - // { - // // We shrunk in height. We don't really need to do anything here. - // // Shrinking in height will remove lines from the top of the buffer - // // (pushing them into scrollback in the terminal). - // // The cursor will stay in the same relative position. - // } - } + // GH#3490 - When the viewport width changed, don't do anything extra here. + // If the buffer had areas that were invalid due to the resize, then the + // buffer will have triggered it's own invalidations for what it knows is + // invalid. Previously, we'd invalidate everything if the width changed, + // because we couldn't be sure if lines were reflowed. + _resized = true; return hr; } @@ -403,11 +382,6 @@ bool VtEngine::_AllIsInvalid() const return S_OK; } -void VtEngine::SetVirtualTop(const short virtualTop) noexcept -{ - _virtualTop = std::max(_virtualTop, virtualTop); -} - void VtEngine::SetTerminalOwner(Microsoft::Console::ITerminalOwner* const terminalOwner) { _terminalOwner = terminalOwner; diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index cedd2722f5..f9c452ca29 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -104,7 +104,6 @@ namespace Microsoft::Console::Render void SetTerminalOwner(Microsoft::Console::ITerminalOwner* const terminalOwner); void BeginResizeRequest(); void EndResizeRequest(); - void SetVirtualTop(const short virtualTop) noexcept; protected: wil::unique_hfile _hFile;