Skip to content

Commit 71a396c

Browse files
anthonykim1anthonykim1
committed
Be less aggressive about removing Python hooks in shell profile
1 parent 2f7e891 commit 71a396c

6 files changed

Lines changed: 182 additions & 33 deletions

File tree

‎src/features/terminal/shells/bash/bashStartup.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import which from 'which';
55
import { traceError, traceInfo, traceVerbose } from '../../../../common/logging';
66
import { ShellConstants } from '../../../common/shellConstants';
77
import { hasStartupCode, insertStartupCode, removeStartupCode } from '../common/editUtils';
8-
import { getShellIntegrationEnabledCache, isWsl, shellIntegrationForActiveTerminal } from '../common/shellUtils';
98
import { ShellScriptEditState, ShellSetupState, ShellStartupScriptProvider } from '../startupProvider';
109
import { BASH_ENV_KEY, BASH_OLD_ENV_KEY, BASH_SCRIPT_VERSION, ZSH_ENV_KEY, ZSH_OLD_ENV_KEY } from './bashConstants';
1110

@@ -69,11 +68,6 @@ async function isStartupSetup(profile: string, key: string): Promise<ShellSetupS
6968
return ShellSetupState.NotSetup;
7069
}
7170
async function setupStartup(profile: string, key: string, name: string): Promise<boolean> {
72-
const shellIntegrationEnabled = await getShellIntegrationEnabledCache();
73-
if ((shellIntegrationEnabled || (await shellIntegrationForActiveTerminal(name, profile))) && !isWsl()) {
74-
removeStartup(profile, key);
75-
return true;
76-
}
7771
const activationContent = getActivationContent(key);
7872
try {
7973
if (await fs.pathExists(profile)) {

‎src/features/terminal/shells/fish/fishStartup.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import which from 'which';
66
import { traceError, traceInfo, traceVerbose } from '../../../../common/logging';
77
import { ShellConstants } from '../../../common/shellConstants';
88
import { hasStartupCode, insertStartupCode, removeStartupCode } from '../common/editUtils';
9-
import { getShellIntegrationEnabledCache, isWsl, shellIntegrationForActiveTerminal } from '../common/shellUtils';
109
import { ShellScriptEditState, ShellSetupState, ShellStartupScriptProvider } from '../startupProvider';
1110
import { FISH_ENV_KEY, FISH_OLD_ENV_KEY, FISH_SCRIPT_VERSION } from './fishConstants';
1211

@@ -58,11 +57,6 @@ async function isStartupSetup(profilePath: string, key: string): Promise<boolean
5857

5958
async function setupStartup(profilePath: string, key: string): Promise<boolean> {
6059
try {
61-
const shellIntegrationEnabled = await getShellIntegrationEnabledCache();
62-
if ((shellIntegrationEnabled || (await shellIntegrationForActiveTerminal('fish', profilePath))) && !isWsl()) {
63-
removeFishStartup(profilePath, key);
64-
return true;
65-
}
6660
const activationContent = getActivationContent(key);
6761
await fs.mkdirp(path.dirname(profilePath));
6862

‎src/features/terminal/shells/pwsh/pwshStartup.ts‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,8 @@ import { ShellConstants } from '../../../common/shellConstants';
1313
import { hasStartupCode, insertStartupCode, removeStartupCode } from '../common/editUtils';
1414
import {
1515
extractProfilePath,
16-
getShellIntegrationEnabledCache,
17-
isWsl,
1816
PROFILE_TAG_END,
1917
PROFILE_TAG_START,
20-
shellIntegrationForActiveTerminal,
2118
} from '../common/shellUtils';
2219
import { POWERSHELL_ENV_KEY, POWERSHELL_OLD_ENV_KEY, PWSH_SCRIPT_VERSION } from './pwshConstants';
2320

@@ -169,13 +166,6 @@ async function isPowerShellStartupSetup(shell: string, profile: string): Promise
169166
}
170167

171168
async function setupPowerShellStartup(shell: string, profile: string): Promise<boolean> {
172-
const shellIntegrationEnabled = await getShellIntegrationEnabledCache();
173-
174-
if ((shellIntegrationEnabled || (await shellIntegrationForActiveTerminal(shell, profile))) && !isWsl()) {
175-
removePowerShellStartup(shell, profile, POWERSHELL_OLD_ENV_KEY);
176-
removePowerShellStartup(shell, profile, POWERSHELL_ENV_KEY);
177-
return true;
178-
}
179169
const activationContent = getActivationContent();
180170

181171
try {

‎src/features/terminal/terminalManager.ts‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -185,27 +185,22 @@ export class TerminalManagerImpl implements TerminalManager {
185185
);
186186

187187
if (shellIntegrationLikelyAvailable && !shouldUseProfileActivation(p.shellType)) {
188-
// Shell integration available and NOT in WSL - skip setup
189-
await p.teardownScripts();
188+
// Shell integration available and NOT in WSL - skip setup.
189+
// NOTE: We intentionally do NOT teardown scripts here. If the user stays in
190+
// shellStartup mode, be less aggressive about clearing profile modifications.
190191
this.shellSetup.set(p.shellType, true);
191192
traceVerbose(
192-
`Shell integration available for ${p.shellType} (not WSL), skipping prompt, and profile modification.`,
193+
`Shell integration likely available. Skipping setup of shell profile for ${p.shellType}.`,
193194
);
194195
} else {
195-
// WSL (regardless of integration) OR no shell integration - needs setup
196+
// WSL (regardless of integration) OR no/disabled shell integration - needs setup
196197
this.shellSetup.set(p.shellType, false);
197198
shellsToSetup.push(p);
198199
traceVerbose(
199-
`Shell integration is NOT available. Shell profile for ${p.shellType} is not setup.`,
200+
`Shell integration is NOT available or disabled. Shell profile for ${p.shellType} is not setup.`,
200201
);
201202
}
202203
} else if (state === ShellSetupState.Setup) {
203-
if (shellIntegrationLikelyAvailable && !shouldUseProfileActivation(p.shellType)) {
204-
await p.teardownScripts();
205-
traceVerbose(
206-
`Shell integration available for ${p.shellType}, removed profile script in favor of shell integration.`,
207-
);
208-
}
209204
this.shellSetup.set(p.shellType, true);
210205
traceVerbose(`Shell profile for ${p.shellType} is setup.`);
211206
} else if (state === ShellSetupState.NotInstalled) {

‎src/test/features/terminal/terminalManager.shellStartupCache.unit.test.ts‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
import * as sinon from 'sinon';
2+
import { EventEmitter } from 'vscode';
3+
import { TerminalManagerImpl } from '../../../features/terminal/terminalManager';
4+
import { ShellSetupState, ShellStartupScriptProvider } from '../../../features/terminal/shells/startupProvider';
5+
import * as workspaceApis from '../../../common/workspace.apis';
6+
7+
suite('TerminalManager - shellStartup caching/clearing', () => {
8+
let disposables: sinon.SinonStub[] = [];
9+
10+
teardown(() => {
11+
disposables.forEach((d) => d.restore());
12+
disposables = [];
13+
});
14+
15+
function createProvider(shellType: string, state: ShellSetupState): ShellStartupScriptProvider {
16+
return {
17+
name: shellType,
18+
shellType,
19+
isSetup: sinon.stub().resolves(state),
20+
setupScripts: sinon.stub().resolves(undefined),
21+
teardownScripts: sinon.stub().resolves(undefined),
22+
clearCache: sinon.stub().resolves(undefined),
23+
} as unknown as ShellStartupScriptProvider;
24+
}
25+
26+
test('does not teardown scripts just because shell integration setting changes', async () => {
27+
const configEmitter = new EventEmitter<{ affectsConfiguration: (s: string) => boolean }>();
28+
29+
const onDidChangeConfigurationStub = sinon
30+
.stub(workspaceApis, 'onDidChangeConfiguration')
31+
.callsFake((listener) => {
32+
const sub = configEmitter.event(listener);
33+
return { dispose: () => sub.dispose() };
34+
});
35+
disposables.push(onDidChangeConfigurationStub);
36+
37+
// TerminalManager constructor wires a bunch of window event listeners too; stub them to no-ops.
38+
const windowApis = require('../../../common/window.apis') as typeof import('../../../common/window.apis');
39+
disposables.push(
40+
sinon.stub(windowApis, 'onDidOpenTerminal').returns({ dispose: () => undefined } as any),
41+
sinon.stub(windowApis, 'onDidCloseTerminal').returns({ dispose: () => undefined } as any),
42+
sinon.stub(windowApis, 'onDidChangeWindowState').returns({ dispose: () => undefined } as any),
43+
sinon.stub(windowApis, 'terminals').returns([] as any),
44+
);
45+
46+
const shellUtils =
47+
require('../../../features/terminal/shells/common/shellUtils') as typeof import('../../../features/terminal/shells/common/shellUtils');
48+
disposables.push(sinon.stub(shellUtils, 'getShellIntegrationEnabledCache').resolves(true));
49+
50+
const ta = {
51+
onDidChangeTerminalActivationState: () => ({ dispose: () => undefined }),
52+
getEnvironment: () => undefined,
53+
} as any;
54+
55+
const provider = createProvider('bash', ShellSetupState.NotSetup);
56+
const tm = new TerminalManagerImpl(ta, [], [provider]);
57+
58+
// Trigger shell integration setting change
59+
configEmitter.fire({
60+
affectsConfiguration: (s: string) => s === 'terminal.integrated.shellIntegration.enabled',
61+
});
62+
63+
// Previously we would sometimes teardown scripts here; now we should not.
64+
sinon.assert.notCalled(provider.teardownScripts as any);
65+
66+
tm.dispose();
67+
});
68+
});

‎src/test/features/terminal/terminalManager.unit.test.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
import * as assert from 'assert';
2+
import * as sinon from 'sinon';
3+
4+
import { EventEmitter } from 'vscode';
5+
6+
import * as windowApis from '../../../common/window.apis';
7+
import * as workspaceApis from '../../../common/workspace.apis';
8+
import { TerminalManagerImpl } from '../../../features/terminal/terminalManager';
9+
import {
10+
ShellScriptEditState,
11+
ShellSetupState,
12+
ShellStartupScriptProvider,
13+
} from '../../../features/terminal/shells/startupProvider';
14+
import { ACT_TYPE_COMMAND } from '../../../features/terminal/utils';
15+
import * as terminalUtils from '../../../features/terminal/utils';
16+
import {
17+
DidChangeTerminalActivationStateEvent,
18+
TerminalActivationInternal,
19+
} from '../../../features/terminal/terminalActivationState';
20+
21+
type DisposableLike = { dispose(): void };
22+
23+
suite('TerminalManager shellStartup profile behavior', () => {
24+
let sandbox: sinon.SinonSandbox;
25+
let onDidChangeConfigurationHandler:
26+
| ((e: { affectsConfiguration: (section: string) => boolean }) => void | Promise<void>)
27+
| undefined;
28+
29+
setup(() => {
30+
sandbox = sinon.createSandbox();
31+
onDidChangeConfigurationHandler = undefined;
32+
33+
const disposable: DisposableLike = { dispose() {} };
34+
35+
sandbox.stub(windowApis, 'onDidOpenTerminal').returns(disposable as any);
36+
sandbox.stub(windowApis, 'onDidCloseTerminal').returns(disposable as any);
37+
sandbox.stub(windowApis, 'onDidChangeWindowState').returns(disposable as any);
38+
39+
sandbox.stub(workspaceApis, 'onDidChangeConfiguration').callsFake((handler: any) => {
40+
onDidChangeConfigurationHandler = handler;
41+
return disposable as any;
42+
});
43+
44+
// Avoid any real window focus concerns.
45+
sandbox.stub(windowApis, 'terminals').returns([] as any);
46+
});
47+
48+
teardown(() => {
49+
sandbox.restore();
50+
});
51+
52+
function createTerminalManager(startupScriptProviders: ShellStartupScriptProvider[]): TerminalManagerImpl {
53+
const emitter = new EventEmitter<DidChangeTerminalActivationStateEvent>();
54+
const ta: TerminalActivationInternal = {
55+
onDidChangeTerminalActivationState: emitter.event,
56+
isActivated: () => false,
57+
getEnvironment: () => undefined,
58+
activate: async () => undefined,
59+
deactivate: async () => undefined,
60+
updateActivationState: () => undefined,
61+
dispose: () => emitter.dispose(),
62+
};
63+
64+
return new TerminalManagerImpl(ta, [], startupScriptProviders);
65+
}
66+
67+
test('does not tear down profile scripts when shellStartup is setup', async () => {
68+
const provider: ShellStartupScriptProvider = {
69+
name: 'bash',
70+
shellType: 'bash',
71+
isSetup: sandbox.stub().resolves(ShellSetupState.Setup),
72+
setupScripts: sandbox.stub().resolves(ShellScriptEditState.Edited),
73+
teardownScripts: sandbox.stub().resolves(ShellScriptEditState.Edited),
74+
clearCache: sandbox.stub().resolves(),
75+
};
76+
77+
const tm = createTerminalManager([provider]);
78+
await (tm as any).handleSetupCheck('bash');
79+
80+
sinon.assert.notCalled(provider.teardownScripts as sinon.SinonStub);
81+
assert.strictEqual((tm as any).shellSetup.get('bash'), true);
82+
});
83+
84+
test('clears profile scripts when switching from shellStartup to command', async () => {
85+
const provider: ShellStartupScriptProvider = {
86+
name: 'bash',
87+
shellType: 'bash',
88+
isSetup: sandbox.stub().resolves(ShellSetupState.Setup),
89+
setupScripts: sandbox.stub().resolves(ShellScriptEditState.Edited),
90+
teardownScripts: sandbox.stub().resolves(ShellScriptEditState.Edited),
91+
clearCache: sandbox.stub().resolves(),
92+
};
93+
94+
sandbox.stub(terminalUtils, 'getAutoActivationType').returns(ACT_TYPE_COMMAND);
95+
96+
const tm = createTerminalManager([provider]);
97+
// Seed a cached setup state so we can verify it is cleared.
98+
(tm as any).shellSetup.set('bash', true);
99+
100+
assert.ok(onDidChangeConfigurationHandler, 'Expected onDidChangeConfiguration handler to be registered');
101+
await onDidChangeConfigurationHandler!({
102+
affectsConfiguration: (section: string) => section === 'python-envs.terminal.autoActivationType',
103+
});
104+
105+
sinon.assert.calledOnce(provider.teardownScripts as sinon.SinonStub);
106+
assert.strictEqual((tm as any).shellSetup.size, 0);
107+
});
108+
});

0 commit comments

Comments
 (0)