Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintain scrollbar position during a resize operation #4903

Merged
5 commits merged into from
Mar 16, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::optional<std::reference_wrapper<PositionInformation>> positionInfo);
std::optional<std::reference_wrapper<PositionInformation>> positionInfo = std::nullopt);

I think you can do it for the lastCharacterViewport parameter too.

Then the call you have in screenInfo doesn't have two std::nullopts passed through.

Up to you tho


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).
// * then new value of visibleViewportTop will be used to calculate the
DHowett-MSFT marked this conversation as resolved.
Show resolved Hide resolved
// new scrollOffsett in the new buffer, so that the visible lines on
// the sceren remain roughly the same.
DHowett-MSFT marked this conversation as resolved.
Show resolved Hide resolved
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