Raise ShortcutActions with the sender (tab, control) context #15773
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d the event, as a parameter to ShortcutActionDispatch::DoAction
microsoft-github-policy-service
bot
added
Issue-Bug
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
DHowett
approved these changes
Aug 10, 2023
Comment on lines
+534
to
+535
| winrt::Microsoft::Terminal::Control::TermControl _senderOrActiveControl(const winrt::Windows::Foundation::IInspectable& sender); | ||
| winrt::com_ptr<TerminalTab> _senderOrFocusedTab(const IInspectable& sender); |
There was a problem hiding this comment.
For the future: technically, we might want the concept of a "stack" of senders. A message coming from a control has an associated pane and tab and eventually, window. Those things might matter to different recipients, or different action types.
lhecker
approved these changes
Aug 23, 2023
…ispatch-with-sender
4 tasks
DHowett
approved these changes
Aug 24, 2023
DHowett
pushed a commit
that referenced
this pull request
Sep 22, 2023
This PR's goal is to allow something like a `Tab` to raise a ShortcutAction, by saying "this action should be performed on ME". We've had a whole category of these issues in the past: * #15734 * #15760 * #13579 * #13942 * #13942 * Heck even dating back to #10832 So, this tries to remove a bit of that footgun. This probably isn't the _final_ form of what this refactor might look like, but the code is certainly better than before. Basically, there's a few bits: * `ShortcutActionDispatch.DoAction` now takes a `sender`, which can be _anything_. * Most actions that use a "Get the focused _thing_ then do something to it" are changed to "If there was a sender, let's use that - otherwise, we'll use the focused _thing_". * TerminalTab was largely refactored to use this, instead of making requests to the `TerminalPage` to just do a thing to it. I've got a few TODO!s left, but wanted to get initial feedback. * [x] `TerminalPage::_HandleTogglePaneZoom` * [x] `TerminalPage::_HandleFocusPane` * [x] `TerminalPage::_MoveTab` Closes #15734 (cherry picked from commit 30eb9ee) Service-Card-Id: 90438493 Service-Version: 1.18
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR's goal is to allow something like a
Tabto raise a ShortcutAction, by saying "this action should be performed on ME". We've had a whole category of these issues in the past:So, this tries to remove a bit of that footgun. This probably isn't the final form of what this refactor might look like, but the code is certainly better than before.
Basically, there's a few bits:
ShortcutActionDispatch.DoActionnow takes asender, which can be anything.TerminalPageto just do a thing to it.I've got a few TODO!s left, but wanted to get initial feedback.
TerminalPage::_HandleTogglePaneZoomTerminalPage::_HandleFocusPaneTerminalPage::_MoveTabCloses #15734