From f1d3136a24b1bc6df51c1adb81f7a5dd0b34c9f8 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 16 Mar 2020 07:55:25 -0500 Subject: [PATCH] Maintain scrollbar position during a resize operation (#4903) ## Summary of the Pull Request Currently, when the user resizes the Terminal, we'll snap the visible viewport back to the bottom of the buffer. This PR changes the visible viewport of the Terminal to instead remain in the same relative location it was before the resize. ## References Made possible by our sponsors at #4741, and listeners like you. ## PR Checklist * [x] Closes #3494 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments We already hated the `std::optional&` thing I yeet'd into #4741 right at the end to replace a `short*`. So I was already going to change that to a `std::optional>`, which is more idomatic. But then I was looking through the list of bugs and #3494 caught my eye. I realized it would be trivial to not only track the top of the `mutableViewport` during a resize, but we could use the same code path to track the _visible_ viewport's start as well. So basically I'm re-using that bit of code in `Reflow` to calculate the visible viewport's position too. ## Validation Steps Performed Gotta love just resizing things all day, errday --- src/buffer/out/textBuffer.cpp | 31 ++++++++++++++------- src/buffer/out/textBuffer.hpp | 8 +++++- src/cascadia/TerminalCore/Terminal.cpp | 37 +++++++++++++++++++++++--- src/host/screenInfo.cpp | 4 +-- 4 files changed, 63 insertions(+), 17 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index f37ff1a2810..a8c516aa220 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1869,15 +1869,15 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi // - 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. -// - oldViewportTop - Optional. The caller can provide a row in this parameter -// and we'll calculate the position of the _end_ of that row in the new -// buffer. The row's new value is placed back into this parameter. +// - positionInfo - Optional. The caller can provide a pair of rows in this +// parameter and we'll calculate the position of the _end_ of those rows in +// the new buffer. The rows's new value is placed back into this parameter. // 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, - std::optional& oldViewportTop) + std::optional> positionInfo) { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); @@ -1896,7 +1896,8 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, COORD cNewCursorPos = { 0 }; bool fFoundCursorPos = false; - bool foundOldRow = false; + bool foundOldMutable = false; + bool foundOldVisible = false; 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++) @@ -1960,12 +1961,24 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, // If we found the old row that the caller was interested in, set the // out value of that parameter to the cursor's current Y position (the // new location of the _end_ of that row in the buffer). - if (oldViewportTop.has_value() && !foundOldRow) + if (positionInfo.has_value()) { - if (iOldRow >= oldViewportTop.value()) + if (!foundOldMutable) { - oldViewportTop = newCursor.GetPosition().Y; - foundOldRow = true; + if (iOldRow >= positionInfo.value().get().mutableViewportTop) + { + positionInfo.value().get().mutableViewportTop = newCursor.GetPosition().Y; + foundOldMutable = true; + } + } + + if (!foundOldVisible) + { + if (iOldRow >= positionInfo.value().get().visibleViewportTop) + { + positionInfo.value().get().visibleViewportTop = newCursor.GetPosition().Y; + foundOldVisible = true; + } } } diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 7b2ee52df39..086eea214d6 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -161,10 +161,16 @@ class TextBuffer final const std::wstring_view fontFaceName, const COLORREF backgroundColor); + struct PositionInformation + { + short mutableViewportTop{ 0 }; + short visibleViewportTop{ 0 }; + }; + static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport, - std::optional& oldViewportTop); + std::optional> positionInfo); private: std::deque _storage; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 14e5e0b0f6e..ad88230eb36 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -186,6 +186,11 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting // This will be used to determine where the viewport should be in the new buffer. const short oldViewportTop = _mutableViewport.Top(); short newViewportTop = oldViewportTop; + short newVisibleTop = ::base::saturated_cast(_VisibleStartIndex()); + + // If the original buffer had _no_ scroll offset, then we should be at the + // bottom in the new buffer as well. Track that case now. + const bool originalOffsetWasZero = _scrollOffset == 0; // First allocate a new text buffer to take the place of the current one. std::unique_ptr newTextBuffer; @@ -196,12 +201,28 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting 0, // temporarily set size to 0 so it won't render. _buffer->GetRenderTarget()); - std::optional oldViewStart{ oldViewportTop }; + // Build a PositionInformation to track the position of both the top of + // the mutable viewport and the top of the visible viewport in the new + // buffer. + // * the new value of mutableViewportTop will be used to figure out + // where we should place the mutable viewport in the new buffer. This + // requires a bit of trickiness to remain consistent with conpty's + // buffer (as seen below). + // * the new value of visibleViewportTop will be used to calculate the + // new scrollOffsett in the new buffer, so that the visible lines on + // the screen remain roughly the same. + TextBuffer::PositionInformation oldRows{ 0 }; + oldRows.mutableViewportTop = oldViewportTop; + oldRows.visibleViewportTop = newVisibleTop; + + const std::optional oldViewStart{ oldViewportTop }; RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), *newTextBuffer.get(), _mutableViewport, - oldViewStart)); - newViewportTop = oldViewStart.value(); + { oldRows })); + + newViewportTop = oldRows.mutableViewportTop; + newVisibleTop = oldRows.visibleViewportTop; } CATCH_RETURN(); @@ -310,7 +331,15 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting _buffer.swap(newTextBuffer); - _scrollOffset = 0; + // GH#3494: Maintain scrollbar position during resize + // Make sure that we don't scroll past the mutableViewport at the bottom of the buffer + newVisibleTop = std::min(newVisibleTop, _mutableViewport.Top()); + // Make sure we don't scroll past the top of the scrollback + newVisibleTop = std::max(newVisibleTop, 0); + + // If the old scrolloffset was 0, then we weren't scrolled back at all + // before, and shouldn't be now either. + _scrollOffset = originalOffsetWasZero ? 0 : _mutableViewport.Top() - newVisibleTop; _NotifyScrollEvent(); return S_OK; diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index 6b9d6ab340f..437b07300fc 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1414,9 +1414,7 @@ bool SCREEN_INFORMATION::IsMaximizedY() const // Save cursor's relative height versus the viewport SHORT const sCursorHeightInViewportBefore = _textBuffer->GetCursor().GetPosition().Y - _viewport.Top(); - // Reflow requires a optional& , which can't just be done inline with the call. - std::optional unused{ std::nullopt }; - HRESULT hr = TextBuffer::Reflow(*_textBuffer.get(), *newTextBuffer.get(), std::nullopt, unused); + HRESULT hr = TextBuffer::Reflow(*_textBuffer.get(), *newTextBuffer.get(), std::nullopt, std::nullopt); if (SUCCEEDED(hr)) {