Ensure a refresh is only triggered after all handlers are registered in quickpicker (#19579)

* Ensure a refresh is only triggered after all handlers are registered for the quickpick

* Do not show install python item until we're sure no python is installed

* Fix tests

Author: Kartik Raj

src/client/common/utils/multiStepInput.ts

@@ -54,6 +54,10 @@ export interface IQuickPickParameters<T extends QuickPickItem, E = any> {
     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<T>) => void;
         event: Event<E>;
@@ -120,6 +124,7 @@ export class MultiStepInput<S> implements IMultiStepInput<S> {
         onChangeItem,
         keepScrollPosition,
         sortByLabel,
+        initialize,
     }: P): Promise<MultiStepInputQuickPicResponseType<T, P>> {
         const disposables: Disposable[] = [];
         try {
@@ -175,6 +180,9 @@ export class MultiStepInput<S> implements IMultiStepInput<S> {
                     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.

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,13 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
                     }
                 });
             } else {
-                items.push(this.noPythonInstalled);
-                if (this.wasNoPythonInstalledItemClicked) {
+                if (!items.some((i) => isSpecialQuickPickItem(i) && i.label === this.noPythonInstalled.label)) {
+                    items.push(this.noPythonInstalled);
+                }
+                if (
+                    this.wasNoPythonInstalledItemClicked &&
+                    !items.some((i) => isSpecialQuickPickItem(i) && i.label === this.tipToReloadWindow.label)
+                ) {
                     items.push(this.tipToReloadWindow);
                 }
             }

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;
 };

src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts

@@ -104,8 +104,8 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
         const stopWatch = new StopWatch();
         let refreshPromise = this.getRefreshPromiseForQuery(query);
         if (!refreshPromise) {
-            if (options?.ifNotTriggerredAlready && this.hasRefreshFinished(query) && this.getEnvs().length) {
-                // Do not trigger another refresh if a refresh has previously finished and envs were found.
+            if (options?.ifNotTriggerredAlready && this.hasRefreshFinished(query)) {
+                // Do not trigger another refresh if a refresh has previously finished.
                 return Promise.resolve();
             }
             refreshPromise = this.startRefresh(query, options);

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;