Repository: microsoft/terminal (language: cpp)

Issue:
All TermControl/TermApp/TermConnection event handlers should be TypedEventHandlers
WinRT [design guidelines](https://docs.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/author-events#design-guidelines) dictate that event handlers should have two arguments, `T` and `EventArgs`. For any class `T` that produces an event, this ensures that the event _handler_ can identify which instance of `T` fired the currently-handled event.

> The signature for an event handler delegate should consist of two parameters: sender (`IInspectable`), and args (some event argument type, for example `RoutedEventArgs`).

Given that we are exposing an internal API, we can specify a more specific `T` than `IInspectable`.

C++/WinRT supports this by way of `winrt::TypedEventHandler<T, Args...>`.

We should move all the TermConnection, TermApp and TermControl event handlers to use `TypedEventHandler`s.

### bonus

We can get rid of `DECLARE_EVENT` and `DEFINE_EVENT`, and augment `DECLARE_EVENT_WITH_TYPED_HANDLER` (and `DEFINE...TYPED`)  to use `typename class_type` as the sender type, so that we don't need to repeat ourselves:

https://github.com/microsoft/terminal/blob/880272c7483a3bb6893dbec3725d0eb7375ab78a/src/cascadia/inc/cppwinrt_utils.h#L42-L47

```diff
-#define DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(name, eventHandler, sender, args) \
+#define DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(name, eventHandler, args) \
     public: \
-    winrt::event_token name(Windows::Foundation::TypedEventHandler<sender, args> const& handler); \
+    winrt::event_token name(Windows::Foundation::TypedEventHandler<typename class_type, args> const& handler); \     void name(winrt::event_token const& token) noexcept; \
     private: \
-    winrt::event<Windows::Foundation::TypedEventHandler<sender, args>> eventHandler;
+    winrt::event<Windows::Foundation::TypedEventHandler<typename class_type, args>> eventHandler;
```

which, when used, becomes:

```diff
-DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(PasteFromClipboard, _clipboardPasteHandlers, TerminalControl::TermControl, TerminalControl::PasteFromClipboardEventArgs);
+DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(PasteFromClipboard, _clipboardPasteHandlers, TerminalControl::PasteFromClipboardEventArgs);
```


Discussion / hints:

Fix patch:
diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp
index 87e98c0a1b7..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(actualNewSize.width, actualNewSize.height, true);
+        _FontSizeChangedHandlers(*this, winrt::make<FontSizeChangedArgs>(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)
+    // <none>
+    void ControlCore::_updateFont()
     {
         const auto newDpi = static_cast<int>(lrint(_compositionScale * USER_DEFAULT_SCREEN_DPI));
 
@@ -947,7 +946,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
 
         const auto actualNewSize = _actualFont.GetSize();
-        _FontSizeChangedHandlers(actualNewSize.width, actualNewSize.height, initialUpdate);
+        _FontSizeChangedHandlers(*this, winrt::make<FontSizeChangedArgs>(actualNewSize.width, actualNewSize.height));
     }
 
     // Method Description:
diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h
index 5f9f23a3e89..0851f746056 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);
@@ -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/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<Object, ShowWindowArgs> ShowWindowChanged;
 
         // These events are always called from the UI thread (bugs aside)
-        event FontSizeChangedEventArgs FontSizeChanged;
+        event Windows.Foundation.TypedEventHandler<Object, FontSizeChangedArgs> FontSizeChanged;
         event Windows.Foundation.TypedEventHandler<Object, ScrollPositionChangedArgs> ScrollPositionChanged;
         event Windows.Foundation.TypedEventHandler<Object, Object> CursorPositionChanged;
         event Windows.Foundation.TypedEventHandler<Object, Object> 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..9d3f3e2a3f6 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,21 @@
 
 namespace winrt::Microsoft::Terminal::Control::implementation
 {
+
+    struct FontSizeChangedArgs : public FontSizeChangedArgsT<FontSizeChangedArgs>
+    {
+    public:
+        FontSizeChangedArgs(int32_t width,
+                            int32_t height) :
+            Width(width),
+            Height(height)
+        {
+        }
+
+        til::property<int32_t> Width;
+        til::property<int32_t> Height;
+    };
+
     struct TitleChangedEventArgs : public TitleChangedEventArgsT<TitleChangedEventArgs>
     {
     public:
diff --git a/src/cascadia/TerminalControl/EventArgs.idl b/src/cascadia/TerminalControl/EventArgs.idl
index 2f3c6adfefa..75e1a20211c 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,13 @@ namespace Microsoft.Terminal.Control
         All = 0xffffffff
     };
 
+
+    runtimeclass FontSizeChangedArgs
+    {
+        Int32 Width { get; };
+        Int32 Height { get; };
+    }
+
     runtimeclass CopyToClipboardEventArgs
     {
         String Text { get; };
diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp
index 2bce8ecc0d0..550d13eae30 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);
@@ -3308,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.
-        _FontSizeChangedHandlers(fontWidth, fontHeight, isInitialChange);
     }
 
     void TermControl::_coreRaisedNotice(const IInspectable& /*sender*/,
diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h
index 6b05e7e9a83..895f46128e4 100644
--- a/src/cascadia/TerminalControl/TermControl.h
+++ b/src/cascadia/TerminalControl/TermControl.h
@@ -160,10 +160,9 @@ 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);
+        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 +353,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..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 FontSizeChangedEventArgs FontSizeChanged;
         event Windows.Foundation.TypedEventHandler<Object, TitleChangedEventArgs> TitleChanged;
         event Windows.Foundation.TypedEventHandler<Object, CopyToClipboardEventArgs> CopyToClipboard;
         event Windows.Foundation.TypedEventHandler<Object, PasteFromClipboardEventArgs> 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<args> 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<sender, args>& handler); \
-    void name(const winrt::event_token& token) noexcept;                                          \
-                                                                                                  \
-private:                                                                                          \
-    winrt::event<Windows::Foundation::TypedEventHandler<sender, args>> 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<sender, args>& 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


Test patch: