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

Allowing richer integration with integrated terminal (co-processes) #13337

Closed
jrieken opened this issue Oct 7, 2016 · 21 comments
Closed

Allowing richer integration with integrated terminal (co-processes) #13337

jrieken opened this issue Oct 7, 2016 · 21 comments
Assignees
Labels
api *duplicate Issue identified as a duplicate of another issue(s) feature-request Request for new features or functionality terminal Integrated terminal issues
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Oct 7, 2016

Popular terminal emulators such as iTerm2 and ConEmu allow for user defined interaction with the content being displayed. For instance there are coprocesses and triggers in iTerm2 which allow for very powerful interactions. For instance I have a coprocess that automagically make diagnostics in VS Code when running tsc -w in iTerm2.

I think VS Code would greatly benefit from such rich interaction capabilities between the terminal and the outside-world

@jrieken jrieken added the feature-request Request for new features or functionality label Oct 7, 2016
@jrieken jrieken assigned jrieken and Tyriar and unassigned jrieken Oct 7, 2016
@Tyriar Tyriar added the terminal Integrated terminal issues label Oct 7, 2016
@Tyriar Tyriar added this to the Backlog milestone Oct 7, 2016
@Tyriar Tyriar added the api label Nov 16, 2016
@Tyriar Tyriar changed the title Allowing richer integration with integrated terminal Allowing richer integration with integrated terminal (co-processes) Nov 16, 2016
@Tyriar
Copy link
Member

Tyriar commented Jan 5, 2017

So triggers seem to be mainly used for syntax highlighting based on some Googling. Moving the tasks framework to an extension built on top of the integrated terminal (#15584) could probably be done using some form of triggers that run a callback as it allows matching on a particular line.

It also doesn't expose the potential security holes that coprocesses do as it doesn't touch stdin.

@Tyriar
Copy link
Member

Tyriar commented Jan 5, 2017

@jrieken can you go into a little more detail for your particular co-process? Could this be done with a trigger if you could grab stdout and listen for a line matching a regex?

@jrieken
Copy link
Member Author

jrieken commented Jan 6, 2017

My co-process goes like this:

#!/usr/local/bin/node

// process.stdin.setEncoding('utf8');

const hook = process.argv[process.argv.length - 1];

const {createConnection} = require('net');

function onError(err) {
  console.error(err);
  process.exit(1);
}

const socket = createConnection(hook, () => {
  socket.removeListener('error', onError);
  process.stdin.pipe(socket);
});

socket.once('error', onError);

It gets started by iTerm and simply pipes stdin to another socket. That socket is read from an extensions and marker are created for whatever comes in (a very simplifies version of the task logic).

The interesting bit is that stdin change when a new line is send but also when an existing line is modified, like a progress spinner renders etc.

The trigger bit in this is more that the trigger can be used to start a process. Like so

  1. Have a trigger that regexes for tsc -w and then start a specific co-process
  2. Make the co-process read and process stdin (which mirrors the terminal contents)

@Tyriar
Copy link
Member

Tyriar commented Jan 6, 2017

I'm concerned about extensions listening to stdin (eg. credentials) without the users knowledge. We could always do something like this not as part of an extension without worrying about that though.

@jrieken
Copy link
Member Author

jrieken commented Jan 6, 2017

It's not stdin of the process that runs. iTerm takes whenever is on the screen and sends it the co-process, by that becoming stdin. That is literally the characters (including control-sequences) that are rendered. From https://www.iterm2.com/documentation-coprocesses.html:

All output in a terminal window (that is, what you see on the screen) is also input to the coprocess. All output from the coprocess acts like text that the user is typing at the keyboard.

@Tyriar
Copy link
Member

Tyriar commented Jan 6, 2017

Ah I was thinking the wrong thing for some reason. Is there anything else other than a Terminal.addTrigger(string, callback) and some way of grabbing the terminal's data stream that would be needed for your use case? Here's an example:

var coprocessNotRunning = false;
var t = window.createTerminal();
t.addTrigger(/tsc -w/, line => {
  console.log('tsc -w started: ' + data);
  if (coprocessNotRunning) {
    coprocessNotRunning = true;
    t.on('data', data => coprocess(data));
  }
});

function coprocess(data) {
    // Do something with the data sent from the pty process
    // Terminate at some point?
    //   t.off('data', coprocess);
}

@Tyriar
Copy link
Member

Tyriar commented Jan 6, 2017

I was looking at problemMatcher.ts yesterday for @dbaeumer's use case which is similar (if not the same) to yours which scans for "problems" and registers them as problems in the product. This is what I was leaning towards for the solution for that:

const tscMatcher = /^([^\s].*)\((\d+|\d+,\d+|\d+,\d+,\d+,\d+)\):\s+(error|warning|info)\s+(TS\d+)\s*:\s*(.*)$/;
t.addTrigger(tscMatcher, lineMatch => {
	const file = lineMatch[1];
	const location = lineMatch[2];
	const severity = lineMatch[3];
	const code = lineMatch[4];
	const message = lineMatch[5];
	// register as a problem
});

It's not critical to turn the trigger on and off here as the tasks framework would launch the process and add the trigger immediately, when the process closes the terminal will no longer accept input.

@jrieken
Copy link
Member Author

jrieken commented Jan 7, 2017

Yeah - I think we don't need to copy the concept on co-process one-by-one but get inspired by the idea of being a 'terminal reader'

@dbaeumer
Copy link
Member

dbaeumer commented Jan 9, 2017

@Tyriar I am not sure if adding the concept of a problem matcher to the terminal (although only done by regexp) is a good idea. ESLint for example needs multiline matching which would complicate things in the terminal. If I am interested in every line would I add a trigger /.*/ ?

@Tyriar
Copy link
Member

Tyriar commented Jan 9, 2017

@dbaeumer so from your perspective Terminal.on('data', someCallback); is all you really need, and you would then do the regex handling yourself?

@dbaeumer
Copy link
Member

@Tyriar yes, that would be sufficient for me.

Tyriar added a commit that referenced this issue Jan 11, 2017
@KalitaAlexey
Copy link

How to use .on('data', callback) to separate stdout and stderr?
I'd like to have that.

@Tyriar
Copy link
Member

Tyriar commented Jan 16, 2017

@KalitaAlexey right now this is only an internal api, good to know though.

@KalitaAlexey
Copy link

@Tyriar,
Have you tried it already?
Does it invoke callback for both stdout and stderr data?

@Tyriar
Copy link
Member

Tyriar commented Jan 16, 2017

It sends whatever is sent to the terminal including escape sequences. This is being changed though to what is printed to the terminal, something like onData(listener: (line: string) => void), see #18453

I'm not sure we can differentiate between stderr from stdout as the process is not being called directly but being run within a pseudoterminal.

@KalitaAlexey
Copy link

@Tyriar,
What if I would like to have a way to process stdout and stderr separately?
onStdoutData and onStderrData will be good enough.
How hard is it to add support for this feature?

@Tyriar
Copy link
Member

Tyriar commented Jan 16, 2017

I think it's a limitation of the library we use to only have a single fd for stderr and stdout. But anyway it's probably some time out before something like this manifests itself in the terminal api. I would need to investigate more deeply at that point.

@BartNetJS
Copy link

@Tyriar you mentioned this thread on #18453 (comment)

So I continue here.

What I try to do is to get the response from the commands in the terminal window into an extension (to evaluate the result and give suggestions).

You said that the ondata will not be exposed to extensions.
Is there another way to grab the command result into an extension.

Or is it possible to create a custom xterm.js integrated terminal?

@Tyriar
Copy link
Member

Tyriar commented Dec 22, 2017

@BartNetJS no it's not possible to get the data right now and needs this or a similar feature to be implemented.

@zikalino
Copy link

@Tyriar so i understand security issues that could occur if extension was able to access Terminal's stdin/stdout.
But how about changing createTerminal function so it could pass stdin/stdout as a parameter.

This way extension could create its own process, or SSH connection or whatever, and then create custom terminal for that connection.

@Tyriar
Copy link
Member

Tyriar commented Apr 5, 2018

This case should be covered by #46192, merging into that

@Tyriar Tyriar closed this as completed Apr 5, 2018
@Tyriar Tyriar added the *duplicate Issue identified as a duplicate of another issue(s) label Apr 5, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators May 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api *duplicate Issue identified as a duplicate of another issue(s) feature-request Request for new features or functionality terminal Integrated terminal issues
Projects
None yet
Development

No branches or pull requests

6 participants