diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 266b1bd3db..1894c7b65b 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -48,9 +48,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { TermControl::TermControl(IControlSettings settings, TerminalConnection::ITerminalConnection connection) : - _initializedTerminal{ false }, _settings{ settings }, - _closing{ false }, _isInternalScrollBarUpdate{ false }, _autoScrollVelocity{ 0 }, _autoScrollingPointerPoint{ std::nullopt }, @@ -110,11 +108,15 @@ 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. + // + // These four throttled functions are triggered by terminal output and interact 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. _tsfTryRedrawCanvas = std::make_shared>( Dispatcher(), TsfRedrawInterval, [weakThis = get_weak()]() { - if (auto control{ weakThis.get() }) + if (auto control{ weakThis.get() }; !control->_IsClosing()) { control->TSFInputControl().TryRedrawCanvas(); } @@ -124,7 +126,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation Dispatcher(), UpdatePatternLocationsInterval, [weakThis = get_weak()]() { - if (auto control{ weakThis.get() }) + if (auto control{ weakThis.get() }; !control->_IsClosing()) { control->_core->UpdatePatternLocations(); } @@ -134,7 +136,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation Dispatcher(), TerminalWarningBellInterval, [weakThis = get_weak()]() { - if (auto control{ weakThis.get() }) + if (auto control{ weakThis.get() }; !control->_IsClosing()) { control->_WarningBellHandlers(*control, nullptr); } @@ -144,14 +146,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation Dispatcher(), ScrollBarUpdateInterval, [weakThis = get_weak()](const auto& update) { - if (auto control{ weakThis.get() }) + if (auto control{ weakThis.get() }; !control->_IsClosing()) { control->_isInternalScrollBarUpdate = true; auto scrollBar = control->ScrollBar(); - if (update.newValue.has_value()) + if (update.newValue) { - scrollBar.Value(update.newValue.value()); + scrollBar.Value(*update.newValue); } scrollBar.Maximum(update.newMaximum); scrollBar.Minimum(update.newMinimum); @@ -205,7 +207,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::SearchMatch(const bool goForward) { - if (_closing) + if (_IsClosing()) { return; } @@ -296,7 +298,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - newSettings: the new settings to set void TermControl::_UpdateSettingsFromUIThread(IControlSettings newSettings) { - if (_closing) + if (_IsClosing()) { return; } @@ -314,7 +316,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - newAppearance: the new appearance to set void TermControl::_UpdateAppearanceFromUIThread(IControlAppearance newAppearance) { - if (_closing) + if (_IsClosing()) { return; } @@ -527,7 +529,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation Windows::UI::Xaml::Automation::Peers::AutomationPeer TermControl::OnCreateAutomationPeer() try { - if (_initializedTerminal && !_closing) // only set up the automation peer if we're ready to go live + if (_initializedTerminal && !_IsClosing()) // only set up the automation peer if we're ready to go live { // create a custom automation peer with this code pattern: // (https://docs.microsoft.com/en-us/windows/uwp/design/accessibility/custom-automation-peers) @@ -747,7 +749,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_CharacterHandler(winrt::Windows::Foundation::IInspectable const& /*sender*/, Input::CharacterReceivedRoutedEventArgs const& e) { - if (_closing) + if (_IsClosing()) { return; } @@ -845,7 +847,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // win32-input-mode, then we'll send all these keystrokes to the // terminal - it's smart enough to ignore the keys it doesn't care // about. - if (_closing || + if (_IsClosing() || e.OriginalKey() == VirtualKey::LeftWindows || e.OriginalKey() == VirtualKey::RightWindows) @@ -997,12 +999,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation keyDown) : true; - if (_cursorTimer.has_value()) + if (_cursorTimer) { // Manually show the cursor when a key is pressed. Restarting // the timer prevents flickering. _core->CursorOn(true); - _cursorTimer.value().Start(); + _cursorTimer->Start(); } return handled; @@ -1027,7 +1029,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_PointerPressedHandler(Windows::Foundation::IInspectable const& sender, Input::PointerRoutedEventArgs const& args) { - if (_closing) + if (_IsClosing()) { return; } @@ -1078,7 +1080,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_PointerMovedHandler(Windows::Foundation::IInspectable const& /*sender*/, Input::PointerRoutedEventArgs const& args) { - if (_closing) + if (_IsClosing()) { return; } @@ -1151,7 +1153,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_PointerReleasedHandler(Windows::Foundation::IInspectable const& sender, Input::PointerRoutedEventArgs const& args) { - if (_closing) + if (_IsClosing()) { return; } @@ -1193,7 +1195,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_MouseWheelHandler(Windows::Foundation::IInspectable const& /*sender*/, Input::PointerRoutedEventArgs const& args) { - if (_closing) + if (_IsClosing()) { return; } @@ -1295,7 +1297,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_ScrollbarChangeHandler(Windows::Foundation::IInspectable const& /*sender*/, Controls::Primitives::RangeBaseValueChangedEventArgs const& args) { - if (_isInternalScrollBarUpdate || _closing) + if (_isInternalScrollBarUpdate || _IsClosing()) { // The update comes from ourselves, more specifically from the // terminal. So we don't have to update the terminal because it @@ -1361,15 +1363,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_TryStartAutoScroll(Windows::UI::Input::PointerPoint const& pointerPoint, const double scrollVelocity) { // Allow only one pointer at the time - if (!_autoScrollingPointerPoint.has_value() || - _autoScrollingPointerPoint.value().PointerId() == pointerPoint.PointerId()) + if (!_autoScrollingPointerPoint || + _autoScrollingPointerPoint->PointerId() == pointerPoint.PointerId()) { _autoScrollingPointerPoint = pointerPoint; _autoScrollVelocity = scrollVelocity; // If this is first time the auto scroll update is about to be called, // kick-start it by initializing its time delta as if it started now - if (!_lastAutoScrollUpdateTime.has_value()) + if (!_lastAutoScrollUpdateTime) { _lastAutoScrollUpdateTime = std::chrono::high_resolution_clock::now(); } @@ -1388,8 +1390,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - pointerId: id of pointer for which to stop auto scroll void TermControl::_TryStopAutoScroll(const uint32_t pointerId) { - if (_autoScrollingPointerPoint.has_value() && - pointerId == _autoScrollingPointerPoint.value().PointerId()) + if (_autoScrollingPointerPoint && + pointerId == _autoScrollingPointerPoint->PointerId()) { _autoScrollingPointerPoint = std::nullopt; _autoScrollVelocity = 0; @@ -1415,15 +1417,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation { const auto timeNow = std::chrono::high_resolution_clock::now(); - if (_lastAutoScrollUpdateTime.has_value()) + if (_lastAutoScrollUpdateTime) { static constexpr double microSecPerSec = 1000000.0; - const double deltaTime = std::chrono::duration_cast(timeNow - _lastAutoScrollUpdateTime.value()).count() / microSecPerSec; + const double deltaTime = std::chrono::duration_cast(timeNow - *_lastAutoScrollUpdateTime).count() / microSecPerSec; ScrollBar().Value(ScrollBar().Value() + _autoScrollVelocity * deltaTime); - if (_autoScrollingPointerPoint.has_value()) + if (_autoScrollingPointerPoint) { - _SetEndSelectionPointAtCursor(_autoScrollingPointerPoint.value().Position()); + _SetEndSelectionPointAtCursor(_autoScrollingPointerPoint->Position()); } } @@ -1439,7 +1441,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_GotFocusHandler(Windows::Foundation::IInspectable const& /* sender */, RoutedEventArgs const& /* args */) { - if (_closing) + if (_IsClosing()) { return; } @@ -1468,16 +1470,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation TSFInputControl().NotifyFocusEnter(); } - if (_cursorTimer.has_value()) + if (_cursorTimer) { // When the terminal focuses, show the cursor immediately _core->CursorOn(true); - _cursorTimer.value().Start(); + _cursorTimer->Start(); } - if (_blinkTimer.has_value()) + if (_blinkTimer) { - _blinkTimer.value().Start(); + _blinkTimer->Start(); } _interactivity->GainFocus(); @@ -1498,7 +1500,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_LostFocusHandler(Windows::Foundation::IInspectable const& /* sender */, RoutedEventArgs const& /* args */) { - if (_closing) + if (_IsClosing()) { return; } @@ -1517,15 +1519,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation TSFInputControl().NotifyFocusLeave(); } - if (_cursorTimer.has_value()) + if (_cursorTimer) { - _cursorTimer.value().Stop(); + _cursorTimer->Stop(); _core->CursorOn(false); } - if (_blinkTimer.has_value()) + if (_blinkTimer) { - _blinkTimer.value().Stop(); + _blinkTimer->Stop(); } // Check if there is an unfocused config we should set the appearance to @@ -1544,7 +1546,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_SwapChainSizeChanged(winrt::Windows::Foundation::IInspectable const& /*sender*/, SizeChangedEventArgs const& e) { - if (!_initializedTerminal || _closing) + if (!_initializedTerminal || _IsClosing()) { return; } @@ -1596,7 +1598,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_CursorTimerTick(Windows::Foundation::IInspectable const& /* sender */, Windows::Foundation::IInspectable const& /* e */) { - if (!_closing) + if (!_IsClosing()) { _core->BlinkCursor(); } @@ -1610,7 +1612,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_BlinkTimerTick(Windows::Foundation::IInspectable const& /* sender */, Windows::Foundation::IInspectable const& /* e */) { - if (!_closing) + if (!_IsClosing()) { _core->BlinkAttributeTick(); } @@ -1638,13 +1640,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_ScrollPositionChanged(const IInspectable& /*sender*/, const Control::ScrollPositionChangedArgs& args) { - // Since this callback fires from non-UI thread, we might be already - // closed/closing. - if (_closing.load()) - { - return; - } - ScrollBarUpdate update; const auto hiddenContent = args.BufferSize() - args.ViewHeight(); update.newMaximum = hiddenContent; @@ -1664,10 +1659,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_CursorPositionChanged(const IInspectable& /*sender*/, const IInspectable& /*args*/) { - if (_tsfTryRedrawCanvas) - { - _tsfTryRedrawCanvas->Run(); - } + _tsfTryRedrawCanvas->Run(); } hstring TermControl::Title() @@ -1700,7 +1692,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // if we should defer which formats are copied to the global setting bool TermControl::CopySelectionToClipboard(bool singleLine, const Windows::Foundation::IReference& formats) { - if (_closing) + if (_IsClosing()) { return false; } @@ -1717,21 +1709,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::Close() { - if (!_closing.exchange(true)) + if (!_IsClosing()) { + _closing = true; + _core->ReceivedOutput(_coreOutputEventToken); - _RestorePointerCursorHandlers(*this, nullptr); - - // These four throttled functions are triggered by terminal output and interact with the UI. - // Since Close() is the point after which we are removed from the UI, but before the destructor - // 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(); - // Disconnect the TSF input control so it doesn't receive EditContext events. TSFInputControl().Close(); _autoScrollTimer.Stop(); @@ -2097,7 +2080,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - void TermControl::_CompositionCompleted(winrt::hstring text) { - if (_closing) + if (_IsClosing()) { return; } @@ -2174,7 +2157,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation winrt::fire_and_forget TermControl::_DragDropHandler(Windows::Foundation::IInspectable /*sender*/, DragEventArgs e) { - if (_closing) + if (_IsClosing()) { return; } @@ -2261,7 +2244,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_DragOverHandler(Windows::Foundation::IInspectable const& /*sender*/, DragEventArgs const& e) { - if (_closing) + if (_IsClosing()) { return; } @@ -2422,7 +2405,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Stop the timer and switch off the light _bellLightTimer.Stop(); - if (!_closing) + if (!_IsClosing()) { VisualBellLight::SetIsTarget(RootGrid(), false); } @@ -2465,7 +2448,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (auto self{ weakThis.get() }) { auto lastHoveredCell = _core->GetHoveredCell(); - if (lastHoveredCell.has_value()) + if (lastHoveredCell) { const auto uriText = _core->GetHoveredUriText(); if (!uriText.empty()) diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 429ece345b..9cdb67e976 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -149,9 +149,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation winrt::com_ptr _searchBox; IControlSettings _settings; - std::atomic _closing; - bool _focused; - bool _initializedTerminal; + bool _closing{ false }; + bool _focused{ false }; + bool _initializedTerminal{ false }; std::shared_ptr> _tsfTryRedrawCanvas; std::shared_ptr> _updatePatternLocations; @@ -183,6 +183,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker; + inline bool _IsClosing() const noexcept + { + // _closing isn't atomic and may only be accessed from the main thread. + assert(Dispatcher().HasThreadAccess()); + return _closing; + } + void _UpdateSettingsFromUIThread(IControlSettings newSettings); void _UpdateAppearanceFromUIThread(IControlAppearance newAppearance); void _ApplyUISettings(const IControlSettings&);