Skip to content

Commit

Permalink
Ensure interpreter quickpick is initialized synchronously (microsoft#…
Browse files Browse the repository at this point in the history 
…19828)

For microsoft#19101

Before the change item events start coming in, we have to ensure
quickpick is ready to receive those events.
  • Loading branch information
Kartik Raj authored Sep 13, 2022
1 parent 5cc9092 commit 2ef2db4
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 41 deletions.
75 changes: 38 additions & 37 deletions src/client/common/utils/multiStepInput.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export interface IQuickPickParameters<T extends QuickPickItem, E = any> {
totalSteps?: number;
canGoBack?: boolean;
items: T[];
activeItem?: T;
activeItem?: T | Promise<T>;
placeholder: string;
customButtonSetups?: QuickInputButtonSetup[];
matchOnDescription?: boolean;
Expand Down Expand Up @@ -127,29 +127,45 @@ export class MultiStepInput<S> implements IMultiStepInput<S> {
initialize,
}: P): Promise<MultiStepInputQuickPicResponseType<T, P>> {
const disposables: Disposable[] = [];
const input = this.shell.createQuickPick<T>();
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<MultiStepInputQuickPicResponseType<T, P>>((resolve, reject) => {
const input = this.shell.createQuickPick<T>();
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) {
Expand All @@ -176,21 +192,6 @@ export class MultiStepInput<S> implements IMultiStepInput<S> {
}),
);
}
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());
Expand Down
2 changes: 1 addition & 1 deletion src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
12 changes: 9 additions & 3 deletions src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,6 @@ suite('Set Interpreter Command', () => {
const expectedParameters: IQuickPickParameters<QuickPickItem> = {
placeholder: `Selected Interpreter: ${currentPythonPath}`,
items: suggestions,
activeItem: recommended,
matchOnDetail: true,
matchOnDescription: true,
title: InterpreterQuickPickList.browsePath.openButtonLabel,
Expand All @@ -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');
});

Expand All @@ -281,7 +283,6 @@ suite('Set Interpreter Command', () => {
const expectedParameters: IQuickPickParameters<QuickPickItem> = {
placeholder: `Selected Interpreter: ${currentPythonPath}`,
items: suggestions, // Verify suggestions
activeItem: noPythonInstalled, // Verify active item
matchOnDetail: true,
matchOnDescription: true,
title: InterpreterQuickPickList.browsePath.openButtonLabel,
Expand All @@ -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');
});

Expand Down Expand Up @@ -525,7 +529,6 @@ suite('Set Interpreter Command', () => {
const expectedParameters: IQuickPickParameters<QuickPickItem> = {
placeholder: `Selected Interpreter: ${currentPythonPath}`,
items: suggestions,
activeItem: recommended,
matchOnDetail: true,
matchOnDescription: true,
title: InterpreterQuickPickList.browsePath.openButtonLabel,
Expand All @@ -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');
});
Expand Down

0 comments on commit 2ef2db4

Please sign in to comment.