Expose shell type to extensions #237624

anthonykim1 · 2025-01-10T07:53:34Z

No description provided.

Tyriar · 2025-01-10T13:22:39Z

src/vs/workbench/api/browser/mainThreadTerminalService.ts

+		const terminalInstance = this._terminalService.getInstanceFromId(terminalId)?.shellType;
+		if (terminalInstance) {
+			this._proxy.$acceptTerminalShellType(terminalId, shellType);
+		}


You'll also want to set it when shellType is undefined or '', otherwise the value could get stale. For example moving from bash to an unknown shell

Tyriar · 2025-01-10T13:22:57Z

src/vs/workbench/api/common/extHost.protocol.ts

@@ -2417,6 +2417,7 @@ export interface ExtHostTerminalServiceShape {
 	$acceptTerminalMaximumDimensions(id: number, cols: number, rows: number): void;
 	$acceptTerminalInteraction(id: number): void;
 	$acceptTerminalSelection(id: number, selection: string | undefined): void;
+	$acceptTerminalShellType(id: number, shellType: string): Promise<TerminalShellType>;


Should return void here

Tyriar · 2025-01-10T13:23:21Z

src/vs/workbench/api/browser/mainThreadTerminalService.ts

@@ -87,6 +87,7 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape
 		this._store.add(_terminalService.onAnyInstanceTitleChange(instance => instance && this._onTitleChanged(instance.instanceId, instance.title)));
 		this._store.add(_terminalService.onAnyInstanceDataInput(instance => this._proxy.$acceptTerminalInteraction(instance.instanceId)));
 		this._store.add(_terminalService.onAnyInstanceSelectionChange(instance => this._proxy.$acceptTerminalSelection(instance.instanceId, instance.selection)));
+		this._store.add(_terminalService.onAnyInstanceShellTypeChange(instance => this._onShellTypeChanged(instance.instanceId, instance.shellType))); // Do I need this? I don't think so


Yes this is the main listener that triggers the send to the exthost

Tyriar · 2025-01-10T13:24:28Z

src/vs/workbench/api/common/extHostTerminalService.ts

@@ -419,6 +419,8 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
 	readonly onDidChangeTerminalState = this._onDidChangeTerminalState.event;
 	protected readonly _onDidChangeShell = new Emitter<string>();
 	readonly onDidChangeShell = this._onDidChangeShell.event;
+	// Should onDidChange/AcceptShellType go here?


Nope, the event would be on the ExtHostTerminal object

Tyriar · 2025-01-10T13:25:38Z

src/vs/workbench/api/common/extHostTerminalService.ts

+	// Take in shellType as a string and return VSCode Terminal Shell Type?
+	public async $acceptTerminalShellType(id: number, shellType: string): Promise<TerminalShellType> {
+		// TODO: Implement
+		return GeneralShellType.Python;
+	}


Look at this for something similar to what you need to do:

vscode/src/vs/workbench/api/common/extHostTerminalService.ts

Lines 686 to 691 in e8184ed

public $acceptTerminalInteraction(id: number): void {

const terminal = this.getTerminalById(id);

if (terminal?.setInteractedWith()) {

this._onDidChangeTerminalState.fire(terminal.value);

}

}

Basically get the terminal for the ID, set it on it (interacted with is also on the vscode.TerminalState object), then fire the onDidChangeTerminalState event.

Tyriar · 2025-01-10T13:26:31Z

src/vs/workbench/contrib/terminal/browser/terminal.ts

@@ -262,7 +262,7 @@ export interface ITerminalService extends ITerminalInstanceHost {
 	readonly onDidRequestStartExtensionTerminal: Event<IStartExtensionTerminalRequest>;
 	readonly onDidRegisterProcessSupport: Event<void>;
 	readonly onDidChangeConnectionState: Event<void>;
-
+	readonly onAnyInstanceShellTypeChanged: Event<ITerminalInstance>; // Here or on proposed api? - also there is onDidChangeShellType that already exists..


onAnyInstance events are helpers we can add when needed, this is a good thing but we should move it into the // Multiplexed events section

Tyriar · 2025-01-10T13:31:10Z

src/vs/workbench/contrib/terminal/browser/terminal.ts

@@ -705,7 +705,7 @@ export interface ITerminalInstance extends IBaseTerminalInstance {
 	onDidExecuteText: Event<void>;
 	onDidChangeTarget: Event<TerminalLocation | undefined>;
 	onDidSendText: Event<string>;
-	onDidChangeShellType: Event<TerminalShellType>;
+	onDidChangeShellType: Event<TerminalShellType>; // how should this be used in correlation to my api which should expose shell type


This is the raw event that your onAnyInstance one will be derived from. This is the source event for you but actually how shell type is set is typically via the pty host.

This is sent on the pty host:

vscode/src/vs/platform/terminal/node/terminalProcess.ts

Line 423 in e8184ed

this._onDidChangeProperty.fire({ type: ProcessPropertyType.ShellType, value: GeneralShellType.Python });

This forwards it to TerminalInstance on the renderer process (aka main thread):

vscode/src/vs/platform/terminal/node/ptyService.ts

Lines 757 to 759 in e8184ed

this._register(this._terminalProcess.onDidChangeProperty(e => {

this._onDidChangeProperty.fire(e);

}));

vscode/src/vs/platform/terminal/node/ptyService.ts

Line 312 in e8184ed

persistentProcess.onDidChangeProperty(property => this._onDidChangeProperty.fire({ id, property }));

vscode/src/vs/platform/terminal/node/ptyHostService.ts

Line 178 in e8184ed

this._register(proxy.onDidChangeProperty(e => this._onDidChangeProperty.fire(e)));

vscode/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts

Line 142 in e8184ed

directProxy.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property));

vscode/src/vs/workbench/contrib/terminal/common/basePty.ts

Line 83 in e8184ed

this._onDidChangeProperty.fire({ type, value });

vscode/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts

Line 384 in e8184ed

this._onDidChangeProperty.fire({ type, value });

vscode/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

Lines 1419 to 1421 in e8184ed

case ProcessPropertyType.ShellType:

this.setShellType(value);

break;

It's so complicated mainly due to the process jumps and multiple types of pty host (remote and local).

Tyriar · 2025-01-10T13:31:50Z

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

+			// Can I fire here to let my new API know? but how should I do that
+


Nope, you got the main listener done with the onAnyInstance event on TerminalService and listening to that in mainThreadTerminalService.

anthonykim1 added 2 commits January 10, 2025 02:52

Try to start on shell type API

1affd37

fix myself

63a931c

vs-code-engineering bot assigned anthonykim1 Jan 10, 2025

leave some questions

26b6986

Tyriar requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose shell type to extensions #237624

Expose shell type to extensions #237624

anthonykim1 commented Jan 10, 2025

Tyriar Jan 10, 2025

Tyriar Jan 10, 2025

Tyriar Jan 10, 2025

Tyriar Jan 10, 2025

Tyriar Jan 10, 2025

Tyriar Jan 10, 2025

Tyriar Jan 10, 2025

Tyriar Jan 10, 2025

	public $acceptTerminalInteraction(id: number): void {
	const terminal = this.getTerminalById(id);
	if (terminal?.setInteractedWith()) {
	this._onDidChangeTerminalState.fire(terminal.value);
	}
	}

	this._register(this._terminalProcess.onDidChangeProperty(e => {
	this._onDidChangeProperty.fire(e);
	}));

	case ProcessPropertyType.ShellType:
	this.setShellType(value);
	break;

		// Can I fire here to let my new API know? but how should I do that

Expose shell type to extensions #237624

Are you sure you want to change the base?

Expose shell type to extensions #237624

Conversation

anthonykim1 commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment