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

Broadcast Input ship list #15812

Closed
4 of 6 tasks
Tracked by #16600
zadjii-msft opened this issue Aug 9, 2023 · 6 comments
Closed
4 of 6 tasks
Tracked by #16600

Broadcast Input ship list #15812

zadjii-msft opened this issue Aug 9, 2023 · 6 comments
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Aug 9, 2023

From the 1.19.2201 Bug Bash.
x-ref:

Tasks

  1. Area-TerminalControl Area-User Interface Help Wanted Issue-Bug Product-Terminal
    zadjii-msft

dev/migrie/b/15812-broadcast-nits

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 9, 2023
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 9, 2023
@zadjii-msft zadjii-msft added this to the Terminal v1.19 milestone Aug 9, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 9, 2023
@zadjii-msft zadjii-msft added the Severity-Blocking We won't ship a release like this! No-siree. label Aug 31, 2023
@zadjii-msft

This comment was marked as resolved.

@zadjii-msft
Copy link
Member Author

  • TermControl::_PasteCommandHandler is called for the context menu. (The same path is followed for the normal r-click paste)
    • ControlInteractivity::RequestPasteTextFromClipboard raises a PasteFromClipboard event
      • handled in TerminalPage::_PasteFromClipboardHandler
        • Calls the event's HandleClipboardData() property
          • calls to ControlInteractivity::_sendPastedTextToConnection
            • calls ControlCore::PasteText

BUT

TermControl::_pasteTextWithBroadcast is only on TermControl (while the rest of that madness occurs in TerminalPage and ControlInteractivity). GAH.


When we do a paste action (TerminalPage::_PasteText), we just have every control in the tab do a TermControl::PasteTextFromClipboard, which does a _interactivity.RequestPasteTextFromClipboard. The app is orchestrating them all asking for the clipboard contents.

@zadjii-msft
Copy link
Member Author

a dumb solution:

  • don't have the app orchestrate telling all the broadcasted-to panes ask for a paste on paste.
  • INSTEAD on ControlInteractivity::_sendPastedTextToConnection, raise a StringSent to TermControl.
    • TermControl can then check if it's got _StringSentHandlers and re-raise if needed

I feel like that's how it was originally done in an earlier form of #14393...

@DHowett
Copy link
Member

DHowett commented Sep 18, 2023

I remember we were talking about adding an API to TermControl along the lines of WriteString(Source, String), where Source would control whether it was (1) bracketed (2) stripped of control sequences (3) etc. Like, "Pasted" strings got the configured treatment, Raw or Input or whatever strings get no treatment at all.

That would afford us the ability to add Source::Broadcast (as a flag(!)), funnel almost all inputs through WriteString, and not accidentally rebroadcast broadcast strings.

@zadjii-msft
Copy link
Member Author

okay something else I tried: 35aba0c

even simpler than before. The way this is layered:

  • TerminalPage will only be asked to look up the clipboard once
  • It will apply the paste warnings as needed. One time.
  • Then, it'll write it to the control that asked for the paste's callback.
  • The control itself will then StringSent that text to the other controls.
    • The sending control does get to do the bracketed paste thing
    • The others however just get the RawWriteString. That ends up in a non-bracketed paste
      • uhg okay, so I would need to send the source with the StringSentEventArgs. That'll pollute the tree a bit...

@zadjii-msft zadjii-msft self-assigned this Sep 18, 2023
zadjii-msft added a commit that referenced this issue 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.
@zadjii-msft zadjii-msft removed the Severity-Blocking We won't ship a release like this! No-siree. label Sep 26, 2023
@zadjii-msft zadjii-msft removed their assignment Sep 26, 2023
@zadjii-msft zadjii-msft self-assigned this Oct 17, 2023
@zadjii-msft zadjii-msft removed their assignment Nov 10, 2023
@zadjii-msft
Copy link
Member Author

Closing since there's only one thing left in here (and we've shipped a couple times with that one thing)

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

2 participants