From 0f61b5f97d649b74af90877e971f7fd035dd5535 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 24 Aug 2023 14:53:03 -0500 Subject: [PATCH] Remove the FontSizeChanged event from TermControl (#15867) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I originally just wanted to close #1104, but then discovered that hey, this event wasn't even used anymore. Excerpts of Teams convo: * [Snap to character grid when resizing window by mcpiroman · Pull Request #3181 · microsoft/terminal (github.com)](https://github.com/microsoft/terminal/pull/3181/files#diff-d7ca72e0d5652fee837c06532efa614191bd5c41b18aa4d3ee6711f40138f04c) added it to Tab.cpp * where it was added * which called `pane->Relayout` which I don't even REMEMBER * By [Add functionality to open the Settings UI tab through openSettings by leonMSFT · Pull Request #7802 · microsoft/terminal (github.com)](https://github.com/microsoft/terminal/pull/7802/files#diff-83d260047bed34d3d9d5a12ac62008b65bd6dc5f3b9642905a007c3efce27efd), there was seemingly no FontSizeChanged in Tab.cpp (when it got moved to terminaltab.cpp) > `Pane::Relayout` functionally did nothing because sizing was switched to `star` sizing at some point in the past, so it was just deleted. From [Misc pane refactoring by Rosefield · Pull Request #11373 · microsoft/terminal](https://github.com/microsoft/terminal/pull/11373/files#r736900998) So, great. We can kill part of it, and convert the rest to a `TypedEvent`, and get rid of `DECLARE_` / `DEFINE_`. `ScrollPositionChangedEventArgs` was ALSO apparently already promoted to a typed event, so kill that too. --- src/cascadia/TerminalControl/ControlCore.cpp | 11 ++--- src/cascadia/TerminalControl/ControlCore.h | 4 +- src/cascadia/TerminalControl/ControlCore.idl | 2 +- src/cascadia/TerminalControl/EventArgs.cpp | 1 + src/cascadia/TerminalControl/EventArgs.h | 16 ++++++ src/cascadia/TerminalControl/EventArgs.idl | 10 ++-- src/cascadia/TerminalControl/TermControl.cpp | 17 ++----- src/cascadia/TerminalControl/TermControl.h | 7 +-- src/cascadia/TerminalControl/TermControl.idl | 1 - src/cascadia/inc/cppwinrt_utils.h | 52 -------------------- 10 files changed, 39 insertions(+), 82 deletions(-) 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(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(actualNewSize.width, actualNewSize.height, initialUpdate); + _FontSizeChangedHandlers(*this, winrt::make(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 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..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 + { + public: + FontSizeChangedArgs(int32_t width, + int32_t height) : + Width(width), + Height(height) + { + } + + til::property Width; + til::property Height; + }; + struct TitleChangedEventArgs : public TitleChangedEventArgsT { 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 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