Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/client/common/utils/multiStepInput.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down Expand Up @@ -120,6 +124,7 @@ export class MultiStepInput<S> implements IMultiStepInput<S> {
onChangeItem,
keepScrollPosition,
sortByLabel,
initialize,
}: P): Promise<MultiStepInputQuickPicResponseType<T, P>> {
const disposables: Disposable[] = [];
try {
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead the consumers themselves can implement this option.

} 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
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/client/pythonEnvironments/base/locator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Causes an infinite loop #19574.

// 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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;

Expand Down