diff --git a/src/common/pickers/environments.ts b/src/common/pickers/environments.ts index 86a9ba059..3031cccb1 100644 --- a/src/common/pickers/environments.ts +++ b/src/common/pickers/environments.ts @@ -8,6 +8,7 @@ import { sendTelemetryEvent } from '../telemetry/sender'; import { isWindows } from '../utils/platformUtils'; import { handlePythonPath } from '../utils/pythonPath'; import { + QuickPickWithButtonsOptions, showErrorMessage, showInputBox, showOpenDialog, @@ -47,6 +48,9 @@ interface EnvironmentPickOptions { showBackButton?: boolean; projects: PythonProject[]; } + +type EnvironmentPickItem = QuickPickItem | (QuickPickItem & { result: PythonEnvironment }); + async function browseForPython( managers: InternalEnvironmentManager[], projectEnvManagers: InternalEnvironmentManager[], @@ -144,15 +148,17 @@ async function createEnvironment( } async function pickEnvironmentImpl( - items: (QuickPickItem | (QuickPickItem & { result: PythonEnvironment }))[], + items: EnvironmentPickItem[], managers: InternalEnvironmentManager[], projectEnvManagers: InternalEnvironmentManager[], options: EnvironmentPickOptions, + itemLoader?: QuickPickWithButtonsOptions['itemLoader'], ): Promise { const selected = await showQuickPickWithButtons(items, { placeHolder: Pickers.Environments.selectEnvironment, ignoreFocusOut: true, showBackButton: options?.showBackButton, + itemLoader, }); if (selected && !Array.isArray(selected)) { @@ -177,7 +183,7 @@ export async function pickEnvironment( projectEnvManagers: InternalEnvironmentManager[], options: EnvironmentPickOptions, ): Promise { - const items: (QuickPickItem | (QuickPickItem & { result: PythonEnvironment }))[] = [ + const items: EnvironmentPickItem[] = [ { label: Interpreter.enterInterpreterPath, iconPath: new ThemeIcon('edit'), @@ -222,30 +228,45 @@ export async function pickEnvironment( ); } - for (const manager of managers) { - items.push({ - label: manager.displayName, - kind: QuickPickItemKind.Separator, - }); - const envs = await manager.getEnvironments('all'); - items.push( - ...envs.map((e) => { - const pathDescription = e.displayPath; - const description = - e.description && e.description.trim() ? `${e.description} (${pathDescription})` : pathDescription; + const managerItems: (EnvironmentPickItem[] | undefined)[] = []; + const itemLoader: QuickPickWithButtonsOptions['itemLoader'] = + managers.length > 0 + ? async (updateItems) => { + await Promise.all( + managers.map(async (manager, index) => { + try { + const envs = await manager.getEnvironments('all'); + managerItems[index] = [ + { + label: manager.displayName, + kind: QuickPickItemKind.Separator, + }, + ...envs.map((e) => { + const pathDescription = e.displayPath; + const description = + e.description && e.description.trim() + ? `${e.description} (${pathDescription})` + : pathDescription; - return { - label: e.displayName ?? e.name, - description: description, - result: e, - manager: manager, - iconPath: getIconPath(e.iconPath), - }; - }), - ); - } + return { + label: e.displayName ?? e.name, + description: description, + result: e, + manager: manager, + iconPath: getIconPath(e.iconPath), + }; + }), + ]; + updateItems([...items, ...managerItems.flatMap((current) => current ?? [])]); + } catch (ex) { + traceError(`Failed to get environments from ${manager.id}`, ex); + } + }), + ); + } + : undefined; - return pickEnvironmentImpl(items, managers, projectEnvManagers, options); + return pickEnvironmentImpl(items, managers, projectEnvManagers, options, itemLoader); } export async function pickEnvironmentFrom(environments: PythonEnvironment[]): Promise { diff --git a/src/common/window.apis.ts b/src/common/window.apis.ts index 13ee0393c..bb6c1f3d4 100644 --- a/src/common/window.apis.ts +++ b/src/common/window.apis.ts @@ -32,6 +32,7 @@ import { window, WindowState, } from 'vscode'; +import { traceError } from './logging'; import { createDeferred } from './utils/deferred'; export function createStatusBarItem(id: string, alignment?: StatusBarAlignment, priority?: number): StatusBarItem { @@ -152,6 +153,13 @@ export interface QuickPickButtonEvent { readonly button: QuickInputButton; } +export interface QuickPickWithButtonsOptions extends QuickPickOptions { + showBackButton?: boolean; + buttons?: QuickInputButton[]; + selected?: T[]; + itemLoader?: (updateItems: (items: readonly T[]) => void) => Promise; +} + export function showQuickPick( items: readonly T[] | Thenable, options?: QuickPickOptions, @@ -175,7 +183,7 @@ export function withProgress( export async function showQuickPickWithButtons( items: readonly T[], - options?: QuickPickOptions & { showBackButton?: boolean; buttons?: QuickInputButton[]; selected?: T[] }, + options?: QuickPickWithButtonsOptions, token?: CancellationToken, itemButtonHandler?: (e: QuickPickItemButtonEvent) => void, ): Promise { @@ -191,6 +199,8 @@ export async function showQuickPickWithButtons( quickPick.placeholder = options?.placeHolder; quickPick.title = options?.title; quickPick.selectedItems = options?.selected ?? []; + const itemLoader = options?.itemLoader; + quickPick.busy = itemLoader !== undefined; if (options?.showBackButton) { quickPick.buttons = [QuickInputButtons.Back]; @@ -241,6 +251,35 @@ export async function showQuickPickWithButtons( } quickPick.show(); + if (itemLoader) { + void Promise.resolve() + .then(() => + itemLoader((updatedItems) => { + if (deferred.completed) { + return; + } + const activeItems = quickPick.activeItems; + const selectedItems = quickPick.selectedItems; + quickPick.keepScrollPosition = true; + quickPick.items = updatedItems; + const retainedSelectedItems = selectedItems.filter((item) => updatedItems.includes(item)); + const retainedActiveItems = activeItems.filter((item) => updatedItems.includes(item)); + if (retainedSelectedItems.length > 0) { + quickPick.selectedItems = retainedSelectedItems; + } + if (retainedActiveItems.length > 0) { + quickPick.activeItems = retainedActiveItems; + } + }), + ) + .catch((ex) => traceError('Failed to update quick pick items', ex)) + .finally(() => { + if (!deferred.completed) { + quickPick.busy = false; + } + }); + } + try { return await deferred.promise; } finally { diff --git a/src/test/common/apiWrappers.unit.test.ts b/src/test/common/apiWrappers.unit.test.ts index ccc814ab3..211825cbb 100644 --- a/src/test/common/apiWrappers.unit.test.ts +++ b/src/test/common/apiWrappers.unit.test.ts @@ -1,8 +1,20 @@ import assert from 'node:assert'; import { reset, verify, when } from 'ts-mockito'; -import { Disposable, MessageItem, MessageOptions, TextDocument, WindowState } from 'vscode'; +import { + Disposable, + EventEmitter, + MessageItem, + MessageOptions, + QuickInputButton, + QuickPick, + QuickPickItem, + QuickPickItemButtonEvent, + TextDocument, + WindowState, +} from 'vscode'; import { executeCommand, registerCommand } from '../../common/command.api'; -import { onDidChangeWindowState, showWarningMessage } from '../../common/window.apis'; +import { createDeferred } from '../../common/utils/deferred'; +import { onDidChangeWindowState, showQuickPickWithButtons, showWarningMessage } from '../../common/window.apis'; import { onDidOpenTextDocument } from '../../common/workspace.apis'; import { mockedVSCodeNamespaces } from '../unittests'; @@ -11,6 +23,67 @@ suite('Typed API wrappers', () => { const window = mockedVSCodeNamespaces.window!; const workspace = mockedVSCodeNamespaces.workspace!; + function createFakeQuickPick() { + const didAccept = new EventEmitter(); + const didHide = new EventEmitter(); + const didTriggerButton = new EventEmitter(); + const didTriggerItemButton = new EventEmitter>(); + let items: readonly T[] = []; + let activeItems: readonly T[] = []; + let selectedItems: readonly T[] = []; + let activeItemsSetCount = 0; + let showCount = 0; + let disposeCount = 0; + const quickPick = { + get items() { + return items; + }, + set items(value: readonly T[]) { + items = value; + activeItems = []; + selectedItems = []; + }, + get activeItems() { + return activeItems; + }, + set activeItems(value: readonly T[]) { + activeItems = value; + activeItemsSetCount += 1; + }, + get selectedItems() { + return selectedItems; + }, + set selectedItems(value: readonly T[]) { + selectedItems = value; + }, + buttons: [], + show: () => { + showCount += 1; + }, + hide: () => didHide.fire(), + dispose: () => { + disposeCount += 1; + }, + onDidAccept: didAccept.event, + onDidHide: didHide.event, + onDidTriggerButton: didTriggerButton.event, + onDidTriggerItemButton: didTriggerItemButton.event, + } as unknown as QuickPick; + return { + quickPick, + hide: () => didHide.fire(), + get showCount() { + return showCount; + }, + get disposeCount() { + return disposeCount; + }, + get activeItemsSetCount() { + return activeItemsSetCount; + }, + }; + } + teardown(() => { reset(commands); reset(window); @@ -115,4 +188,93 @@ suite('Typed API wrappers', () => { assert.strictEqual(selected, action); assert.strictEqual(selected?.id, 1); }); + + test('updates quick pick items without losing focus while loading', async () => { + const fake = createFakeQuickPick(); + const initial = { label: 'initial' }; + const discovered = { label: 'discovered' }; + const release = createDeferred(); + const loaderStarted = createDeferred(); + const loaderFinished = createDeferred(); + when(window.createQuickPick()).thenReturn(fake.quickPick); + + const selection = showQuickPickWithButtons([initial], { + itemLoader: async (updateItems) => { + loaderStarted.resolve(); + await release.promise; + updateItems([initial, discovered]); + loaderFinished.resolve(); + }, + }); + await loaderStarted.promise; + + fake.quickPick.activeItems = [initial]; + fake.quickPick.selectedItems = [initial]; + assert.strictEqual(fake.quickPick.busy, true); + assert.strictEqual(fake.showCount, 1); + + release.resolve(); + await loaderFinished.promise; + await new Promise((resolve) => setImmediate(resolve)); + + assert.deepStrictEqual(fake.quickPick.items, [initial, discovered]); + assert.deepStrictEqual(fake.quickPick.activeItems, [initial]); + assert.deepStrictEqual(fake.quickPick.selectedItems, [initial]); + assert.strictEqual(fake.activeItemsSetCount, 2); + assert.strictEqual(fake.quickPick.keepScrollPosition, true); + assert.strictEqual(fake.quickPick.busy, false); + + fake.hide(); + assert.strictEqual(await selection, undefined); + }); + + test('ignores item updates after the quick pick is dismissed', async () => { + const fake = createFakeQuickPick(); + const initial = { label: 'initial' }; + const release = createDeferred(); + const loaderStarted = createDeferred(); + const loaderFinished = createDeferred(); + when(window.createQuickPick()).thenReturn(fake.quickPick); + + const selection = showQuickPickWithButtons([initial], { + itemLoader: async (updateItems) => { + loaderStarted.resolve(); + await release.promise; + updateItems([{ label: 'late result' }]); + loaderFinished.resolve(); + }, + }); + await loaderStarted.promise; + + fake.hide(); + assert.strictEqual(await selection, undefined); + assert.strictEqual(fake.disposeCount, 1); + + release.resolve(); + await loaderFinished.promise; + await new Promise((resolve) => setImmediate(resolve)); + + assert.deepStrictEqual(fake.quickPick.items, [initial]); + assert.strictEqual(fake.showCount, 1); + }); + + test('does not clear focus when items load before the initial focus is established', async () => { + const fake = createFakeQuickPick(); + const loaderFinished = createDeferred(); + when(window.createQuickPick()).thenReturn(fake.quickPick); + + const selection = showQuickPickWithButtons([{ label: 'initial' }], { + itemLoader: async (updateItems) => { + updateItems([{ label: 'initial' }, { label: 'discovered' }]); + loaderFinished.resolve(); + }, + }); + await loaderFinished.promise; + await new Promise((resolve) => setImmediate(resolve)); + + assert.strictEqual(fake.activeItemsSetCount, 0); + + fake.hide(); + assert.strictEqual(await selection, undefined); + }); }); diff --git a/src/test/common/environmentPicker.unit.test.ts b/src/test/common/environmentPicker.unit.test.ts index f93ba6714..1ce7db52d 100644 --- a/src/test/common/environmentPicker.unit.test.ts +++ b/src/test/common/environmentPicker.unit.test.ts @@ -7,7 +7,9 @@ import * as sinon from 'sinon'; import { CancellationTokenSource, Uri } from 'vscode'; import { PythonEnvironment } from '../../api'; import { Interpreter } from '../../common/localize'; +import * as logging from '../../common/logging'; import { pickEnvironment } from '../../common/pickers/environments'; +import { createDeferred } from '../../common/utils/deferred'; import * as managerPicker from '../../common/pickers/managers'; import * as pythonPath from '../../common/utils/pythonPath'; import * as windowApis from '../../common/window.apis'; @@ -175,6 +177,108 @@ suite('Environment Picker Interpreter Path Entry', () => { }); }); +suite('Environment Picker Discovery', () => { + teardown(() => { + sinon.restore(); + }); + + function createManager( + id: string, + getEnvironments: sinon.SinonStub, + ): InternalEnvironmentManager { + return { + id, + displayName: id, + getEnvironments, + } as unknown as InternalEnvironmentManager; + } + + test('shows known choices immediately and publishes concurrent discovery results in manager order', async () => { + const recommended = createMockPythonEnvironment({ envPath: path.join(process.cwd(), 'recommended', 'python') }); + const environmentA = createMockPythonEnvironment({ envPath: path.join(process.cwd(), 'manager-a', 'python') }); + const environmentB = createMockPythonEnvironment({ envPath: path.join(process.cwd(), 'manager-b', 'python') }); + const environmentsA = createDeferred(); + const environmentsB = createDeferred(); + const getEnvironmentsA = sinon.stub().returns(environmentsA.promise); + const getEnvironmentsB = sinon.stub().returns(environmentsB.promise); + const managers = [ + createManager('manager-a', getEnvironmentsA), + createManager('manager-b', getEnvironmentsB), + ]; + const pickerResult = createDeferred(); + const firstUpdate = createDeferred(); + const updates: (readonly { label: string; result?: PythonEnvironment }[])[] = []; + let initialItems: readonly { label: string; result?: PythonEnvironment }[] = []; + let loader: Promise | undefined; + + const picker = sinon.stub(windowApis, 'showQuickPickWithButtons').callsFake(async (items, options) => { + initialItems = items; + loader = options?.itemLoader?.((updatedItems) => { + updates.push(updatedItems); + firstUpdate.resolve(); + }); + return pickerResult.promise; + }); + + const selection = pickEnvironment(managers, [], { projects: [], recommended }); + + sinon.assert.calledOnce(picker); + assert.ok(initialItems.some((item) => item.label === Interpreter.browsePath)); + assert.ok(initialItems.some((item) => item.result === recommended)); + sinon.assert.calledOnceWithExactly(getEnvironmentsA, 'all'); + sinon.assert.calledOnceWithExactly(getEnvironmentsB, 'all'); + + environmentsB.resolve([environmentB]); + await firstUpdate.promise; + + assert.ok(updates[0].some((item) => item.result === environmentB)); + assert.ok(!updates[0].some((item) => item.result === environmentA)); + + environmentsA.resolve([environmentA]); + await loader; + + const finalLabels = updates[updates.length - 1].map((item) => item.label); + assert.ok(finalLabels.indexOf('manager-a') < finalLabels.indexOf('manager-b')); + const finalItems = updates[updates.length - 1]; + assert.ok(finalItems.some((item) => item.result === environmentA)); + assert.ok(finalItems.some((item) => item.result === environmentB)); + + pickerResult.resolve(undefined); + assert.strictEqual(await selection, undefined); + }); + + test('keeps healthy choices selectable when a manager fails', async () => { + const error = new Error('discovery failed'); + const environment = createMockPythonEnvironment({ envPath: path.join(process.cwd(), 'healthy', 'python') }); + const failedGetEnvironments = sinon.stub().throws(error); + const healthyGetEnvironments = sinon.stub().resolves([environment]); + const trace = sinon.stub(logging, 'traceError'); + + sinon.stub(windowApis, 'showQuickPickWithButtons').callsFake(async (initialItems, options) => { + let currentItems = initialItems; + await options?.itemLoader?.((updatedItems) => { + currentItems = updatedItems; + }); + assert.ok(currentItems.some((item) => item.label === Interpreter.browsePath)); + return currentItems.find( + (item) => (item as { result?: PythonEnvironment }).result === environment, + ); + }); + + const selected = await pickEnvironment( + [ + createManager('failed-manager', failedGetEnvironments), + createManager('healthy-manager', healthyGetEnvironments), + ], + [], + { projects: [] }, + ); + + assert.strictEqual(selected, environment); + sinon.assert.calledOnceWithExactly(trace, 'Failed to get environments from failed-manager', error); + }); +}); + /** * Test the logic used in environment pickers to include interpreter paths in descriptions */