Terminal API: 'sendText' should be more specific

[egamma]

The documentation describes it as Send text to the terminal.

The text is actually a command that is then executed by the terminal (once terminated by a new line).

I would suggest something like this without an addNewLine parameter:

executeCommand(command: string): void;

What is a scenario where you want to append raw text to terminal? If there is one then I would suggest a separate function append.

[jrieken]

@Tyriar Suggestions? Reactions? The confusion about sendText that it's unclear if that is like write or not. For instance is term.sendText('\x1b[31mWelcome to term.js!\x1b[m\r\n'); supposed to insert control sequences (which it doesn't) and allow arbitrary 'drawing' or is really just to a execute command?

[jrieken]

@Tyriar ping

[jrieken]

So, after some talks with @weinand I am getting a better understanding of this. We might along with some improved jsdoc around the topic

[weinand]

I think the sendText makes sense as a low level API (and needs good documentation to explain what it does what it doesn't).

For the next release I suggest to add some runInTerminal API on top of this. In debug land we already provide this to debug adapters:

https://github.com/Microsoft/vscode-debugadapter-node/blob/master/protocol/src/debugProtocol.ts#L175

The VS Code implementation of runInTerminal massages the arguments for the three platforms and then calls sendText:

https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/debug/electron-browser/rawDebugSession.ts#L352

[jrieken]

closing as dupe of #11276

[Tyriar]

Sorry for not seeing this, I took Tuesday off and didn't get through all my issues yesterday.

Terminal.sendText sends the text/data to the underlying pseudo terminal (shell), note that the text is not processed by the terminal (xterm.js) which means that it can't handle ANSI escape codes such as clearing the terminal display or @jrieken's example of printing a formatted string directly to the terminal. sendText(text, false) is essentially write to stdin for the shell process.

The typical usage should be something like this:

let someCommand = 'echo -e "abc"';
terminal.sendText(someCommand);

My concerns with @weinand's runInTerminal wrapper is that it may give the impression that it will always run some command but in actuality it may send the text to the shell while it is running something (which would queue it up in most shells) or it could send it to the program that the shell is currently running such as a command line text editor.

Also any shell could be running at the time, if Ubuntu bash or git bash is running in the Windows integrated terminal then prepareCommand will probably fail. This could even happen if the configured shell is cmd if the user ran another shell inside cmd. This was one of the reasons behind my last minute change the createTerminal to accept a configuration object (#11332), as #10917 will be essential for many consumers (I've already had 2 requests to add it).

[weinand]

@Tyriar yes, my prepareCommand might not handle all cases, but if it handles 95% of them it is still much better than your low level sendText which places the burden of implementing a runInTerminal that works on all platforms and for all shells completely on the extension author.

Can you imagine how many prepareCommand versions will be written for all the extensions that have a need for it? If one of them fixes a bug for the 'bash on Windows' problem, can you imagine how long it takes until that fix has rippled to all the copies of the prepareCommand floating around?

I believe it is preferable to have the promise of a prepareCommand that works in 99.9% of all cases in the core of VS Code, and have all the bugs reported against this coming to the same central place.

We in debug land are looking forward to the first bug filed against prepareCommand because we know that from the corresponding fix all clients will profit automatically.

The problem with commands being stuck in a queue of not yet processed commands could be solved if you would surface whether a terminal is 'busy' executing a command or whether it is 'available'. Or you could automatically launch a new instance of a terminal if the current one is busy.

[Tyriar]

I agree, just pointing out some holes 😃 #10917 will help in alleviating most of the issues.

We went over a few of the hurdles in getting the status of the command in #9957, the most damning of which is the fact that the shell could be running any number of background processes for the life the terminal. So there may not be a way of reliably reporting whether it's truly busy or just idling with a background process. I just created #11422 and referenced these discussions so we don't lose track.