Use one throttle for scrollbar updates, without per-line allocations

src/cascadia/TerminalControl/ControlCore.cpp

@@ -242,18 +242,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             });
 
         // Scrollbar updates are also expensive (XAML), so we'll throttle them as well.
-        shared->updateScrollBar = std::make_shared<ThrottledFunc<Control::ScrollPositionChangedArgs>>(
+        // The callback runs on _dispatcher (the UI thread), so handlers may update XAML directly.
+        shared->updateScrollBar = std::make_shared<ThrottledFunc<int, int, int>>(
             _dispatcher,
             til::throttled_func_options{
                 .delay = std::chrono::milliseconds{ 8 },
                 .trailing = true,
             },
-            [weakThis = get_weak()](const auto& update) {
+            [weakThis = get_weak()](const int viewTop, const int viewHeight, const int bufferSize) {
                 if (auto core{ weakThis.get() }; core && !core->_IsClosing())
                 {
                     // GH#20219: re-evaluate if we're hovering over a hyperlink after scrolling
                     core->_refreshHoveredCell();
-                    core->ScrollPositionChanged.raise(*core, update);
+                    core->ScrollPositionChanged.raise(*core, winrt::make<ScrollPositionChangedArgs>(viewTop, viewHeight, bufferSize));
                 }
             });
     }
@@ -758,6 +759,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             //      itself - it was initiated by the mouse wheel, or the scrollbar.
             const auto lock = _terminal->LockForWriting();
             _terminal->UserScrollViewport(viewTop);
+
+            // Terminal::UserScrollViewport doesn't notify us, so update the scrollbar here.
+            // Do it under the lock, like output does, so that the newest position wins.
+            const auto visible = _terminal->GetViewport();
+            _terminalScrollPositionChanged(visible.Top(), visible.Height(), _terminal->GetBufferHeight());
         }
 
         // GH#20219: re-evaluate if we're hovering over a hyperlink after scrolling
@@ -1678,21 +1684,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             return;
         }
 
-        // Start the throttled update of our scrollbar.
-        auto update{ winrt::make<ScrollPositionChangedArgs>(viewTop,
-                                                            viewHeight,
-                                                            bufferSize) };
-
         if (_inUnitTests) [[unlikely]]
         {
-            ScrollPositionChanged.raise(*this, update);
+            ScrollPositionChanged.raise(*this, winrt::make<ScrollPositionChangedArgs>(viewTop, viewHeight, bufferSize));
         }
         else
         {
+            // This runs for every line of output: queue plain values, not event args.
             const auto shared = _shared.lock_shared();
             if (shared->updateScrollBar)
             {
-                shared->updateScrollBar->Run(update);
+                shared->updateScrollBar->Run(viewTop, viewHeight, bufferSize);
             }
         }
     }
@@ -2721,27 +2723,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
         }
 
-        const auto viewHeight = ViewportSize().Height;
-        const auto bufferSize = BufferHeight();
-
-        // UserScrollViewport, to update the Terminal about where the viewport should be
-        // then raise a _terminalScrollPositionChanged to inform the control to update the scrollbar.
+        // UserScrollViewport also updates the scrollbar.
         if (tgt.has_value())
         {
             UserScrollViewport(tgt->start.y);
-            _terminalScrollPositionChanged(tgt->start.y, viewHeight, bufferSize);
         }
         else
         {
             if (direction == ScrollToMarkDirection::Last || direction == ScrollToMarkDirection::Next)
             {
                 UserScrollViewport(BufferHeight());
-                _terminalScrollPositionChanged(BufferHeight(), viewHeight, bufferSize);
             }
             else if (direction == ScrollToMarkDirection::First || direction == ScrollToMarkDirection::Previous)
             {
                 UserScrollViewport(0);
-                _terminalScrollPositionChanged(0, viewHeight, bufferSize);
             }
         }
     }

src/cascadia/TerminalControl/ControlCore.h

@@ -313,7 +313,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         {
             std::unique_ptr<til::throttled_func<>> outputIdle;
             std::unique_ptr<til::throttled_func<bool>> focusChanged;
-            std::shared_ptr<ThrottledFunc<Control::ScrollPositionChangedArgs>> updateScrollBar;
+            // viewTop, viewHeight, bufferSize
+            std::shared_ptr<ThrottledFunc<int, int, int>> updateScrollBar;
         };
 
         void _setupDispatcherAndCallbacks();

src/cascadia/TerminalControl/ControlInteractivity.cpp

@@ -513,8 +513,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                 const auto currentOffset = _core->ScrollOffset();
                 const auto newValue = numRows + currentOffset;
 
-                // Update the Core's viewport position, and raise a
-                // ScrollPositionChanged event to update the scrollbar
+                // Update the Core's viewport position, which also
+                // updates the scrollbar
                 UpdateScrollbar(newValue);
 
                 // Use this point as our new scroll anchor.
@@ -690,8 +690,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         const auto rowsToScroll{ _rowsToScroll == WHEEL_PAGESCROLL ? _core->ViewportSize().Height : _rowsToScroll };
         const auto newValue = rowsToScroll * rowDelta + currentOffset;
 
-        // Update the Core's viewport position, and raise a
-        // ScrollPositionChanged event to update the scrollbar
+        // Update the Core's viewport position, which also
+        // updates the scrollbar
         UpdateScrollbar(newValue);
 
         if (isLeftButtonPressed)
@@ -705,15 +705,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // Method Description:
     // - Update the scroll position in such a way that should update the
     //   scrollbar. For example, when scrolling the buffer with the mouse or
-    //   touch input. This will both update the Core's Terminal's buffer
-    //   location, then also raise our own ScrollPositionChanged event.
-    //   UserScrollViewport _won't_ raise the core's ScrollPositionChanged
-    //   event, because it's assumed that's already being called from a context
-    //   that knows about the change to the scrollbar. So we need to raise the
-    //   event on our own.
-    // - The hosting control should make sure to listen to our own
-    //   ScrollPositionChanged event and use that as an opportunity to update
-    //   the location of the scrollbar.
+    //   touch input. This scrolls the Core, which updates the scrollbar through
+    //   its throttled ScrollPositionChanged event.
+    // - May be called off the UI thread (auto-scroll runs on a render timer).
     // Arguments:
     // - newValue: The new top of the viewport
     // Return Value:
@@ -726,18 +720,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         _internalScrollbarPosition = std::clamp(newValue, 0.0f, static_cast<float>(_core->BufferHeight()));
 
         // If the new scrollbar position, rounded to an int, is at a different
-        // row, then actually update the scroll position in the core, and raise
-        // a ScrollPositionChanged to inform the control.
+        // row, then actually update the scroll position in the core.
         const auto viewTop = std::lround(_internalScrollbarPosition);
         if (viewTop != _core->ScrollOffset())
         {
             _core->UserScrollViewport(viewTop);
-
-            // _core->ScrollOffset() is now set to newValue
-            ScrollPositionChanged.raise(*this,
-                                        winrt::make<ScrollPositionChangedArgs>(_core->ScrollOffset(),
-                                                                               _core->ViewportSize().Height,
-                                                                               _core->BufferHeight()));
         }
     }
 

src/cascadia/TerminalControl/ControlInteractivity.h

@@ -93,7 +93,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
         til::typed_event<IInspectable, Control::OpenHyperlinkEventArgs> OpenHyperlink;
         til::typed_event<IInspectable, Control::PasteFromClipboardEventArgs> PasteFromClipboard;
-        til::typed_event<IInspectable, Control::ScrollPositionChangedArgs> ScrollPositionChanged;
         til::typed_event<IInspectable, Control::ContextMenuRequestedEventArgs> ContextMenuRequested;
 
         til::typed_event<IInspectable, IInspectable> Closed;

src/cascadia/TerminalControl/TermControl.cpp

@@ -27,10 +27,6 @@ using namespace winrt::Windows::System;
 using namespace winrt::Windows::ApplicationModel::DataTransfer;
 using namespace winrt::Windows::Storage::Streams;
 
-// 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.
 // This is already throttled primarily in the ControlCore, with a timeout of 100ms. We're adding another smaller one here, as the (potentially x-proc) call will come in off the UI thread
 constexpr const auto TsfRedrawInterval = std::chrono::milliseconds(8);
@@ -334,7 +330,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         _revokers.UpdateSelectionMarkers = _core->UpdateSelectionMarkers(winrt::auto_revoke, { get_weak(), &TermControl::_updateSelectionMarkers });
         _revokers.coreOpenHyperlink = _core->OpenHyperlink(winrt::auto_revoke, { get_weak(), &TermControl::_HyperlinkHandler });
         _revokers.interactivityOpenHyperlink = _interactivity->OpenHyperlink(winrt::auto_revoke, { get_weak(), &TermControl::_HyperlinkHandler });
-        _revokers.interactivityScrollPositionChanged = _interactivity->ScrollPositionChanged(winrt::auto_revoke, { get_weak(), &TermControl::_ScrollPositionChanged });
         _revokers.ContextMenuRequested = _interactivity->ContextMenuRequested(winrt::auto_revoke, { get_weak(), &TermControl::_contextMenuHandler });
 
         // "Bubbled" events - ones we want to handle, by raising our own event.
@@ -375,7 +370,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         // TermControl::Dispatcher().
         auto dispatcher = winrt::Windows::System::DispatcherQueue::GetForCurrentThread();
 
-        // These three throttled functions are triggered by terminal output and interact with the UI.
+        // This throttled function is triggered by terminal output and interacts with the UI.
         // Since Close() is the point after which we are removed from the UI, but before the
         // destructor has run, we MUST check control->_IsClosing() before actually doing anything.
         _playWarningBell = std::make_shared<ThrottledFunc<>>(
@@ -391,25 +386,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                 }
             });
 
-        _updateScrollBar = std::make_shared<ThrottledFunc<ScrollBarUpdate>>(
-            dispatcher,
-            til::throttled_func_options{
-                .delay = ScrollBarUpdateInterval,
-                .trailing = true,
-            },
-            [weakThis = get_weak()](const auto& update) {
-                if (auto control{ weakThis.get() }; control && !control->_IsClosing())
-                {
-                    control->_throttledUpdateScrollbar(update);
-                }
-            });
-
         // These events might all be triggered by the connection, but that
         // should be drained and closed before we complete destruction. So these
         // are safe.
         //
-        // NOTE: _ScrollPositionChanged has to be registered after we set up the
-        // _updateScrollBar func. Otherwise, we could get a callback from an
+        // NOTE: WarningBell has to be registered after we set up the
+        // _playWarningBell func. Otherwise, we could get a callback from an
         // attached content before we set up the throttled func, and that'll A/V
         _revokers.coreScrollPositionChanged = _core->ScrollPositionChanged(winrt::auto_revoke, { get_weak(), &TermControl::_ScrollPositionChanged });
         _revokers.WarningBell = _core->WarningBell(winrt::auto_revoke, { get_weak(), &TermControl::_coreWarningBell });
@@ -553,7 +535,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         _core->HardResetWithoutErase();
     }
 
-    void TermControl::_throttledUpdateScrollbar(const ScrollBarUpdate& update)
+    void TermControl::_applyScrollBarUpdate(const ScrollBarUpdate& update)
     {
         if (!_initializedTerminal)
         {
@@ -567,7 +549,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         _isInternalScrollBarUpdate = true;
 
         auto scrollBar = ScrollBar();
-        if (update.newValue)
+        // Don't snap a dragged thumb onto the row it's already on: its value is
+        // fractional, and the core reports the row that value rounds to.
+        if (update.newValue && std::lround(scrollBar.Value()) != std::lround(*update.newValue))
         {
             scrollBar.Value(*update.newValue);
         }
@@ -839,7 +823,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                 .newMinimum = scrollBar.Minimum(),
                 .newViewportSize = scrollBar.ViewportSize(),
             };
-            _updateScrollBar->Run(update);
+            _applyScrollBarUpdate(update);
         }
 
         // Set focus back to terminal control
@@ -2187,12 +2171,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
         const auto newValue = args.NewValue();
         _interactivity->UpdateScrollbar(static_cast<float>(newValue));
-
-        // 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:
@@ -2423,14 +2401,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     void TermControl::_ScrollPositionChanged(const IInspectable& /*sender*/,
                                              const Control::ScrollPositionChangedArgs& args)
     {
+        // ControlCore raises this from its scrollbar throttle, on our UI thread.
+        if (_IsClosing())
+        {
+            return;
+        }
+
         ScrollBarUpdate update;
         const auto hiddenContent = args.BufferSize() - args.ViewHeight();
         update.newMaximum = hiddenContent;
         update.newMinimum = 0;
         update.newViewportSize = args.ViewHeight();
         update.newValue = args.ViewTop();
 
-        _updateScrollBar->Run(update);
+        _applyScrollBarUpdate(update);
 
         // If we have a selection with markers (exposed via selection mode),
         //   update the position of the markers
@@ -3725,7 +3709,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                     .newMinimum = scrollBar.Minimum(),
                     .newViewportSize = scrollBar.ViewportSize(),
                 };
-                _updateScrollBar->Run(update);
+                _applyScrollBarUpdate(update);
             }
         }
 

src/cascadia/TerminalControl/TermControl.h

@@ -305,8 +305,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             double newViewportSize;
         };
 
-        std::shared_ptr<ThrottledFunc<ScrollBarUpdate>> _updateScrollBar;
-
         bool _isInternalScrollBarUpdate;
 
         winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellLightAnimation{ nullptr };
@@ -424,7 +422,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         void _coreOutputIdle(const IInspectable& sender, const IInspectable& args);
 
         winrt::Windows::Foundation::Point _toPosInDips(const Core::Point terminalCellPos);
-        void _throttledUpdateScrollbar(const ScrollBarUpdate& update);
+        void _applyScrollBarUpdate(const ScrollBarUpdate& update);
 
         void _pasteTextWithBroadcast(const winrt::hstring& text);
 
@@ -475,7 +473,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             Windows::UI::ViewManagement::AccessibilitySettings::HighContrastChanged_revoker HighContrastChanged;
 
             til::event_revoker interactivityOpenHyperlink;
-            til::event_revoker interactivityScrollPositionChanged;
             til::event_revoker PasteFromClipboard;
             til::event_revoker ContextMenuRequested;
         } _revokers{};