From bd3d0ea446c47aca3fa556f90a0f3eaa78809d25 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 13 Mar 2020 11:45:19 -0500 Subject: [PATCH 1/5] Implement the entire thing --- src/buffer/out/textBuffer.cpp | 25 +++++++++++++++++++------ src/buffer/out/textBuffer.hpp | 8 +++++++- src/cascadia/TerminalCore/Terminal.cpp | 21 +++++++++++++++++---- src/host/screenInfo.cpp | 4 +--- 4 files changed, 44 insertions(+), 14 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index f37ff1a2810..bba02fa1bf1 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1877,7 +1877,7 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport, - std::optional& oldViewportTop) + std::optional> oldRows) { 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 (oldRows.has_value()) { - if (iOldRow >= oldViewportTop.value()) + if (!foundOldMutable) { - oldViewportTop = newCursor.GetPosition().Y; - foundOldRow = true; + if (iOldRow >= oldRows.value().get().mutableViewportTop) + { + oldRows.value().get().mutableViewportTop = newCursor.GetPosition().Y; + foundOldMutable = true; + } + } + + if (!foundOldVisible) + { + if (iOldRow >= oldRows.value().get().visibleViewportTop) + { + oldRows.value().get().visibleViewportTop = newCursor.GetPosition().Y; + foundOldVisible = true; + } } } diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 7b2ee52df39..3b6f43b7198 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 LinesToReflow + { + short mutableViewportTop{ 0 }; + short visibleViewportTop{ 0 }; + }; + static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport, - std::optional& oldViewportTop); + std::optional> oldRows); private: std::deque _storage; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index c019b398a90..2c99b06bd39 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -186,7 +186,8 @@ 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()); + const bool originalOffsetWasZero = _scrollOffset == 0; // First allocate a new text buffer to take the place of the current one. std::unique_ptr newTextBuffer; try @@ -196,12 +197,18 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting 0, // temporarily set size to 0 so it won't render. _buffer->GetRenderTarget()); + TextBuffer::LinesToReflow oldRows{ 0 }; + oldRows.mutableViewportTop = oldViewportTop; + oldRows.visibleViewportTop = newVisibleTop; + 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 +317,13 @@ 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); + + _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)) { From 0144be492d22443eb9179b2f3b3ec6b1ba772462 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 13 Mar 2020 11:51:20 -0500 Subject: [PATCH 2/5] Add comments --- src/buffer/out/textBuffer.cpp | 6 +++--- src/cascadia/TerminalCore/Terminal.cpp | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index bba02fa1bf1..523ad1aaff7 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1869,9 +1869,9 @@ 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. +// - oldRows - 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, diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 2c99b06bd39..a01eb0747cf 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -187,7 +187,11 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting 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; try @@ -197,6 +201,16 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting 0, // temporarily set size to 0 so it won't render. _buffer->GetRenderTarget()); + // Build a LinesToReflow 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). + // * then new value of visibleViewportTop will be used to calculate the + // new scrollOffsett in the new buffer, so that the visible lines on + // the sceren remain roughly the same. TextBuffer::LinesToReflow oldRows{ 0 }; oldRows.mutableViewportTop = oldViewportTop; oldRows.visibleViewportTop = newVisibleTop; @@ -323,6 +337,8 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting // 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(); From 3a9e49a4bda0756986ce85161b019c4ee73db21d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 13 Mar 2020 14:55:05 -0500 Subject: [PATCH 3/5] s/LinesToReflow/PositionInformantion/g --- src/buffer/out/textBuffer.cpp | 18 +++++++++--------- src/buffer/out/textBuffer.hpp | 4 ++-- src/cascadia/TerminalCore/Terminal.cpp | 8 ++++---- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 523ad1aaff7..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. -// - oldRows - 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. +// - 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> oldRows) + std::optional> positionInfo) { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); @@ -1961,22 +1961,22 @@ 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 (oldRows.has_value()) + if (positionInfo.has_value()) { if (!foundOldMutable) { - if (iOldRow >= oldRows.value().get().mutableViewportTop) + if (iOldRow >= positionInfo.value().get().mutableViewportTop) { - oldRows.value().get().mutableViewportTop = newCursor.GetPosition().Y; + positionInfo.value().get().mutableViewportTop = newCursor.GetPosition().Y; foundOldMutable = true; } } if (!foundOldVisible) { - if (iOldRow >= oldRows.value().get().visibleViewportTop) + if (iOldRow >= positionInfo.value().get().visibleViewportTop) { - oldRows.value().get().visibleViewportTop = newCursor.GetPosition().Y; + positionInfo.value().get().visibleViewportTop = newCursor.GetPosition().Y; foundOldVisible = true; } } diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 3b6f43b7198..086eea214d6 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -161,7 +161,7 @@ class TextBuffer final const std::wstring_view fontFaceName, const COLORREF backgroundColor); - struct LinesToReflow + struct PositionInformation { short mutableViewportTop{ 0 }; short visibleViewportTop{ 0 }; @@ -170,7 +170,7 @@ class TextBuffer final static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport, - std::optional> oldRows); + std::optional> positionInfo); private: std::deque _storage; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index a01eb0747cf..3659f29fa41 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -201,8 +201,8 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting 0, // temporarily set size to 0 so it won't render. _buffer->GetRenderTarget()); - // Build a LinesToReflow to track the position of both the top of the - // mutable viewport and the top of the visible viewport in the new + // 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 @@ -211,11 +211,11 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting // * then new value of visibleViewportTop will be used to calculate the // new scrollOffsett in the new buffer, so that the visible lines on // the sceren remain roughly the same. - TextBuffer::LinesToReflow oldRows{ 0 }; + TextBuffer::PositionInformation oldRows{ 0 }; oldRows.mutableViewportTop = oldViewportTop; oldRows.visibleViewportTop = newVisibleTop; - std::optional oldViewStart{ oldViewportTop }; + const std::optional oldViewStart{ oldViewportTop }; RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), *newTextBuffer.get(), _mutableViewport, From b68c9583bf2d28f756799a134cd3be68478014c9 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Fri, 13 Mar 2020 15:18:02 -0700 Subject: [PATCH 4/5] Update src/cascadia/TerminalCore/Terminal.cpp Co-Authored-By: Carlos Zamora --- src/cascadia/TerminalCore/Terminal.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 3659f29fa41..3ae69868aba 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -208,7 +208,7 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting // 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). - // * then new value of visibleViewportTop will be used to calculate the + // * the new value of visibleViewportTop will be used to calculate the // new scrollOffsett in the new buffer, so that the visible lines on // the sceren remain roughly the same. TextBuffer::PositionInformation oldRows{ 0 }; From 546ede91ce6c553dafb6f6ca956cb6c5c56331ff Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Fri, 13 Mar 2020 15:18:10 -0700 Subject: [PATCH 5/5] Update src/cascadia/TerminalCore/Terminal.cpp Co-Authored-By: Carlos Zamora --- src/cascadia/TerminalCore/Terminal.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 3ae69868aba..865798339e5 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -210,7 +210,7 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting // 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 sceren remain roughly the same. + // the screen remain roughly the same. TextBuffer::PositionInformation oldRows{ 0 }; oldRows.mutableViewportTop = oldViewportTop; oldRows.visibleViewportTop = newVisibleTop;