-
Notifications
You must be signed in to change notification settings - Fork 42
Refactor environment change events to make getEnvironment a pure read and add refreshEnvironment event #1495
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
Changes from all commits
63f5c6a
38d65a4
edbc422
37c7f31
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 |
|---|---|---|
| @@ -1,35 +1,40 @@ | ||
| import { Uri, Disposable, Event, EventEmitter, Terminal, TaskExecution } from 'vscode'; | ||
| import { Disposable, Event, EventEmitter, TaskExecution, Terminal, Uri } from 'vscode'; | ||
| import { | ||
| PythonEnvironmentApi, | ||
| PythonEnvironment, | ||
| EnvironmentManager, | ||
| PackageManager, | ||
| CreateEnvironmentOptions, | ||
| CreateEnvironmentScope, | ||
| DidChangeEnvironmentEventArgs, | ||
| DidChangeEnvironmentsEventArgs, | ||
| DidChangeEnvironmentVariablesEventArgs, | ||
| DidChangePackagesEventArgs, | ||
| DidChangePythonProjectsEventArgs, | ||
| EnvironmentManager, | ||
| GetEnvironmentScope, | ||
| GetEnvironmentsScope, | ||
| Package, | ||
| PythonEnvironmentInfo, | ||
| PythonProject, | ||
| RefreshEnvironmentsScope, | ||
| DidChangePackagesEventArgs, | ||
| PythonEnvironmentId, | ||
| CreateEnvironmentScope, | ||
| SetEnvironmentScope, | ||
| GetEnvironmentScope, | ||
| PackageInfo, | ||
| PackageId, | ||
| PythonProjectCreator, | ||
| ResolveEnvironmentContext, | ||
| PackageInfo, | ||
| PackageManagementOptions, | ||
| PackageManager, | ||
| PythonBackgroundRunOptions, | ||
| PythonEnvironment, | ||
| PythonEnvironmentApi, | ||
| PythonEnvironmentId, | ||
| PythonEnvironmentInfo, | ||
| PythonProcess, | ||
| PythonProject, | ||
| PythonProjectCreator, | ||
| PythonTaskExecutionOptions, | ||
| PythonTerminalExecutionOptions, | ||
| PythonBackgroundRunOptions, | ||
| PythonTerminalCreateOptions, | ||
| DidChangeEnvironmentVariablesEventArgs, | ||
| CreateEnvironmentOptions, | ||
| PythonTerminalExecutionOptions, | ||
| RefreshEnvironmentsScope, | ||
| ResolveEnvironmentContext, | ||
| SetEnvironmentScope, | ||
| } from '../api'; | ||
| import { traceError, traceInfo } from '../common/logging'; | ||
| import { pickEnvironmentManager } from '../common/pickers/managers'; | ||
| import { createDeferred } from '../common/utils/deferred'; | ||
| import { checkUri } from '../common/utils/pathUtils'; | ||
| import { handlePythonPath } from '../common/utils/pythonPath'; | ||
| import { | ||
| EnvironmentManagers, | ||
| InternalEnvironmentManager, | ||
|
|
@@ -38,17 +43,12 @@ import { | |
| PythonPackageImpl, | ||
| PythonProjectManager, | ||
| } from '../internal.api'; | ||
| import { createDeferred } from '../common/utils/deferred'; | ||
| import { traceInfo } from '../common/logging'; | ||
| import { pickEnvironmentManager } from '../common/pickers/managers'; | ||
| import { handlePythonPath } from '../common/utils/pythonPath'; | ||
| import { TerminalManager } from './terminal/terminalManager'; | ||
| import { waitForAllEnvManagers, waitForEnvManager, waitForEnvManagerId } from './common/managerReady'; | ||
| import { EnvVarManager } from './execution/envVariableManager'; | ||
| import { runAsTask } from './execution/runAsTask'; | ||
| import { runInTerminal } from './terminal/runInTerminal'; | ||
| import { runInBackground } from './execution/runInBackground'; | ||
| import { EnvVarManager } from './execution/envVariableManager'; | ||
| import { checkUri } from '../common/utils/pathUtils'; | ||
| import { waitForAllEnvManagers, waitForEnvManager, waitForEnvManagerId } from './common/managerReady'; | ||
| import { runInTerminal } from './terminal/runInTerminal'; | ||
| import { TerminalManager } from './terminal/terminalManager'; | ||
|
|
||
| class PythonEnvironmentApiImpl implements PythonEnvironmentApi { | ||
| private readonly _onDidChangeEnvironments = new EventEmitter<DidChangeEnvironmentsEventArgs>(); | ||
|
|
@@ -71,7 +71,7 @@ class PythonEnvironmentApiImpl implements PythonEnvironmentApi { | |
| this._onDidChangePythonProjects, | ||
| this._onDidChangePackages, | ||
| this._onDidChangeEnvironmentVariables, | ||
| this.envManagers.onDidChangeEnvironmentFiltered((e) => { | ||
| this.envManagers.onDidChangeActiveEnvironment((e) => { | ||
| this._onDidChangeEnvironment.fire(e); | ||
| const location = e.uri?.fsPath ?? 'global'; | ||
| traceInfo( | ||
|
|
@@ -91,14 +91,14 @@ class PythonEnvironmentApiImpl implements PythonEnvironmentApi { | |
| if (manager.onDidChangeEnvironment) { | ||
| disposables.push( | ||
| manager.onDidChangeEnvironment((e) => { | ||
| setImmediate(async () => { | ||
| // This will ensure that we use the right manager and only trigger the event | ||
| // if the user selected manager decided to change the environment. | ||
| // This ensures that if a unselected manager changes environment and raises events | ||
| // we don't trigger the Python API event which can cause issues with the consumers. | ||
| // This will trigger onDidChangeEnvironmentFiltered event in envManagers, which the Python | ||
| // API listens to, and re-triggers the onDidChangeEnvironment event. | ||
| await this.envManagers.getEnvironment(e.uri); | ||
| setImmediate(() => { | ||
| // Refresh the central cache for this scope. This ensures that only the | ||
| // *selected* manager's changes propagate (refreshEnvironment checks | ||
| // getEnvironmentManager(scope) internally). It updates the cache and | ||
| // fires onDidChangeActiveEnvironment, which the Python API listens to. | ||
| this.envManagers.refreshEnvironment(e.uri).catch((err) => | ||
| traceError('Failed to refresh environment on change:', err), | ||
| ); | ||
| }); | ||
|
Comment on lines
93
to
102
|
||
| }), | ||
| ); | ||
|
|
||
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.
The new EventEmitters
_onDidChangeManagerEnvironment/_onDidChangeActiveEnvironmentare created here, butdispose()only disposes_onDidChangeEnvironmentManager,_onDidChangePackageManager,_onDidChangeEnvironments, and_onDidChangePackages. Please also dispose these new emitters (and any corresponding public events) to avoid leaking listeners whenPythonEnvironmentManagersis disposed.