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

Revert "Reliably detect whether shell integration is working" #22445

Merged
merged 1 commit into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@
"testObserver",
"quickPickItemTooltip",
"saveEditor",
"terminalDataWriteEvent",
"terminalExecuteCommandEvent"
"terminalDataWriteEvent"
],
"author": {
"name": "Microsoft Corporation"
Expand Down
10 changes: 1 addition & 9 deletions src/client/common/application/applicationShell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import {
WorkspaceFolderPickOptions,
} from 'vscode';
import { traceError } from '../../logging';
import { IApplicationShell, TerminalDataWriteEvent, TerminalExecutedCommand } from './types';
import { IApplicationShell, TerminalDataWriteEvent } from './types';

@injectable()
export class ApplicationShell implements IApplicationShell {
Expand Down Expand Up @@ -182,12 +182,4 @@ export class ApplicationShell implements IApplicationShell {
return new EventEmitter<TerminalDataWriteEvent>().event;
}
}
public get onDidExecuteTerminalCommand(): Event<TerminalExecutedCommand> | undefined {
try {
return window.onDidExecuteTerminalCommand;
} catch (ex) {
traceError('Failed to get proposed API TerminalExecutedCommand', ex);
return undefined;
}
}
}
34 changes: 0 additions & 34 deletions src/client/common/application/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,42 +78,8 @@ export interface TerminalDataWriteEvent {
readonly data: string;
}

export interface TerminalExecutedCommand {
/**
* The {@link Terminal} the command was executed in.
*/
terminal: Terminal;
/**
* The full command line that was executed, including both the command and the arguments.
*/
commandLine: string | undefined;
/**
* The current working directory that was reported by the shell. This will be a {@link Uri}
* if the string reported by the shell can reliably be mapped to the connected machine.
*/
cwd: Uri | string | undefined;
/**
* The exit code reported by the shell.
*/
exitCode: number | undefined;
/**
* The output of the command when it has finished executing. This is the plain text shown in
* the terminal buffer and does not include raw escape sequences. Depending on the shell
* setup, this may include the command line as part of the output.
*/
output: string | undefined;
}

export const IApplicationShell = Symbol('IApplicationShell');
export interface IApplicationShell {
/**
* An event that is emitted when a terminal with shell integration activated has completed
* executing a command.
*
* Note that this event will not fire if the executed command exits the shell, listen to
* {@link onDidCloseTerminal} to handle that case.
*/
readonly onDidExecuteTerminalCommand: Event<TerminalExecutedCommand> | undefined;
/**
* An [event](#Event) which fires when the focus state of the current window
* changes. The value of the event represents whether the window is focused.
Expand Down
29 changes: 22 additions & 7 deletions src/client/terminals/envCollectionActivation/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ import { TerminalShellType } from '../../common/terminal/types';
import { OSType } from '../../common/utils/platform';
import { normCase } from '../../common/platform/fs-paths';
import { PythonEnvType } from '../../pythonEnvironments/base/info';
import { IShellIntegrationService, ITerminalEnvVarCollectionService } from '../types';
import { ITerminalEnvVarCollectionService } from '../types';
import { ShellIntegrationShells } from './shellIntegration';
import { ProgressService } from '../../common/application/progressService';

@injectable()
Expand Down Expand Up @@ -79,7 +80,6 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
@inject(IWorkspaceService) private workspaceService: IWorkspaceService,
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
@inject(IPathUtils) private readonly pathUtils: IPathUtils,
@inject(IShellIntegrationService) private readonly shellIntegrationService: IShellIntegrationService,
) {
this.separator = platform.osType === OSType.Windows ? ';' : ':';
this.progressService = new ProgressService(this.shell);
Expand Down Expand Up @@ -121,7 +121,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
this.disposables,
);
const { shell } = this.applicationEnvironment;
const isActive = this.shellIntegrationService.isWorking(shell);
const isActive = this.isShellIntegrationActive(shell);
const shellType = identifyShellFromShellPath(shell);
if (!isActive && shellType !== TerminalShellType.commandPrompt) {
traceWarn(`Shell integration is not active, environment activated maybe overriden by the shell.`);
Expand Down Expand Up @@ -184,7 +184,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ

// PS1 in some cases is a shell variable (not an env variable) so "env" might not contain it, calculate it in that case.
env.PS1 = await this.getPS1(shell, resource, env);
const prependOptions = await this.getPrependOptions(shell);
const prependOptions = this.getPrependOptions(shell);

// Clear any previously set env vars from collection
envVarCollection.clear();
Expand Down Expand Up @@ -277,7 +277,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
// PS1 should be set but no PS1 was set.
return;
}
const config = await this.shellIntegrationService.isWorking(shell);
const config = this.isShellIntegrationActive(shell);
if (!config) {
traceVerbose('PS1 is not set when shell integration is disabled.');
return;
Expand Down Expand Up @@ -332,8 +332,8 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
}
}

private async getPrependOptions(shell: string): Promise<EnvironmentVariableMutatorOptions> {
const isActive = await this.shellIntegrationService.isWorking(shell);
private getPrependOptions(shell: string): EnvironmentVariableMutatorOptions {
const isActive = this.isShellIntegrationActive(shell);
// Ideally we would want to prepend exactly once, either at shell integration or process creation.
// TODO: Stop prepending altogether once https://github.com/microsoft/vscode/issues/145234 is available.
return isActive
Expand All @@ -347,6 +347,21 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
};
}

private isShellIntegrationActive(shell: string): boolean {
const isEnabled = this.workspaceService
.getConfiguration('terminal')
.get<boolean>('integrated.shellIntegration.enabled')!;
if (isEnabled && ShellIntegrationShells.includes(identifyShellFromShellPath(shell))) {
// Unfortunately shell integration could still've failed in remote scenarios, we can't know for sure:
// https://code.visualstudio.com/docs/terminal/shell-integration#_automatic-script-injection
return true;
}
if (!isEnabled) {
traceVerbose('Shell integrated is disabled in user settings.');
}
return false;
}

private getEnvironmentVariableCollection(scope: EnvironmentVariableScope = {}) {
const envVarCollection = this.context.environmentVariableCollection as GlobalEnvironmentVariableCollection;
return envVarCollection.getScoped(scope);
Expand Down
13 changes: 13 additions & 0 deletions src/client/terminals/envCollectionActivation/shellIntegration.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { TerminalShellType } from '../../common/terminal/types';

/**
* This is a list of shells which support shell integration:
* https://code.visualstudio.com/docs/terminal/shell-integration
*/
export const ShellIntegrationShells = [
TerminalShellType.powershell,
TerminalShellType.powershellCore,
TerminalShellType.bash,
TerminalShellType.zsh,
TerminalShellType.fish,
];

This file was deleted.

3 changes: 0 additions & 3 deletions src/client/terminals/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,13 @@ import {
ICodeExecutionHelper,
ICodeExecutionManager,
ICodeExecutionService,
IShellIntegrationService,
ITerminalAutoActivation,
ITerminalEnvVarCollectionService,
} from './types';
import { TerminalEnvVarCollectionService } from './envCollectionActivation/service';
import { IExtensionActivationService, IExtensionSingleActivationService } from '../activation/types';
import { TerminalDeactivateLimitationPrompt } from './envCollectionActivation/deactivatePrompt';
import { TerminalIndicatorPrompt } from './envCollectionActivation/indicatorPrompt';
import { ShellIntegrationService } from './envCollectionActivation/shellIntegrationService';

export function registerTypes(serviceManager: IServiceManager): void {
serviceManager.addSingleton<ICodeExecutionHelper>(ICodeExecutionHelper, CodeExecutionHelper);
Expand Down Expand Up @@ -52,6 +50,5 @@ export function registerTypes(serviceManager: IServiceManager): void {
IExtensionSingleActivationService,
TerminalDeactivateLimitationPrompt,
);
serviceManager.addSingleton<IShellIntegrationService>(IShellIntegrationService, ShellIntegrationService);
serviceManager.addBinding(ITerminalEnvVarCollectionService, IExtensionActivationService);
}
5 changes: 0 additions & 5 deletions src/client/terminals/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,3 @@ export interface ITerminalEnvVarCollectionService {
*/
isTerminalPromptSetCorrectly(resource?: Resource): boolean;
}

export const IShellIntegrationService = Symbol('IShellIntegrationService');
export interface IShellIntegrationService {
isWorking(shell: string): Promise<boolean>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
GlobalEnvironmentVariableCollection,
ProgressLocation,
Uri,
WorkspaceConfiguration,
WorkspaceFolder,
} from 'vscode';
import {
Expand All @@ -37,7 +38,6 @@ import { IInterpreterService } from '../../../client/interpreter/contracts';
import { PathUtils } from '../../../client/common/platform/pathUtils';
import { PythonEnvType } from '../../../client/pythonEnvironments/base/info';
import { PythonEnvironment } from '../../../client/pythonEnvironments/info';
import { IShellIntegrationService } from '../../../client/terminals/types';

suite('Terminal Environment Variable Collection Service', () => {
let platform: IPlatformService;
Expand All @@ -50,28 +50,29 @@ suite('Terminal Environment Variable Collection Service', () => {
let applicationEnvironment: IApplicationEnvironment;
let environmentActivationService: IEnvironmentActivationService;
let workspaceService: IWorkspaceService;
let workspaceConfig: WorkspaceConfiguration;
let terminalEnvVarCollectionService: TerminalEnvVarCollectionService;
const progressOptions = {
location: ProgressLocation.Window,
title: Interpreters.activatingTerminals,
};
let configService: IConfigurationService;
let shellIntegrationService: IShellIntegrationService;
const displayPath = 'display/path';
const customShell = 'powershell';
const defaultShell = defaultShells[getOSType()];

setup(() => {
workspaceService = mock<IWorkspaceService>();
workspaceConfig = mock<WorkspaceConfiguration>();
when(workspaceService.getWorkspaceFolder(anything())).thenReturn(undefined);
when(workspaceService.workspaceFolders).thenReturn(undefined);
when(workspaceService.getConfiguration('terminal')).thenReturn(instance(workspaceConfig));
when(workspaceConfig.get<boolean>('integrated.shellIntegration.enabled')).thenReturn(true);
platform = mock<IPlatformService>();
when(platform.osType).thenReturn(getOSType());
interpreterService = mock<IInterpreterService>();
context = mock<IExtensionContext>();
shell = mock<IApplicationShell>();
shellIntegrationService = mock<IShellIntegrationService>();
when(shellIntegrationService.isWorking(anything())).thenResolve(true);
globalCollection = mock<GlobalEnvironmentVariableCollection>();
collection = mock<EnvironmentVariableCollection>();
when(context.environmentVariableCollection).thenReturn(instance(globalCollection));
Expand Down Expand Up @@ -107,7 +108,6 @@ suite('Terminal Environment Variable Collection Service', () => {
instance(workspaceService),
instance(configService),
new PathUtils(getOSType() === OSType.Windows),
instance(shellIntegrationService),
);
});

Expand Down Expand Up @@ -445,8 +445,8 @@ suite('Terminal Environment Variable Collection Service', () => {
});

test('Correct track that prompt was set for PS1 if shell integration is disabled', async () => {
reset(shellIntegrationService);
when(shellIntegrationService.isWorking(anything())).thenResolve(false);
reset(workspaceConfig);
when(workspaceConfig.get<boolean>('integrated.shellIntegration.enabled')).thenReturn(false);
when(platform.osType).thenReturn(OSType.Linux);
const envVars: NodeJS.ProcessEnv = { VIRTUAL_ENV: 'prefix/to/venv', PS1: '(.venv)', ...process.env };
const ps1Shell = 'bash';
Expand Down
3 changes: 0 additions & 3 deletions src/test/terminals/serviceRegistry.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,11 @@ import { TerminalCodeExecutionProvider } from '../../client/terminals/codeExecut
import { TerminalDeactivateLimitationPrompt } from '../../client/terminals/envCollectionActivation/deactivatePrompt';
import { TerminalIndicatorPrompt } from '../../client/terminals/envCollectionActivation/indicatorPrompt';
import { TerminalEnvVarCollectionService } from '../../client/terminals/envCollectionActivation/service';
import { ShellIntegrationService } from '../../client/terminals/envCollectionActivation/shellIntegrationService';
import { registerTypes } from '../../client/terminals/serviceRegistry';
import {
ICodeExecutionHelper,
ICodeExecutionManager,
ICodeExecutionService,
IShellIntegrationService,
ITerminalAutoActivation,
ITerminalEnvVarCollectionService,
} from '../../client/terminals/types';
Expand All @@ -37,7 +35,6 @@ suite('Terminal - Service Registry', () => {
[ITerminalEnvVarCollectionService, TerminalEnvVarCollectionService],
[IExtensionSingleActivationService, TerminalIndicatorPrompt],
[IExtensionSingleActivationService, TerminalDeactivateLimitationPrompt],
[IShellIntegrationService, ShellIntegrationService],
].forEach((args) => {
if (args.length === 2) {
services
Expand Down
Loading
Loading