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

All TermControl/TermApp/TermConnection event handlers should be TypedEventHandlers #1104

Closed
DHowett-MSFT opened this issue Jun 1, 2019 · 0 comments · Fixed by #15867
Closed
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Milestone

Comments

@DHowett-MSFT
Copy link
Contributor

WinRT 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 TypedEventHandlers.

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:

#define DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(name, eventHandler, sender, args) \
public: \
winrt::event_token name(Windows::Foundation::TypedEventHandler<sender, args> const& handler); \
void name(winrt::event_token const& token) noexcept; \
private: \
winrt::event<Windows::Foundation::TypedEventHandler<sender, args>> eventHandler;

-#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:

-DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(PasteFromClipboard, _clipboardPasteHandlers, TerminalControl::TermControl, TerminalControl::PasteFromClipboardEventArgs);
+DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(PasteFromClipboard, _clipboardPasteHandlers, TerminalControl::PasteFromClipboardEventArgs);
@DHowett-MSFT DHowett-MSFT added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels Jun 1, 2019
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 1, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 3, 2019
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Dec 19, 2019
@zadjii-msft zadjii-msft modified the milestones: Terminal Backlog, Backlog Jan 4, 2022
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 22, 2023
zadjii-msft added a commit that referenced this issue Aug 24, 2023
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.
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Aug 24, 2023
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.) In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants