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

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

Merged
merged 31 commits into from
Jul 19, 2023

Conversation

zadjii-msft
Copy link
Member

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.

Co-authored-by: Don-Vito khvitaly@gmail.com

@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Nov 15, 2022
@@ -1190,6 +1203,21 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}

bool TermControl::BroadcastKeyEvent(const WORD vkey,
Copy link
Member

Choose a reason for hiding this comment

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

Additionally, is there a way to converge this with IDirectKeyListener's manual key handler?

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.

Ideology discussion

src/cascadia/TerminalControl/ControlCore.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 15, 2022
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.

Okay, now that I've nitted more naming concerns (with apologies; I just think that while we're doing this we might as well get it right...) I've got some deeper worries!

Does anything that inputs a whole string at once work? This includes...

  • IME
  • Clipboard
  • Drag/drop a filename

If I had to guess, the one most likely to cause folks trouble is the clipboard. Rarely am I going to key in a long command that I wanted to send to four servers. I'm almost certainly gonna paste it.

Also, what does mouse input do? Should it do something? I imagine clicking on things that are different sizes and locations will just suck... but what about using the scroll wheel?

I guess this is where the layering discussion comes up again. Bah.

src/cascadia/TerminalApp/App.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/ControlCore.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/ControlCore.h Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 16, 2022
@zadjii-msft
Copy link
Member Author

zadjii-msft commented Nov 16, 2022

WIP, it's the EOD here

  • IME
    • TermControl::_CompositionCompleted. That should be easy.
  • Clipboard
    • TerminalPage::_PasteText this should be easy enough to check if broadcast is available and paste to all the controls.
  • Drag/drop a filename
    • TermControl::_DragDropHandler calls to ControlCore::PasteText in a bunch of places. All these could just CharSent the whole string, which would broadcast it.
  • sendInput

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 16, 2022
@zadjii-msft
Copy link
Member Author

Notes from Teams, abridged

so, the thing that it hinges on for me is
If you have two panes selected, and one is in DECKPAM and the other is in DECCKM or whatever... does one arrow key send two different things properly, or does it send the same thing to both panes?
so, the thing is
if we choose to send the same content on either side, I think that makes our lives easier... and talking with Leonard I am much more comfortable with the "TerminalControl raises an event when it sends text" solution
with one caveat
that event must not ever be raised from any layer unless someone is listening
the cool thing is, events can be checked for truthiness - we don't need to add a separate boolean to plumb down to ControlCore
Leonard talked me into this if i recall
if the answer is 1 ("they send different things") we need to bifurcate all event generation
which.. ugh

okay so
tldr: make sure we don't raise events unless there's a listener

yea... and we can kill the bifurcated "paste" and everything else handlers
and boil it down to the simple "all terminal input sent via Write gets reflected"
like your OG work IIRC

I have been working on reviving the wpf m.t.c.dll
and looking at the public API of TerminalControl
and it's sorta all over the place admittedly but
this feels less intrusive for the API than having to poke through all the different types of user input as event callers
you know?

as usual, you come up with the right solution to begin with and I say something dumb like "can it be totally different" before coming all the way back to a modified version of your original idea

@github-actions

This comment has been minimized.

src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved

control.KeySent(events.keySentToken);
control.CharSent(events.charSentToken);
control.StringSent(events.stringSentToken);
Copy link
Member

Choose a reason for hiding this comment

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

dumb question: do we properly revoke when you move pane to tab (!) or move tab to window (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure seem to

src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
@@ -620,7 +622,7 @@ namespace winrt::TerminalApp::implementation
_nextPaneId++;
}
});

pane->EnableBroadcast(_tabStatus.IsInputBroadcastActive());
Copy link
Member

Choose a reason for hiding this comment

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

this is the scary for me - will a pane moved into a different tab continue broadcasting to the old tab? we don't do any event hookups or unhooks here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, works like a charm

Copy link
Member

Choose a reason for hiding this comment

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

Huh! Can you walk me through why it works?
As a reviewer I couldn't see the machinery that kept it safe. I'd rather know that we know the code is safe than that we tested and found the behavior safe 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I know this is gonna cause you physical pain to read:

  • TerminalPage::_HandleMovePane
    • TerminalPage::_MovePane
      • TerminalTab::DetachPane
        • Pane::DetachPane
          • this._DetachedHandlers -> handled in a lambda in TerminalTab::_AttachEventHandlersToPane
            • revokes all our per-pane event handlers
            • TerminalTab::_DetachEventHandlersFromControl
              • revokes per-control handlers (including these)
      • TerminalTab::AttachPane
        • reattaches them to the new tab.

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
}
}
}

void TermControl::_pasteTextWithBroadcast(const winrt::hstring& text)
Copy link
Member

Choose a reason for hiding this comment

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

This name really confused me - it actually ISN'T used in paste(!) but it made me double-take because we are manually broadcasting the paste event and the incoming text doesn't come back through here. Does it? Or, does it...

Copy link
Member

Choose a reason for hiding this comment

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

(Since I was re-requested for review) Does this have anything to do with pasting? It looks like it doesn't.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 13, 2023
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

I really don't like how the control flow goes from UI to TermControl, then back up into UI, then back down into TermControl. That seems kind of unnecessary tbh. 😅 Why can't it distribute the input across all TermControls from the get-go?
But since I'm wholly unqualified to say how I'd actually do it differently (maybe this is the only way to do it?), I can't see it as an actual fault in the PR either. ✅

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
}
}
}

void TermControl::_pasteTextWithBroadcast(const winrt::hstring& text)
Copy link
Member

Choose a reason for hiding this comment

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

FYI technically you can use winrt::param::hstring here for 0 copy string passing, but I've heard it's considered an internal API so we can't use it. 😟 (Or can we? Technically we are in control over our code... Oh well...)

@zadjii-msft zadjii-msft requested a review from DHowett July 17, 2023 13:41
@zadjii-msft zadjii-msft assigned DHowett and unassigned DHowett Jul 17, 2023
@DHowett
Copy link
Member

DHowett commented Jul 17, 2023

Why can't it distribute the input across all TermControls from the get-go?

Four words: XAML

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 trust you that it works for moving panes around, but I would love to know why. Approved with comments -- that "paste" thing still confuses me :)

@zadjii-msft zadjii-msft enabled auto-merge (squash) July 19, 2023 15:11
@zadjii-msft zadjii-msft merged commit 6a10ea5 into main Jul 19, 2023
14 of 15 checks passed
@zadjii-msft zadjii-msft deleted the dev/migrie/f/broadcast-resurrections branch July 19, 2023 15:45
@zadjii-msft
Copy link
Member Author

leaving here for future me
broadcast-input-demo-000

zadjii-msft added a commit that referenced this pull request Sep 19, 2023
## Summary of the Pull Request

Resolves the following in #15812 

> - [x] `toggleBroadcastInput` isn't in the default settings
> - [x] The cursors forget to keep blinking if you focus each pane and
then unfocus them
> - [x] They don't stop blinking when you unbroadcast
> - [x] Broadcast border doesn't appear when you make new panes, but
they ARE broadcasted-to!

## References and Relevant Issues
x-ref:
* #2634
* #14393

## Detailed Description of the Pull Request / Additional comments

There was literally no logic in the original PR for starting the cursor
blinking. It's entirely unknowable how that ever worked. This makes it
all much more explicit.

We're taking the hacky `DisplayCursorWhileBlurred` from #15363, and
promoting that to the less-hacky `CursorVisibility`. Broadcast input
mode can use that to force the cursor to be visible always.


The last checkbox in that issue is harder, and I didn't want to further
pollute this delta with the paste plumbing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support broadcast input?
4 participants