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.

Sign up for GitHub

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

Expose shell type to extensions #237624

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
+30 −5
Draft

Expose shell type to extensions #237624

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1affd37
Try to start on shell type API
anthonykim1 Jan 10, 2025
63a931c
fix myself
anthonykim1 Jan 10, 2025
26b6986
leave some questions
anthonykim1 Jan 10, 2025
2020869
return void for $acceptTerminalShellType
anthonykim1 Jan 10, 2025
73a79dd
partial changes after feedback!
anthonykim1 Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/vs/workbench/api/browser/mainThreadTerminalService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Set initial ext host state
for (const instance of this._terminalService.instances) {
Expand Down Expand Up @@ -353,6 +354,13 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape
this._proxy.$acceptTerminalTitleChange(terminalId, name);
}

private _onShellTypeChanged(terminalId: number, shellType: string): void {
const terminalInstance = this._terminalService.getInstanceFromId(terminalId)?.shellType;
if (terminalInstance) {
this._proxy.$acceptTerminalShellType(terminalId, shellType);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private _onTerminalDisposed(terminalInstance: ITerminalInstance): void {
this._proxy.$acceptTerminalClosed(terminalInstance.instanceId, terminalInstance.exitCode, terminalInstance.exitReason ?? TerminalExitReason.Unknown);
}
Expand Down
3 changes: 2 additions & 1 deletion src/vs/workbench/api/common/extHost.protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import { AuthInfo, Credentials } from '../../../platform/request/common/request.
import { ClassifiedEvent, IGDPRProperty, OmitMetadata, StrictPropertyCheck } from '../../../platform/telemetry/common/gdprTypings.js';
import { TelemetryLevel } from '../../../platform/telemetry/common/telemetry.js';
import { ISerializableEnvironmentDescriptionMap, ISerializableEnvironmentVariableCollection } from '../../../platform/terminal/common/environmentVariable.js';
import { ICreateContributedTerminalProfileOptions, IProcessProperty, IProcessReadyWindowsPty, IShellLaunchConfigDto, ITerminalEnvironment, ITerminalLaunchError, ITerminalProfile, TerminalExitReason, TerminalLocation } from '../../../platform/terminal/common/terminal.js';
import { ICreateContributedTerminalProfileOptions, IProcessProperty, IProcessReadyWindowsPty, IShellLaunchConfigDto, ITerminalEnvironment, ITerminalLaunchError, ITerminalProfile, TerminalExitReason, TerminalLocation, TerminalShellType } from '../../../platform/terminal/common/terminal.js';
import { ProvidedPortAttributes, TunnelCreationOptions, TunnelOptions, TunnelPrivacyId, TunnelProviderFeatures } from '../../../platform/tunnel/common/tunnel.js';
import { EditSessionIdentityMatch } from '../../../platform/workspace/common/editSessions.js';
import { WorkspaceTrustRequestOptions } from '../../../platform/workspace/common/workspaceTrust.js';
Expand Down Expand Up @@ -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>;
anthonykim1 marked this conversation as resolved.
Show resolved Hide resolved
$startExtensionTerminal(id: number, initialDimensions: ITerminalDimensionsDto | undefined): Promise<ITerminalLaunchError | undefined>;
$acceptProcessAckDataEvent(id: number, charCount: number): void;
$acceptProcessInput(id: number, data: string): void;
Expand Down
9 changes: 8 additions & 1 deletion src/vs/workbench/api/common/extHostTerminalService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { serializeEnvironmentDescriptionMap, serializeEnvironmentVariableCollect
import { CancellationTokenSource } from '../../../base/common/cancellation.js';
import { generateUuid } from '../../../base/common/uuid.js';
import { IEnvironmentVariableCollectionDescription, IEnvironmentVariableMutator, ISerializableEnvironmentVariableCollection } from '../../../platform/terminal/common/environmentVariable.js';
import { ICreateContributedTerminalProfileOptions, IProcessReadyEvent, IShellLaunchConfigDto, ITerminalChildProcess, ITerminalLaunchError, ITerminalProfile, TerminalIcon, TerminalLocation, IProcessProperty, ProcessPropertyType, IProcessPropertyMap } from '../../../platform/terminal/common/terminal.js';
import { ICreateContributedTerminalProfileOptions, IProcessReadyEvent, IShellLaunchConfigDto, ITerminalChildProcess, ITerminalLaunchError, ITerminalProfile, TerminalIcon, TerminalLocation, IProcessProperty, ProcessPropertyType, IProcessPropertyMap, TerminalShellType, GeneralShellType } from '../../../platform/terminal/common/terminal.js';
import { TerminalDataBufferer } from '../../../platform/terminal/common/terminalDataBuffering.js';
import { ThemeColor } from '../../../base/common/themables.js';
import { Promises } from '../../../base/common/async.js';
Expand Down Expand Up @@ -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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, the event would be on the ExtHostTerminal object



protected readonly _onDidWriteTerminalData = new Emitter<vscode.TerminalDataWriteEvent>({
onWillAddFirstListener: () => this._proxy.$startSendingDataEvents(),
Expand Down Expand Up @@ -764,6 +766,11 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I

return completions;
}
// 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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public registerTerminalQuickFixProvider(id: string, extensionId: string, provider: vscode.TerminalQuickFixProvider): vscode.Disposable {
if (this._quickFixProviders.has(id)) {
Expand Down
4 changes: 2 additions & 2 deletions src/vs/workbench/contrib/terminal/browser/terminal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Group events
readonly onDidChangeActiveGroup: Event<ITerminalGroup | undefined>;

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

onDidChangeVisibility: Event<boolean>;

/**
Expand Down
2 changes: 2 additions & 0 deletions src/vs/workbench/contrib/terminal/browser/terminalInstance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1910,6 +1910,8 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
this._shellType = shellType;
this._terminalShellTypeContextKey.set(shellType?.toString());
this._onDidChangeShellType.fire(shellType);
// Can I fire here to let my new API know? but how should I do that

Comment on lines +1913 to +1914
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand Down
Loading