Move terminal renderer API to stable

[Tyriar]

Fixes #58660

FYI @chrmarti @GabeDeBacker @alpaix @IlyaBiryukov @alexr00 @dbaeumer

[Tyriar]

Plan:

• Add show, dispose, hide to TerminalRenderer
• Deprecate TerminalRenderer.terminal and keep in proposed (to be removed in future version)
• Move event to be global (deprecate old command and move to proposed)
• Only fire event when someone is listening
• Stop firing event when everyone stops listening
• Keep all old APIs around so Live Share can adapt to the change

[jrieken]

• Only fire event when someone is listening

The Emitter has some helpers that makes this easy:
https://github.com/Microsoft/vscode/blob/41c7d3788f254be5fa75c4738086cf037f9ff7ab/src/vs/base/common/event.ts#L364-L371

[Tyriar]

@jrieken ready for another review, see #67923 (comment) for a summary of the changes. I've kept the old APIs are in proposed to give extensions a chance to move off them, I created #69865 for May to remind me to do this.

[jrieken]

not anymore, right?

[Tyriar]

It's still true, there's just no longer a direct link on TerminalRenderer to the Terminal.

[jrieken]

That would be a "normal" terminal being discoverable via window.terminals, window.activeTerminal etc?

[chrmarti]

I would need an 'onClose' event to dispose of all resources behind the renderer.

[Tyriar]

@chrmarti you used to be able to do this:

const renderer = window.createTerminalRenderer();
window.onDidCloseTerminal(t => {
  if (t === renderer.terminal) {
    // dispose
  }
});

Now the only option for this is:

const renderer = window.createTerminalRenderer({ name: 'my renderer 1' });
window.onDidCloseTerminal(t => {
  if (t.name === renderer.name) {
    // dispose
  }
});

But that won't work if you have multiple renderers with the same name.

@jrieken should we add this?

export interface TerminalRenderer {
	readonly onDidClose: Event<void>;
}

[jrieken]

Sorry, but I think this is a big misunderstanding because I was under the impression that no terminal exist for a renderer and that no such events (terminal, not terminal renderer) are fired. I am surprised that a terminal renderer can be observed/detected via a terminal and I am not sure I understand why that is.

[Tyriar]

@jrieken extensions have access to all Terminals created, even when its backed by a TerminalRenderer. Imagine some extension creates a TerminalRenderer, live share will still want to multiplex it with its guests.

[jrieken]

Ok, dumb question: If a terminal is always created when creating a terminal renderer why isn't the renderer passed as creation-option when creating a terminal?

Why is a terminal the side effect of creating a terminal renderer? The doc says "This is similar to an output window but has the same VT sequence compatibility as the regular terminal." which I think is misleading because it seems that a renderer isn't anything that can stand alone and something that always surfaces as terminal. That drove my feedback of removing the renderer.terminal-pointer and adding show/hide et al function but I think that was with a false understanding.

Another part of the docs contradicts the output channel analogy "the TerminalRenderer essentially acts as a process" and if a renderer is more or less a process replacement than it seems we should design it different, e.g. make creating a renderer free of side effect, allow a terminal to be created with a renderer, don't call it renderer but FakeProcess, VirtualProcess, or something like it

[Tyriar]

@jrieken so something more like this?

interface TerminalOptions {
  virtualProcess?: TerminalVirtualProcess;
}

interface TerminalVirtualProcess {
  dimensionsOverride: TerminalDimensions | undefined;
  write(text: string): void);
  readonly onDidAcceptInput: Event<string>;
}

Certainly looks simpler than what we landed on.

[jrieken]

Yes, something along those lines. I think that would make everything much clearer.

[Tyriar]

After a discussion in the API sync this is what we have so far for a rethink/simplification of terminal renderers:

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

export interface TerminalVirtualProcessOptions {
	// For a name property for TerminalVirtualProcessOptions (it's optional on TerminalOptions)
	name: string;
	virtualProcess: TerminalVirtualProcess;
}

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);

@GabeDeBacker @alpaix @IlyaBiryukov @alexr00 feedback welcome as this impacts Live Share and CustomExecution.

[GabeDeBacker]

Couple of comments.

First around naming. The instance members and the callbacks for custom execution names are:

virtualProcess: TerminalVirtualProcess

Which when just reading the names implies there is something else called a "virtual process". I would prefer keeping the name verbose.

terminalVirtualProcess: TerminalVirtualProcess

In the custom execution case, the virtual terminal process is passed into the constructor.

I'd like to see an example of using this with custom executions. At first glance, it seems a little odd for a consumer of the API to have to create an event emitter just to write to the console.

I believe custom task executions should perhaps be passed something that "wraps" this concept because writing output and receiving input is something tasks should just "get" (or maybe even be required) to do. @alexr00 thoughts on this?

[alexr00]

I actually think that the current proposal for TerminalVirtualProcess makes a lot of sense. Implementing the interface should be very lightweight for tasks with Custom Execution. I also think it aligns well with the place a custom task execution takes in how tasks work. We used have a process or a script. With this we have a process, a script, and something that isn't a process, but acts like one.

[GabeDeBacker]

@Tyriar and @alexr00 - Sounds good. One final question, should the virtual terminal process be optional for the custom execution? Is it required for a task to actually produce output?

I.e.

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

	callback: (cancellationToken: CancellationToken, thisArg?: any) => Thenable<number>;
}

I'm okay with saying a custom execution is expected to produce terminal output and hence it is required, but I'd like @alexr00 to weigh in.

Otherwise, I'm good, other than considering using more verbose names.

[Tyriar]

I think for live share to support linking in the terminal we would also need the following:

interface TerminalVirtualProcess {
  os?: OperatingSystem;
  userHome?: string;
}

[alexr00]

@GabeDeBacker to encourage custom task users to always provide some kind of output (hiding the output is something that individuals can configure in tasks.json) lets keep the VirtualTerminalProcess a required parameter.

[Tyriar]

Closing in favor of #70978