Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the FontSizeChanged event from TermControl #15867

Merged
merged 4 commits into from
Aug 24, 2023

Conversation

zadjii-msft
Copy link
Member

I originally just wanted to close #1104, but then discovered that hey, this event wasn't even used anymore. Excerpts of Teams convo:

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

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.

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Aug 22, 2023
@@ -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_self<FontSizeChangedArgs>(actualNewSize.width, actualNewSize.height));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*make_self<T>() is equivalent to make<T> when T's default interface is the same as the one you're converting to.

For new code, we should prefer the clearer one :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that make works even on a non projected ctor

@zadjii-msft zadjii-msft enabled auto-merge (squash) August 23, 2023 18:36
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that make works even on a non projected ctor

@DHowett
Copy link
Member

DHowett commented Aug 24, 2023

I pushed a merge so that it would kick the CI

@zadjii-msft zadjii-msft merged commit 0f61b5f into main Aug 24, 2023
15 of 17 checks passed
@zadjii-msft zadjii-msft deleted the dev/migrie/b/1104-its-high-time-to-remove-this branch August 24, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All TermControl/TermApp/TermConnection event handlers should be TypedEventHandlers
3 participants