Skip to content

Commit

Permalink
Fix session restoration of full buffers (microsoft#17654)
Browse files Browse the repository at this point in the history
This removes the `Terminal::SetViewportPosition` call from session
restoration which was responsible for putting the viewport below
the buffer height and caused the renderer to fail.

In order to prevent such issues in the future, `SetViewportPosition`
now protects itself against out of bounds requests.

Closes microsoft#17639

## Validation Steps Performed
* Enable persistence
* Print `big.txt`
* Restart
* Looks good ✅
  • Loading branch information
lhecker authored Aug 13, 2024
1 parent 2478c64 commit 9074e9d
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 27 deletions.
30 changes: 6 additions & 24 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1850,33 +1850,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation

if (read < sizeof(buffer))
{
// Normally the cursor should already be at the start of the line, but let's be absolutely sure it is.
if (_terminal->GetCursorPosition().x != 0)
{
_terminal->Write(L"\r\n");
}
_terminal->Write(message);
break;
}
}

{
const auto lock = _terminal->LockForWriting();

// Normally the cursor should already be at the start of the line, but let's be absolutely sure it is.
if (_terminal->GetCursorPosition().x != 0)
{
_terminal->Write(L"\r\n");
}

_terminal->Write(message);

// Show 3 lines of scrollback to the user, so they know it's there. Otherwise, in particular with the well
// hidden touch scrollbars in WinUI 2 and later, there's no indication that there's something to scroll up to.
//
// We only show 3 lines because ConPTY doesn't know about our restored buffer contents initially,
// and so ReadConsole calls will return whitespace.
//
// We also avoid using actual newlines or similar here, because if we ever change our text buffer implementation
// to actually track the written contents, we don't want this to be part of the next buffer snapshot.
const auto cursorPosition = _terminal->GetCursorPosition();
const auto y = std::max(0, cursorPosition.y - 4);
_terminal->SetViewportPosition({ 0, y });
}
}

void ControlCore::_rendererWarning(const HRESULT hr, wil::zwstring_view parameter)
Expand Down
12 changes: 9 additions & 3 deletions src/cascadia/TerminalCore/TerminalApi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,21 @@ ITerminalApi::BufferState Terminal::GetBufferAndViewport() noexcept
return { _activeBuffer(), til::rect{ _GetMutableViewport().ToInclusive() }, !_inAltBuffer() };
}

void Terminal::SetViewportPosition(const til::point position) noexcept
void Terminal::SetViewportPosition(til::point position) noexcept
try
{
// The viewport is fixed at 0,0 for the alt buffer, so this is a no-op.
if (!_inAltBuffer())
{
const auto bufferSize = _mainBuffer->GetSize().Dimensions();
const auto viewSize = _GetMutableViewport().Dimensions();

// Ensure the given position is in bounds.
position.x = std::clamp(position.x, 0, bufferSize.width - viewSize.width);
position.y = std::clamp(position.y, 0, bufferSize.height - viewSize.height);

const auto viewportDelta = position.y - _GetMutableViewport().Origin().y;
const auto dimensions = _GetMutableViewport().Dimensions();
_mutableViewport = Viewport::FromDimensions(position, dimensions);
_mutableViewport = Viewport::FromDimensions(position, viewSize);
_PreserveUserScrollOffset(viewportDelta);
_NotifyScrollEvent();
}
Expand Down

0 comments on commit 9074e9d

Please sign in to comment.