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

Change TerminalRenderer API to "Extension Terminal" #70978

Closed
Tyriar opened this issue Mar 22, 2019 · 17 comments · Fixed by #76481
Closed

Change TerminalRenderer API to "Extension Terminal" #70978

Tyriar opened this issue Mar 22, 2019 · 17 comments · Fixed by #76481
Assignees
Labels
api api-proposal feature-request Request for new features or functionality on-testplan terminal Integrated terminal issues
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Mar 22, 2019

Initially proposed in #67923 (comment), the TerminalRenderer API will be be deprecated and replaced by a simpler to understand model where an extension implements a "virtual process," essentially acting as a complete replacement for the terminal's process.

export function createTerminal(options: TerminalOptions | TerminalVirtualProcessOptions): Terminal;

export interface TerminalVirtualProcessOptions {
	// For a name property for TerminalVirtualProcessOptions.
	// Note that this is mandatory here as there's no process/shell to grab the title from
	name: string;

	virtualProcess: TerminalVirtualProcess;

	// Allows Windows or non-Windows local link handler to be used based on Live Share host OS
	os?: OperatingSystem;

	// Allows ~ to be resolved in Live Share
	userHome?: string;
}

interface TerminalVirtualProcess {
	// The ext should fire this when they want to write to the terminal
	write: Event<string>;

	// Lets the extension override the dimensions of the terminal
	overrideDimensions?: Event<TerminalDimensions>;

	// Lets the extension exit the process with an exit code, this was not in the TerminalRenderer
	// API but it makes sense to include this as it's the main thing missing for a virtual process
	// to truly act like a process
	exit?: Event<number>;

	// This will be called when the user types
	onDidAcceptInput?(text: string): void;

	// This is called fire when window.onDidChangeTerminalDimensions fires as CustomExecution need
	// access to the "maximum" dimensions and don't want access to Terminal
	onDidChangeDimensions?(dimensions: TerminalDimensions): void;
}

export class CustomExecution {
	constructor(virtualProcess: TerminalVirtualProcess, callback: (cancellationToken: CancellationToken, thisArg?: any) => Thenable<number>);
	callback: (cancellationToken: CancellationToken, thisArg?: any) => Thenable<number>;
}

Example usage:

const writeEmitter: EventEmitter<string>();
const virtualProcess: TerminalVirtualProcess = {
	write: writeEmitter.event,
	onDidAcceptInput: (data) => {
		// do something with typed input
		writeEmitter.fire('echo: ' + data);
	}
};
const terminal = createTerminal({
  name: 'my process',
  virtualProcess
});
writeEmitter.fire('writing to the terminal');
setTimeout(() => {
	terminal.dispose();
}, 5000);

Some of #67923 might need to be pulled over as there was a lot of work in that PR.

@Tyriar Tyriar added feature-request Request for new features or functionality api terminal Integrated terminal issues api-proposal labels Mar 22, 2019
@Tyriar Tyriar added this to the April 2019 milestone Mar 22, 2019
@Tyriar Tyriar self-assigned this Mar 22, 2019
@IlyaBiryukov
Copy link

Hi,
Thanks for doing this refactoring. Everything looks OK, though I have a couple of questions:

  1. How TerminalVirtualProcessOptions.os and userHome can be used?
  2. What is CustomExecution for and how is it used?

@Tyriar
Copy link
Member Author

Tyriar commented Mar 23, 2019

@IlyaBiryukov

  1. Say you're in a live share session with a Windows host and Linux guest, on the Linux side you would use this:

    {
      ...,
      userHome: windowsHomeDir, // os.homedir() evaluated on host
      os: OperatingSystem.Windows
    }

    Links would then work because the terminal link logic on the guest knows how to evaluate ~ and to handle Windows-style links (ie. C:\...).

  2. CustomExecution is a new tasks API that depends on terminal renderers, you shouldn't need to worry about that part.

@jrieken
Copy link
Member

jrieken commented Mar 25, 2019

@Tyriar Why must the userHome and os sit on the process? Is that because the terminal does link detection by itself? How would it have worked in the old terminal-renderer approach?

@Tyriar
Copy link
Member Author

Tyriar commented Mar 25, 2019

@jrieken it never worked with the old approach. It sits on the virtual process because in the case of live share the home dir and OS could differ from whatever environment vscode is running on. It's owned by TerminalProcessManager on the renderer side:

https://github.com/Microsoft/vscode/blob/2b0b602af887c3239df519597935bdfb1c9b7768/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts#L48-L49

And yeah, the link detection needs to evaluate this stuff and validate it before triggering FS calls to verify the file exists.

@jbruett
Copy link

jbruett commented Jun 16, 2019

@Tyriar what's the story with this issue. It's been moved in and out of the monthly milestone queue for two months without any follow-up comments. We're waiting on this fix as a dependency in another project. Additional info would be helpful here.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 16, 2019

@jbruett it's not a priority right now. If you're using terminal renderers I'd subscribe to this issue and #69865 and watch out for a notification as the plan is still to do this and deprecate/remove terminal renderers eventually.

@gdraganic
Copy link

@Tyriar any estimate on implementing TerminalVirtualProcess in the API?

@Tyriar
Copy link
Member Author

Tyriar commented Jun 19, 2019

Nope 🙁

@Tyriar
Copy link
Member Author

Tyriar commented Jul 1, 2019

@gdraganic ETA for first cut is now sometime this week in Insiders

@Tyriar
Copy link
Member Author

Tyriar commented Jul 2, 2019

Updated proposal below, main changes from original:

  • Removed os and userHome for the time being to simplify the change, we can add this later on if we want to
  • overrideDimensions accepts undefined
  • Added onDidShutdownTerminal to allow extensions to handle when users exit the terminal (eg. bin icon)
  • Added docs/examples
export namespace window {
	/**
	 * Creates a [Terminal](#Terminal) where an extension acts as the process.
	 *
	 * @param options A [TerminalVirtualProcessOptions](#TerminalVirtualProcessOptions) object describing the
	 * characteristics of the new terminal.
	 * @return A new Terminal.
	 */
	export function createTerminal(options: TerminalVirtualProcessOptions): Terminal;
}

/**
 * Value-object describing what options a virtual process terminal should use.
 */
export interface TerminalVirtualProcessOptions {
	/**
	 * A human-readable string which will be used to represent the terminal in the UI.
	 */
	name: string;

	/**
	 * An implementation of [TerminalVirtualProcess](#TerminalVirtualProcess) that allows an
	 * extension to act as a terminal's backing process.
	 */
	virtualProcess: TerminalVirtualProcess;
}

/**
 * Defines the interface of a terminal virtual process, enabling extensions to act as a process
 * in the terminal.
 */
interface TerminalVirtualProcess {
	/**
	 * An event that when fired will write data to the terminal. Unlike
	 * [Terminal.sendText](#Terminal.sendText) which sends text to the underlying _process_,
	 * this will write the text to the terminal itself.
	 *
	 * **Example:** Write red text to the terminal
	 * ```typescript
	 * const writeEmitter = new vscode.EventEmitter<string>();
	 * const virtualProcess: TerminalVirtualProcess = {
	 *   write: writeEmitter.event
	 * };
	 * vscode.window.createTerminal({ name: 'My terminal', virtualProcess });
	 * writeEmitter.fire('\x1b[31mHello world\x1b[0m');
	 * ```
	 *
	 * **Example:** Move the cursor to the 10th row and 20th column and write an asterisk
	 * ```typescript
	 * writeEmitter.fire('\x1b[10;20H*');
	 * ```
	 */
	write: Event<string>;

	/**
	 * An event that when fired allows overriding the [dimensions](#Terminal.dimensions) of the
	 * terminal. Note that when set the overridden dimensions will only take effect when they
	 * are lower than the actual dimensions of the terminal (ie. there will never be a scroll
	 * bar). Set to `undefined` for the terminal to go back to the regular dimensions.
	 *
	 * **Example:** Override the dimensions of a terminal to 20 columns and 10 rows
	 * ```typescript
	 * const dimensionsEmitter = new vscode.EventEmitter<string>();
	 * const virtualProcess: TerminalVirtualProcess = {
	 *   write: writeEmitter.event,
	 *   overrideDimensions: dimensionsEmitter.event
	 * };
	 * vscode.window.createTerminal({ name: 'My terminal', virtualProcess });
	 * dimensionsEmitter.fire({
	 *   columns: 20,
	 *   rows: 10
	 * });
	 * ```
	 */
	overrideDimensions?: Event<TerminalDimensions | undefined>;

	/**
	 * An event that when fired will exit the process with an exit code, this will behave the
	 * same for a virtual process as when a regular process exits with an exit code.
	 */
	exit?: Event<number>;

	/**
	 * Implement to handle keystrokes in the terminal or when an extension calls
	 * [Terminal.sendText](#Terminal.sendText). Keystrokes are converted into their
	 * corresponding VT sequence representation.
	 *
	 * @param data The sent data.
	 *
	 * **Example:** Echo input in the terminal. The sequence for enter (`\r`) is translated to
	 * CRLF to go to a new line and move the cursor to the start of the line.
	 * ```typescript
	 * const writeEmitter = new vscode.EventEmitter<string>();
	 * const virtualProcess: TerminalVirtualProcess = {
	 *   write: writeEmitter.event,
	 *   onDidAcceptInput: data => writeEmitter.fire(data === '\r' ? '\r\n' : data);
	 * };
	 * vscode.window.createTerminal({ name: 'Local echo', virtualProcess });
	 * ```
	 */
	onDidAcceptInput?(data: string): void;

	/**
	 * Implement to handle when the number of rows and columns that fit into the terminal panel
	 * changes, for example when font size changes or when the panel is resized. The initial
	 * state of a terminal's dimensions should be treated as `undefined` until this is triggered
	 * as the size of a terminal isn't know until it shows up in the user interface.
	 *
	 * @param dimensions The new dimensions.
	 */
	onDidChangeDimensions?(dimensions: TerminalDimensions): void;

	/**
	 * Implement to handle when the terminal shuts down by an act of the user.
	 */
	onDidShutdownTerminal?(): void;
}

@jrieken
Copy link
Member

jrieken commented Jul 3, 2019

Looking pretty good but there are some naming confusions with events, they should be onDid-prefixed, e.g onDidExit and onDidChangeOverrideDimensions

@Tyriar
Copy link
Member Author

Tyriar commented Jul 3, 2019

@jrieken should I flip the other names as well, something like this?

onDidWrite: Event<string>;
onDidChangeDimensionsOverride?: Event<TerminalDimensions | undefined>;
onDidExit?: Event<number>;
acceptInput?(data: string): void;
setDimensions?(dimensions: TerminalDimensions): void;
shutdown?(): void;

@jrieken
Copy link
Member

jrieken commented Jul 3, 2019

Yeah, that makes sense

Tyriar added a commit that referenced this issue Jul 3, 2019
Tyriar added a commit that referenced this issue Jul 3, 2019
@Tyriar
Copy link
Member Author

Tyriar commented Jul 3, 2019

Just pushed the name changes:

onDidWrite
onDidOverrideDimensions
onDidExit
handleInput
setDimensions
shutdown

Feedback always welcome 😃

Tyriar added a commit that referenced this issue Jul 8, 2019
@Tyriar
Copy link
Member Author

Tyriar commented Jul 12, 2019

Latest changes were adding a start method so the virtual process knows when events are safe to fire, initial dimensions are passed into that function from #77298:

start?(initialDimensions: TerminalDimensions | undefined): void;

@Tyriar
Copy link
Member Author

Tyriar commented Jul 26, 2019

Ok hopefully the final round of changes have just happened, here they are:

Name changes:

  • TerminalVirtualProcessOptions -> ExtensionTerminalOptions
  • TerminalVirtualProcess -> Pseudoterminal
  • start -> open
  • shutdown -> close
  • onDidExit -> onDidClose

Other changes:

  • open and close are now both mandatory
  • onDidClose is now an Event<void>, no longer accepting an exit code. Extensions can handle errors via notifications or some other mechanism.

Changes considered but rejected:

  • I considered when onDidClose is fired to call into close but decided against it, they're just the 2 ways a pty can be closed (via ext or ext host respectively). Both close and onDidClose can handle teardown how every they want, even if it's just calling close when onDidClose is fired.

Current proposed API:

export namespace window {
	export function createTerminal(options: ExtensionTerminalOptions): Terminal;
}

export interface ExtensionTerminalOptions {
	name: string;
	pty: Pseudoterminal;
}

interface Pseudoterminal {
	onDidWrite: Event<string>;
	onDidOverrideDimensions?: Event<TerminalDimensions | undefined>;
	onDidClose?: Event<void>;
	open(initialDimensions: TerminalDimensions | undefined): void;
	close(): void;
	handleInput?(data: string): void;
	setDimensions?(dimensions: TerminalDimensions): void;
}

@Tyriar Tyriar changed the title Change TerminalRenderer API to TerminalVirtualProcess Change TerminalRenderer API to "Extension Terminal" Jul 29, 2019
@Tyriar
Copy link
Member Author

Tyriar commented Aug 5, 2019

Stabilize issue: #78514
Remove TerminalRenderer issue: #69865

@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-proposal feature-request Request for new features or functionality on-testplan terminal Integrated terminal issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants