Vscode: TerminalVirtualProcess naming discussion

[Tyriar]

Fits more with the rest of the vscode api.

FYI @alexr00 @jrieken

[jrieken]

Hm, not so sure about that. I am always +1 on consistency but I also like that there is the start and shutdown pair...

[GabeDeBacker]

Here is a pull-request that shows an implementation using this API and some of the interesting aspects of doing so.

https://microsoft.visualstudio.com/DefaultCollection/EngSys/_git/devprod.wave.nmakeLanguageServer/pullrequest/3492795?_a=overview

According to @Tyriar, "start" implies "the terminal is ready for events".

Having vscode.TerminalVirtualProcess be disposable it makes it clear that VSCode is responsible for ensuring that the instance is cleaned up. You could say that the "shutdown" method does that as well, but it isn't a widely used contract.

Perhaps rename "start" to something else? Say "terminalReady"?

I was also confused as to why "start" didn't take a cancellation token and return some type of thenable. As it is proposed today, if your terminal virtual process implementation wants to use cancellation of some sort, it has to manage it's own cancellation token source, and fire the "onDidExt" event. Seems like the API could simplify the requirements of extension developers.

If it did so, then the contract becomes even more clear and cancellation of the token or diposal of the terminal virtual process means "stop your task process".

I would propose something like this:

export interface TerminalVirtualProcess2 extends vscode.Disposable {
    onDidWrite: vscode.Event<string>;
    onDidOverrideDimensions?: vscode.Event<vscode.TerminalDimensions | undefined>;
    handleInput?(data: string): void;
    setDimensions?(dimensions: vscode.TerminalDimensions): void;

    /**
     * Indicates that the terminal is ready to handle this virtual process.
     * @param cancellationToken Cancellation token used to stop this virtual process. (This could be an actual OS process, or something else entirely).
     * @returns Optionally returns a process exit code. (Not all virtual processes are backed by an OS process, so requiring a number be returned is odd.)
     */
    readyToHandleProcess?(cancellationToken: vscode.CancellationToken): Thenable<number | void>;
}

[GabeDeBacker]

As an FYI in the pull request above, I've updated it to have an implementation "wrapper" around the current version similar what is proposed above.

[jrieken]

IDK but since TerminalVirtualProcess mimics a process it seems quite reasonable to use the process lingo, e.g start/shutdown. Calling this readyToHandleProcess seems super weird to me as it doesn't explain itself at all

[alexr00]

@jrieken what do you think about having a cancellation token passed in to start and returning a thenable? It seems like that would duplicate shutdown.

[GabeDeBacker]

The fact that VSCode has decided to essentially tie the concept of a process to a terminal is causing a lot of API decisions that may not be the desired outcome.

A terminal is simply an input and output mechanism. What is handling the input and output could be anything. In the use cases thus far, we have already seen two. One tied to an actual operating system process and another handled by an extension directly.

If you removed the word "Process" from "TerminalVirtualProcess" then, in my opinion, the API is much more aligned with what it actually is.

export interface ExtensionTerminal extends vscode.Disposable {
    onDidWrite: vscode.Event<string>;
    onDidOverrideDimensions?: vscode.Event<vscode.TerminalDimensions | undefined>;
    handleInput?(data: string): void;
    setDimensions?(dimensions: vscode.TerminalDimensions): void;
    readyToHandleProcess?(cancellationToken: vscode.CancellationToken): Thenable<number | void>;
}

[jrieken]

what do you think about having a cancellation token passed in to start and returning a thenable?

Not a fan of that - for me a cancellation token that is passed into a function should only be valid for the time the function runs, e.g returns or resolves a returned promise. Keeping a token as state feels weird and I am much more in favour of explicit start/stop signals.

[Tyriar]

You got me thinking with ExtensionTerminal 🤔. Behind the scenes it's pretending to be a process but maybe bringing that terminology into the API is complicating things? I'm starting to lean towards bringing in "pty" into the name as that's more accurately describing what it's pretending to be, maybe something like this:

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>;
    onDidExit?: Event<number>;
    handleInput?(data: string): void;
    setDimensions?(dimensions: TerminalDimensions): void;
    shutdown?(): void;
    start?(initialDimensions: TerminalDimensions | undefined): void;
}

---

We could also remove the exit code idea and just not handle that for these terminals, leave it up to the extension to show the error notification if it wants to? Here are the options there:

interface Psuedoterminal {
    // a. Show a standard "the process has exited" notification when a non-zero number is used
    onDidExit?: Event<number>;

    // b. Don't hook these in with the terminal's regular system since it's not an actual
    // process, instead the extension can show a notification using the regular extension
    // API. The big benefit of this one is we don't need to mention or workaround the
    // fact that exit codes must be positive numbers.
    onDidExit?: Event<void>;

    // c. Support both options
    onDidExit?: Event<number | void>;
}

---

Another thought is making start non-optional as extensions might try to write before it's called otherwise.

[jrieken]

I like Pseudoterminal (tho maybe better with upper-case T, eg. PseudoTerminal). Also, agree that start and also shutdown shouldn't be optional, esp. since it's easy not to do anything in case an extension doesn't have a shutdown-routine

[jrieken]

Question is if shutdown and onDidExit should be aligned, name-wise. Regarding the event type of onDidExit I am unsure. Right, it's not a real process but it's I think it still makes somewhat sense.

[Tyriar]

@jrieken

tho maybe better with upper-case T, eg. PseudoTerminal

Pseudo-terminal and pseudoterminal are both popular usages, I think the latter is more correct though as that's what's used on the pty man page.

Question is if shutdown and onDidExit should be aligned, name-wise.

onDidExit/exit, onDidClose/close, onDidClose/dispose?

[jrieken]

I like that best: onDidExit/exit

[Tyriar]

We could align with pty naming even more and use:

start -> open (like openpty)
onDidExit -> onWillClose (this can trigger close, not sure if it does atm)
shutdown -> close (like close fd, this happens when the terminal instance is being teared down, either by user killing it or onWillClose firing)

[GabeDeBacker]

I like this pattern. Thanks for updating!

[Tyriar]

Fixed in https://github.com/microsoft/vscode/pull/77961, adding an update in https://github.com/microsoft/vscode/issues/70978