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

Fixed #3799: Introduce sendInput command #7249

Merged
4 commits merged into from
Aug 12, 2020
Merged

Fixed #3799: Introduce sendInput command #7249

4 commits merged into from
Aug 12, 2020

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Aug 11, 2020

Summary of the Pull Request

This PR enables users to send arbitrary text input to the shell via a keybinding.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Added the following keybindings:

{ "keys": "p", "command": { "action": "sendInput", "input": "foobar" } },
{ "keys": "q", "command": { "action": "sendInput", "input": "\u001b[A" } },

Ensured that when pressing P "foobar" is echoed to the shell and when pressing Q the shell history is being navigated backwards.

@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Aug 11, 2020
@DHowett
Copy link
Member

DHowett commented Aug 11, 2020

It should be possible to replicate the clear command using a keybinding for \u001b[H\u001b[2J\u001b[3J.

This probably doesn't work because it's unusual to receive the sequence for clearing the screen on the input handle 😄

@lhecker
Copy link
Member Author

lhecker commented Aug 11, 2020

This was my suspicion as well. I've removed the comment from the PR description. Thanks @DHowett!

src/cascadia/TerminalApp/ActionArgs.h Show resolved Hide resolved
@@ -1769,7 +1781,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// - wstr: the string of characters to write to the terminal connection.
// Return Value:
// - <none>
void TermControl::_SendInputToConnection(const std::wstring& wstr)
void TermControl::_SendInputToConnection(const winrt::param::hstring& wstr)
Copy link
Member

Choose a reason for hiding this comment

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

unfortunately ☹️ we can't actually use the winrt::param namespace even though it is literally the best namespace for taking in anything that can convert to hstring

We might have to prefer std::wstring_view, which can be natively converted to an hstring (though it could also crash because hstring expects string_views to be null-terminated)

Copy link
Member

Choose a reason for hiding this comment

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

(I've discussed this with the cppwinrt folks before. param::hstring does everything we might possibly want with regards to automatic promotion to hstring, but it's not part of the public interface :/)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure I understand... Am I supposed to not use winrt::param because it's a private interface, or is there another technical reason?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, just because it is a private namespace.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 11, 2020
@@ -142,6 +142,7 @@ For commands with arguments:
| `scrollUp` | Move the screen up. | | | |
| `scrollUpPage` | Move the screen up a whole page. | | | |
| `scrollDownPage` | Move the screen down a whole page. | | | |
| `sendInput` | Sends some text input to the shell. | `input` | string | The text input to feed into the shell.<br>ANSI escape sequences may be used. Escape codes like `\x1b` must be written as `\u001b`.<br>For instance the input `"text\n"` will write "text" followed by a newline. `"\u001b[D"` will behave as if the left arrow button had been pressed. |
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this classify as "Documentation updated"? Do I need to submit a PR to the docs repo?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, you do!

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 11, 2020
@DHowett
Copy link
Member

DHowett commented Aug 11, 2020

Ah, that might be inconvenient. It looks like we use std::bind to capture _SendInputToConnection... somewhere

@zadjii-msft
Copy link
Member

Ah, that might be inconvenient. It looks like we use std::bind to capture _SendInputToConnection... somewhere

TermControl.cpp:107

        auto inputFn = std::bind(&TermControl::_SendInputToConnection, this, std::placeholders::_1);
        _terminal->SetWriteInputCallback(inputFn);

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I feel like the resources thing is nitpicking, but hey, there are build breaks too so I don't feel so bad making you change them 😛

Otherwise everything else here looks good 👍

@@ -284,6 +288,7 @@ namespace winrt::TerminalApp::implementation
{ ShortcutAction::ToggleFocusMode, RS_(L"ToggleFocusModeCommandKey") },
{ ShortcutAction::ToggleFullscreen, RS_(L"ToggleFullscreenCommandKey") },
{ ShortcutAction::ToggleAlwaysOnTop, RS_(L"ToggleAlwaysOnTopCommandKey") },
{ ShortcutAction::SendInput, RS_(L"SendInputCommandKey") },
Copy link
Member

Choose a reason for hiding this comment

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

IMO this should be mapped to L"", so that we don't accidentally generate a name for a sendInput action that doesn't have a value for input

see for example: CloseOtherTabs

// * "Send Input: ...input..."

return winrt::hstring{
fmt::format(L"{}: {}", RS_(L"SendInputCommandKey"), _Input)
Copy link
Member

Choose a reason for hiding this comment

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

Typically, it's best practice to have just two entirely different resources in Resources.resw for something like this.

In this case though, I don't think we need the version without the argument at all, we could probably get away with a single resource for the formatted version:

  <data name="SendInputCommandKey" xml:space="preserve">
    <value>Send Input: "{0}"</value>
    <comment>{0} will be replaced with a string of input as defined by the user.</comment>
  </data>

Copy link
Member

Choose a reason for hiding this comment

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

I'm suddenly worried about dropping the user's input right into a TextBlock. Should we transform the control characters <0x20 to their "control pictures" equivalents? It's as easy as adding 0x2400 to them!

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 what we do in the debug tap ... which I can't get a screenshot of because we broke it o_O

Copy link
Member

Choose a reason for hiding this comment

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

Or perhaps my right alt key no longer works. This would come as no big surprise.

Copy link
Member

@zadjii-msft zadjii-msft Aug 11, 2020

Choose a reason for hiding this comment

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

Should we transform the control characters <0x20 to their "control pictures" equivalents? It's as easy as adding 0x2400 to them!

I'm hugely in favor of this actually. Raw control characters might definitely will break the text block

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 11, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 11, 2020
@DHowett DHowett dismissed zadjii-msft’s stale review August 11, 2020 23:57

"Otherwise everything else here looks good 👍"

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 12, 2020
@ghost
Copy link

ghost commented Aug 12, 2020

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit a2721c1 into microsoft:master Aug 12, 2020
@lhecker lhecker deleted the issue-3799-send-input branch August 12, 2020 17:05
@damnskippy
Copy link

Thanks for doing this @lhecker! Thanks as well and as always to @zadjii-msft & @DHowett, of course!

@ghost
Copy link

ghost commented Aug 26, 2020

🎉Windows Terminal Preview v1.3.2382.0 has been released which incorporates this pull request.:tada:

Handy links:

@musm
Copy link

musm commented Sep 22, 2020

How would I actually use this functionality to send input from an external program into an active terminal ?

@lhecker
Copy link
Member Author

lhecker commented Sep 22, 2020

@musm This "command" is currently / at this point only for use with key bindings inside the Windows Terminal (WT) app.
You can send input to any Windows application, including WT (!), using the identically named, but unrelated SendInput API and its relatives. Tools like AutoHotkey make use of SendInput to do just that.

@unphased
Copy link

unphased commented Apr 16, 2021

Hi, Windows Terminal sends <ESC>[21;2~ for Shift+F10, but I wanted to configure it to send <ESC>[34~ instead. This seems pretty reasonable, but upon setting the following configuration under actions

{
    "command": {
        "action": "sendInput", 
        "input": "\u001b[34~"
    },
    "keys": "shift+f10"
}

it now just eats the key and does nothing. This shows that the configuration had an effect, but it seems like it's not working properly for escape codes as the documentation suggests. Any ideas?

@unphased
Copy link

unphased commented Apr 16, 2021

I did some tests, and I found that it seems like the key will send if the input matches some kind of whitelist of "known ANSI codes". Although I understand that motivations and justifications for this may exist, it makes the documentation misleading.

For example, \u001b[A thru \u001b[D work, but \u001b[E conspicuously does not work. \u001b[a (lowercase a) also does not work.

@DHowett
Copy link
Member

DHowett commented Apr 16, 2021

We do not have an allowlist. The literal string in the sendInput action is sent through unfiltered. If the receiving application rejects it, that’s not our fault.

@unphased
Copy link

unphased commented Apr 16, 2021

How can I prove that it's just eating the key? That's just what it's doing. I'm currently running

Windows Terminal Preview
Version: 1.7.572.0

I'm searching for an equivalent of cat that is a part of powershell or something similar.

So far the procedure has been

  1. ssh into linux machine
  2. run cat on zsh command line
  3. type shift+f10, ^[[21;2~ is emitted
  4. Add config as described above. save config json file.
  5. type shift+f10, no response
  6. adjust "input" to \u001b[A. save config json file.
  7. type shift+f10, ^[A is emitted
    etc.

@unphased
Copy link

unphased commented Apr 16, 2021

I'm pretty sure then (similar to how i eventually found out that openssh is responsible for making mouse codes fail) that ssh is perhaps at fault here. I now have 2 reasons to do something about that.

@unphased
Copy link

Great, I've got MSYS2 installed, and with the OpenSSH obtained through there it behaves as you described. Mouse events still not working but that's ok for now.

@DHowett
Copy link
Member

DHowett commented Apr 16, 2021

Oh yeah, so SSH 7.7 shipped with Windows actually tells us that it cannot handle VT input and needs us to translate it into Windows events ... so that it can translate them back into VT.

this has since been fixed, but the damage is done for two fairly popular windows versions (1904x and 1836x)

This pull request was closed.
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-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met 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.

Enable sending input to the Terminal with a keybinding
6 participants