From 41ef5554f5cb946c4a48e2b8db791265c7217fce Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 14 Apr 2022 21:33:34 -0500 Subject: [PATCH] Don't reflow the alt buffer on resize (#12719) VTE only rewraps the contents of the (normal screen + its scrollback buffer) on a resize event. It doesn't rewrap the contents of the alternate screen. The alternate screen is used by applications which repaint it after a resize event. So, it doesn't really matter. However, in that short time window, after resizing the terminal but before the application catches up, this prevents vertical lines It was really hard to get a gif of this where it happened and was small enough to upload to GH, but there is one in #12719. There's something in this branch that fixes a scrolling issue in the parent PR. I'm partially filing this so I can look at the diffs here and try and figure out what that is. I kinda want to just take all 3 alt buffer PRs as a single atomic unit, but splitting them up made sense from a review standpoint. Closes #3493 --- src/cascadia/TerminalCore/Terminal.cpp | 152 ++++++++-------- src/cascadia/TerminalCore/Terminal.hpp | 3 + src/cascadia/TerminalCore/TerminalApi.cpp | 11 +- .../TerminalCore/TerminalSelection.cpp | 8 +- .../ControlInteractivityTests.cpp | 168 ++++++++++++++++++ .../ConptyRoundtripTests.cpp | 53 ++++++ src/host/screenInfo.cpp | 112 +++++++++--- src/host/screenInfo.hpp | 4 + src/host/ut_host/ScreenBufferTests.cpp | 113 ++++++++++++ src/renderer/vt/vtrenderer.hpp | 2 + 10 files changed, 513 insertions(+), 113 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 6ebaa29eb59..90928513fce 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -228,12 +228,38 @@ void Terminal::UpdateAppearance(const ICoreAppearance& appearance) // appropriate HRESULT for failing to resize. [[nodiscard]] HRESULT Terminal::UserResize(const COORD viewportSize) noexcept { - const auto oldDimensions = _mutableViewport.Dimensions(); + const auto oldDimensions = _GetMutableViewport().Dimensions(); if (viewportSize == oldDimensions) { return S_FALSE; } + // Shortcut: if we're in the alt buffer, just resize the + // alt buffer and put off resizing the main buffer till we switch back. Fortunately, this is easy. We don't need to + // worry about the viewport and scrollback at all! The alt buffer never has + // any scrollback, so we just need to resize it and presto, we're done. + if (_inAltBuffer()) + { + // stash this resize for the future. + _deferredResize = til::size{ viewportSize }; + + _altBuffer->GetCursor().StartDeferDrawing(); + // we're capturing `this` here because when we exit, we want to EndDefer on the (newly created) active buffer. + auto endDefer = wil::scope_exit([this]() noexcept { _altBuffer->GetCursor().EndDeferDrawing(); }); + + // GH#3494: We don't need to reflow the alt buffer. Apps that use the + // alt buffer will redraw themselves. This prevents graphical artifacts. + // + // This is consistent with VTE + RETURN_IF_FAILED(_altBuffer->ResizeTraditional(viewportSize)); + + // Since the _mutableViewport is no longer the size of the actual + // viewport, then update our _altBufferSize tracker we're using to help + // us out here. + _altBufferSize = til::size{ viewportSize }; + return S_OK; + } + const auto dx = ::base::ClampSub(viewportSize.X, oldDimensions.X); const short newBufferHeight = ::base::ClampAdd(viewportSize.Y, _scrollbackLines); @@ -415,48 +441,6 @@ void Terminal::UpdateAppearance(const ICoreAppearance& appearance) // before, and shouldn't be now either. _scrollOffset = originalOffsetWasZero ? 0 : static_cast(::base::ClampSub(_mutableViewport.Top(), newVisibleTop)); - // Now that we've finished the hard work of resizing the main buffer and - // getting the viewport back into the right spot, we need to ALSO resize the - // alt buffer, if one exists. Fortunately, this is easy. We don't need to - // worry about the viewport and scrollback at all! The alt buffer never has - // any scrollback, so we just need to resize it and presto, we're done. - if (_inAltBuffer()) - { - _altBuffer->GetCursor().StartDeferDrawing(); - // we're capturing `this` here because when we exit, we want to EndDefer on the (newly created) active buffer. - auto endDefer = wil::scope_exit([this]() noexcept { _altBuffer->GetCursor().EndDeferDrawing(); }); - - // First allocate a new text buffer to take the place of the current one. - std::unique_ptr newTextBuffer; - try - { - // GH#3848 - Stash away the current attributes - const auto oldBufferAttributes = _altBuffer->GetCurrentAttributes(); - newTextBuffer = std::make_unique(bufferSize, - TextAttribute{}, - 0, // temporarily set size to 0 so it won't render. - true, - _mainBuffer->GetRenderer()); - - // start defer drawing on the new buffer - newTextBuffer->GetCursor().StartDeferDrawing(); - - // We don't need any fancy position information. We're just gonna - // resize the buffer, it's gonna be in exactly the place it is now. - // There's no scrollback to worry about! - - RETURN_IF_FAILED(TextBuffer::Reflow(*_altBuffer.get(), - *newTextBuffer.get(), - _GetMutableViewport(), - std::nullopt)); - - // Restore the active text attributes - newTextBuffer->SetCurrentAttributes(oldBufferAttributes); - _altBuffer.swap(newTextBuffer); - } - CATCH_RETURN(); - } - // GH#5029 - make sure to InvalidateAll here, so that we'll paint the entire visible viewport. try { @@ -723,7 +707,7 @@ bool Terminal::SendMouseEvent(const COORD viewportPos, const unsigned int uiButt // clamp them to be within the range [(0, 0), (W, H)]. #pragma warning(suppress : 26496) // analysis can't tell we're assigning through a reference below auto clampedPos{ viewportPos }; - _mutableViewport.ToOrigin().Clamp(clampedPos); + _GetMutableViewport().ToOrigin().Clamp(clampedPos); return _terminalInput->HandleMouse(clampedPos, uiButton, GET_KEYSTATE_WPARAM(states.Value()), wheelDelta, state); } @@ -951,7 +935,10 @@ WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept Viewport Terminal::_GetMutableViewport() const noexcept { - return _inAltBuffer() ? Viewport::FromDimensions(_mutableViewport.Dimensions()) : + // GH#3493: if we're in the alt buffer, then it's possible that the mutable + // viewport's size hasn't been updated yet. In that case, use the + // temporarily stashed _altBufferSize instead. + return _inAltBuffer() ? Viewport::FromDimensions(_altBufferSize.to_win32_coord()) : _mutableViewport; } @@ -968,7 +955,7 @@ int Terminal::ViewStartIndex() const noexcept int Terminal::ViewEndIndex() const noexcept { - return _inAltBuffer() ? _mutableViewport.Height() - 1 : _mutableViewport.BottomInclusive(); + return _inAltBuffer() ? _altBufferSize.height : _mutableViewport.BottomInclusive(); } // _VisibleStartIndex is the first visible line of the buffer @@ -986,9 +973,14 @@ int Terminal::_VisibleEndIndex() const noexcept Viewport Terminal::_GetVisibleViewport() const noexcept { + // GH#3493: if we're in the alt buffer, then it's possible that the mutable + // viewport's size hasn't been updated yet. In that case, use the + // temporarily stashed _altBufferSize instead. const COORD origin{ 0, gsl::narrow(_VisibleStartIndex()) }; + const COORD size{ _inAltBuffer() ? _altBufferSize.to_win32_coord() : + _mutableViewport.Dimensions() }; return Viewport::FromDimensions(origin, - _mutableViewport.Dimensions()); + size); } // Writes a string of text to the buffer, then moves the cursor (and viewport) @@ -1121,46 +1113,50 @@ void Terminal::_AdjustCursorPosition(const COORD proposedPosition) cursor.SetPosition(proposedCursorPosition); // Move the viewport down if the cursor moved below the viewport. - bool updatedViewport = false; - const auto scrollAmount = std::max(0, proposedCursorPosition.Y - _mutableViewport.BottomInclusive()); - if (scrollAmount > 0) + // Obviously, don't need to do this in the alt buffer. + if (!_inAltBuffer()) { - const auto newViewTop = std::max(0, proposedCursorPosition.Y - (_mutableViewport.Height() - 1)); - // In the alt buffer, we never need to adjust _mutableViewport, which is the viewport of the main buffer. - if (!_inAltBuffer() && newViewTop != _mutableViewport.Top()) + bool updatedViewport = false; + const auto scrollAmount = std::max(0, proposedCursorPosition.Y - _mutableViewport.BottomInclusive()); + if (scrollAmount > 0) { - _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow(newViewTop) }, - _mutableViewport.Dimensions()); - updatedViewport = true; + const auto newViewTop = std::max(0, proposedCursorPosition.Y - (_mutableViewport.Height() - 1)); + // In the alt buffer, we never need to adjust _mutableViewport, which is the viewport of the main buffer. + if (newViewTop != _mutableViewport.Top()) + { + _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow(newViewTop) }, + _mutableViewport.Dimensions()); + updatedViewport = true; + } } - } - // If the viewport moved, or we circled the buffer, we might need to update - // our _scrollOffset - if (!_inAltBuffer() && (updatedViewport || newRows != 0)) - { - const auto oldScrollOffset = _scrollOffset; + // If the viewport moved, or we circled the buffer, we might need to update + // our _scrollOffset + if (updatedViewport || newRows != 0) + { + const auto oldScrollOffset = _scrollOffset; - // scroll if... - // - no selection is active - // - viewport is already at the bottom - const bool scrollToOutput = !IsSelectionActive() && _scrollOffset == 0; + // scroll if... + // - no selection is active + // - viewport is already at the bottom + const bool scrollToOutput = !IsSelectionActive() && _scrollOffset == 0; - _scrollOffset = scrollToOutput ? 0 : _scrollOffset + scrollAmount + newRows; + _scrollOffset = scrollToOutput ? 0 : _scrollOffset + scrollAmount + newRows; - // Clamp the range to make sure that we don't scroll way off the top of the buffer - _scrollOffset = std::clamp(_scrollOffset, - 0, - _activeBuffer().GetSize().Height() - _mutableViewport.Height()); + // Clamp the range to make sure that we don't scroll way off the top of the buffer + _scrollOffset = std::clamp(_scrollOffset, + 0, + _activeBuffer().GetSize().Height() - _mutableViewport.Height()); - // If the new scroll offset is different, then we'll still want to raise a scroll event - updatedViewport = updatedViewport || (oldScrollOffset != _scrollOffset); - } + // If the new scroll offset is different, then we'll still want to raise a scroll event + updatedViewport = updatedViewport || (oldScrollOffset != _scrollOffset); + } - // If the viewport moved, then send a scrolling notification. - if (updatedViewport) - { - _NotifyScrollEvent(); + // If the viewport moved, then send a scrolling notification. + if (updatedViewport) + { + _NotifyScrollEvent(); + } } if (rowsPushedOffTopOfBuffer != 0) diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 03690e63781..df882f3eed0 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -328,6 +328,9 @@ class Microsoft::Terminal::Core::Terminal final : SHORT _scrollbackLines; bool _detectURLs{ false }; + til::size _altBufferSize; + std::optional _deferredResize{ std::nullopt }; + // _scrollOffset is the number of lines above the viewport that are currently visible // If _scrollOffset is 0, then the visible region of the buffer is the viewport. int _scrollOffset; diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index 7550332c92c..5b9dfe8e8ec 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -598,14 +598,15 @@ void Terminal::PopGraphicsRendition() void Terminal::UseAlternateScreenBuffer() { // the new alt buffer is exactly the size of the viewport. - const COORD bufferSize{ _mutableViewport.Dimensions() }; + _altBufferSize = til::size{ _mutableViewport.Dimensions() }; + const auto cursorSize = _mainBuffer->GetCursor().GetSize(); ClearSelection(); _mainBuffer->ClearPatternRecognizers(); // Create a new alt buffer - _altBuffer = std::make_unique(bufferSize, + _altBuffer = std::make_unique(_altBufferSize.to_win32_coord(), TextAttribute{}, cursorSize, true, @@ -674,6 +675,12 @@ void Terminal::UseMainScreenBuffer() // destroy the alt buffer _altBuffer = nullptr; + if (_deferredResize.has_value()) + { + LOG_IF_FAILED(UserResize(_deferredResize.value().to_win32_coord())); + _deferredResize = std::nullopt; + } + // update all the hyperlinks on the screen _mainBuffer->ClearPatternRecognizers(); _updateUrlDetection(); diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 2a29b109eb0..56a42b639c0 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -426,15 +426,15 @@ void Terminal::_MoveByViewport(SelectionDirection direction, COORD& pos) break; case SelectionDirection::Up: { - const auto viewportHeight{ _mutableViewport.Height() }; + const auto viewportHeight{ _GetMutableViewport().Height() }; const auto newY{ base::ClampSub(pos.Y, viewportHeight) }; pos = newY < bufferSize.Top() ? bufferSize.Origin() : COORD{ pos.X, newY }; break; } case SelectionDirection::Down: { - const auto viewportHeight{ _mutableViewport.Height() }; - const auto mutableBottom{ _mutableViewport.BottomInclusive() }; + const auto viewportHeight{ _GetMutableViewport().Height() }; + const auto mutableBottom{ _GetMutableViewport().BottomInclusive() }; const auto newY{ base::ClampAdd(pos.Y, viewportHeight) }; pos = newY > mutableBottom ? COORD{ bufferSize.RightInclusive(), mutableBottom } : COORD{ pos.X, newY }; break; @@ -453,7 +453,7 @@ void Terminal::_MoveByBuffer(SelectionDirection direction, COORD& pos) break; case SelectionDirection::Right: case SelectionDirection::Down: - pos = { bufferSize.RightInclusive(), _mutableViewport.BottomInclusive() }; + pos = { bufferSize.RightInclusive(), _GetMutableViewport().BottomInclusive() }; break; } } diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index 0da22765c15..348336281cc 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -4,6 +4,8 @@ #include "pch.h" #include "../TerminalControl/EventArgs.h" #include "../TerminalControl/ControlInteractivity.h" + +#include "../../inc/TestUtils.h" #include "MockControlSettings.h" #include "MockConnection.h" @@ -38,6 +40,9 @@ namespace ControlUnitTests TEST_METHOD(PointerClickOutsideActiveRegion); TEST_METHOD(IncrementCircularBufferWithSelection); + TEST_METHOD(GetMouseEventsInTest); + TEST_METHOD(AltBufferClampMouse); + TEST_CLASS_SETUP(ClassSetup) { winrt::init_apartment(winrt::apartment_type::single_threaded); @@ -90,6 +95,25 @@ namespace ControlUnitTests VERIFY_ARE_EQUAL(20, core->_terminal->GetViewport().Height()); interactivity->Initialize(); } + + // Returns a scope_exit callback that should be used to ensure all + // output is drained. + auto _addInputCallback(const winrt::com_ptr& conn, + std::deque& expectedOutput) + { + conn->TerminalOutput([&](const hstring& hstr) { + VERIFY_IS_GREATER_THAN(expectedOutput.size(), 0u); + const auto expected = expectedOutput.front(); + expectedOutput.pop_front(); + Log::Comment(fmt::format(L"Received: \"{}\"", TerminalCoreUnitTests::TestUtils::ReplaceEscapes(hstr.c_str())).c_str()); + Log::Comment(fmt::format(L"Expected: \"{}\"", TerminalCoreUnitTests::TestUtils::ReplaceEscapes(expected)).c_str()); + VERIFY_ARE_EQUAL(expected, hstr); + }); + + return std::move(wil::scope_exit([&]() { + VERIFY_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output"); + })); + } }; void ControlInteractivityTests::TestAdjustAcrylic() @@ -798,4 +822,148 @@ namespace ControlUnitTests // Verify that the selection got reset VERIFY_IS_FALSE(core->HasSelection()); } + + void ControlInteractivityTests::GetMouseEventsInTest() + { + // This is just a simple case that proves you can test mouse events + // generated by the terminal + WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{}; + + auto [settings, conn] = _createSettingsAndConnection(); + auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn); + _standardInit(core, interactivity); + + std::deque expectedOutput{}; + auto validateDrained = _addInputCallback(conn, expectedOutput); + + Log::Comment(L"Enable mouse mode"); + auto& term{ *core->_terminal }; + term.Write(L"\x1b[?1000h"); + + Log::Comment(L"Click on the terminal"); + + expectedOutput.push_back(L"\x1b[M &&"); + // For this test, don't use any modifiers + const auto modifiers = ControlKeyStates(); + const Control::MouseButtonState leftMouseDown{ Control::MouseButtonState::IsLeftButtonDown }; + const til::size fontSize{ 9, 21 }; + const til::point terminalPosition0{ 5, 5 }; + const til::point cursorPosition0{ terminalPosition0 * fontSize }; + interactivity->PointerPressed(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + 0, // timestamp + modifiers, + cursorPosition0.to_core_point()); + } + + void ControlInteractivityTests::AltBufferClampMouse() + { + // This is a test for + // * GH#10642 + // * a comment in GH#12719 + WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{}; + + auto [settings, conn] = _createSettingsAndConnection(); + auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn); + _standardInit(core, interactivity); + auto& term{ *core->_terminal }; + + // Output enough text for view to start scrolling + for (int i = 0; i < core->ViewHeight() * 2; ++i) + { + conn->WriteInput(L"Foo\r\n"); + } + + // Start checking output + std::deque expectedOutput{}; + auto validateDrained = _addInputCallback(conn, expectedOutput); + + const auto originalViewport{ term.GetViewport() }; + VERIFY_ARE_EQUAL(originalViewport.Width(), 30); + + Log::Comment(L" --- Enable mouse mode ---"); + term.Write(L"\x1b[?1000h"); + + Log::Comment(L" --- Click on the terminal ---"); + // Recall: + // + // > ! specifies the value 1. The upper left character position on + // > the terminal is denoted as 1,1 + // + // So 5 in our buffer is 32+5+1 = '&' + expectedOutput.push_back(L"\x1b[M &&"); + // For this test, don't use any modifiers + const auto modifiers = ControlKeyStates(); + const Control::MouseButtonState leftMouseDown{ Control::MouseButtonState::IsLeftButtonDown }; + const til::size fontSize{ 9, 21 }; + const til::point terminalPosition0{ 5, 5 }; + const til::point cursorPosition0{ terminalPosition0 * fontSize }; + interactivity->PointerPressed(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + 0, // timestamp + modifiers, + cursorPosition0.to_core_point()); + VERIFY_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output"); + + // These first two bits are a test for GH#10642 + Log::Comment(L" --- Click on the terminal outside the width of the mutable viewport, see that it's clamped to the viewport ---"); + // Not actually possible, but for validation. + const til::point terminalPosition1{ originalViewport.Width() + 5, 5 }; + const til::point cursorPosition1{ terminalPosition1 * fontSize }; + + // The viewport is only 30 wide, so clamping 35 to the buffer size gets + // us 29, which converted is (32 + 29 + 1) = 62 = '>' + expectedOutput.push_back(L"\x1b[M >&"); + interactivity->PointerPressed(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + 0, // timestamp + modifiers, + cursorPosition1.to_core_point()); + VERIFY_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output"); + + Log::Comment(L" --- Scroll up, click the terminal. We shouldn't get any event. ---"); + core->UserScrollViewport(10); + VERIFY_IS_GREATER_THAN(core->ScrollOffset(), 0); + + // Viewport is now above the mutable viewport, so the mouse event + // straight up won't be sent to the terminal. + + expectedOutput.push_back(L"sentinel"); // Clearly, it won't be this string + interactivity->PointerPressed(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + 0, // timestamp + modifiers, + cursorPosition0.to_core_point()); + // Flush it out. + conn->WriteInput(L"sentinel"); + VERIFY_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output"); + + // This is the part as mentioned in GH#12719 + Log::Comment(L" --- Switch to alt buffer ---"); + term.Write(L"\x1b[?1049h"); + auto returnToMain = wil::scope_exit([&]() { term.Write(L"\x1b[?1049h"); }); + + VERIFY_ARE_EQUAL(0, core->ScrollOffset()); + Log::Comment(L" --- Click on a spot that's still outside the buffer ---"); + expectedOutput.push_back(L"\x1b[M >&"); + interactivity->PointerPressed(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + 0, // timestamp + modifiers, + cursorPosition1.to_core_point()); + + Log::Comment(L" --- Resize the terminal to be 10 columns wider ---"); + const til::size newSizeInDips{ til::size{ 40, 20 } * fontSize }; + core->SizeChanged(newSizeInDips.width, newSizeInDips.height); + + Log::Comment(L" --- Click on a spot that's NOW INSIDE the buffer ---"); + // (32 + 35 + 1) = 68 = 'D' + expectedOutput.push_back(L"\x1b[M D&"); + interactivity->PointerPressed(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + 0, // timestamp + modifiers, + cursorPosition1.to_core_point()); + VERIFY_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output"); + } } diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 0ac3822d4e0..b9c162ddda0 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -225,6 +225,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(SimpleAltBufferTest); TEST_METHOD(AltBufferToAltBufferTest); + TEST_METHOD(AltBufferResizeCrash); + private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -3864,6 +3866,8 @@ void ConptyRoundtripTests::SimpleAltBufferTest() gci.LockConsole(); // Lock must be taken to manipulate alt/main buffer state. auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); sm.ProcessString(L"\x1b[?1049h"); + // Don't leave ourselves in the alt buffer - that'll pollute other tests. + auto leaveAltBuffer = wil::scope_exit([&] { sm.ProcessString(L"\x1b[?1049l"); }); Log::Comment(L"Painting the frame"); VERIFY_SUCCEEDED(renderer.PaintFrame()); @@ -4023,6 +4027,8 @@ void ConptyRoundtripTests::AltBufferToAltBufferTest() gci.LockConsole(); // Lock must be taken to manipulate alt/main buffer state. auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); sm.ProcessString(L"\x1b[?1049h"); + // Don't leave ourselves in the alt buffer - that'll pollute other tests. + auto leaveAltBuffer = wil::scope_exit([&] { sm.ProcessString(L"\x1b[?1049l"); }); Log::Comment(L"Painting the frame"); VERIFY_SUCCEEDED(renderer.PaintFrame()); @@ -4069,3 +4075,50 @@ void ConptyRoundtripTests::AltBufferToAltBufferTest() Log::Comment(L"========== Checking the terminal buffer state (StillInAltBuffer) =========="); verifyBuffer(*termAltTb, til::rect{ term->_GetMutableViewport().ToInclusive() }, Frame::StillInAltBuffer); } + +void ConptyRoundtripTests::AltBufferResizeCrash() +{ + Log::Comment(L"During the review for GH#12719, it was noticed that this " + L"particular combination of resizing could crash the terminal." + L" This test makes sure we don't."); + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& sm = si.GetStateMachine(); + + gci.LockConsole(); // Lock must be taken to manipulate alt/main buffer state. + auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); + + _flushFirstFrame(); + + _checkConptyOutput = false; + + // Don't leave ourselves in the alt buffer - that'll pollute other tests. + auto leaveAltBuffer = wil::scope_exit([&] { sm.ProcessString(L"\x1b[?1049l"); }); + + // Note: we really don't care about the output in this test. We could, but + // mostly we care to ensure we don't crash. If we make it through this test, + // then it's a success. + + Log::Comment(L"========== Resize to 132x24 =========="); + sm.ProcessString(L"\x1b[8;24;132t"); + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Move cursor to (99,11) =========="); + sm.ProcessString(L"\x1b[12;100H"); + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Switch to the alt buffer =========="); + sm.ProcessString(L"\x1b[?1049h"); + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Resize to 80x10 =========="); + sm.ProcessString(L"\x1b[8;10;80t"); + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); +} diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index c1f8d90ec57..0fc2d70e85f 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -723,9 +723,8 @@ void SCREEN_INFORMATION::SetViewportSize(const COORD* const pcoordSize) if (_psiMainBuffer) { - const auto bufferSize = GetBufferSize().Dimensions(); - - _psiMainBuffer->SetViewportSize(&bufferSize); + _psiMainBuffer->_fAltWindowChanged = false; + _psiMainBuffer->_deferredPtyResize = til::size{ GetBufferSize().Dimensions() }; } } _InternalSetViewportSize(pcoordSize, false, false); @@ -1195,20 +1194,6 @@ void SCREEN_INFORMATION::_InternalSetViewportSize(const COORD* const pcoordSize, // Adjust the top of the window instead of the bottom // (so the lines slide upward) srNewViewport.Top -= DeltaY; - - // If we happened to move the top of the window past - // the 0th row (first row in the buffer) - if (srNewViewport.Top < 0) - { - // Find the amount we went past 0, correct the top - // of the window back to 0, and instead adjust the - // bottom even though it will cause us to lose the - // prompt line. - const short cRemainder = 0 - srNewViewport.Top; - srNewViewport.Top += cRemainder; - FAIL_FAST_IF(!(srNewViewport.Top == 0)); - srNewViewport.Bottom += cRemainder; - } } else { @@ -1245,7 +1230,14 @@ void SCREEN_INFORMATION::_InternalSetViewportSize(const COORD* const pcoordSize, srNewViewport.Right -= offRightDelta; srNewViewport.Left = std::max(0, srNewViewport.Left - offRightDelta); } - srNewViewport.Bottom = std::min(srNewViewport.Bottom, gsl::narrow(coordScreenBufferSize.Y - 1)); + const SHORT offBottomDelta = srNewViewport.Bottom - (coordScreenBufferSize.Y - 1); + if (offBottomDelta > 0) // the viewport was off the right of the buffer... + { + // ...so slide both top/bottom back into the buffer. This will prevent us + // from having a negative width later. + srNewViewport.Bottom -= offBottomDelta; + srNewViewport.Top = std::max(0, srNewViewport.Top - offBottomDelta); + } // See MSFT:19917443 // If we're in terminal scrolling mode, and we've changed the height of the @@ -1494,6 +1486,8 @@ bool SCREEN_INFORMATION::IsMaximizedY() const // - Success if successful. Invalid parameter if screen buffer size is unexpected. No memory if allocation failed. [[nodiscard]] NTSTATUS SCREEN_INFORMATION::ResizeTraditional(const COORD coordNewScreenSize) { + _textBuffer->GetCursor().StartDeferDrawing(); + auto endDefer = wil::scope_exit([&]() noexcept { _textBuffer->GetCursor().EndDeferDrawing(); }); return NTSTATUS_FROM_HRESULT(_textBuffer->ResizeTraditional(coordNewScreenSize)); } @@ -1537,7 +1531,15 @@ bool SCREEN_INFORMATION::IsMaximizedY() const CommandLine::Instance().EndAllPopups(); const bool fWrapText = gci.GetWrapText(); - if (fWrapText) + // GH#3493: Don't reflow the alt buffer. + // + // VTE only rewraps the contents of the (normal screen + its scrollback + // buffer) on a resize event. It doesn't rewrap the contents of the + // alternate screen. The alternate screen is used by applications which + // repaint it after a resize event. So it doesn't really matter. However, in + // that short time window, after resizing the terminal but before the + // application catches up, this prevents vertical lines + if (fWrapText && !_IsAltBuffer()) { status = ResizeWithReflow(coordNewScreenSize); } @@ -1911,6 +1913,66 @@ const SCREEN_INFORMATION& SCREEN_INFORMATION::GetMainBuffer() const return Status; } +// Function Description: +// - Handle deferred resizes that may have happened while the alt buffer was +// active. Both resizes on the HWND itself (_fAltWindowChanged), and resizes +// to the viewport of the alt buffer (which in turn should resize the buffer), +// are handled here. +// Arguments: +// - siMain: the main buffer whose resize was deferred. +// Return Value: +// - +void SCREEN_INFORMATION::_handleDeferredResize(SCREEN_INFORMATION& siMain) +{ + if (siMain._fAltWindowChanged) + { + siMain.ProcessResizeWindow(&(siMain._rcAltSavedClientNew), &(siMain._rcAltSavedClientOld)); + siMain._fAltWindowChanged = false; + } + else if (siMain._deferredPtyResize.has_value()) + { + const COORD newViewSize = siMain._deferredPtyResize.value().to_win32_coord(); + // Tricky! We want to make sure to resize the actual main buffer + // here, not just change the size of the viewport. When they resized + // the alt buffer, the dimensions of the alt buffer changed. We + // should make sure the main buffer reflects similar changes. + // + // To do this, we have to emulate bits of + // ConhostInternalGetSet::ResizeWindow. We can't call that + // straight-up, because the main buffer isn't active yet. + const COORD oldScreenBufferSize = siMain.GetBufferSize().Dimensions(); + COORD newBufferSize = oldScreenBufferSize; + + // Always resize the width of the console + newBufferSize.X = newViewSize.X; + + // Only set the new buffer's height if the new size will be TALLER than the current size (e.g., resizing a 80x32 buffer to be 80x124). + if (newViewSize.Y > oldScreenBufferSize.Y) + { + newBufferSize.Y = newViewSize.Y; + } + + // From ApiRoutines::SetConsoleScreenBufferInfoExImpl. We don't need + // the whole call to SetConsoleScreenBufferInfoEx here, that's + // too much work. + if (newBufferSize.X != oldScreenBufferSize.X || + newBufferSize.Y != oldScreenBufferSize.Y) + { + CommandLine& commandLine = CommandLine::Instance(); + commandLine.Hide(FALSE); + LOG_IF_FAILED(siMain.ResizeScreenBuffer(newBufferSize, TRUE)); + commandLine.Show(); + } + + // Not that the buffer is smaller, actually make sure to resize our + // viewport to match. + siMain.SetViewportSize(&newViewSize); + + // Clear out the resize. + siMain._deferredPtyResize = std::nullopt; + } +} + // Routine Description: // - Creates an "alternate" screen buffer for this buffer. In virtual terminals, there exists both a "main" // screen buffer and an alternate. ASBSET creates a new alternate, and switches to it. If there is an already @@ -1924,11 +1986,7 @@ const SCREEN_INFORMATION& SCREEN_INFORMATION::GetMainBuffer() const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); SCREEN_INFORMATION& siMain = GetMainBuffer(); // If we're in an alt that resized, resize the main before making the new alt - if (siMain._fAltWindowChanged) - { - siMain.ProcessResizeWindow(&(siMain._rcAltSavedClientNew), &(siMain._rcAltSavedClientOld)); - siMain._fAltWindowChanged = false; - } + _handleDeferredResize(siMain); SCREEN_INFORMATION* psiNewAltBuffer; NTSTATUS Status = _CreateAltBuffer(&psiNewAltBuffer); @@ -1981,11 +2039,7 @@ void SCREEN_INFORMATION::UseMainScreenBuffer() SCREEN_INFORMATION* psiMain = _psiMainBuffer; if (psiMain != nullptr) { - if (psiMain->_fAltWindowChanged) - { - psiMain->ProcessResizeWindow(&(psiMain->_rcAltSavedClientNew), &(psiMain->_rcAltSavedClientOld)); - psiMain->_fAltWindowChanged = false; - } + _handleDeferredResize(*psiMain); // GH#381: When we switch into the main buffer: // * flush the current frame, to clear out anything that we prepared for this buffer. diff --git a/src/host/screenInfo.hpp b/src/host/screenInfo.hpp index 2be6fc87ea8..cb1b7f7dad3 100644 --- a/src/host/screenInfo.hpp +++ b/src/host/screenInfo.hpp @@ -293,6 +293,10 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console bool _ignoreLegacyEquivalentVTAttributes; + std::optional _deferredPtyResize{ std::nullopt }; + + static void _handleDeferredResize(SCREEN_INFORMATION& siMain); + #ifdef UNIT_TESTING friend class TextBufferIteratorTests; friend class ScreenBufferTests; diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index f0e4684ebc6..cfd86c627c1 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -16,6 +16,7 @@ #include "../interactivity/inc/ServiceLocator.hpp" #include "../../inc/conattrs.hpp" #include "../../types/inc/Viewport.hpp" +#include "../../renderer/vt/Xterm256Engine.hpp" #include "../../inc/TestUtils.h" @@ -231,6 +232,8 @@ class ScreenBufferTests TEST_METHOD(TestReflowEndOfLineColor); TEST_METHOD(TestReflowSmallerLongLineWithColor); TEST_METHOD(TestReflowBiggerLongLineWithColor); + + TEST_METHOD(TestDeferredMainBufferResize); }; void ScreenBufferTests::SingleAlternateBufferCreationTest() @@ -6528,3 +6531,113 @@ void ScreenBufferTests::TestReflowBiggerLongLineWithColor() Log::Comment(L"========== Checking the buffer state (after) =========="); verifyBuffer(si.GetTextBuffer(), til::rect{ si.GetViewport().ToInclusive() }, false); } + +void ScreenBufferTests::TestDeferredMainBufferResize() +{ + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:inConpty", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:reEnterAltBuffer", L"{false, true}") + END_TEST_METHOD_PROPERTIES(); + + INIT_TEST_PROPERTY(bool, inConpty, L"Should we pretend to be in conpty mode?"); + INIT_TEST_PROPERTY(bool, reEnterAltBuffer, L"Should we re-enter the alt buffer when we're already in it?"); + + // A test for https://github.com/microsoft/terminal/pull/12719#discussion_r834860330 + Log::Comment(L"Resize the alt buffer. We should defer the resize of the " + L"main buffer till we swap back to it, for performance."); + + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + gci.LockConsole(); // Lock must be taken to manipulate buffer. + auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); + + // HUGELY cribbed from ConptyRoundtripTests::MethodSetup. This fakes the + // console into thinking that it's in ConPTY mode. Yes, we need all this + // just to get gci.IsInVtIoMode() to return true. The screen buffer gates + // all sorts of internal checks on that. + // + // This could theoretically be a helper if other tests need it. + if (inConpty) + { + Log::Comment(L"Set up ConPTY"); + + auto& currentBuffer = gci.GetActiveOutputBuffer(); + // Set up an xterm-256 renderer for conpty + wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); + Viewport initialViewport = currentBuffer.GetViewport(); + auto vtRenderEngine = std::make_unique(std::move(hFile), + initialViewport); + // We don't care about the output, so let it just drain to the void. + vtRenderEngine->SetTestCallback([](auto&&, auto&&) -> bool { return true; }); + gci.GetActiveOutputBuffer().SetTerminalConnection(vtRenderEngine.get()); + // Manually set the console into conpty mode. We're not actually going + // to set up the pipes for conpty, but we want the console to behave + // like it would in conpty mode. + ServiceLocator::LocateGlobals().EnableConptyModeForTests(std::move(vtRenderEngine)); + } + + auto* siMain = &gci.GetActiveOutputBuffer(); + auto& stateMachine = siMain->GetStateMachine(); + + const til::size oldSize{ siMain->GetBufferSize().Dimensions() }; + const til::size oldView{ siMain->GetViewport().Dimensions() }; + Log::Comment(NoThrowString().Format(L"Original buffer size: %dx%d", oldSize.width, oldSize.height)); + Log::Comment(NoThrowString().Format(L"Original viewport: %dx%d", oldView.width, oldView.height)); + VERIFY_ARE_NOT_EQUAL(oldSize, oldView); + + // printf "\x1b[?1049h" ; sleep 1 ; printf "\x1b[8;24;60t" ; sleep 1 ; printf "\x1b[?1049l" ; sleep 1 ; printf "\n" + Log::Comment(L"Switch to alt buffer"); + stateMachine.ProcessString(L"\x1b[?1049h"); + + auto* siAlt = &gci.GetActiveOutputBuffer(); + VERIFY_ARE_NOT_EQUAL(siMain, siAlt); + + const til::size newSize{ siAlt->GetBufferSize().Dimensions() }; + const til::size newView{ siAlt->GetViewport().Dimensions() }; + VERIFY_ARE_EQUAL(oldView, newSize); + VERIFY_ARE_EQUAL(oldView, newSize); + VERIFY_ARE_EQUAL(newView, newSize); + + Log::Comment(L"Resize alt buffer"); + stateMachine.ProcessString(L"\x1b[8;24;60t"); + const til::size expectedSize{ 60, 24 }; + const til::size altPostResizeSize{ siAlt->GetBufferSize().Dimensions() }; + const til::size altPostResizeView{ siAlt->GetViewport().Dimensions() }; + const til::size mainPostResizeSize{ siMain->GetBufferSize().Dimensions() }; + const til::size mainPostResizeView{ siMain->GetViewport().Dimensions() }; + VERIFY_ARE_NOT_EQUAL(oldView, altPostResizeSize); + VERIFY_ARE_EQUAL(altPostResizeView, altPostResizeSize); + VERIFY_ARE_EQUAL(expectedSize, altPostResizeSize); + + Log::Comment(L"Main buffer should not have resized yet."); + VERIFY_ARE_EQUAL(oldSize, mainPostResizeSize); + VERIFY_ARE_EQUAL(oldView, mainPostResizeView); + + if (reEnterAltBuffer) + { + Log::Comment(L"re-enter alt buffer"); + stateMachine.ProcessString(L"\x1b[?1049h"); + + auto* siAlt2 = &gci.GetActiveOutputBuffer(); + VERIFY_ARE_NOT_EQUAL(siMain, siAlt2); + VERIFY_ARE_NOT_EQUAL(siAlt, siAlt2); + + const til::size alt2Size{ siAlt2->GetBufferSize().Dimensions() }; + const til::size alt2View{ siAlt2->GetViewport().Dimensions() }; + VERIFY_ARE_EQUAL(altPostResizeSize, alt2Size); + VERIFY_ARE_EQUAL(altPostResizeView, alt2View); + } + + Log::Comment(L"Switch to main buffer"); + stateMachine.ProcessString(L"\x1b[?1049l"); + + auto* siFinal = &gci.GetActiveOutputBuffer(); + VERIFY_ARE_NOT_EQUAL(siAlt, siFinal); + VERIFY_ARE_EQUAL(siMain, siFinal); + + const til::size mainPostRestoreSize{ siMain->GetBufferSize().Dimensions() }; + const til::size mainPostRestoreView{ siMain->GetViewport().Dimensions() }; + VERIFY_ARE_NOT_EQUAL(oldView, mainPostRestoreView); + VERIFY_ARE_NOT_EQUAL(oldSize, mainPostRestoreSize); + VERIFY_ARE_EQUAL(altPostResizeView, mainPostRestoreView); + VERIFY_ARE_EQUAL(expectedSize, mainPostRestoreView); +} diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 0d1e36bb94b..cd8a3d5d739 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -27,6 +27,7 @@ namespace TerminalCoreUnitTests { class ConptyRoundtripTests; }; +class ScreenBufferTests; #endif namespace Microsoft::Console::VirtualTerminal @@ -227,6 +228,7 @@ namespace Microsoft::Console::Render friend class VtRendererTest; friend class ConptyOutputTests; + friend class ScreenBufferTests; friend class TerminalCoreUnitTests::ConptyRoundtripTests; #endif