[PoC] Allow broadcasting keys to all panes in tab #9222

Don-Vito · 2021-02-19T19:16:49Z

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

Store the IsInputBroadcastActive flag on the tab level
Add KeySent and CharSent events to TermControl
Register on this events in Tab and send the keys / chars
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

Fix the cursor blinking
Use accent colors for color coding

Don-Vito · 2021-02-19T19:17:51Z

Don-Vito · 2021-02-19T19:20:17Z

@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).

carlos-zamora · 2021-02-19T20:30:47Z

I like this proof of concept. I think it'd be nice if the focus border on the pane would encompass all of the panes somehow, just to make it more clear that you're in broadcast mode.

I'm not familiar with how other terminals do this UI-wise. Might be worth taking a look there.

For anybody that comes along, here's a closer look at the new tab icon:

I'll take a closer look at the actual code changes later, but I'm liking the direction this is going in 😀

zadjii-msft · 2021-02-22T17:28:50Z

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:

A Only send input to the active pane
B send input to all panes in all tabs
C send input to all panes in the current tab
D toggle sending input to the current pane

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:

Enable broadcast input for a pane in tab 1
switch to tab 2
execute action C to send input to all the panes in the current tab

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:

Switch to tab 3

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 toggleBroadcastInput action" followed immediately by "removed the toggleBroadcastInput action, which has been replaced with whateverThingWeComeUpWith"

Don-Vito · 2021-02-22T17:34:16Z

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

Make the top read-only (it gets red border when not focused)
Then we enable broadcast on the tab (all tabs that are not read-only get green border)
Then you type

The next thing we can do is to add "exclude from broadcast" instead of readonly

zadjii-msft · 2021-02-22T17:58:56Z

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.

The next thing we can do is to add "exclude from broadcast" instead of readonly

☝️ 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:

A Only send input to the active pane
- Remove all the panes from the broadcast set
B send input to all panes in all tabs
- If all the panes are in the broadcast set, remove them all. Otherwise, add all panes in all tabs to the broadcast set.
C send input to all panes in the current tab
- If all the panes in the current tab are in the broadcast set, remove them from the broadcast set. Otherwise, add all the panes from this tab to the broadcast set.
D toggle sending input to the current pane
- If this pane is in the broadcast set, remove it. Otherwise add it.

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

Don-Vito · 2021-02-22T18:21:52Z

Thanks! Don't worry about the timing - my last weeks are hectic, so I don't really get to code 😢
I hope I won't forget what I've written here 😄

…mpt1

Don-Vito · 2021-02-25T20:44:13Z

@zadjii-msft - just to make sure, given that we already have the read-only, can we postpone the exclusion to the next phases?

zadjii-msft · 2021-03-01T20:57:59Z

@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.

zljubisic · 2021-03-02T19:08:10Z

In Linux terminator you can define groups, and than put each pane in one of defined groups.
Afterwards, you can choose broadcasting to all panes, only certain group or no broadcast at all.
Anyway, what you have done so far looks quite promising. 👍

zadjii-msft · 2021-03-11T15:51:00Z

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 { "command": "toggleBroadcastInput" } (without other args) meaning "send input to all the panes in this tab". We can sort out the rest of the details in the bigger spec review, but this is a good enough place to start ☺️

I'll go take a closer look at the code later. Thanks again for your patience!

Don-Vito · 2021-03-14T05:50:08Z

@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.

zadjii-msft · 2021-03-15T14:23:02Z

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 ☺️

zadjii-msft

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.

zadjii-msft · 2021-03-11T16:17:39Z

src/cascadia/TerminalApp/App.xaml

@@ -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" />


We might want to make the read-only border it's own PR, just so that it gets its own design review

zadjii-msft · 2021-03-11T16:20:12Z

src/cascadia/TerminalApp/App.xaml

@@ -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" />


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

zadjii-msft · 2021-03-11T16:24:15Z

src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp

@@ -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" };


Oh hmm. I legit do not know whether this should be

toggleInputBroadcast, "Toggle input broadcast", or

toggleBroadcastInput, "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

I prefer the latter option personally. Sounds more sensible.

zadjii-msft · 2021-03-15T14:59:25Z

src/cascadia/TerminalControl/TermControl.cpp

        e.Handled(handled);
    }

+    // Method Description:
+    // - Sends character to terminal


Suggested change

// - 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.

zadjii-msft · 2021-03-15T15:01:08Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -1111,6 +1122,25 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
        }
    }

+    // Method Description:
+    // - Invokes TrySendKeyEvent and triggers KeySent event


Suggested change

// - 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

zadjii-msft · 2021-03-15T15:02:04Z

src/cascadia/TerminalControl/TermControl.cpp

+        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));


Suggested change

_keySentHandlers(*this, std::move(keySentArgs));

// Raise the keySent event to broadcast this key to other potential terminal controls

_keySentHandlers(*this, std::move(keySentArgs));

ghost · 2021-03-25T21:00:07Z

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.

Don-Vito · 2021-03-25T21:41:20Z

Dear bot, I know that I should have been working on this.. but the week was hectic - please don't close it.

zadjii-msft · 2021-03-25T21:51:32Z

yea jeez bot cool it. Take your time - we're in no huge rush to merge this one ☺️

zljubisic · 2021-03-27T15:00:06Z

Do you have a prerelease version or something like that in order to check it?

zadjii-msft · 2021-04-23T11:12:55Z

Hey so for the record, I'm about to blow up TermControl in #9820. Merging this with that is probably gonna be a huge pain, lemme know if you need any help (whenever you come back around on this one)

Don-Vito · 2021-04-23T12:33:37Z

Hey so for the record, I'm about to blow up TermControl in #9820. Merging this with that is probably gonna be a huge pain, lemme know if you need any help (whenever you come back around on this one)

NP. I think this one can be done in parts. We can start with:

Indication of read-only panes. This will create a platform for pane status indication.
Indication of panes getting input (both coloring and cursor blinking)
Broadcast support in term control
Tab level broadcasting
5...

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?

zadjii-msft · 2021-04-26T14:34:24Z

@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

Indication of read-only panes. This will create a platform for pane status indication.

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?

zljubisic · 2021-04-26T16:22:37Z

Linux terminator

uses this kind of logic:
every pane can belong to a group that can have arbitrarily name. Than you can press alt+a in order to type in all panes no mether how panes are grouped. Or you can press alt+g in order to type in all panes that are in the same group as the current pane. Or you can press alt+o in order to type only in the current pane.

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

zadjii-msft · 2021-04-27T15:58:55Z

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!

zadjii-msft · 2022-11-07T22:27:06Z

### ⇒ [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 _\*\*\* read the spec \*\*\*_

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

Don-Vito added 3 commits February 19, 2021 11:09

Introduce Pane broadcast

03d3323

Introduce Char broadcast

f77a69d

Add ToggleInputBroadcast command

32c98f5

Avoid broadcasting to read-only panes

38ca587

zadjii-msft added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Feb 22, 2021

zadjii-msft assigned DHowett Feb 22, 2021

Don-Vito added 2 commits February 25, 2021 21:50

Introduce color-coding of read-only and broad-casting panes

4612901

Merge remote-tracking branch 'upstream/main' into 2634-broadcast-atte…

6b961aa

…mpt1

zadjii-msft mentioned this pull request Mar 2, 2021

Support broadcast input? #2634

Closed

zadjii-msft mentioned this pull request Mar 3, 2021

DRAFT Spec for Broadcast Input #9365

Merged

3 tasks

zadjii-msft self-assigned this Mar 11, 2021

zadjii-msft requested changes Mar 15, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 15, 2021

zadjii-msft assigned Don-Vito and unassigned zadjii-msft Mar 15, 2021

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Mar 25, 2021

ghost removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Mar 25, 2021

zadjii-msft mentioned this pull request Apr 20, 2021

Multi-cursor: Keyboard inputs across multiple panes #9899

Closed

zadjii-msft mentioned this pull request Apr 23, 2021

Introduce search status #8588

Closed

6 tasks

Don-Vito closed this Apr 24, 2021

This comment was marked as off-topic.

Sign in to view

zadjii-msft added a commit that referenced this pull request Nov 7, 2022

This is a resurrection of #9222. This needs a lot of love, but it works

c1fb748

zadjii-msft mentioned this pull request Nov 15, 2022

Add support for broadcasting to all panes in a tab #14393

Merged

1 task

-    // - 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.

	// - 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

	_keySentHandlers(*this, std::move(keySentArgs));
	// Raise the keySent event to broadcast this key to other potential terminal controls
	_keySentHandlers(*this, std::move(keySentArgs));

[PoC] Allow broadcasting keys to all panes in tab #9222

[PoC] Allow broadcasting keys to all panes in tab #9222

Conversation

Don-Vito commented Feb 19, 2021 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Don-Vito commented Feb 19, 2021

Don-Vito commented Feb 19, 2021

carlos-zamora commented Feb 19, 2021

zadjii-msft commented Feb 22, 2021

Don-Vito commented Feb 22, 2021 • edited Loading

zadjii-msft commented Feb 22, 2021

Don-Vito commented Feb 22, 2021

Don-Vito commented Feb 25, 2021

zadjii-msft commented Mar 1, 2021

zljubisic commented Mar 2, 2021 • edited Loading

zadjii-msft commented Mar 11, 2021

Don-Vito commented Mar 14, 2021

zadjii-msft commented Mar 15, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Mar 11, 2021

Choose a reason for hiding this comment

zadjii-msft Mar 11, 2021

Choose a reason for hiding this comment

zadjii-msft Mar 11, 2021

Choose a reason for hiding this comment

WSLUser Mar 18, 2021

Choose a reason for hiding this comment

zadjii-msft Mar 15, 2021

Choose a reason for hiding this comment

zadjii-msft Mar 15, 2021

Choose a reason for hiding this comment

zadjii-msft Mar 15, 2021

Choose a reason for hiding this comment

ghost commented Mar 25, 2021

Don-Vito commented Mar 25, 2021

zadjii-msft commented Mar 25, 2021

zljubisic commented Mar 27, 2021

zadjii-msft commented Apr 23, 2021

Don-Vito commented Apr 23, 2021

zadjii-msft commented Apr 26, 2021

zljubisic commented Apr 26, 2021 • edited Loading

zadjii-msft commented Apr 27, 2021

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.

zadjii-msft commented Nov 7, 2022

Don-Vito commented Feb 19, 2021 •

edited

Loading

Don-Vito commented Feb 22, 2021 •

edited

Loading

zljubisic commented Mar 2, 2021 •

edited

Loading

zljubisic commented Apr 26, 2021 •

edited

Loading