8000 Improve shell integration reliability for zsh (#22891) · microsoft/vscode-python@5174d5c · GitHub
[go: up one dir, main page]
Skip to content

Commit 5174d5c

Browse files
author
Kartik Raj
authored
Improve shell integration reliability for zsh (#22891)
Closes #22881 If status changes, re-run activation. Also persist once we know shell integration works for a shell.
1 parent b0c34e3 commit 5174d5c

File tree

2 files changed

+63
-75
lines changed

2 files changed

+63
-75
lines changed

‎src/client/terminals/envCollectionActivation/service.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import {
2626
IPathUtils,
2727
} from '../../common/types';
2828
import { Interpreters } from '../../common/utils/localize';
29-
import { traceError, traceVerbose, traceWarn } from '../../logging';
29+
import { traceError, traceInfo, traceVerbose, traceWarn } from '../../logging';
3030
import { IInterpreterService } from '../../interpreter/contracts';
3131
import { defaultShells } from '../../interpreter/activation/service';
3232
import { IEnvironmentActivationService } from '../../interpreter/activation/types';
@@ -111,6 +111,14 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
111111
this,
112112
this.disposables,
113113
);
114+
this.shellIntegrationService.onDidChangeStatus(
115+
async () => {
10000 116+
traceInfo("Shell integration status changed, can confirm it's working.");
117+
await this._applyCollection(undefined).ignoreErrors();
118+
},
119+
this,
120+
this.disposables,
121+
);
114122
this.applicationEnvironment.onDidChangeShell(
115123
async (shell: string) => {
116124
this.processEnvVars = undefined;
@@ -125,7 +133,9 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
125133
const isActive = await this.shellIntegrationService.isWorking(shell);
126134
const shellType = identifyShellFromShellPath(shell);
127135
if (!isActive && shellType !== TerminalShellType.commandPrompt) {
128-
traceWarn(`Shell integration is not active, environment activated maybe overriden by the shell.`);
136+
traceWarn(
137+
`Shell integration may not be active, environment activated may be overridden by the shell.`,
138+
);
129139
}
130140
this.registeredOnce = true;
131141
}

‎src/client/terminals/envCollectionActivation/shellIntegrationService.ts

Lines changed: 51 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,8 @@ import {
1212
import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors/baseShellDetector';
1313
import { TerminalShellType } from '../../common/terminal/types';
1414
import { IDisposableRegistry, IPersistentStateFactory } from '../../common/types';
15-
import { createDeferred, sleep } from '../../common/utils/async';
16-
import { cache } from '../../common/utils/decorators';
17-
import { traceError, traceInfo, traceVerbose } from '../../logging';
15+
import { sleep } from '../../common/utils/async';
16+
import { traceError, traceVerbose } from '../../logging';
1817
import { IShellIntegrationService } from '../types';
1918

2019
/**
@@ -29,17 +28,12 @@ const ShellIntegrationShells = [
2928
TerminalShellType.fish,
3029
];
3130

32-
export const isShellIntegrationWorkingKey = 'SHELL_INTEGRATION_WORKING_KEY';
31+
export enum isShellIntegrationWorking {
32+
key = 'SHELL_INTEGRATION_WORKING_KEY',
33+
}
3334

3435
@injectable()
3536
export class ShellIntegrationService implements IShellIntegrationService {
36-
/**
37-
* It seems to have a couple of issues:
38-
* * Ends up cluterring terminal history
39-
* * Does not work for hidden terminals: https://github.com/microsoft/vscode/issues/199611
40-
*/
41-
private readonly USE_COMMAND_APPROACH = false;
42-
4337
private isWorkingForShell = new Set<TerminalShellType>();
4438

4539
private readonly didChange = new EventEmitter<void>();
@@ -55,6 +49,12 @@ export class ShellIntegrationService implements IShellIntegrationService {
5549
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
5650
) {
5751
try {
52+
const activeShellType = identifyShellFromShellPath(this.appEnvironment.shell);
53+
const key = getKeyForShell(activeShellType);
54+
const persistedResult = this.persistentStateFactory.createGlobalPersistentState<boolean>(key);
55+
if (persistedResult.value) {
56+
this.isWorkingForShell.add(activeShellType);
57+
}
5858
this.appShell.onDidWriteTerminalData(
5959
(e) => {
6060
if (e.data.includes('\x1b]633;A\x07')) {
@@ -63,6 +63,7 @@ export class ShellIntegrationService implements IShellIntegrationService {
6363
shell = e.terminal.creationOptions.shellPath;
6464
}
6565
const shellType = identifyShellFromShellPath(shell);
66+
traceVerbose('Received shell integration sequence for', shellType);
6667
const wasWorking = this.isWorkingForShell.has(shellType);
6768
this.isWorkingForShell.add(shellType);
6869
if (!wasWorking) {
@@ -86,6 +87,12 @@ export class ShellIntegrationService implements IShellIntegrationService {
8687
this.isDataWriteEventWorking = false;
8788
traceError('Unable to check if shell integration is active', ex);
8889
}
90+
const isEnabled = !!this.workspaceService
91+
.getConfiguration('terminal')
92+
.get<boolean>('integrated.shellIntegration.enabled');
93+
if (!isEnabled) {
94+
traceVerbose('Shell integration is disabled in user settings.');
95+
}
8996
}
9097

9198
public readonly onDidChangeStatus = this.didChange.event;
@@ -97,46 +104,48 @@ export class ShellIntegrationService implements IShellIntegrationService {
97104
});
98105
}
99106

100-
@cache(-1, true)
101107
public async _isWorking(shell: string): Promise<boolean> {
102-
const isEnabled = this.workspaceService
103-
.getConfiguration('terminal')
104-
.get<boolean>('integrated.shellIntegration.enabled')!;
105-
if (!isEnabled) {
106-
traceVerbose('Shell integrated is disabled in user settings.');
107-
}
108108
const shellType = identifyShellFromShellPath(shell);
109-
const isSupposedToWork = isEnabled && ShellIntegrationShells.includes(shellType);
109+
const isSupposedToWork = ShellIntegrationShells.includes(shellType);
110110
if (!isSupposedToWork) {
111111
return false;
112112
}
113-
if (!this.USE_COMMAND_APPROACH) {
114-
// For now, based on problems with using the command approach, use terminal data write event.
115-
if (!this.isDataWriteEventWorking) {
116-
// Assume shell integration is working, if data write event isn't working.
117-
return true;
118-
}
119-
if (shellType === TerminalShellType.powershell || shellType === TerminalShellType.powershellCore) {
120-
// Due to upstream bug: https://github.com/microsoft/vscode/issues/204616, assume shell integration is working for now.
121-
return true;
122-
}
123-
if (!this.isWorkingForShell.has(shellType)) {
124-
// Maybe data write event has not been processed yet, wait a bit.
125-
await sleep(1000);
126-
}
127-
return this.isWorkingForShell.has(shellType);
128-
}
129-
const key = `${isShellIntegrationWorkingKey}_${shellType}`;
113+
const key = getKeyForShell(shellType);
130114
const persistedResult = this.persistentStateFactory.createGlobalPersistentState<boolean>(key);
131115
if (persistedResult.value !== undefined) {
132116
return persistedResult.value;
133117
}
134-
const result = await this.checkIfWorkingByRunningCommand(shell);
135-
// Persist result to storage to avoid running commands unncecessary.
136-
await persistedResult.updateValue(result);
118+
const result = await this.useDataWriteApproach(shellType);
119+
if (result) {
120+
// Once we know that shell integration is working for a shell, persist it so we need not do this check every session.
121+
await persistedResult.updateValue(result);
122+
}
137123
return result;
138124
}
139125

126+
private async useDataWriteApproach(shellType: TerminalShellType) {
127+
// For now, based on problems with using the command approach, use terminal data write event.
128+
if (!this.isDataWriteEventWorking) {
129+
// Assume shell integration is working, if data write event isn't working.
130+
return true;
131+
}
132+
if (shellType === TerminalShellType.powershell || shellType === TerminalShellType.powershellCore) {
133+
// Due to upstream bug: https://github.com/microsoft/vscode/issues/204616, assume shell integration is working for now.
134+
return true;
135+
}
136+
if (!this.isWorkingForShell.has(shellType)) {
137+
// Maybe data write event has not been processed yet, wait a bit.
138+
await sleep(1000);
139+
}
140+
traceVerbose(
141+
'Did we determine shell integration to be working for',
142+
shellType,
143+
'?',
144+
this.isWorkingForShell.has(shellType),
145+
);
146+
return this.isWorkingForShell.has(shellType);
147+
}
148+
140149
/**
141150
* Creates a dummy terminal so that we are guaranteed a data write event for this shell type.
142151
*/
@@ -146,39 +155,8 @@ export class ShellIntegrationService implements IShellIntegrationService {
146155
hideFromUser: true,
147156
});
148157
}
158+
}
149159

150-
private async checkIfWorkingByRunningCommand(shell: string): Promise<boolean> {
151-
const shellType = identifyShellFromShellPath(shell);
152-
const deferred = createDeferred<void>();
153-
const timestamp = new Date().getTime();
154-
const name = `Python ${timestamp}`;
155-
const onDidExecuteTerminalCommand = this.appShell.onDidExecuteTerminalCommand?.bind(this.appShell);
156-
if (!onDidExecuteTerminalCommand) {
157-
// Proposed API is not available, assume shell integration is working at this point.
158-
return true;
159-
}
160-
try {
161-
const disposable = onDidExecuteTerminalCommand((e) => {
162-
if (e.terminal.name === name) {
163-
deferred.resolve();
164-
}
165-
});
166-
const terminal = this.terminalManager.createTerminal({
167-
name,
168-
shellPath: shell,
169-
hideFromUser: true,
170-
});
171-
terminal.sendText(`echo ${shell}`);
172-
const success = await Promise.race([sleep(3000).then(() => false), deferred.promise.then(() => true)]);
173-
disposable.dispose();
174-
if (!success) {
175-
traceInfo(`Shell integration is not working for ${shellType}`);
176-
}
177-
return success;
178-
} catch (ex) {
179-
traceVerbose(`Proposed API is not available, failed to subscribe to onDidExecuteTerminalCommand`, ex);
180-
// Proposed API is not available, assume shell integration is working at this point.
181-
return true;
182-
}
183-
}
160+
function getKeyForShell(shellType: TerminalShellType) {
161+
return `${isShellIntegrationWorking.key}_${shellType}`;
184162
}

0 commit comments

Comments
 (0)
0