Skip to content

Commit

Permalink
Remove the FontSizeChanged event from TermControl (#15867)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
zadjii-msft authored Aug 24, 2023
1 parent 7a05501 commit 0f61b5f
Show file tree
Hide file tree
Showing 10 changed files with 39 additions and 82 deletions.
11 changes: 5 additions & 6 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 };

Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/ControlCore.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/EventArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
16 changes: 16 additions & 0 deletions src/cascadia/TerminalControl/EventArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#pragma once

#include "FontSizeChangedArgs.g.h"
#include "TitleChangedEventArgs.g.h"
#include "CopyToClipboardEventArgs.g.h"
#include "ContextMenuRequestedEventArgs.g.h"
Expand All @@ -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:
Expand Down
10 changes: 7 additions & 3 deletions src/cascadia/TerminalControl/EventArgs.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -14,6 +11,13 @@ namespace Microsoft.Terminal.Control
All = 0xffffffff
};


runtimeclass FontSizeChangedArgs
{
Int32 Width { get; };
Int32 Height { get; };
}

runtimeclass CopyToClipboardEventArgs
{
String Text { get; };
Expand Down
17 changes: 5 additions & 12 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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*/,
Expand Down
7 changes: 2 additions & 5 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalControl/TermControl.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
52 changes: 0 additions & 52 deletions src/cascadia/inc/cppwinrt_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 0f61b5f

Please sign in to comment.