Fix onInterpreterChange event to only fire on interpreter change (microsoft#3847)

Author: IanMatthewHuff

news/2 Fixes/3749.md

@@ -0,0 +1 @@
+Don't restart the Jupyter server on any settings change. Also don't throw interpreter changed events on unrelated settings changes.

src/client/common/configSettings.ts

@@ -4,7 +4,7 @@ import * as child_process from 'child_process';
 import { EventEmitter } from 'events';
 import * as path from 'path';
 import {
- ConfigurationTarget, DiagnosticSeverity, Disposable, Uri,
+ ConfigurationChangeEvent, ConfigurationTarget, DiagnosticSeverity, Disposable, Uri,
  workspace, WorkspaceConfiguration
 } from 'vscode';
 import { sendTelemetryEvent } from '../telemetry';
@@ -348,13 +348,16 @@ export class PythonSettings extends EventEmitter implements IPythonSettings {
  }
  }
  protected initialize(): void {
- this.disposables.push(workspace.onDidChangeConfiguration(() => {
- const currentConfig = workspace.getConfiguration('python', this.workspaceRoot);
- this.update(currentConfig);
-
- // If workspace config changes, then we could have a cascading effect of on change events.
- // Let's defer the change notification.
- setTimeout(() => this.emit('change'), 1);
+ this.disposables.push(workspace.onDidChangeConfiguration((event: ConfigurationChangeEvent) => {
+ if (event.affectsConfiguration('python'))
+ {
+ const currentConfig = workspace.getConfiguration('python', this.workspaceRoot);
+ this.update(currentConfig);
+
+ // If workspace config changes, then we could have a cascading effect of on change events.
+ // Let's defer the change notification.
+ setTimeout(() => this.emit('change'), 1);
+ }
  }));
 
  const initialConfig = workspace.getConfiguration('python', this.workspaceRoot);

src/client/interpreter/interpreterService.ts

@@ -27,14 +27,17 @@ export class InterpreterService implements Disposable, IInterpreterService {
  private readonly fs: IFileSystem;
  private readonly persistentStateFactory: IPersistentStateFactory;
  private readonly helper: IInterpreterHelper;
+ private readonly configService: IConfigurationService;
  private readonly didChangeInterpreterEmitter = new EventEmitter<void>();
+ private pythonPathSetting: string = '';
 
  constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
  this.locator = serviceContainer.get<IInterpreterLocatorService>(IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE);
  this.helper = serviceContainer.get<IInterpreterHelper>(IInterpreterHelper);
  this.pythonPathUpdaterService = this.serviceContainer.get<IPythonPathUpdaterServiceManager>(IPythonPathUpdaterServiceManager);
  this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
  this.persistentStateFactory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
+ this.configService = this.serviceContainer.get<IConfigurationService>(IConfigurationService);
  }
  public get hasInterpreters(): Promise<boolean> {
  return this.locator.hasInterpreters;
@@ -49,8 +52,9 @@ export class InterpreterService implements Disposable, IInterpreterService {
  const disposables = this.serviceContainer.get<Disposable[]>(IDisposableRegistry);
  const documentManager = this.serviceContainer.get<IDocumentManager>(IDocumentManager);
  disposables.push(documentManager.onDidChangeActiveTextEditor((e) => e ? this.refresh(e.document.uri) : undefined));
- const configService = this.serviceContainer.get<IConfigurationService>(IConfigurationService);
- (configService.getSettings() as PythonSettings).addListener('change', this.onConfigChanged);
+ const pySettings = this.configService.getSettings() as PythonSettings;
+ this.pythonPathSetting = pySettings.pythonPath;
+ pySettings.addListener('change', this.onConfigChanged);
  }
 
  public async getInterpreters(resource?: Uri): Promise<PythonInterpreter[]> {
@@ -253,10 +257,15 @@ export class InterpreterService implements Disposable, IInterpreterService {
  }
 
  private onConfigChanged = () => {
- this.didChangeInterpreterEmitter.fire();
- const interpreterDisplay = this.serviceContainer.get<IInterpreterDisplay>(IInterpreterDisplay);
- interpreterDisplay.refresh()
- .catch(ex => console.error('Python Extension: display.refresh', ex));
+ // Check if we actually changed our python path
+ const pySettings = this.configService.getSettings() as PythonSettings;
+ if (this.pythonPathSetting !== pySettings.pythonPath) {
+ this.pythonPathSetting = pySettings.pythonPath;
+ this.didChangeInterpreterEmitter.fire();
+ const interpreterDisplay = this.serviceContainer.get<IInterpreterDisplay>(IInterpreterDisplay);
+ interpreterDisplay.refresh()
+ .catch(ex => console.error('Python Extension: display.refresh', ex));
+ }
  }
  private async collectInterpreterDetails(pythonPath: string, resource: Uri | undefined) {
  const interpreterHelper = this.serviceContainer.get<IInterpreterHelper>(IInterpreterHelper);

src/test/interpreters/interpreterService.unit.test.ts

@@ -7,7 +7,6 @@
 
 import { expect, use } from 'chai';
 import * as chaiAsPromised from 'chai-as-promised';
-import { EventEmitter } from 'events';
 import { Container } from 'inversify';
 import * as path from 'path';
 import { SemVer } from 'semver';
@@ -17,7 +16,7 @@ import { IDocumentManager, IWorkspaceService } from '../../client/common/applica
 import { getArchitectureDisplayName } from '../../client/common/platform/registry';
 import { IFileSystem } from '../../client/common/platform/types';
 import { IPythonExecutionFactory, IPythonExecutionService } from '../../client/common/process/types';
-import { IConfigurationService, IDisposableRegistry, IPersistentStateFactory } from '../../client/common/types';
+import { IConfigurationService, IDisposableRegistry, IPersistentStateFactory, IPythonSettings } from '../../client/common/types';
 import * as EnumEx from '../../client/common/utils/enum';
 import { noop } from '../../client/common/utils/misc';
 import { Architecture } from '../../client/common/utils/platform';
@@ -37,6 +36,7 @@ import { InterpreterService } from '../../client/interpreter/interpreterService'
 import { IVirtualEnvironmentManager } from '../../client/interpreter/virtualEnvs/types';
 import { ServiceContainer } from '../../client/ioc/container';
 import { ServiceManager } from '../../client/ioc/serviceManager';
+import { PYTHON_PATH } from '../common';
 
 use(chaiAsPromised);
 
@@ -69,6 +69,8 @@ suite('Interpreters service', () => {
  let persistentStateFactory: TypeMoq.IMock<IPersistentStateFactory>;
  let pythonExecutionFactory: TypeMoq.IMock<IPythonExecutionFactory>;
  let pythonExecutionService: TypeMoq.IMock<IPythonExecutionService>;
+ let configService: TypeMoq.IMock<IConfigurationService>;
+ let pythonSettings: TypeMoq.IMock<IPythonSettings>;
  type ConfigValue<T> = { key: string; defaultValue?: T; globalValue?: T; workspaceValue?: T; workspaceFolderValue?: T };
 
  function setupSuite() {
@@ -88,6 +90,11 @@ suite('Interpreters service', () => {
  persistentStateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
  pythonExecutionFactory = TypeMoq.Mock.ofType<IPythonExecutionFactory>();
  pythonExecutionService = TypeMoq.Mock.ofType<IPythonExecutionService>();
+ configService = TypeMoq.Mock.ofType<IConfigurationService>();
+
+ pythonSettings = TypeMoq.Mock.ofType<IPythonSettings>();
+ pythonSettings.setup(s => s.pythonPath).returns(() => PYTHON_PATH);
+ configService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object);
 
  pythonExecutionService.setup((p: any) => p.then).returns(() => undefined);
  workspace.setup(x => x.getConfiguration('python', TypeMoq.It.isAny())).returns(() => config.object);
@@ -113,6 +120,7 @@ suite('Interpreters service', () => {
  serviceManager.addSingletonInstance<IPersistentStateFactory>(IPersistentStateFactory, persistentStateFactory.object);
  serviceManager.addSingletonInstance<IPythonExecutionFactory>(IPythonExecutionFactory, pythonExecutionFactory.object);
  serviceManager.addSingletonInstance<IPythonExecutionService>(IPythonExecutionService, pythonExecutionService.object);
+ serviceManager.addSingletonInstance<IConfigurationService>(IConfigurationService, configService.object);
 
  pipenvLocator = TypeMoq.Mock.ofType<IInterpreterLocatorService>();
  wksLocator = TypeMoq.Mock.ofType<IInterpreterLocatorService>();
@@ -149,21 +157,18 @@ suite('Interpreters service', () => {
  });
  });
 
- test('Changes to active document should invoke intrepreter.refresh method', async () => {
+ test('Changes to active document should invoke interpreter.refresh method', async () => {
  const service = new InterpreterService(serviceContainer);
- const configService = TypeMoq.Mock.ofType<IConfigurationService>();
  const documentManager = TypeMoq.Mock.ofType<IDocumentManager>();
 
  let activeTextEditorChangeHandler: Function | undefined;
  documentManager.setup(d => d.onDidChangeActiveTextEditor(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(handler => {
  activeTextEditorChangeHandler = handler;
  return { dispose: noop };
  });
- serviceManager.addSingletonInstance(IConfigurationService, configService.object);
  serviceManager.addSingletonInstance(IDocumentManager, documentManager.object);
 
  // tslint:disable-next-line:no-any
- configService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => new EventEmitter() as any);
  service.initialize();
  const textEditor = TypeMoq.Mock.ofType<TextEditor>();
  const uri = Uri.file(path.join('usr', 'file.py'));
@@ -175,21 +180,18 @@ suite('Interpreters service', () => {
  interpreterDisplay.verify(i => i.refresh(TypeMoq.It.isValue(uri)), TypeMoq.Times.once());
  });
 
- test('If there is no active document then intrepreter.refresh should not be invoked', async () => {
+ test('If there is no active document then interpreter.refresh should not be invoked', async () => {
  const service = new InterpreterService(serviceContainer);
- const configService = TypeMoq.Mock.ofType<IConfigurationService>();
  const documentManager = TypeMoq.Mock.ofType<IDocumentManager>();
 
  let activeTextEditorChangeHandler: Function | undefined;
  documentManager.setup(d => d.onDidChangeActiveTextEditor(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(handler => {
  activeTextEditorChangeHandler = handler;
  return { dispose: noop };
  });
- serviceManager.addSingletonInstance(IConfigurationService, configService.object);
  serviceManager.addSingletonInstance(IDocumentManager, documentManager.object);
 
  // tslint:disable-next-line:no-any
- configService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => new EventEmitter() as any);
  service.initialize();
  activeTextEditorChangeHandler!();