Skip to content

Commit

Permalink
Maintain scrollbar position during a resize operation (#4903)
Browse files Browse the repository at this point in the history
## 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<short>&` 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<std::reference_wrapper<short>>`, 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
  • Loading branch information
zadjii-msft authored Mar 16, 2020
1 parent f5ab042 commit f1d3136
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 17 deletions.
31 changes: 22 additions & 9 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<Viewport> lastCharacterViewport,
std::optional<short>& oldViewportTop)
std::optional<std::reference_wrapper<PositionInformation>> positionInfo)
{
Cursor& oldCursor = oldBuffer.GetCursor();
Cursor& newCursor = newBuffer.GetCursor();
Expand All @@ -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++)
Expand Down Expand Up @@ -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;
}
}
}

Expand Down
8 changes: 7 additions & 1 deletion src/buffer/out/textBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<Microsoft::Console::Types::Viewport> lastCharacterViewport,
std::optional<short>& oldViewportTop);
std::optional<std::reference_wrapper<PositionInformation>> positionInfo);

private:
std::deque<ROW> _storage;
Expand Down
37 changes: 33 additions & 4 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<short>(_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<TextBuffer> newTextBuffer;
Expand All @@ -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<short> 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<short> 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();

Expand Down Expand Up @@ -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<short>(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;
Expand Down
4 changes: 1 addition & 3 deletions src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<short>& , which can't just be done inline with the call.
std::optional<short> 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))
{
Expand Down

0 comments on commit f1d3136

Please sign in to comment.