From 04b751faa70680bf0296063deacec4657c6ff9d6 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 25 May 2021 16:30:51 -0500 Subject: [PATCH] I really shouldn't be doing the throttledfunc thing in this PR --- src/cascadia/TerminalControl/ControlCore.cpp | 95 ++++---------------- src/cascadia/TerminalControl/ControlCore.h | 5 -- src/cascadia/TerminalControl/TermControl.cpp | 43 ++++++++- src/cascadia/TerminalControl/TermControl.h | 12 ++- src/cascadia/TerminalControl/ThrottledFunc.h | 15 ++-- 5 files changed, 68 insertions(+), 102 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 566602cdc..1cd0ce690 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -23,16 +23,6 @@ using namespace winrt::Windows::Graphics::Display; using namespace winrt::Windows::System; 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); - -// The minimum delay between updating the TSF input control. -constexpr const auto TsfRedrawInterval = std::chrono::milliseconds(100); - -// The minimum delay between updating the locations of regex patterns -constexpr const auto UpdatePatternLocationsInterval = std::chrono::milliseconds(500); - namespace winrt::Microsoft::Terminal::Control::implementation { // Helper static function to ensure that all ambiguous-width glyphs are reported as narrow. @@ -104,61 +94,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation auto pfnTerminalTaskbarProgressChanged = std::bind(&ControlCore::_terminalTaskbarProgressChanged, this); _terminal->TaskbarProgressChangedCallback(pfnTerminalTaskbarProgressChanged); - // Get our dispatcher. If we're hosted in-proc with XAML, this will get - // us the same dispatcher as TermControl::Dispatcher(). If we're out of - // proc, this'll return null. We'll need to instead make a new - // DispatcherQueue (on a new thread), so we can use that for throttled - // functions. - _dispatcher = winrt::Windows::System::DispatcherQueue::GetForCurrentThread(); - if (!_dispatcher) - { - auto controller{ winrt::Windows::System::DispatcherQueueController::CreateOnDedicatedThread() }; - _dispatcher = controller.DispatcherQueue(); - } - - // A few different events should be throttled, so they don't fire absolutely all the time: - // * _tsfTryRedrawCanvas: When the cursor position moves, we need to - // inform TSF, so it can move the canvas for the composition. We - // throttle this so that we're not hopping across the process boundary - // every time that the cursor moves. - // * _updatePatternLocations: When there's new output, or we scroll the - // viewport, we should re-check if there are any visible hyperlinks. - // But we don't really need to do this every single time text is - // output, we can limit this update to once every 500ms. - // * _updateScrollBar: Same idea as the TSF update - we don't _really_ - // need to hop across the process boundary every time text is output. - // We can throttle this to once every 8ms, which will get us out of - // the way of the main output & rendering threads. - _tsfTryRedrawCanvas = std::make_shared>( - _dispatcher, - TsfRedrawInterval, - [weakThis = get_weak()]() { - if (auto core{ weakThis.get() }) - { - core->_CursorPositionChangedHandlers(*core, nullptr); - } - }); - - _updatePatternLocations = std::make_shared>( - _dispatcher, - UpdatePatternLocationsInterval, - [weakThis = get_weak()]() { - if (auto core{ weakThis.get() }) - { - core->UpdatePatternLocations(); - } - }); - - _updateScrollBar = std::make_shared>( - _dispatcher, - ScrollBarUpdateInterval, - [weakThis = get_weak()](const auto& update) { - if (auto core{ weakThis.get() }) - { - core->_ScrollPositionChangedHandlers(*core, update); - } - }); - UpdateSettings(settings); } @@ -1141,21 +1076,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation // TODO GH#9617: refine locking around pattern tree _terminal->ClearPatternTree(); - // Start the throttled update of our scrollbar. - auto update{ winrt::make(viewTop, - viewHeight, - bufferSize) }; - _updateScrollBar->Run(update); - - // Additionally, start the throttled update of where our links are. - _updatePatternLocations->Run(); + _ScrollPositionChangedHandlers(*this, + winrt::make(viewTop, + viewHeight, + bufferSize)); } void ControlCore::_terminalCursorPositionChanged() { - // When the buffer's cursor moves, start the throttled func to - // eventually dispatch a CursorPositionChanged event. - _tsfTryRedrawCanvas->Run(); + _CursorPositionChangedHandlers(*this, nullptr); } void ControlCore::_terminalTaskbarProgressChanged() @@ -1443,8 +1372,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _terminal->Write(hstr); - // Start the throttled update of where our hyperlinks are. - _updatePatternLocations->Run(); + // NOTE: We're raising an event here to inform the TermControl that + // output has been received, so it can queue up a throttled + // UpdatePatternLocations call. In the future, we should have the + // _updatePatternLocations ThrottledFunc internal to this class, and + // run on this object's dispatcher queue. + // + // We're not doing that quite yet, because the Core will eventually + // be out-of-proc from the UI thread, and won't be able to just use + // the UI thread as the dispatcher queue thread. + // + // See TODO: https://github.com/microsoft/terminal/projects/5#card-50760282 + _ReceivedOutputHandlers(*this, nullptr); } } diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 3a5536225..16e4ef65b 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -207,11 +207,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation double _panelHeight{ 0 }; double _compositionScale{ 0 }; - winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr }; - std::shared_ptr> _tsfTryRedrawCanvas; - std::shared_ptr> _updatePatternLocations; - std::shared_ptr> _updateScrollBar; - winrt::fire_and_forget _asyncCloseConnection(); void _setFontSize(int fontSize); diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 369e4701b..14004115a 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -67,6 +67,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation _interactivity = winrt::make(settings, connection); _core = _interactivity.Core(); + // Use a manual revoker on the output event, so we can immediately stop + // worrying about it on destruction. + _coreOutputEventToken = _core.ReceivedOutput({ this, &TermControl::_coreReceivedOutput }); + // These events might all be triggered by the connection, but that // should be drained and closed before we complete destruction. So these // are safe. @@ -108,7 +112,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Core eventually won't have access to. When we get to // https://github.com/microsoft/terminal/projects/5#card-50760282 // then we'll move the applicable ones. - _tsfTryRedrawCanvas = std::make_shared>( + _tsfTryRedrawCanvas = std::make_shared>( Dispatcher(), TsfRedrawInterval, [weakThis = get_weak()]() { @@ -118,7 +122,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation } }); - _playWarningBell = std::make_shared>( + _updatePatternLocations = std::make_shared>( + Dispatcher(), + UpdatePatternLocationsInterval, + [weakThis = get_weak()]() { + if (auto control{ weakThis.get() }) + { + control->_core.UpdatePatternLocations(); + } + }); + + _playWarningBell = std::make_shared( Dispatcher(), TerminalWarningBellInterval, [weakThis = get_weak()]() { @@ -128,7 +142,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } }); - _updateScrollBar = std::make_shared>( + _updateScrollBar = std::make_shared>( Dispatcher(), ScrollBarUpdateInterval, [weakThis = get_weak()](const auto& update) { @@ -530,7 +544,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // create a custom automation peer with this code pattern: // (https://docs.microsoft.com/en-us/windows/uwp/design/accessibility/custom-automation-peers) - if (const auto& interactivityAutoPeer{ _interactivity.OnCreateAutomationPeer() }) + if (const auto& interactivityAutoPeer = _interactivity.OnCreateAutomationPeer()) { auto autoPeer = winrt::make_self(this, interactivityAutoPeer); _automationPeer = *autoPeer; @@ -1249,6 +1263,23 @@ namespace winrt::Microsoft::Terminal::Control::implementation CATCH_LOG(); } + void TermControl::_coreReceivedOutput(const IInspectable& /*sender*/, + const IInspectable& /*args*/) + { + // Queue up a throttled UpdatePatternLocations call. In the future, we + // should have the _updatePatternLocations ThrottledFunc internal to + // ControlCore, and run on that object's dispatcher queue. + // + // We're not doing that quite yet, because the Core will eventually + // be out-of-proc from the UI thread, and won't be able to just use + // the UI thread as the dispatcher queue thread. + // + // THIS IS CALLED ON EVERY STRING OF TEXT OUTPUT TO THE TERMINAL. Think + // twice before adding anything here. + + _updatePatternLocations->Run(); + } + // Method Description: // - Reset the font size of the terminal to its default size. // Arguments: @@ -1286,6 +1317,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation _updateScrollBar->ModifyPending([](auto& update) { update.newValue.reset(); }); + + _updatePatternLocations->Run(); } // Method Description: @@ -1623,6 +1656,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation update.newValue = args.ViewTop(); _updateScrollBar->Run(update); + _updatePatternLocations->Run(); } // Method Description: @@ -1697,6 +1731,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // has run, we should disconnect them *right now*. If we don't, they may fire between the // throttle delay (from the final output) and the dtor. _tsfTryRedrawCanvas.reset(); + _updatePatternLocations.reset(); _updateScrollBar.reset(); _playWarningBell.reset(); diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index a24172dcc..976828221 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -28,8 +28,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation { TermControl(IControlSettings settings, TerminalConnection::ITerminalConnection connection); - static Control::TermControl FromConnectionInfo(IControlSettings settings, TerminalConnection::ConnectionInformation connectInfo); - winrt::fire_and_forget UpdateSettings(); winrt::fire_and_forget UpdateAppearance(const IControlAppearance newAppearance); @@ -156,8 +154,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool _focused; bool _initializedTerminal; - std::shared_ptr> _tsfTryRedrawCanvas; - std::shared_ptr> _playWarningBell; + std::shared_ptr> _tsfTryRedrawCanvas; + std::shared_ptr> _updatePatternLocations; + std::shared_ptr _playWarningBell; struct ScrollBarUpdate { @@ -166,9 +165,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation double newMinimum; double newViewportSize; }; - - std::shared_ptr> _updateScrollBar; - + std::shared_ptr> _updateScrollBar; bool _isInternalScrollBarUpdate; // Auto scroll occurs when user, while selecting, drags cursor outside viewport. View is then scrolled to 'follow' the cursor. @@ -260,6 +257,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const int fontHeight, const bool isInitialChange); winrt::fire_and_forget _coreTransparencyChanged(IInspectable sender, Control::TransparencyChangedEventArgs args); + void _coreReceivedOutput(const IInspectable& sender, const IInspectable& args); void _coreRaisedNotice(const IInspectable& s, const Control::NoticeEventArgs& args); void _coreWarningBell(const IInspectable& sender, const IInspectable& args); }; diff --git a/src/cascadia/TerminalControl/ThrottledFunc.h b/src/cascadia/TerminalControl/ThrottledFunc.h index 24076809c..7c8c1b31b 100644 --- a/src/cascadia/TerminalControl/ThrottledFunc.h +++ b/src/cascadia/TerminalControl/ThrottledFunc.h @@ -78,13 +78,13 @@ private: // pending, then the previous call with the previous arguments will be // cancelled and the call will be made with the new arguments instead. // - The function will be run on the the specified dispatcher. -template -class ThrottledFunc : public std::enable_shared_from_this> +template +class ThrottledFunc : public std::enable_shared_from_this> { public: using Func = std::function; - ThrottledFunc(T dispatcher, winrt::Windows::Foundation::TimeSpan delay, Func func) : + ThrottledFunc(winrt::Windows::UI::Core::CoreDispatcher dispatcher, winrt::Windows::Foundation::TimeSpan delay, Func func) : _dispatcher{ std::move(dispatcher) }, _delay{ std::move(delay) }, _func{ std::move(func) } @@ -174,14 +174,13 @@ private: } } - T _dispatcher; + winrt::Windows::UI::Core::CoreDispatcher _dispatcher; winrt::Windows::Foundation::TimeSpan _delay; Func _func; ThrottledFuncStorage _storage; }; -template -using ThrottledFuncTrailing = ThrottledFunc; -template -using ThrottledFuncLeading = ThrottledFunc; +template +using ThrottledFuncTrailing = ThrottledFunc; +using ThrottledFuncLeading = ThrottledFunc;