From 2ef2db46ffff6694e40269db10ada2c511e99a86 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 13 Sep 2022 16:22:10 -0700 Subject: [PATCH] Ensure interpreter quickpick is initialized synchronously (#19828) For https://github.com/microsoft/vscode-python/issues/19101 Before the change item events start coming in, we have to ensure quickpick is ready to receive those events. --- src/client/common/utils/multiStepInput.ts | 75 ++++++++++--------- .../commands/setInterpreter.ts | 2 +- .../commands/setInterpreter.unit.test.ts | 12 ++- 3 files changed, 48 insertions(+), 41 deletions(-) diff --git a/src/client/common/utils/multiStepInput.ts b/src/client/common/utils/multiStepInput.ts index 1cb6bad11d66..daac8574227d 100644 --- a/src/client/common/utils/multiStepInput.ts +++ b/src/client/common/utils/multiStepInput.ts @@ -46,7 +46,7 @@ export interface IQuickPickParameters { totalSteps?: number; canGoBack?: boolean; items: T[]; - activeItem?: T; + activeItem?: T | Promise; placeholder: string; customButtonSetups?: QuickInputButtonSetup[]; matchOnDescription?: boolean; @@ -127,29 +127,45 @@ export class MultiStepInput implements IMultiStepInput { initialize, }: P): Promise> { const disposables: Disposable[] = []; + const input = this.shell.createQuickPick(); + input.title = title; + input.step = step; + input.sortByLabel = sortByLabel || false; + input.totalSteps = totalSteps; + input.placeholder = placeholder; + input.ignoreFocusOut = true; + input.items = items; + input.matchOnDescription = matchOnDescription || false; + input.matchOnDetail = matchOnDetail || false; + input.buttons = this.steps.length > 1 ? [QuickInputButtons.Back] : []; + if (customButtonSetups) { + for (const customButtonSetup of customButtonSetups) { + input.buttons = [...input.buttons, customButtonSetup.button]; + } + } + if (this.current) { + this.current.dispose(); + } + this.current = input; + if (onChangeItem) { + disposables.push(onChangeItem.event((e) => onChangeItem.callback(e, input))); + } + // Quickpick should be initialized synchronously and on changed item handlers are registered synchronously. + if (initialize) { + initialize(); + } + if (activeItem) { + input.activeItems = [await activeItem]; + } else { + input.activeItems = []; + } + this.current.show(); + // 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. + input.keepScrollPosition = keepScrollPosition; try { return await new Promise>((resolve, reject) => { - const input = this.shell.createQuickPick(); - input.title = title; - input.step = step; - input.sortByLabel = sortByLabel || false; - input.totalSteps = totalSteps; - input.placeholder = placeholder; - input.ignoreFocusOut = true; - input.items = items; - input.matchOnDescription = matchOnDescription || false; - input.matchOnDetail = matchOnDetail || false; - if (activeItem) { - input.activeItems = [activeItem]; - } else { - input.activeItems = []; - } - input.buttons = this.steps.length > 1 ? [QuickInputButtons.Back] : []; - if (customButtonSetups) { - for (const customButtonSetup of customButtonSetups) { - input.buttons = [...input.buttons, customButtonSetup.button]; - } - } disposables.push( input.onDidTriggerButton(async (item) => { if (item === QuickInputButtons.Back) { @@ -176,21 +192,6 @@ export class MultiStepInput implements IMultiStepInput { }), ); } - if (this.current) { - this.current.dispose(); - } - this.current = input; - if (onChangeItem) { - 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. - input.keepScrollPosition = keepScrollPosition; }); } finally { disposables.forEach((d) => d.dispose()); diff --git a/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts b/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts index 1d6bd31edf29..aad89c58959b 100644 --- a/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts +++ b/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts @@ -145,7 +145,7 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { items: suggestions, sortByLabel: !preserveOrderWhenFiltering, keepScrollPosition: true, - activeItem: await this.getActiveItem(state.workspace, suggestions), + activeItem: this.getActiveItem(state.workspace, suggestions), // Use a promise here to ensure quickpick is initialized synchronously. matchOnDetail: true, matchOnDescription: true, title: InterpreterQuickPickList.browsePath.openButtonLabel, diff --git a/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts b/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts index be94c5f456dc..1bd27b5d2cbb 100644 --- a/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts +++ b/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts @@ -244,7 +244,6 @@ suite('Set Interpreter Command', () => { const expectedParameters: IQuickPickParameters = { placeholder: `Selected Interpreter: ${currentPythonPath}`, items: suggestions, - activeItem: recommended, matchOnDetail: true, matchOnDescription: true, title: InterpreterQuickPickList.browsePath.openButtonLabel, @@ -267,6 +266,9 @@ suite('Set Interpreter Command', () => { delete actualParameters!.initialize; delete actualParameters!.customButtonSetups; delete actualParameters!.onChangeItem; + const activeItem = await actualParameters!.activeItem; + assert.deepStrictEqual(activeItem, recommended); + delete actualParameters!.activeItem; assert.deepStrictEqual(actualParameters, expectedParameters, 'Params not equal'); }); @@ -281,7 +283,6 @@ suite('Set Interpreter Command', () => { const expectedParameters: IQuickPickParameters = { placeholder: `Selected Interpreter: ${currentPythonPath}`, items: suggestions, // Verify suggestions - activeItem: noPythonInstalled, // Verify active item matchOnDetail: true, matchOnDescription: true, title: InterpreterQuickPickList.browsePath.openButtonLabel, @@ -308,6 +309,9 @@ suite('Set Interpreter Command', () => { delete actualParameters!.initialize; delete actualParameters!.customButtonSetups; delete actualParameters!.onChangeItem; + const activeItem = await actualParameters!.activeItem; + assert.deepStrictEqual(activeItem, noPythonInstalled); + delete actualParameters!.activeItem; assert.deepStrictEqual(actualParameters, expectedParameters, 'Params not equal'); }); @@ -525,7 +529,6 @@ suite('Set Interpreter Command', () => { const expectedParameters: IQuickPickParameters = { placeholder: `Selected Interpreter: ${currentPythonPath}`, items: suggestions, - activeItem: recommended, matchOnDetail: true, matchOnDescription: true, title: InterpreterQuickPickList.browsePath.openButtonLabel, @@ -549,6 +552,9 @@ suite('Set Interpreter Command', () => { delete actualParameters!.initialize; delete actualParameters!.customButtonSetups; delete actualParameters!.onChangeItem; + const activeItem = await actualParameters!.activeItem; + assert.deepStrictEqual(activeItem, recommended); + delete actualParameters!.activeItem; assert.deepStrictEqual(actualParameters, expectedParameters, 'Params not equal'); });