-
Notifications
You must be signed in to change notification settings - Fork 39
Better handle activateEnvInCurrentTerminal from Python extension #1318
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ import { | |
| AutoActivationType, | ||
| getAutoActivationType, | ||
| getEnvironmentForTerminal, | ||
| shouldActivateInCurrentTerminal, | ||
| waitForShellIntegration, | ||
| } from './utils'; | ||
|
|
||
|
|
@@ -405,8 +406,21 @@ export class TerminalManagerImpl implements TerminalManager { | |
|
|
||
| public async initialize(api: PythonEnvironmentApi): Promise<void> { | ||
| const actType = getAutoActivationType(); | ||
|
|
||
| // When activateEnvInCurrentTerminal is explicitly false, | ||
| // skip activation for ALL pre-existing terminals (terminals open before extension load). | ||
| // New terminals opened after extension load are still activated via autoActivateOnTerminalOpen. | ||
| const skipPreExistingTerminals = !shouldActivateInCurrentTerminal() && terminals().length > 0; | ||
| if (skipPreExistingTerminals) { | ||
| traceVerbose( | ||
| 'python.terminal.activateEnvInCurrentTerminal is explicitly disabled, skipping activation for pre-existing terminals', | ||
| ); | ||
| } | ||
|
|
||
| if (actType === ACT_TYPE_COMMAND) { | ||
| await Promise.all(terminals().map(async (t) => this.activateUsingCommand(api, t))); | ||
| if (!skipPreExistingTerminals) { | ||
| await Promise.all(terminals().map(async (t) => this.activateUsingCommand(api, t))); | ||
| } | ||
|
Comment on lines
+413
to
+423
|
||
| } else if (actType === ACT_TYPE_SHELL) { | ||
| const shells = new Set( | ||
| terminals() | ||
|
|
@@ -415,14 +429,16 @@ export class TerminalManagerImpl implements TerminalManager { | |
| ); | ||
| if (shells.size > 0) { | ||
| await this.handleSetupCheck(shells); | ||
| await Promise.all( | ||
| terminals().map(async (t) => { | ||
| // If the shell is not set up, we activate using command fallback. | ||
| if (this.shellSetup.get(identifyTerminalShell(t)) === false) { | ||
| await this.activateUsingCommand(api, t); | ||
| } | ||
| }), | ||
| ); | ||
| if (!skipPreExistingTerminals) { | ||
| await Promise.all( | ||
| terminals().map(async (t) => { | ||
| // If the shell is not set up, we activate using command fallback. | ||
| if (this.shellSetup.get(identifyTerminalShell(t)) === false) { | ||
| await this.activateUsingCommand(api, t); | ||
| } | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -262,6 +262,52 @@ export async function setAutoActivationType(value: AutoActivationType): Promise< | |
| return await config.update('terminal.autoActivationType', value, true); | ||
| } | ||
|
|
||
| /** | ||
| * Determines whether activation commands should be sent to pre-existing terminals | ||
| * (terminals open before extension load). | ||
| * | ||
| * Checks the legacy `python.terminal.activateEnvInCurrentTerminal` setting using `inspect()` | ||
| * to distinguish between the default value and an explicitly user-set value. | ||
| * | ||
| * Priority: workspaceFolderValue > workspaceValue > globalRemoteValue > globalLocalValue > globalValue | ||
| * (matches the precedence used by getShellIntegrationEnabledCache and getAutoActivationType) | ||
| * | ||
| * - If the user has explicitly set the value to `false` at any scope, returns `false`. | ||
| * - Otherwise (default or explicitly `true`), returns `true`. | ||
| * | ||
| * @returns `false` only when the user has explicitly set the setting to `false`; `true` otherwise. | ||
| */ | ||
| export function shouldActivateInCurrentTerminal(): boolean { | ||
| const pythonConfig = getConfiguration('python'); | ||
| const inspected = pythonConfig.inspect<boolean>('terminal.activateEnvInCurrentTerminal'); | ||
|
|
||
| if (!inspected) { | ||
| return true; | ||
| } | ||
|
|
||
| // Only respect `false` when the user has deliberately set it. | ||
| // Priority: workspaceFolder > workspace > globalRemote > globalLocal > global | ||
| const inspectValue = inspected as Record<string, unknown>; | ||
|
|
||
| if (inspected.workspaceFolderValue === false) { | ||
| return false; | ||
| } | ||
| if (inspected.workspaceValue === false) { | ||
| return false; | ||
| } | ||
| if ('globalRemoteValue' in inspected && inspectValue.globalRemoteValue === false) { | ||
| return false; | ||
| } | ||
| if ('globalLocalValue' in inspected && inspectValue.globalLocalValue === false) { | ||
| return false; | ||
| } | ||
| if (inspected.globalValue === false) { | ||
| return false; | ||
| } | ||
|
Comment on lines
+292
to
+306
|
||
|
|
||
| return true; | ||
| } | ||
|
|
||
| export async function getAllDistinctProjectEnvironments( | ||
| api: PythonProjectGetterApi & PythonProjectEnvironmentApi, | ||
| ): Promise<PythonEnvironment[] | undefined> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -421,3 +421,129 @@ suite('TerminalManager - terminal naming', () => { | |
| } | ||
| }); | ||
| }); | ||
|
|
||
| suite('TerminalManager - initialize() with activateEnvInCurrentTerminal', () => { | ||
| let terminalActivation: TestTerminalActivation; | ||
| let terminalManager: TerminalManagerImpl; | ||
| let mockGetAutoActivationType: sinon.SinonStub; | ||
| let mockShouldActivateInCurrentTerminal: sinon.SinonStub; | ||
| let mockTerminals: sinon.SinonStub; | ||
| let mockGetEnvironmentForTerminal: sinon.SinonStub; | ||
|
|
||
| const createMockTerminal = (name: string): Terminal => | ||
| ({ | ||
| name, | ||
| creationOptions: {} as TerminalOptions, | ||
| shellIntegration: undefined, | ||
| show: sinon.stub(), | ||
| sendText: sinon.stub(), | ||
| }) as unknown as Terminal; | ||
|
|
||
| const createMockEnvironment = (): PythonEnvironment => ({ | ||
| envId: { id: 'test-env-id', managerId: 'test-manager' }, | ||
| name: 'Test Environment', | ||
| displayName: 'Test Environment', | ||
| shortDisplayName: 'TestEnv', | ||
| displayPath: '/path/to/env', | ||
| version: '3.9.0', | ||
| environmentPath: Uri.file('/path/to/python'), | ||
| sysPrefix: '/path/to/env', | ||
| execInfo: { | ||
| run: { executable: '/path/to/python' }, | ||
| activation: [{ executable: '/path/to/activate' }], | ||
| }, | ||
| }); | ||
|
|
||
| setup(() => { | ||
| terminalActivation = new TestTerminalActivation(); | ||
|
|
||
| mockGetAutoActivationType = sinon.stub(terminalUtils, 'getAutoActivationType'); | ||
| mockShouldActivateInCurrentTerminal = sinon.stub(terminalUtils, 'shouldActivateInCurrentTerminal'); | ||
| mockGetEnvironmentForTerminal = sinon.stub(terminalUtils, 'getEnvironmentForTerminal'); | ||
| sinon.stub(terminalUtils, 'waitForShellIntegration').resolves(false); | ||
| sinon.stub(activationUtils, 'isActivatableEnvironment').returns(true); | ||
| sinon.stub(shellDetector, 'identifyTerminalShell').returns('bash'); | ||
|
|
||
| sinon.stub(windowApis, 'createTerminal').callsFake(() => createMockTerminal('new')); | ||
| sinon.stub(windowApis, 'onDidOpenTerminal').returns(new Disposable(() => {})); | ||
| sinon.stub(windowApis, 'onDidCloseTerminal').returns(new Disposable(() => {})); | ||
| sinon.stub(windowApis, 'onDidChangeWindowState').returns(new Disposable(() => {})); | ||
| sinon.stub(windowApis, 'activeTerminal').returns(undefined); | ||
|
|
||
| mockTerminals = sinon.stub(windowApis, 'terminals'); | ||
|
|
||
| sinon.stub(windowApis, 'withProgress').callsFake(async (_options, task) => { | ||
| const mockProgress = { report: () => {} }; | ||
| const mockToken = { | ||
| isCancellationRequested: false, | ||
| onCancellationRequested: () => new Disposable(() => {}), | ||
| }; | ||
| return task(mockProgress as never, mockToken as never); | ||
| }); | ||
| sinon.stub(workspaceApis, 'onDidChangeConfiguration').returns(new Disposable(() => {})); | ||
| }); | ||
|
|
||
| teardown(() => { | ||
| sinon.restore(); | ||
| terminalActivation.dispose(); | ||
| }); | ||
|
|
||
| function createTerminalManager(): TerminalManagerImpl { | ||
| return new TerminalManagerImpl(terminalActivation, [], []); | ||
| } | ||
|
|
||
| test('initialize activates all pre-existing terminals when shouldActivateInCurrentTerminal returns true', async () => { | ||
| const terminal1 = createMockTerminal('terminal1'); | ||
| const terminal2 = createMockTerminal('terminal2'); | ||
| const env = createMockEnvironment(); | ||
|
|
||
| mockGetAutoActivationType.returns(terminalUtils.ACT_TYPE_COMMAND); | ||
| mockShouldActivateInCurrentTerminal.returns(true); | ||
| mockTerminals.returns([terminal1, terminal2]); | ||
| mockGetEnvironmentForTerminal.resolves(env); | ||
|
Comment on lines
+495
to
+503
|
||
|
|
||
| terminalManager = createTerminalManager(); | ||
| await terminalManager.initialize({} as never); | ||
|
|
||
| assert.strictEqual( | ||
| terminalActivation.activateCalls, | ||
| 2, | ||
| 'Should activate all pre-existing terminals when activateEnvInCurrentTerminal is true/default', | ||
| ); | ||
| }); | ||
|
|
||
| test('initialize skips all pre-existing terminals when shouldActivateInCurrentTerminal returns false', async () => { | ||
| const terminal1 = createMockTerminal('terminal1'); | ||
| const terminal2 = createMockTerminal('terminal2'); | ||
| const env = createMockEnvironment(); | ||
|
|
||
| mockGetAutoActivationType.returns(terminalUtils.ACT_TYPE_COMMAND); | ||
| mockShouldActivateInCurrentTerminal.returns(false); | ||
| mockTerminals.returns([terminal1, terminal2]); | ||
| mockGetEnvironmentForTerminal.resolves(env); | ||
|
|
||
| terminalManager = createTerminalManager(); | ||
| await terminalManager.initialize({} as never); | ||
|
|
||
| assert.strictEqual( | ||
| terminalActivation.activateCalls, | ||
| 0, | ||
| 'Should skip all pre-existing terminals when activateEnvInCurrentTerminal is explicitly false', | ||
| ); | ||
| }); | ||
|
|
||
| test('initialize proceeds normally when shouldActivateInCurrentTerminal returns false but no pre-existing terminals', async () => { | ||
| mockGetAutoActivationType.returns(terminalUtils.ACT_TYPE_COMMAND); | ||
| mockShouldActivateInCurrentTerminal.returns(false); | ||
| mockTerminals.returns([]); | ||
|
|
||
| terminalManager = createTerminalManager(); | ||
| await terminalManager.initialize({} as never); | ||
|
|
||
| assert.strictEqual( | ||
| terminalActivation.activateCalls, | ||
| 0, | ||
| 'Should have no activations when there are no terminals', | ||
| ); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karthiknadig can you confirm the meaning of activateEnvInCurrentTerminal here- does that mean all that are currently open or the one the user is in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I remember it is one which VSCode says is the focused terminal.
We should confirm this by seeing what Python ext specifically looks at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is indeed focused terminal.
The thing is Python extension did not activate any terminal is
activateEnvInCurrentTerminalis false.Environment tries to activate ALL terminal by default, and with this PR (see the gif), we won't activate current (focused AND previous terminals in background) to more smoothly match Python ext behavior.