Better handle activateEnvInCurrentTerminal from Python extension#1318
Better handle activateEnvInCurrentTerminal from Python extension#1318anthonykim1 wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves how the extension honors the Python extension’s legacy terminal activation setting (python.terminal.activateEnvInCurrentTerminal) when initializing terminal activation for terminals that already exist at extension load time, aligning behavior with issue #1300.
Changes:
- Add
shouldActivateInCurrentTerminal()to interpretpython.terminal.activateEnvInCurrentTerminalusinginspect()so defaults can be distinguished from user-explicit values. - Update
TerminalManagerImpl.initialize()to skip activating pre-existing terminals when that setting is explicitly disabled. - Add unit tests covering the new helper and the initialize-time skip behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/features/terminal/utils.ts | Adds shouldActivateInCurrentTerminal() helper to evaluate the legacy Python setting via inspect(). |
| src/features/terminal/terminalManager.ts | Skips activation for pre-existing terminals during initialize when the legacy setting is explicitly disabled. |
| src/test/features/terminal/utils.unit.test.ts | Adds unit tests for shouldActivateInCurrentTerminal(). |
| src/test/features/terminal/terminalManager.unit.test.ts | Adds unit tests for initialize behavior gated by shouldActivateInCurrentTerminal(). |
Comments suppressed due to low confidence (1)
src/features/terminal/utils.ts:283
- This inspect() call is created without a scope/resource (getConfiguration('python') with no Uri). In VS Code,
workspaceFolderValuewill beundefinedunless configuration is scoped to a specific resource, so the workspace-folder precedence described here won’t be honored in multi-root workspaces. Consider accepting a Uri/scope parameter (e.g., derived from terminal CWD/workspace folder) and calling getConfiguration('python', scope) so workspaceFolderValue can be evaluated.
const pythonConfig = getConfiguration('python');
const inspected = pythonConfig.inspect<boolean>('terminal.activateEnvInCurrentTerminal');
You can also share your feedback on Copilot code review. Take the survey.
| 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))); | ||
| } |
There was a problem hiding this comment.
initialize() calls terminals() multiple times (including while computing skipPreExistingTerminals and later when iterating). Capture a single snapshot (e.g., const existing = terminals();) and use it throughout initialize() so behavior is consistent and avoids repeated lookups while async work is in-flight.
| test('should return true when no explicit values are set (all undefined)', () => { | ||
| pythonConfig.inspect.withArgs('terminal.activateEnvInCurrentTerminal').returns({ | ||
| key: 'terminal.activateEnvInCurrentTerminal', | ||
| defaultValue: false, | ||
| globalValue: undefined, | ||
| workspaceValue: undefined, | ||
| workspaceFolderValue: undefined, | ||
| }); | ||
|
|
||
| assert.strictEqual( | ||
| shouldActivateInCurrentTerminal(), | ||
| true, | ||
| 'Should return true when only defaultValue is set (not user-explicit)', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Tests don’t cover the key precedence case where a higher-precedence explicit true should override a lower-precedence explicit false (e.g., workspaceValue: true with globalValue: false). Adding this case would catch incorrect precedence handling in shouldActivateInCurrentTerminal().
| 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); |
There was a problem hiding this comment.
initialize() behavior was changed for both ACT_TYPE_COMMAND and ACT_TYPE_SHELL, but tests only exercise the command path. Add coverage for ACT_TYPE_SHELL to ensure activation fallback is also skipped (or performed) correctly when shouldActivateInCurrentTerminal() is false/true.
| 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; | ||
| } |
There was a problem hiding this comment.
shouldActivateInCurrentTerminal() does not apply VS Code settings precedence correctly: a lower-precedence globalValue === false will force false even when a higher-precedence scope (workspace/workspaceFolder) explicitly sets true. Instead, follow the same pattern as getShellIntegrationEnabledCache(): return the first defined boolean in precedence order (workspaceFolderValue > workspaceValue > globalRemoteValue > globalLocalValue > globalValue), and default to true only when none are defined.
This issue also appears on line 281 of the same file.
| public async initialize(api: PythonEnvironmentApi): Promise<void> { | ||
| const actType = getAutoActivationType(); | ||
|
|
||
| // When activateEnvInCurrentTerminal is explicitly false, |
There was a problem hiding this comment.
@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.
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.
It is indeed focused terminal.
The thing is Python extension did not activate any terminal is activateEnvInCurrentTerminal is 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.
Resolves: #1300
Behaviors after this PR:
Note that the default false and explicit true behaves the same.
explicitFalse.mov
explicittrueorrDefault.mov