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

Terminal API: clarify onData event #48516

Closed
bpasero opened this issue Apr 24, 2018 · 8 comments
Closed

Terminal API: clarify onData event #48516

bpasero opened this issue Apr 24, 2018 · 8 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug terminal Integrated terminal issues verified Verification succeeded
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Apr 24, 2018

Refs: #48434

I am a bit confused what the onData event for the terminal API actually provides me. It seems to me that each keystroke I do is reflected (even when I press backspace I get some weird [K showing up). I think this deserves more documentation and possibly also a different name. E.g. if this is about notifying whenever something is showing in the integrated terminal, should it be onText or onOutput instead?

@Tyriar
Copy link
Member

Tyriar commented Apr 24, 2018

@bpasero opps I didn't include jsdoc for it.

if this is about notifying whenever something is showing in the integrated terminal, should it be onText or onOutput instead?

This is not what it is, it's data from the raw pty, including escape sequences. That's why #48517 is as expected; because on Windows it redraws the screen whenever you resize. I'll clarify in a comment 👍

@Tyriar Tyriar added this to the April 2018 milestone Apr 24, 2018
@bpasero
Copy link
Member Author

bpasero commented Apr 24, 2018

@Tyriar and what is the use case for having this API?

@Tyriar Tyriar closed this as completed in 9cab59b Apr 24, 2018
@Tyriar
Copy link
Member

Tyriar commented Apr 24, 2018

	export interface Terminal {
		/**
		 * Fires when the terminal's pty slave pseudo-device is written to. In other words, this
		 * provides access to the raw data stream from the process running within the terminal,
		 * including ANSI sequences.
		 */
		onData: Event<string>;
	}

@Tyriar
Copy link
Member

Tyriar commented Apr 24, 2018

@bpasero enabling multiplexing of the terminal.

@bpasero
Copy link
Member Author

bpasero commented Apr 26, 2018

I strongly suggest to stay away from the word slave.

@bpasero bpasero reopened this Apr 26, 2018
@bpasero bpasero added the verification-found Issue verification failed label Apr 26, 2018
@Tyriar
Copy link
Member

Tyriar commented Apr 26, 2018

@bpasero that's the technical term and helps describe exactly what it does, there is a master and a slave side to the pty https://en.wikipedia.org/wiki/Pseudoterminal

@bpasero
Copy link
Member Author

bpasero commented Apr 26, 2018

Dunno, at one point we got bashed for using words such as slave in the code. E.g. we say "master / detail" and not "master / slave".

@Tyriar
Copy link
Member

Tyriar commented Apr 26, 2018

I think this is a little different though as it's an established technical term of which there is no other name, not a UI paradigm we're introducing.

@Tyriar Tyriar closed this as completed Apr 26, 2018
@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug terminal Integrated terminal issues and removed verification-found Issue verification failed labels Apr 26, 2018
@roblourens roblourens added the verified Verification succeeded label Apr 27, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug terminal Integrated terminal issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants