From 3599c4f3cb5d81b0dcea8086f62f1f247fa72743 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 22 Aug 2023 15:46:49 -0500 Subject: [PATCH 1/3] INTERESTING --- src/cascadia/TerminalControl/ControlCore.cpp | 4 +- src/cascadia/TerminalControl/ControlCore.h | 2 +- src/cascadia/TerminalControl/ControlCore.idl | 2 +- src/cascadia/TerminalControl/EventArgs.cpp | 1 + src/cascadia/TerminalControl/EventArgs.h | 19 +++++++ src/cascadia/TerminalControl/EventArgs.idl | 11 +++-- src/cascadia/TerminalControl/TermControl.cpp | 13 +++-- src/cascadia/TerminalControl/TermControl.h | 9 ++-- src/cascadia/TerminalControl/TermControl.idl | 2 +- src/cascadia/inc/cppwinrt_utils.h | 52 -------------------- 10 files changed, 43 insertions(+), 72 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 87e98c0a1b7..f883df56402 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -241,7 +241,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation _setupDispatcherAndCallbacks(); const auto actualNewSize = _actualFont.GetSize(); // Bubble this up, so our new control knows how big we want the font. - _FontSizeChangedHandlers(actualNewSize.width, actualNewSize.height, true); + _FontSizeChangedHandlers(*this, *winrt::make_self(actualNewSize.width, actualNewSize.height, true)); // The renderer will be re-enabled in Initialize @@ -947,7 +947,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } const auto actualNewSize = _actualFont.GetSize(); - _FontSizeChangedHandlers(actualNewSize.width, actualNewSize.height, initialUpdate); + _FontSizeChangedHandlers(*this, *winrt::make_self(actualNewSize.width, actualNewSize.height, initialUpdate)); } // Method Description: diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 5f9f23a3e89..2ce9b9b9af6 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -248,7 +248,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // -------------------------------- WinRT Events --------------------------------- // clang-format off - WINRT_CALLBACK(FontSizeChanged, Control::FontSizeChangedEventArgs); + TYPED_EVENT(FontSizeChanged, IInspectable, Control::FontSizeChangedArgs); TYPED_EVENT(CopyToClipboard, IInspectable, Control::CopyToClipboardEventArgs); TYPED_EVENT(TitleChanged, IInspectable, Control::TitleChangedEventArgs); diff --git a/src/cascadia/TerminalControl/ControlCore.idl b/src/cascadia/TerminalControl/ControlCore.idl index 8157346dcd1..af898ff3d5a 100644 --- a/src/cascadia/TerminalControl/ControlCore.idl +++ b/src/cascadia/TerminalControl/ControlCore.idl @@ -165,7 +165,7 @@ namespace Microsoft.Terminal.Control event Windows.Foundation.TypedEventHandler ShowWindowChanged; // These events are always called from the UI thread (bugs aside) - event FontSizeChangedEventArgs FontSizeChanged; + event Windows.Foundation.TypedEventHandler FontSizeChanged; event Windows.Foundation.TypedEventHandler ScrollPositionChanged; event Windows.Foundation.TypedEventHandler CursorPositionChanged; event Windows.Foundation.TypedEventHandler ConnectionStateChanged; diff --git a/src/cascadia/TerminalControl/EventArgs.cpp b/src/cascadia/TerminalControl/EventArgs.cpp index 36a9121957b..93e147feaa8 100644 --- a/src/cascadia/TerminalControl/EventArgs.cpp +++ b/src/cascadia/TerminalControl/EventArgs.cpp @@ -3,6 +3,7 @@ #include "pch.h" #include "EventArgs.h" +#include "FontSizeChangedArgs.g.cpp" #include "TitleChangedEventArgs.g.cpp" #include "CopyToClipboardEventArgs.g.cpp" #include "ContextMenuRequestedEventArgs.g.cpp" diff --git a/src/cascadia/TerminalControl/EventArgs.h b/src/cascadia/TerminalControl/EventArgs.h index 20cb222e008..a07c6885112 100644 --- a/src/cascadia/TerminalControl/EventArgs.h +++ b/src/cascadia/TerminalControl/EventArgs.h @@ -3,6 +3,7 @@ #pragma once +#include "FontSizeChangedArgs.g.h" #include "TitleChangedEventArgs.g.h" #include "CopyToClipboardEventArgs.g.h" #include "ContextMenuRequestedEventArgs.g.h" @@ -22,6 +23,24 @@ namespace winrt::Microsoft::Terminal::Control::implementation { + + struct FontSizeChangedArgs : public FontSizeChangedArgsT + { + public: + FontSizeChangedArgs(int32_t width, + int32_t height, + bool isInitialChange) : + Width(width), + Height(height), + IsInitialChange(isInitialChange) + { + } + + til::property Width; + til::property Height; + til::property IsInitialChange; + }; + struct TitleChangedEventArgs : public TitleChangedEventArgsT { public: diff --git a/src/cascadia/TerminalControl/EventArgs.idl b/src/cascadia/TerminalControl/EventArgs.idl index 2f3c6adfefa..b01adb4781f 100644 --- a/src/cascadia/TerminalControl/EventArgs.idl +++ b/src/cascadia/TerminalControl/EventArgs.idl @@ -3,9 +3,6 @@ namespace Microsoft.Terminal.Control { - delegate void FontSizeChangedEventArgs(Int32 width, Int32 height, Boolean isInitialChange); - delegate void ScrollPositionChangedEventArgs(Int32 viewTop, Int32 viewHeight, Int32 bufferLength); - [flags] enum CopyFormat { @@ -14,6 +11,14 @@ namespace Microsoft.Terminal.Control All = 0xffffffff }; + + runtimeclass FontSizeChangedArgs + { + Int32 Width { get; }; + Int32 Height { get; }; + Boolean IsInitialChange{ get; }; + } + runtimeclass CopyToClipboardEventArgs { String Text { get; }; diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 2bce8ecc0d0..690239db903 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -3287,16 +3287,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation return posInDIPs + marginsInDips; } - void TermControl::_coreFontSizeChanged(const int fontWidth, - const int fontHeight, - const bool isInitialChange) + void TermControl::_coreFontSizeChanged(const IInspectable& /*sender*/, + const Control::FontSizeChangedArgs& args) { // scale the selection markers to be the size of a cell - auto scaleMarker = [fontWidth, fontHeight, dpiScale{ SwapChainPanel().CompositionScaleX() }](const Windows::UI::Xaml::Shapes::Path& shape) { + auto scaleMarker = [args, dpiScale{ SwapChainPanel().CompositionScaleX() }](const Windows::UI::Xaml::Shapes::Path& shape) { // The selection markers were designed to be 5x14 in size, // so use those dimensions below for the scaling - const auto scaleX = fontWidth / 5.0 / dpiScale; - const auto scaleY = fontHeight / 14.0 / dpiScale; + const auto scaleX = args.Width() / 5.0 / dpiScale; + const auto scaleY = args.Height() / 14.0 / dpiScale; Windows::UI::Xaml::Media::ScaleTransform transform; transform.ScaleX(scaleX); @@ -3313,7 +3312,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // it's holding its write lock. If the handlers calls back to some // method on the TermControl on the same thread, and that _method_ calls // to ControlCore, we might be in danger of deadlocking. - _FontSizeChangedHandlers(fontWidth, fontHeight, isInitialChange); + FontSizeChanged.raise(*this, args); } void TermControl::_coreRaisedNotice(const IInspectable& /*sender*/, diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 6b05e7e9a83..c9c55d6e2e3 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -160,10 +160,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation TerminalConnection::ITerminalConnection Connection(); void Connection(const TerminalConnection::ITerminalConnection& connection); - WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); // -------------------------------- WinRT Events --------------------------------- // clang-format off - WINRT_CALLBACK(FontSizeChanged, Control::FontSizeChangedEventArgs); + til::typed_event FontSizeChanged; + + WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); // UNDER NO CIRCUMSTANCES SHOULD YOU ADD A (PROJECTED_)FORWARDED_TYPED_EVENT HERE // Those attach the handler to the core directly, and will explode if @@ -354,9 +355,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void _hoveredHyperlinkChanged(const IInspectable& sender, const IInspectable& args); winrt::fire_and_forget _updateSelectionMarkers(IInspectable sender, Control::UpdateSelectionMarkersEventArgs args); - void _coreFontSizeChanged(const int fontWidth, - const int fontHeight, - const bool isInitialChange); + void _coreFontSizeChanged(const IInspectable& s, const Control::FontSizeChangedArgs& args); winrt::fire_and_forget _coreTransparencyChanged(IInspectable sender, Control::TransparencyChangedEventArgs args); void _coreRaisedNotice(const IInspectable& s, const Control::NoticeEventArgs& args); void _coreWarningBell(const IInspectable& sender, const IInspectable& args); diff --git a/src/cascadia/TerminalControl/TermControl.idl b/src/cascadia/TerminalControl/TermControl.idl index 63495109325..cf4c6c794d6 100644 --- a/src/cascadia/TerminalControl/TermControl.idl +++ b/src/cascadia/TerminalControl/TermControl.idl @@ -40,7 +40,7 @@ namespace Microsoft.Terminal.Control Microsoft.Terminal.Control.IControlSettings Settings { get; }; - event FontSizeChangedEventArgs FontSizeChanged; + event Windows.Foundation.TypedEventHandler FontSizeChanged; event Windows.Foundation.TypedEventHandler TitleChanged; event Windows.Foundation.TypedEventHandler CopyToClipboard; event Windows.Foundation.TypedEventHandler PasteFromClipboard; diff --git a/src/cascadia/inc/cppwinrt_utils.h b/src/cascadia/inc/cppwinrt_utils.h index 142730610c1..7ca1943d4b4 100644 --- a/src/cascadia/inc/cppwinrt_utils.h +++ b/src/cascadia/inc/cppwinrt_utils.h @@ -17,58 +17,6 @@ Revision History: #pragma once -// This is a helper macro to make declaring events easier. -// This will declare the event handler and the methods for adding and removing a -// handler callback from the event -#define DECLARE_EVENT(name, eventHandler, args) \ -public: \ - winrt::event_token name(const args& handler); \ - void name(const winrt::event_token& token) noexcept; \ - \ -protected: \ - winrt::event eventHandler; - -// This is a helper macro for defining the body of events. -// Winrt events need a method for adding a callback to the event and removing -// the callback. This macro will define them both for you, because they -// don't really vary from event to event. -#define DEFINE_EVENT(className, name, eventHandler, args) \ - winrt::event_token className::name(const args& handler) \ - { \ - return eventHandler.add(handler); \ - } \ - void className::name(const winrt::event_token& token) noexcept \ - { \ - eventHandler.remove(token); \ - } - -// This is a helper macro to make declaring events easier. -// This will declare the event handler and the methods for adding and removing a -// handler callback from the event. -// Use this if you have a Windows.Foundation.TypedEventHandler -#define DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(name, eventHandler, sender, args) \ -public: \ - winrt::event_token name(const Windows::Foundation::TypedEventHandler& handler); \ - void name(const winrt::event_token& token) noexcept; \ - \ -private: \ - winrt::event> eventHandler; - -// This is a helper macro for defining the body of events. -// Winrt events need a method for adding a callback to the event and removing -// the callback. This macro will define them both for you, because they -// don't really vary from event to event. -// Use this if you have a Windows.Foundation.TypedEventHandler -#define DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(className, name, eventHandler, sender, args) \ - winrt::event_token className::name(const Windows::Foundation::TypedEventHandler& handler) \ - { \ - return eventHandler.add(handler); \ - } \ - void className::name(const winrt::event_token& token) noexcept \ - { \ - eventHandler.remove(token); \ - } - // This is a helper macro for both declaring the signature of an event, and // defining the body. Winrt events need a method for adding a callback to the // event and removing the callback. This macro will both declare the method From 830fecf432ed5f42eeb37c1bb32d93f052c3c02c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 22 Aug 2023 16:16:29 -0500 Subject: [PATCH 2/3] turns out that was UNUSED AF --- src/cascadia/TerminalControl/ControlCore.cpp | 11 +++++------ src/cascadia/TerminalControl/ControlCore.h | 2 +- src/cascadia/TerminalControl/EventArgs.h | 7 ++----- src/cascadia/TerminalControl/EventArgs.idl | 1 - src/cascadia/TerminalControl/TermControl.cpp | 6 ------ src/cascadia/TerminalControl/TermControl.h | 2 -- src/cascadia/TerminalControl/TermControl.idl | 1 - 7 files changed, 8 insertions(+), 22 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index f883df56402..c95b20bef0b 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -241,7 +241,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation _setupDispatcherAndCallbacks(); const auto actualNewSize = _actualFont.GetSize(); // Bubble this up, so our new control knows how big we want the font. - _FontSizeChangedHandlers(*this, *winrt::make_self(actualNewSize.width, actualNewSize.height, true)); + _FontSizeChangedHandlers(*this, *winrt::make_self(actualNewSize.width, actualNewSize.height)); // The renderer will be re-enabled in Initialize @@ -344,7 +344,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Initialize our font with the renderer // We don't have to care about DPI. We'll get a change message immediately if it's not 96 // and react accordingly. - _updateFont(true); + _updateFont(); const til::size windowSize{ til::math::rounding, windowWidth, windowHeight }; @@ -897,9 +897,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation // appropriately call _doResizeUnderLock after this method is called. // - The write lock should be held when calling this method. // Arguments: - // - initialUpdate: whether this font update should be considered as being - // concerned with initialization process. Value forwarded to event handler. - void ControlCore::_updateFont(const bool initialUpdate) + // + void ControlCore::_updateFont() { const auto newDpi = static_cast(lrint(_compositionScale * USER_DEFAULT_SCREEN_DPI)); @@ -947,7 +946,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } const auto actualNewSize = _actualFont.GetSize(); - _FontSizeChangedHandlers(*this, *winrt::make_self(actualNewSize.width, actualNewSize.height, initialUpdate)); + _FontSizeChangedHandlers(*this, *winrt::make_self(actualNewSize.width, actualNewSize.height)); } // Method Description: diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 2ce9b9b9af6..0851f746056 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -340,7 +340,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void _setupDispatcherAndCallbacks(); bool _setFontSizeUnderLock(float fontSize); - void _updateFont(const bool initialUpdate = false); + void _updateFont(); void _refreshSizeUnderLock(); void _updateSelectionUI(); bool _shouldTryUpdateSelection(const WORD vkey); diff --git a/src/cascadia/TerminalControl/EventArgs.h b/src/cascadia/TerminalControl/EventArgs.h index a07c6885112..9d3f3e2a3f6 100644 --- a/src/cascadia/TerminalControl/EventArgs.h +++ b/src/cascadia/TerminalControl/EventArgs.h @@ -28,17 +28,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation { public: FontSizeChangedArgs(int32_t width, - int32_t height, - bool isInitialChange) : + int32_t height) : Width(width), - Height(height), - IsInitialChange(isInitialChange) + Height(height) { } til::property Width; til::property Height; - til::property IsInitialChange; }; struct TitleChangedEventArgs : public TitleChangedEventArgsT diff --git a/src/cascadia/TerminalControl/EventArgs.idl b/src/cascadia/TerminalControl/EventArgs.idl index b01adb4781f..75e1a20211c 100644 --- a/src/cascadia/TerminalControl/EventArgs.idl +++ b/src/cascadia/TerminalControl/EventArgs.idl @@ -16,7 +16,6 @@ namespace Microsoft.Terminal.Control { Int32 Width { get; }; Int32 Height { get; }; - Boolean IsInitialChange{ get; }; } runtimeclass CopyToClipboardEventArgs diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 690239db903..550d13eae30 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -3307,12 +3307,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation }; scaleMarker(SelectionStartMarker()); scaleMarker(SelectionEndMarker()); - - // Don't try to inspect the core here. The Core is raising this while - // it's holding its write lock. If the handlers calls back to some - // method on the TermControl on the same thread, and that _method_ calls - // to ControlCore, we might be in danger of deadlocking. - FontSizeChanged.raise(*this, args); } void TermControl::_coreRaisedNotice(const IInspectable& /*sender*/, diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index c9c55d6e2e3..895f46128e4 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -162,8 +162,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation // -------------------------------- WinRT Events --------------------------------- // clang-format off - til::typed_event FontSizeChanged; - WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); // UNDER NO CIRCUMSTANCES SHOULD YOU ADD A (PROJECTED_)FORWARDED_TYPED_EVENT HERE diff --git a/src/cascadia/TerminalControl/TermControl.idl b/src/cascadia/TerminalControl/TermControl.idl index cf4c6c794d6..f01a11104ef 100644 --- a/src/cascadia/TerminalControl/TermControl.idl +++ b/src/cascadia/TerminalControl/TermControl.idl @@ -40,7 +40,6 @@ namespace Microsoft.Terminal.Control Microsoft.Terminal.Control.IControlSettings Settings { get; }; - event Windows.Foundation.TypedEventHandler FontSizeChanged; event Windows.Foundation.TypedEventHandler TitleChanged; event Windows.Foundation.TypedEventHandler CopyToClipboard; event Windows.Foundation.TypedEventHandler PasteFromClipboard; From 3879af7c4c0e5fa1dc3de9f305ef93f110cc1caf Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 23 Aug 2023 13:35:54 -0500 Subject: [PATCH 3/3] I forgot that make works even on a non projected ctor --- src/cascadia/TerminalControl/ControlCore.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index c95b20bef0b..2fb25987178 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -241,7 +241,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation _setupDispatcherAndCallbacks(); const auto actualNewSize = _actualFont.GetSize(); // Bubble this up, so our new control knows how big we want the font. - _FontSizeChangedHandlers(*this, *winrt::make_self(actualNewSize.width, actualNewSize.height)); + _FontSizeChangedHandlers(*this, winrt::make(actualNewSize.width, actualNewSize.height)); // The renderer will be re-enabled in Initialize @@ -946,7 +946,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } const auto actualNewSize = _actualFont.GetSize(); - _FontSizeChangedHandlers(*this, *winrt::make_self(actualNewSize.width, actualNewSize.height)); + _FontSizeChangedHandlers(*this, winrt::make(actualNewSize.width, actualNewSize.height)); } // Method Description: