-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[PoC] Allow broadcasting keys to all panes in tab #9222
Conversation
@zadjii-msft, @carlos-zamora - this is a PoC. Simply want to hear some feedback to learn if this is a good direction (product and engineering wise). |
Alright so 👀 wow, this is awesome. I think my real concern though is how the plumbing for this works. It seems like you've got it set up so that input can be broadcast to all the panes in a tab, but not a subset of the panes in a tab, or to panes in other tabs. I think that's going to end up being one of the more important parts of this functionality. The actions that were in the OP look like they are:
So I'm not sure exactly how those play together in certain scenarios, and unfortunately don't have a Mac to test iterm2 myself. The scenario that gets me is like:
Does that mean that you're now broadcasting to the pane in tab 1 and all the panes in tab 2? If at this point you:
now who's getting input? All the panes in tab 3? All the panes in tab 2? All the panes in 2, plus that first pane, plus the active pane in 3? Okay maybe I'm off track. This is a good proof-of-concept, and the bubbling of the events seems sound. So I guess my concern is in the trick of how do we determine which panes should be getting the broadcasted input. I'm all for incremental progress, but I want to make sure we have our eyes on the end goal. Maybe we should have a plan for what the actions will look like in the end, so we can start here and work towards those actions? I'd hate to have release notes with "added the |
@zadjii-msft - I have a subset of what you mentioned, due to the read-only tabs 😄 Please see the following gif below. I tried btw to use color coding of the pane border to show what panes are read-only (red) and what get broadcast (green), but I guess this gonna be an accessibility issue (that IMHO we can resolve only with pane headers) In the gif below we:
The next thing we can do is to add "exclude from broadcast" instead of readonly |
I kinda like the border colorizing. I agree that pane headers would be the best way of solving the indicator problem. However, those are probably still a good ways out. The colorized borders might be a decent temporary solution. I'd definitely want the other broadcasted panes identified in some way. I wonder if we should be using some variant of the accent color (link) for the broadcasted pane's borders. At least until there's a proper way to stylize the borders with theming (#3327). Using a variation on the accent color would make sure that the color of a broadcasted pane border is always distinct, even if the user has some green-like color set as their accent color.
☝️ I like that. After that we can work out how we broadcast to panes in other tabs too. Not to do the whole spec in comments, but I'm thinking: We'll maintain a set of panes that are the "broadcast set". All the panes in the broadcast set would also get the KeySent and CharSent events (in addition to the active pane, which can be a part of the broadcast set). If a pane is read-only in the broadcast set, then it won't handle those broadcasted events (obviously). As far as actions, we're looking at something like:
I'm gonna try socializing this with the team today, but we've already got a lot on the agenda today 😬 this might get bumped slowly, sorry in advance |
Thanks! Don't worry about the timing - my last weeks are hectic, so I don't really get to code 😢 |
@zadjii-msft - just to make sure, given that we already have the read-only, can we postpone the exclusion to the next phases? |
@Don-Vito Sorry, I hadn't had a chance to come back around on this since my last post. I think I want to settle the design of this a bit more. What you've got here is basically the "C send input to all panes in the current tab" action. So I think at this point I just want to try and come up with a design for the actual serialization of all these various actions. That way we can align this PR as implementation for the scenario C action. We won't need to worry about the rest of the implementation now, but I want to have the design be future proof. If anything, the only thing that will need to change for this PR is the json key name. I think I've got a bit of time open in my schedule later this week to whip something more formal up. I'll put together 25% of a spec and socialize that with the team. |
In Linux terminator you can define groups, and than put each pane in one of defined groups. |
Okay sorry for the radio silence here. We talked this over as a team - not all the details in #9365, but we did talk over the important conclusion. Regardless how we end up going about implementing the rest of the broadcast input functionality, we're definitely cool with I'll go take a closer look at the code later. Thanks again for your patience! |
@zadjii-msft - please pay attention that the solution is not final (e.g., I still need to make sure that the cursor blinking is explicitly enabled and disabled in all panes)! If you believe that this PR is a good start, I will continue investing. |
Yea, I definitely think that this PR is a good enough start. Hopefully by the time we're done reviewing it, we can sign off on #9365 and have follow-up issues ready to file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a couple spots I'm suggesting comments only because it's not immediately obvious the difference between _TrySendKeyEvent
and TrySendKeyEvent
. Plus I've got proposal for the pane border color, but I'd love more input on that.
In general, this seems pretty straightforward.
@@ -42,11 +42,15 @@ the MIT License. See LICENSE in the project root for license information. --> | |||
<ResourceDictionary x:Key="Dark"> | |||
<!-- Define resources for Dark mode here --> | |||
<SolidColorBrush x:Key="TabViewBackground" Color="#FF333333" /> | |||
<SolidColorBrush x:Key="ReadOnlyPaneBorderColor" Color="#FFFF6F69" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to make the read-only border it's own PR, just so that it gets its own design review
@@ -42,11 +42,15 @@ the MIT License. See LICENSE in the project root for license information. --> | |||
<ResourceDictionary x:Key="Dark"> | |||
<!-- Define resources for Dark mode here --> | |||
<SolidColorBrush x:Key="TabViewBackground" Color="#FF333333" /> | |||
<SolidColorBrush x:Key="ReadOnlyPaneBorderColor" Color="#FFFF6F69" /> | |||
<SolidColorBrush x:Key="BroadcastPaneBorderColor" Color="#FF96CEB4" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any way we could make the broadcast border SystemAccentColorLight2
/ SystemAccentColorDark2
? That would make it similar to the active pane border, but still definitely different.
@cinnamon-msft for design input here
@@ -53,6 +53,7 @@ static constexpr std::string_view BreakIntoDebuggerKey{ "breakIntoDebugger" }; | |||
static constexpr std::string_view FindMatchKey{ "findMatch" }; | |||
static constexpr std::string_view TogglePaneReadOnlyKey{ "toggleReadOnlyMode" }; | |||
static constexpr std::string_view NewWindowKey{ "newWindow" }; | |||
static constexpr std::string_view ToggleInputBroadcastKey{ "toggleInputBroadcast" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hmm. I legit do not know whether this should be
toggleInputBroadcast
, "Toggle input broadcast", ortoggleBroadcastInput
, "Toggle broadcast input"
In my head, I've been calling it "broadcast input mode", which lines up with the menu entry in iTerm2, but I don't know if it makes more sense the other way around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the latter option personally. Sounds more sensible.
e.Handled(handled); | ||
} | ||
|
||
// Method Description: | ||
// - Sends character to terminal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// - Sends character to terminal | |
// - Sends character to terminal. Might be called by the CharacterHandler | |
// for a char typed in this terminal, or by the application for a | |
// character broadcasted to us from another terminal. |
@@ -1111,6 +1122,25 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation | |||
} | |||
} | |||
|
|||
// Method Description: | |||
// - Invokes TrySendKeyEvent and triggers KeySent event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// - Invokes TrySendKeyEvent and triggers KeySent event | |
// - Invokes TrySendKeyEvent and triggers KeySent event. This is only called | |
// for keys that are directly typed into this control |
const auto modifiers = keyStates.Value(); | ||
const auto result = TrySendKeyEvent(vkey, scanCode, modifiers, keyDown); | ||
auto keySentArgs = winrt::make<KeySentEventArgs>(vkey, scanCode, modifiers, keyDown); | ||
_keySentHandlers(*this, std::move(keySentArgs)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_keySentHandlers(*this, std::move(keySentArgs)); | |
// Raise the keySent event to broadcast this key to other potential terminal controls | |
_keySentHandlers(*this, std::move(keySentArgs)); |
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
Dear bot, I know that I should have been working on this.. but the week was hectic - please don't close it. |
yea jeez bot cool it. Take your time - we're in no huge rush to merge this one |
Do you have a prerelease version or something like that in order to check it? |
Hey so for the record, I'm about to blow up |
NP. I think this one can be done in parts. We can start with:
We can start with something very basic and improve it gradually. Because, it seems that smaller contributions work while in bigger ones it takes time and by reply I already lose the focus. @zadjii-msft - do you want to go through this journey with me? |
@Don-Vito I'm happy to help with this. It generally sems like a good plan, and I think it'll make a lot of folks happy. The only thing that I'm not totally sure about is
That one I'm a little worried about only for a couple reasons. One - if we just go with like, a red border, then I'm worried that someone will have their accent color set to that same color, so the border doesn't actually provide any new info. However, if we don't go with the border color, then what do we go with? That question starts getting into the UI design realm, and those kinds of questions are always harder to come to a consensus. We've already got so many design questions swirling around the settings UI, I'm worried that this would more easily get lost in those weeds. So I kinda want to skip that particular step, because it requires so much more review overhead. The others I think we're all more on board with so those are easier to ship. Does that make sense? |
Linux terminator uses this kind of logic: You can set the group with menu accessible from each pane. For example at the picture bellow, you can see that three panes are in the same group "sboddy". There are many terminals out there, but when linux professionals are in question many of them choose: https://terminator-gtk2.readthedocs.io/en/latest/grouping.html |
Thanks for the feedback @zljubisic. I've moved that comment over to #9365 where we're discussing more of the big picture ideas for broadcast input mode. Thanks! |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
### ⇒ [doc link](https://github.com/microsoft/terminal/blob/dev/migrie/s/2634-broadcast-input/doc/specs/drafts/%232634%20-%20Broadcast%20Input/%232634%20-%20Broadcast%20Input.md) ⇐ ## Summary of the Pull Request This is supposed to be a quick and dirty spec to socialize the various different options for Broadcast Input mode with the team. Hopefully we can come up with a big-picture design for the feature, so we can unblock #9222. ### Abstract > With a viable prototype in #9222, it's important that we have a well-defined > plan for how we want this feature to be exposed before merging that PR. This > spec is intended to be a lighter-than-usual spec to build consensus on the > design of how the actions should be expressed. ... > _**Fortunately**_: All these proposals actually use the same set of actions. So > it doesn't _really_ matter which we pick right now. We can unblock #9222 as > the implementation of the `"tab"` scope, and address other scopes in the future. > We should still decide long-term which of these we'd like, but the actions seem > universal. ## PR Checklist * [x] Specs: #2634 * [x] References: #9222, #4998 * [x] I work here ## Detailed Description of the Pull Request / Additional comments _\*<sup>\*</sup><sub>\*</sub> read the spec <sub>\*</sub><sup>\*</sup>\*_
Resurrection of #9222. Spec draft in #9365. Consensus from community feedback is that the whole of that spec is _nice to have_, but what REALLY matters is just broadcasting to all the panes in a tab. So, in the interest of best serving our community, I'm pushing this out as the initial implementation, before we figure out the rest of design. Regardless of how we choose to implement the rest of the features detailed in the spec, the UX for this part of the feature remains the same. This PR adds a new action: `toggleBroadcastInput`. Performing this action starts broadcasting to all panes in this tab. Keystrokes in one pane will be sent to all panes in the tab. An icon in the tab is used to indicate when this mode is active. Furthermore, the borders of all panes will be highlighted with `SystemAccentColorDark2`/`SystemAccentColorLight2` (depending on the theme), to indicate they're also active. * [x] Closes #2634. - (we should lick a reserved thread for follow-ups) Co-authored-by: Don-Vito khvitaly@gmail.com
Summary of the Pull Request
Introduces ToggleInputBroadcast command,
that makes Terminal Tab to broadcast its input to all panes.
This is an early draft to see if the implementation
and UX make any sense at all.
References
This is an attempt to address #2634.
PR Checklist
Detailed Description of the Pull Request / Additional comments
to other terminals if IsInputBroadcastActive
One of the dilemmas was were to hook the input.
The most robust way would be to do it upon SendInputToConnection.
But it will prevent auto scrolling and deselection.
TODO