From 25df52774347a5e22d2c417172ef2f00f27fc06d Mon Sep 17 00:00:00 2001 From: greg904 <56923875+greg904@users.noreply.github.com> Date: Fri, 12 Jun 2020 21:51:37 +0200 Subject: [PATCH] Throttle scrollbar updates in TermControl to ~one per 8ms (#4608) In addition to the below (original) description, this commit introduces a ThrottledFunc template that can throttle _any_ function. It applies that type to muffle updates to the scrollbar. --- Redo #3531 but without the bug that it caused (#3622) which is why it was reverted. I'm sorry if I explain this badly. If you don't understand a part, make sure to let me know and I will explain it better. ### Explanation How it worked before: `Terminal` signals that viewport changed -> `TermControl::_TerminalScrollPositionChanged` gets called on the terminal thread -> it dispatches work for later to be ran the UI thread to updates the scrollbar's values Why it's bad: * If we have many viewport changes, it will create a long stack of operations to run. Instead, we should just update the scroll bar with the most recent information that we know. * Imagine if the rate that the work gets pushed on the UI thread is greater than the rate that it can handle: it might freeze? * No need to be real time, we can wait just a little bit (8ms) to accumulate viewport changes before we actually change the scroll bar's value because it appears to be expensive (see perf below). Now: `Terminal` signals that viewport changed -> `TermControl::_TerminalScrollPositionChanged` gets called on the terminal thread -> it tells the `ScrollBarUpdater` about a new update -> the `ScrollBarUpdater` only runs one job (I don't know if that's the right term) on the UI thread at a time. If a job is already running but hasn't updated the scroll bar yet, it changes the setting in the already existing job to update the scroll bar with the new values. A job "waits" some time before doing the update to throttle updates because we don't need real time scroll bar updates. -> eventually, it updates the scroll bar If the user scrolls when a scroll bar update is pending, we keep the scroll bar's Maximum and Minimum but let the user choose its new Value with the `CancelPendingValueChange` method. ### Note Also I changed a little bit the code from the Terminal to notify the TermControl less often when possible. I tried to scroll with the scroll bar, with the mouse wheel. I tried to scroll while content is being outputted. I tried to reproduce the crash from #2248 without success (good). Co-authored-by: Leonard Hecker Closes #3622 --- .../actions/spell-check/dictionary/apis.txt | 1 + src/cascadia/TerminalControl/TermControl.cpp | 103 ++++----- src/cascadia/TerminalControl/TermControl.h | 15 +- .../TerminalControl/TerminalControl.vcxproj | 2 + .../TerminalControl.vcxproj.filters | 2 + .../TerminalControl/ThreadSafeOptional.h | 54 +++++ src/cascadia/TerminalControl/ThrottledFunc.h | 97 ++++++++ src/cascadia/TerminalCore/Terminal.cpp | 29 +-- src/cascadia/TerminalCore/Terminal.hpp | 4 +- .../UnitTests_TerminalCore/ScrollTest.cpp | 217 ++++++++++++++++++ .../TerminalAndRendererTests.cpp | 177 -------------- .../UnitTests_TerminalCore/UnitTests.vcxproj | 2 +- 12 files changed, 453 insertions(+), 250 deletions(-) create mode 100644 src/cascadia/TerminalControl/ThreadSafeOptional.h create mode 100644 src/cascadia/TerminalControl/ThrottledFunc.h create mode 100644 src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp delete mode 100644 src/cascadia/UnitTests_TerminalCore/TerminalAndRendererTests.cpp diff --git a/.github/actions/spell-check/dictionary/apis.txt b/.github/actions/spell-check/dictionary/apis.txt index ffb96e272f9..17b73e780d6 100644 --- a/.github/actions/spell-check/dictionary/apis.txt +++ b/.github/actions/spell-check/dictionary/apis.txt @@ -7,6 +7,7 @@ EXPCMDFLAGS EXPCMDSTATE fullkbd href +IAsync IBox IBind IClass diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index a02f0bdca3a..2a42a008ec0 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -27,6 +27,10 @@ using namespace winrt::Windows::System; using namespace winrt::Microsoft::Terminal::Settings; using namespace winrt::Windows::ApplicationModel::DataTransfer; +// The minimum delay between updates to the scroll bar's values. +// The updates are throttled to limit power usage. +constexpr const auto ScrollBarUpdateInterval = std::chrono::milliseconds(8); + namespace winrt::Microsoft::Terminal::TerminalControl::implementation { // Helper static function to ensure that all ambiguous-width glyphs are reported as narrow. @@ -58,7 +62,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _initializedTerminal{ false }, _settings{ settings }, _closing{ false }, - _isTerminalInitiatedScroll{ false }, + _isInternalScrollBarUpdate{ false }, _autoScrollVelocity{ 0 }, _autoScrollingPointerPoint{ std::nullopt }, _autoScrollTimer{}, @@ -120,6 +124,32 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation } }); + _updateScrollBar = std::make_shared>( + [weakThis = get_weak()](const auto& update) { + if (auto control{ weakThis.get() }) + { + control->Dispatcher() + .RunAsync(CoreDispatcherPriority::Normal, [=]() { + if (auto control2{ weakThis.get() }) + { + control2->_isInternalScrollBarUpdate = true; + + auto scrollBar = control2->ScrollBar(); + if (update.newValue.has_value()) + { + scrollBar.Value(update.newValue.value()); + } + scrollBar.Maximum(update.newMaximum); + scrollBar.Minimum(update.newMinimum); + scrollBar.ViewportSize(update.newViewportSize); + + control2->_isInternalScrollBarUpdate = false; + } + }); + } + }, + ScrollBarUpdateInterval); + static constexpr auto AutoScrollUpdateInterval = std::chrono::microseconds(static_cast(1.0 / 30.0 * 1000000)); _autoScrollTimer.Interval(AutoScrollUpdateInterval); _autoScrollTimer.Tick({ this, &TermControl::_UpdateAutoScroll }); @@ -214,7 +244,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { if (_closing) { - return; + co_return; } // Update our control settings @@ -1437,8 +1467,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void TermControl::_ScrollbarChangeHandler(Windows::Foundation::IInspectable const& /*sender*/, Controls::Primitives::RangeBaseValueChangedEventArgs const& args) { - if (_isTerminalInitiatedScroll || _closing) + if (_isInternalScrollBarUpdate || _closing) { + // The update comes from ourselves, more specifically from the + // terminal. So we don't have to update the terminal because it + // already knows. return; } @@ -1448,9 +1481,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // itself - it was initiated by the mouse wheel, or the scrollbar. _terminal->UserScrollViewport(newValue); - // We've just told the terminal to update its viewport to reflect the - // new scroll value so the scroll bar matches the viewport now. - _willUpdateScrollBarToMatchViewport.store(false); + // User input takes priority over terminal events so cancel + // any pending scroll bar update if the user scrolls. + _updateScrollBar->ModifyPending([](auto& update) { + update.newValue.reset(); + }); } // Method Description: @@ -1949,35 +1984,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _titleChangedHandlers(winrt::hstring{ wstr }); } - // Method Description: - // - Update the position and size of the scrollbar to match the given - // viewport top, viewport height, and buffer size. - // The change will be actually handled in _ScrollbarChangeHandler. - // This should be done on the UI thread. Make sure the caller is calling - // us in a RunAsync block. - // Arguments: - // - viewTop: the top of the visible viewport, in rows. 0 indicates the top - // of the buffer. - // - viewHeight: the height of the viewport in rows. - // - bufferSize: the length of the buffer, in rows - void TermControl::_ScrollbarUpdater(Controls::Primitives::ScrollBar scrollBar, - const int viewTop, - const int viewHeight, - const int bufferSize) - { - // The terminal is already in the scroll position it wants, so no need - // to tell it to scroll. - _isTerminalInitiatedScroll = true; - - const auto hiddenContent = bufferSize - viewHeight; - scrollBar.Maximum(hiddenContent); - scrollBar.Minimum(0); - scrollBar.ViewportSize(viewHeight); - scrollBar.Value(viewTop); - - _isTerminalInitiatedScroll = false; - } - // Method Description: // - Update the position and size of the scrollbar to match the given // viewport top, viewport height, and buffer size. @@ -1988,9 +1994,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // of the buffer. // - viewHeight: the height of the viewport in rows. // - bufferSize: the length of the buffer, in rows - winrt::fire_and_forget TermControl::_TerminalScrollPositionChanged(const int viewTop, - const int viewHeight, - const int bufferSize) + void TermControl::_TerminalScrollPositionChanged(const int viewTop, + const int viewHeight, + const int bufferSize) { // Since this callback fires from non-UI thread, we might be already // closed/closing. @@ -2001,21 +2007,14 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _scrollPositionChangedHandlers(viewTop, viewHeight, bufferSize); - auto weakThis{ get_weak() }; - - co_await winrt::resume_foreground(Dispatcher()); + ScrollBarUpdate update; + const auto hiddenContent = bufferSize - viewHeight; + update.newMaximum = hiddenContent; + update.newMinimum = 0; + update.newViewportSize = viewHeight; + update.newValue = viewTop; - // Even if we weren't closed/closing few lines above, we might be - // while waiting for this block of code to be dispatched. - // If 'weakThis' is locked, then we can safely work with 'this' - if (auto control{ weakThis.get() }) - { - if (!_closing.load()) - { - // Update our scrollbar - _ScrollbarUpdater(ScrollBar(), viewTop, viewHeight, bufferSize); - } - } + _updateScrollBar->Run(update); } // Method Description: diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 035100b6444..81f60e495c6 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -15,6 +15,7 @@ #include "../buffer/out/search.h" #include "cppwinrt_utils.h" #include "SearchBoxControl.h" +#include "ThrottledFunc.h" namespace winrt::Microsoft::Terminal::TerminalControl::implementation { @@ -135,8 +136,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation FontInfoDesired _desiredFont; FontInfo _actualFont; - bool _isTerminalInitiatedScroll; - std::atomic _willUpdateScrollBarToMatchViewport; + struct ScrollBarUpdate + { + std::optional newValue; + double newMaximum; + double newMinimum; + double newViewportSize; + }; + std::shared_ptr> _updateScrollBar; + bool _isInternalScrollBarUpdate; int _rowsToScroll; @@ -201,7 +209,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void _DoResizeUnderLock(const double newWidth, const double newHeight); void _RefreshSizeUnderLock(); void _TerminalTitleChanged(const std::wstring_view& wstr); - winrt::fire_and_forget _TerminalScrollPositionChanged(const int viewTop, const int viewHeight, const int bufferSize); + void _TerminalScrollPositionChanged(const int viewTop, const int viewHeight, const int bufferSize); winrt::fire_and_forget _TerminalCursorPositionChanged(); void _MouseScrollHandler(const double mouseDelta, const Windows::Foundation::Point point, const bool isLeftButtonPressed); @@ -216,7 +224,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void _TryStopAutoScroll(const uint32_t pointerId); void _UpdateAutoScroll(Windows::Foundation::IInspectable const& sender, Windows::Foundation::IInspectable const& e); - void _ScrollbarUpdater(Windows::UI::Xaml::Controls::Primitives::ScrollBar scrollbar, const int viewTop, const int viewHeight, const int bufferSize); static Windows::UI::Xaml::Thickness _ParseThicknessFromPadding(const hstring padding); void _KeyHandler(Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e, const bool keyDown); diff --git a/src/cascadia/TerminalControl/TerminalControl.vcxproj b/src/cascadia/TerminalControl/TerminalControl.vcxproj index 217d524c4dd..b6fc6f1643b 100644 --- a/src/cascadia/TerminalControl/TerminalControl.vcxproj +++ b/src/cascadia/TerminalControl/TerminalControl.vcxproj @@ -43,6 +43,8 @@ TermControlAutomationPeer.idl + + TSFInputControl.xaml diff --git a/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters b/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters index 004f3cafbb1..04b21717a90 100644 --- a/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters +++ b/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters @@ -26,6 +26,8 @@ + + diff --git a/src/cascadia/TerminalControl/ThreadSafeOptional.h b/src/cascadia/TerminalControl/ThreadSafeOptional.h new file mode 100644 index 00000000000..cca4d941a75 --- /dev/null +++ b/src/cascadia/TerminalControl/ThreadSafeOptional.h @@ -0,0 +1,54 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#pragma once +#include "pch.h" + +template +class ThreadSafeOptional +{ +public: + template + bool Emplace(Args&&... args) + { + std::lock_guard guard{ _lock }; + + bool hadValue = _inner.has_value(); + _inner.emplace(std::forward(args)...); + return !hadValue; + } + + std::optional Take() + { + std::lock_guard guard{ _lock }; + + std::optional value; + _inner.swap(value); + + return value; + } + + // Method Description: + // - If the optional has a value, then call the specified function with a + // reference to the value. + // - This method is always thread-safe. It can be called multiple times on + // different threads. + // Arguments: + // - f: the function to call with a reference to the value + // Return Value: + // - + template + void ModifyValue(F f) + { + std::lock_guard guard{ _lock }; + + if (_inner.has_value()) + { + f(_inner.value()); + } + } + +private: + std::mutex _lock; + std::optional _inner; +}; diff --git a/src/cascadia/TerminalControl/ThrottledFunc.h b/src/cascadia/TerminalControl/ThrottledFunc.h new file mode 100644 index 00000000000..3d02b3bba83 --- /dev/null +++ b/src/cascadia/TerminalControl/ThrottledFunc.h @@ -0,0 +1,97 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- ThrottledFunc.h + +Abstract: +- This module defines a class to throttle function calls. +- You create an instance of a `ThrottledFunc` with a function and the delay + between two function calls. +- The function takes an argument of type `T`, the template argument of + `ThrottledFunc`. +- Use the `Run` method to wait and then call the function. +--*/ + +#pragma once +#include "pch.h" + +#include "ThreadSafeOptional.h" + +template +class ThrottledFunc : public std::enable_shared_from_this> +{ +public: + using Func = std::function; + + ThrottledFunc(Func func, winrt::Windows::Foundation::TimeSpan delay) : + _func{ func }, + _delay{ delay } + { + } + + // Method Description: + // - Runs the function later with the specified argument, except if `Run` + // is called again before with a new argument, in which case the new + // argument will be instead. + // - For more information, read the "Abstract" section in the header file. + // Arguments: + // - arg: the argument to pass to the function + // Return Value: + // - + winrt::fire_and_forget Run(T arg) + { + if (!_pendingCallArg.Emplace(arg)) + { + // already pending + return; + } + + auto weakThis = this->weak_from_this(); + + co_await winrt::resume_after(_delay); + + if (auto self{ weakThis.lock() }) + { + auto arg = self->_pendingCallArg.Take(); + if (arg.has_value()) + { + self->_func(arg.value()); + } + else + { + // should not happen + } + } + } + + // Method Description: + // - Modifies the pending argument for the next function invocation, if + // there is one pending currently. + // - Let's say that you just called the `Run` method with argument A. + // After the delay specified in the constructor, the function R + // specified in the constructor will be called with argument A. + // - By using this method, you can modify argument A before the function R + // is called with argument A. + // - You pass a function to this method which will take a reference to + // argument A and will modify it. + // - When there is no pending invocation of function R, this method will + // not do anything. + // - This method is always thread-safe. It can be called multiple times on + // different threads. + // Arguments: + // - f: the function to call with a reference to the argument + // Return Value: + // - + template + void ModifyPending(F f) + { + _pendingCallArg.ModifyValue(f); + } + +private: + Func _func; + winrt::Windows::Foundation::TimeSpan _delay; + ThreadSafeOptional _pendingCallArg; +}; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 93cd0e1ae51..efd9d5f7770 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -781,48 +781,49 @@ void Terminal::_AdjustCursorPosition(const COORD proposedPosition) auto proposedCursorPosition = proposedPosition; auto& cursor = _buffer->GetCursor(); const Viewport bufferSize = _buffer->GetSize(); - bool notifyScroll = false; // If we're about to scroll past the bottom of the buffer, instead cycle the // buffer. - // GH#5540 - Make sure this is a positive number. We can't create a - // negative number of new rows. - const auto newRows = std::max(0, proposedCursorPosition.Y - bufferSize.Height() + 1); - if (newRows > 0) + SHORT rowsPushedOffTopOfBuffer = 0; + if (proposedCursorPosition.Y >= bufferSize.Height()) { + const auto newRows = proposedCursorPosition.Y - bufferSize.Height() + 1; for (auto dy = 0; dy < newRows; dy++) { _buffer->IncrementCircularBuffer(); proposedCursorPosition.Y--; + rowsPushedOffTopOfBuffer++; } - notifyScroll = true; } // Update Cursor Position cursor.SetPosition(proposedCursorPosition); - const COORD cursorPosAfter = cursor.GetPosition(); - // Move the viewport down if the cursor moved below the viewport. - if (cursorPosAfter.Y > _mutableViewport.BottomInclusive()) + bool updatedViewport = false; + if (proposedCursorPosition.Y > _mutableViewport.BottomInclusive()) { - const auto newViewTop = std::max(0, cursorPosAfter.Y - (_mutableViewport.Height() - 1)); + const auto newViewTop = std::max(0, proposedCursorPosition.Y - (_mutableViewport.Height() - 1)); if (newViewTop != _mutableViewport.Top()) { _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow(newViewTop) }, _mutableViewport.Dimensions()); - notifyScroll = true; + updatedViewport = true; } } - if (notifyScroll) + if (updatedViewport) + { + _NotifyScrollEvent(); + } + + if (rowsPushedOffTopOfBuffer != 0) { // We have to report the delta here because we might have circled the text buffer. // That didn't change the viewport and therefore the TriggerScroll(void) // method can't detect the delta on its own. - COORD delta{ 0, -gsl::narrow(newRows) }; + COORD delta{ 0, -rowsPushedOffTopOfBuffer }; _buffer->GetRenderTarget().TriggerScroll(&delta); - _NotifyScrollEvent(); } _NotifyTerminalCursorPositionChanged(); diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 7ece2ad718b..7fa047cb90a 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -36,7 +36,7 @@ namespace TerminalCoreUnitTests class TerminalBufferTests; class TerminalApiTest; class ConptyRoundtripTests; - class TerminalAndRendererTests; + class ScrollTest; }; #endif @@ -301,6 +301,6 @@ class Microsoft::Terminal::Core::Terminal final : friend class TerminalCoreUnitTests::TerminalBufferTests; friend class TerminalCoreUnitTests::TerminalApiTest; friend class TerminalCoreUnitTests::ConptyRoundtripTests; - friend class TerminalCoreUnitTests::TerminalAndRendererTests; + friend class TerminalCoreUnitTests::ScrollTest; #endif }; diff --git a/src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp b/src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp new file mode 100644 index 00000000000..bf786ea6ca2 --- /dev/null +++ b/src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp @@ -0,0 +1,217 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "precomp.h" +#include + +#include +#include + +#include "../renderer/inc/DummyRenderTarget.hpp" +#include "../renderer/base/Renderer.hpp" +#include "../renderer/dx/DxRenderer.hpp" + +#include "../cascadia/TerminalCore/Terminal.hpp" +#include "MockTermSettings.h" +#include "consoletaeftemplates.hpp" +#include "TestUtils.h" + +using namespace winrt::Microsoft::Terminal::Settings; +using namespace Microsoft::Terminal::Core; +using namespace ::Microsoft::Console::Types; + +using namespace WEX::Common; +using namespace WEX::Logging; +using namespace WEX::TestExecution; + +namespace +{ + class MockScrollRenderTarget final : public ::Microsoft::Console::Render::IRenderTarget + { + public: + ~MockScrollRenderTarget() override{}; + + std::optional TriggerScrollDelta() const + { + return _triggerScrollDelta; + } + + void Reset() + { + _triggerScrollDelta.reset(); + } + + virtual void TriggerRedraw(const Microsoft::Console::Types::Viewport&){}; + virtual void TriggerRedraw(const COORD* const){}; + virtual void TriggerRedrawCursor(const COORD* const){}; + virtual void TriggerRedrawAll(){}; + virtual void TriggerTeardown(){}; + virtual void TriggerSelection(){}; + virtual void TriggerScroll(){}; + virtual void TriggerScroll(const COORD* const delta) + { + _triggerScrollDelta = { *delta }; + }; + virtual void TriggerCircling(){}; + void TriggerTitleChange(){}; + + private: + std::optional _triggerScrollDelta; + }; + + struct ScrollBarNotification + { + int ViewportTop; + int ViewportHeight; + int BufferHeight; + }; +} + +namespace TerminalCoreUnitTests +{ + class ScrollTest; +}; +using namespace TerminalCoreUnitTests; + +class TerminalCoreUnitTests::ScrollTest final +{ + // !!! DANGER: Many tests in this class expect the Terminal buffer + // to be 80x32. If you change these, you'll probably inadvertently break a + // bunch of tests !!! + static const SHORT TerminalViewWidth = 80; + static const SHORT TerminalViewHeight = 32; + // For TestNotifyScrolling, it's important that this value is ~=9000. + // Something smaller like 100 won't cause the test to fail. + static const SHORT TerminalHistoryLength = 9001; + + TEST_CLASS(ScrollTest); + + TEST_METHOD(TestNotifyScrolling); + + TEST_METHOD_SETUP(MethodSetup) + { + _term = std::make_unique<::Microsoft::Terminal::Core::Terminal>(); + + _scrollBarNotification = std::make_shared>(); + _term->SetScrollPositionChangedCallback([scrollBarNotification = _scrollBarNotification](const int top, const int height, const int bottom) { + ScrollBarNotification tmp; + tmp.ViewportTop = top; + tmp.ViewportHeight = height; + tmp.BufferHeight = bottom; + *scrollBarNotification = { tmp }; + }); + + _renderTarget = std::make_unique(); + _term->Create({ TerminalViewWidth, TerminalViewHeight }, TerminalHistoryLength, *_renderTarget); + return true; + } + + TEST_METHOD_CLEANUP(MethodCleanup) + { + _term = nullptr; + return true; + } + +private: + std::unique_ptr _term; + std::unique_ptr _renderTarget; + std::shared_ptr> _scrollBarNotification; +}; + +void ScrollTest::TestNotifyScrolling() +{ + // See https://github.com/microsoft/terminal/pull/5630 + // + // This is a test for GH#5540, in the most bizarre way. The origin of that + // bug was that as newlines were emitted, we'd accumulate an enormous scroll + // delta into a selection region, to the point of overflowing a SHORT. When + // the overflow occurred, the Terminal would fail to send a NotifyScroll() to + // the TermControl hosting it. + // + // For this bug to repro, we need to: + // - Have a sufficiently large buffer, because each newline we'll accumulate + // a delta of (0, ~bufferHeight), so (bufferHeight^2 + bufferHeight) > + // SHRT_MAX + // - Have a selection + + Log::Comment(L"Watch out - this test takes a while to run, and won't " + L"output anything unless in encounters an error. This is expected."); + + auto& termTb = *_term->_buffer; + auto& termSm = *_term->_stateMachine; + + const auto totalBufferSize = termTb.GetSize().Height(); + + auto currentRow = 0; + + // We're outputting like 18000 lines of text here, so emitting 18000*4 lines + // of output to the console is actually quite unnecessary + WEX::TestExecution::SetVerifyOutput settings(WEX::TestExecution::VerifyOutputSettings::LogOnlyFailures); + + // Emit a bunch of newlines to test scrolling. + for (; currentRow < totalBufferSize * 2; currentRow++) + { + *_scrollBarNotification = std::nullopt; + _renderTarget->Reset(); + + termSm.ProcessString(L"X\r\n"); + + // When we're on TerminalViewHeight-1, we'll emit the newline that + // causes the first scroll event + bool scrolled = currentRow >= TerminalViewHeight - 1; + + // When we circle the buffer, the scroll bar's position does not + // change. + bool circledBuffer = currentRow >= totalBufferSize - 1; + bool expectScrollBarNotification = scrolled && !circledBuffer; + + if (expectScrollBarNotification) + { + VERIFY_IS_TRUE(_scrollBarNotification->has_value(), + fmt::format(L"Expected a 'scroll bar position changed' notification for row {}", currentRow).c_str()); + } + else + { + VERIFY_IS_FALSE(_scrollBarNotification->has_value(), + fmt::format(L"Expected to not see a 'scroll bar position changed' notification for row {}", currentRow).c_str()); + } + + // If we scrolled but it circled the buffer, then the terminal will + // call `TriggerScroll` with a delta to tell the renderer about it. + if (scrolled && circledBuffer) + { + VERIFY_IS_TRUE(_renderTarget->TriggerScrollDelta().has_value(), + fmt::format(L"Expected a 'trigger scroll' notification in RenderTarget for row {}", currentRow).c_str()); + + COORD expectedDelta; + expectedDelta.X = 0; + expectedDelta.Y = -1; + VERIFY_ARE_EQUAL(expectedDelta, _renderTarget->TriggerScrollDelta().value(), fmt::format(L"Wrong value in 'trigger scroll' notification in RenderTarget for row {}", currentRow).c_str()); + } + else + { + VERIFY_IS_FALSE(_renderTarget->TriggerScrollDelta().has_value(), + fmt::format(L"Expected to not see a 'trigger scroll' notification in RenderTarget for row {}", currentRow).c_str()); + } + + if (_scrollBarNotification->has_value()) + { + const auto tmp = _scrollBarNotification->value(); + + const int expectedTop = std::clamp(currentRow - TerminalViewHeight + 2, + 0, + TerminalHistoryLength); + const int expectedHeight = TerminalViewHeight; + const int expectedBottom = expectedTop + TerminalViewHeight; + if ((tmp.ViewportTop != expectedTop) || + (tmp.ViewportHeight != expectedHeight) || + (tmp.BufferHeight != expectedBottom)) + { + Log::Comment(NoThrowString().Format(L"Expected viewport values did not match on line %d", currentRow)); + } + VERIFY_ARE_EQUAL(tmp.ViewportTop, expectedTop); + VERIFY_ARE_EQUAL(tmp.ViewportHeight, expectedHeight); + VERIFY_ARE_EQUAL(tmp.BufferHeight, expectedBottom); + } + } +} diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalAndRendererTests.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalAndRendererTests.cpp deleted file mode 100644 index f8b82cc2b26..00000000000 --- a/src/cascadia/UnitTests_TerminalCore/TerminalAndRendererTests.cpp +++ /dev/null @@ -1,177 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. -// -// This test class is useful for cases where we need a Terminal, a Renderer, and -// a DxEngine. Some bugs won't repro without all three actually being hooked up. -// Note however, that the DxEngine is not wired up to actually be PaintFrame'd -// in this test - it pretty heavily depends on being able to actually get a -// render target, and as we're running in a unit test, we don't have one of -// those. However, this class is good for testing how invalidation works across -// all three. - -#include "precomp.h" -#include - -#include -#include - -#include "../renderer/inc/DummyRenderTarget.hpp" -#include "../renderer/base/Renderer.hpp" -#include "../renderer/dx/DxRenderer.hpp" - -#include "../cascadia/TerminalCore/Terminal.hpp" -#include "MockTermSettings.h" -#include "consoletaeftemplates.hpp" -#include "TestUtils.h" - -using namespace winrt::Microsoft::Terminal::Settings; -using namespace Microsoft::Terminal::Core; -using namespace ::Microsoft::Console::Types; - -using namespace WEX::Common; -using namespace WEX::Logging; -using namespace WEX::TestExecution; - -namespace TerminalCoreUnitTests -{ - class TerminalAndRendererTests; -}; -using namespace TerminalCoreUnitTests; - -class TerminalCoreUnitTests::TerminalAndRendererTests final -{ - // !!! DANGER: Many tests in this class expect the Terminal buffer - // to be 80x32. If you change these, you'll probably inadvertently break a - // bunch of tests !!! - static const SHORT TerminalViewWidth = 80; - static const SHORT TerminalViewHeight = 32; - // For TestNotifyScrolling, it's important that this value is ~=9000. - // Something smaller like 100 won't cause the test to fail. - static const SHORT TerminalHistoryLength = 9001; - - TEST_CLASS(TerminalAndRendererTests); - - TEST_METHOD(TestNotifyScrolling); - - TEST_METHOD_SETUP(MethodSetup) - { - _term = std::make_unique<::Microsoft::Terminal::Core::Terminal>(); - - // Create the renderer - _renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(_term.get(), nullptr, 0, nullptr); - ::Microsoft::Console::Render::IRenderTarget& renderTarget = *_renderer; - - // Create the engine - _dxEngine = std::make_unique<::Microsoft::Console::Render::DxEngine>(); - _renderer->AddRenderEngine(_dxEngine.get()); - - // Initialize the renderer, engine for a default font size - _renderer->TriggerFontChange(USER_DEFAULT_SCREEN_DPI, _desiredFont, _actualFont); - const til::size viewportSize{ TerminalViewWidth, TerminalViewHeight }; - const til::size fontSize = _actualFont.GetSize(); - const til::size windowSize = viewportSize * fontSize; - VERIFY_SUCCEEDED(_dxEngine->SetWindowSize(windowSize)); - const auto vp = _dxEngine->GetViewportInCharacters(Viewport::FromDimensions({ 0, 0 }, windowSize)); - VERIFY_ARE_EQUAL(viewportSize, til::size{ vp.Dimensions() }); - - // Set up the Terminal, using the Renderer (which has the engine in it) - _term->Create({ TerminalViewWidth, TerminalViewHeight }, TerminalHistoryLength, renderTarget); - return true; - } - - TEST_METHOD_CLEANUP(MethodCleanup) - { - _term = nullptr; - return true; - } - -private: - std::unique_ptr _term; - std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer; - std::unique_ptr<::Microsoft::Console::Render::DxEngine> _dxEngine; - - FontInfoDesired _desiredFont{ DEFAULT_FONT_FACE, 0, 10, { 0, DEFAULT_FONT_SIZE }, CP_UTF8 }; - FontInfo _actualFont{ DEFAULT_FONT_FACE, 0, 10, { 0, DEFAULT_FONT_SIZE }, CP_UTF8, false }; -}; - -void TerminalAndRendererTests::TestNotifyScrolling() -{ - // See https://github.com/microsoft/terminal/pull/5630 - // - // This is a test for GH#5540, in the most bizarre way. The origin of that - // bug was that as newlines were emitted, we'd accumulate an enormous scroll - // delta into a selection region, to the point of overflowing a SHORT. When - // the overflow occurred, the Terminal would fail to send a NotifyScroll() to - // the TermControl hosting it. - // - // For this bug to repro, we need to: - // - Have a sufficiently large buffer, because each newline we'll accumulate - // a delta of (0, ~bufferHeight), so (bufferHeight^2 + bufferHeight) > - // SHRT_MAX - // - Have a selection - - Log::Comment(L"Watch out - this test takes a while to run, and won't " - L"output anything unless in encounters an error. This is expected."); - - auto& termTb = *_term->_buffer; - auto& termSm = *_term->_stateMachine; - - const auto totalBufferSize = termTb.GetSize().Height(); - - auto currentRow = 0; - bool gotScrollingNotification = false; - - // We're outputting like 18000 lines of text here, so emitting 18000*4 lines - // of output to the console is actually quite unnecessary - WEX::TestExecution::SetVerifyOutput settings(WEX::TestExecution::VerifyOutputSettings::LogOnlyFailures); - - auto verifyScrolling = [&](const int top, const int height, const int bottom) { - const int expectedTop = std::clamp(currentRow - TerminalViewHeight + 2, - 0, - TerminalHistoryLength); - - const int expectedHeight = TerminalViewHeight; - const int expectedBottom = expectedTop + TerminalViewHeight; - if ((expectedTop != top) || - (expectedHeight != height) || - (expectedBottom != bottom)) - { - Log::Comment(NoThrowString().Format(L"Expected values did not match on line %d", currentRow)); - } - VERIFY_ARE_EQUAL(expectedTop, top); - VERIFY_ARE_EQUAL(expectedHeight, height); - VERIFY_ARE_EQUAL(expectedBottom, bottom); - - gotScrollingNotification = true; - }; - - // Hook up the scrolling callback - _term->SetScrollPositionChangedCallback(verifyScrolling); - - // Create a selection - the actual bounds don't matter, we just need to have one. - _term->SetSelectionAnchor(COORD{ 0, 0 }); - _term->SetSelectionEnd(COORD{ TerminalViewWidth - 1, 0 }); - _renderer->TriggerSelection(); - - // Emit a bunch of newlines. Eventually, the accumulated scroll delta will - // cause an overflow, and cause us to miss a NotifyScroll. - for (; currentRow < totalBufferSize * 2; currentRow++) - { - gotScrollingNotification = false; - - termSm.ProcessString(L"X\r\n"); - - // When we're on TerminalViewHeight-1, we'll emit the newline that - // causes the first scroll event - if (currentRow >= TerminalViewHeight - 1) - { - VERIFY_IS_TRUE(gotScrollingNotification, - fmt::format(L"Expected a scrolling notification for row {}", currentRow).c_str()); - } - else - { - VERIFY_IS_FALSE(gotScrollingNotification, - fmt::format(L"Expected to not see scrolling notification for row {}", currentRow).c_str()); - } - } -} diff --git a/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj b/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj index d23726c0231..48734244141 100644 --- a/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj +++ b/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj @@ -20,7 +20,7 @@ - +