From 2adafd4a3102b844e6d5a3573e013c5bfc349158 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 27 Jul 2022 11:15:35 -0700 Subject: [PATCH 1/3] Ensure a refresh is only triggered after all handlers are registered for the quickpick --- src/client/common/utils/multiStepInput.ts | 8 +++++ .../commands/setInterpreter.ts | 30 ++++++++++++++----- src/client/pythonEnvironments/base/locator.ts | 2 +- .../composite/envsCollectionService.ts | 4 +-- 4 files changed, 34 insertions(+), 10 deletions(-) diff --git a/src/client/common/utils/multiStepInput.ts b/src/client/common/utils/multiStepInput.ts index b06906f00754..5e978d3b716c 100644 --- a/src/client/common/utils/multiStepInput.ts +++ b/src/client/common/utils/multiStepInput.ts @@ -54,6 +54,10 @@ export interface IQuickPickParameters { keepScrollPosition?: boolean; sortByLabel?: boolean; acceptFilterBoxTextAsSelection?: boolean; + /** + * A method called only after quickpick has been created and all handlers are registered. + */ + initialize?: () => void; onChangeItem?: { callback: (event: E, quickPick: QuickPick) => void; event: Event; @@ -120,6 +124,7 @@ export class MultiStepInput implements IMultiStepInput { onChangeItem, keepScrollPosition, sortByLabel, + initialize, }: P): Promise> { const disposables: Disposable[] = []; try { @@ -175,6 +180,9 @@ export class MultiStepInput implements IMultiStepInput { disposables.push(onChangeItem.event((e) => onChangeItem.callback(e, input))); } this.current.show(); + if (initialize) { + initialize(); + } // Keep scroll position is only meant to keep scroll position when updating items, // so do it after initialization. This ensures quickpick starts with the active // item in focus when this is true, instead of having scroll position at top. diff --git a/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts b/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts index f344a85765ae..bb6f0c9f5dcf 100644 --- a/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts +++ b/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts @@ -126,10 +126,6 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { // times so that the visible items do not change. const preserveOrderWhenFiltering = !!this.interpreterService.refreshPromise; const suggestions = this.getItems(state.workspace); - // Discovery is no longer guranteed to be auto-triggered on extension load, so trigger it when - // user interacts with the interpreter picker but only once per session. Users can rely on the - // refresh button if they want to trigger it more than once. - this.interpreterService.triggerRefresh(undefined, { ifNotTriggerredAlready: true }).ignoreErrors(); state.path = undefined; const currentInterpreterPathDisplay = this.pathUtils.getDisplayName( this.configurationService.getSettings(state.workspace).pythonPath, @@ -155,6 +151,17 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { }, callback: () => this.interpreterService.triggerRefresh().ignoreErrors(), }, + initialize: () => { + // Note discovery is no longer guranteed to be auto-triggered on extension load, so trigger it when + // user interacts with the interpreter picker but only once per session. Users can rely on the + // refresh button if they want to trigger it more than once. However if no envs were found previously, + // always trigger a refresh. + if (this.interpreterService.getInterpreters().length === 0) { + this.interpreterService.triggerRefresh().ignoreErrors(); + } else { + this.interpreterService.triggerRefresh(undefined, { ifNotTriggerredAlready: true }).ignoreErrors(); + } + }, onChangeItem: { event: this.interpreterService.onDidChangeInterpreters, // It's essential that each callback is handled synchronously, as result of the previous @@ -341,7 +348,8 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { private finalizeItems(items: QuickPickType[], resource: Resource) { const interpreterSuggestions = this.interpreterSelector.getSuggestions(resource, true); - if (!this.interpreterService.refreshPromise) { + const r = this.interpreterService.refreshPromise; + if (!r) { if (interpreterSuggestions.length) { this.setRecommendedItem(interpreterSuggestions, items, resource); // Add warning label to certain environments @@ -357,8 +365,16 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { } }); } else { - items.push(this.noPythonInstalled); - if (this.wasNoPythonInstalledItemClicked) { + const doesNoPyItemExist = items.some( + (item) => isSpecialQuickPickItem(item) && item.label === this.noPythonInstalled.label, + ); + if (doesNoPyItemExist) { + items.push(this.noPythonInstalled); + } + if ( + this.wasNoPythonInstalledItemClicked && + items.some((item) => isSpecialQuickPickItem(item) && item.label === this.tipToReloadWindow.label) + ) { items.push(this.tipToReloadWindow); } } diff --git a/src/client/pythonEnvironments/base/locator.ts b/src/client/pythonEnvironments/base/locator.ts index 0e0347229df9..609010501d63 100644 --- a/src/client/pythonEnvironments/base/locator.ts +++ b/src/client/pythonEnvironments/base/locator.ts @@ -199,7 +199,7 @@ export type TriggerRefreshOptions = { */ clearCache?: boolean; /** - * Only trigger a refresh if it hasn't already been triggered for this session, or if no envs were found previously. + * Only trigger a refresh if it hasn't already been triggered for this session. */ ifNotTriggerredAlready?: boolean; }; diff --git a/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts b/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts index eda2c25fdb70..f652b420ecf8 100644 --- a/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts +++ b/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts @@ -104,8 +104,8 @@ export class EnvsCollectionService extends PythonEnvsWatcher Date: Wed, 27 Jul 2022 11:35:21 -0700 Subject: [PATCH 2/3] Do not show install python item until we're sure no python is installed --- .../interpreterSelector/commands/setInterpreter.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts b/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts index bb6f0c9f5dcf..d47edd192172 100644 --- a/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts +++ b/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts @@ -365,15 +365,12 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { } }); } else { - const doesNoPyItemExist = items.some( - (item) => isSpecialQuickPickItem(item) && item.label === this.noPythonInstalled.label, - ); - if (doesNoPyItemExist) { + if (!items.some((i) => isSpecialQuickPickItem(i) && i.label === this.noPythonInstalled.label)) { items.push(this.noPythonInstalled); } if ( this.wasNoPythonInstalledItemClicked && - items.some((item) => isSpecialQuickPickItem(item) && item.label === this.tipToReloadWindow.label) + !items.some((i) => isSpecialQuickPickItem(i) && i.label === this.tipToReloadWindow.label) ) { items.push(this.tipToReloadWindow); } From f6ff879cdfb5e83c8fe9b1499411c16459779a95 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 27 Jul 2022 16:37:50 -0700 Subject: [PATCH 3/3] Fix tests --- .../interpreterSelector/commands/setInterpreter.unit.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts b/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts index ef73ef4cec04..856ba6a81639 100644 --- a/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts +++ b/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts @@ -263,6 +263,7 @@ suite('Set Interpreter Command', () => { expect(actualParameters).to.not.equal(undefined, 'Parameters not set'); const refreshButtonCallback = actualParameters!.customButtonSetup?.callback; expect(refreshButtonCallback).to.not.equal(undefined, 'Callback not set'); + delete actualParameters!.initialize; delete actualParameters!.customButtonSetup; delete actualParameters!.onChangeItem; assert.deepStrictEqual(actualParameters, expectedParameters, 'Params not equal'); @@ -303,6 +304,7 @@ suite('Set Interpreter Command', () => { expect(actualParameters).to.not.equal(undefined, 'Parameters not set'); const refreshButtonCallback = actualParameters!.customButtonSetup?.callback; expect(refreshButtonCallback).to.not.equal(undefined, 'Callback not set'); + delete actualParameters!.initialize; delete actualParameters!.customButtonSetup; delete actualParameters!.onChangeItem; assert.deepStrictEqual(actualParameters, expectedParameters, 'Params not equal'); @@ -458,6 +460,7 @@ suite('Set Interpreter Command', () => { expect(actualParameters).to.not.equal(undefined, 'Parameters not set'); const refreshButtonCallback = actualParameters!.customButtonSetup?.callback; expect(refreshButtonCallback).to.not.equal(undefined, 'Callback not set'); + delete actualParameters!.initialize; delete actualParameters!.customButtonSetup; delete actualParameters!.onChangeItem; assert.deepStrictEqual(actualParameters?.items, expectedParameters.items, 'Params not equal'); @@ -542,6 +545,7 @@ suite('Set Interpreter Command', () => { const refreshButtonCallback = actualParameters!.customButtonSetup?.callback; expect(refreshButtonCallback).to.not.equal(undefined, 'Callback not set'); + delete actualParameters!.initialize; delete actualParameters!.customButtonSetup; delete actualParameters!.onChangeItem;