Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix buttons in the interpreter quickpick list #19611

Merged
merged 8 commits into from
Aug 4, 2022
Merged

Fix buttons in the interpreter quickpick list #19611

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
28eadb3
Change refresh icon when interpreter list is refreshing
Aug 2, 2022
cbcc76b
Add tests
Aug 2, 2022
c3b84c0
Minor tweaks
Aug 2, 2022
bba4eec
Fix situation if dialog box is cancelled
Aug 2, 2022
3c5fd34
Fix tests
Aug 2, 2022
848ebde
Improve ignoreErrors() typing
Aug 2, 2022
7c20eb7
Update to not use custom svg, instead use built VSCode icon
Aug 3, 2022
afd0c68
Add vscode mock
Aug 3, 2022
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
5 changes: 5 additions & 0 deletions src/client/common/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ export namespace Octicons {
export const Lightbulb = '$(lightbulb)';
}

export namespace ThemeIcons {
export const Refresh = 'refresh';
export const SpinningLoader = 'loading~spin';
}

export const DEFAULT_INTERPRETER_SETTING = 'python';

export const STANDARD_OUTPUT_CHANNEL = 'STANDARD_OUTPUT_CHANNEL';
Expand Down
4 changes: 2 additions & 2 deletions src/client/common/extensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,14 @@ declare interface Promise<T> {
/**
* Catches task error and ignores them.
*/
ignoreErrors(): void;
ignoreErrors(): Promise<void>;
}

/**
* Explicitly tells that promise should be run asynchonously.
*/
Promise.prototype.ignoreErrors = function <T>(this: Promise<T>) {
this.catch(() => {});
return this.catch(() => {});
};

if (!String.prototype.format) {
Expand Down
4 changes: 4 additions & 0 deletions src/client/common/utils/localize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,10 @@ export namespace InterpreterQuickPickList {
'InterpreterQuickPickList.refreshInterpreterList',
'Refresh Interpreter list',
);
export const refreshingInterpreterList = localize(
'InterpreterQuickPickList.refreshingInterpreterList',
'Refreshing Interpreter list...',
);
}

export namespace OutputChannelNames {
Expand Down
9 changes: 3 additions & 6 deletions src/client/common/utils/multiStepInput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { IApplicationShell } from '../application/types';
// Borrowed from https://github.com/Microsoft/vscode-extension-samples/blob/master/quickinput-sample/src/multiStepInput.ts
// Why re-invent the wheel :)

class InputFlowAction {
export class InputFlowAction {
public static back = new InputFlowAction();

public static cancel = new InputFlowAction();
Expand Down Expand Up @@ -152,11 +152,8 @@ export class MultiStepInput<S> implements IMultiStepInput<S> {
input.onDidTriggerButton(async (item) => {
if (item === QuickInputButtons.Back) {
reject(InputFlowAction.back);
} else if (item === customButtonSetup?.button) {
await customButtonSetup.callback(input);
} else {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
resolve(item as any);
} else if (JSON.stringify(item) === JSON.stringify(customButtonSetup?.button)) {
await customButtonSetup?.callback(input);
}
}),
input.onDidChangeSelection((selectedItems) => resolve(selectedItems[0])),
Expand Down
46 changes: 31 additions & 15 deletions src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,23 @@
import { inject, injectable } from 'inversify';
import { cloneDeep } from 'lodash';
import * as path from 'path';
import { QuickPick, QuickPickItem, QuickPickItemKind } from 'vscode';
import { QuickPick, QuickPickItem, QuickPickItemKind, ThemeIcon } from 'vscode';
import * as nls from 'vscode-nls';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../../../common/application/types';
import { Commands, Octicons } from '../../../../common/constants';
import { Commands, Octicons, ThemeIcons } from '../../../../common/constants';
import { isParentPath } from '../../../../common/platform/fs-paths';
import { IPlatformService } from '../../../../common/platform/types';
import { IConfigurationService, IPathUtils, Resource } from '../../../../common/types';
import { getIcon } from '../../../../common/utils/icons';
import { Common, InterpreterQuickPickList } from '../../../../common/utils/localize';
import { noop } from '../../../../common/utils/misc';
import {
IMultiStepInput,
IMultiStepInputFactory,
InputFlowAction,
InputStep,
IQuickPickParameters,
} from '../../../../common/utils/multiStepInput';
import { SystemVariables } from '../../../../common/variables/systemVariables';
import { REFRESH_BUTTON_ICON } from '../../../../debugger/extension/attachQuickPick/types';
import { EnvironmentType } from '../../../../pythonEnvironments/info';
import { captureTelemetry, sendTelemetryEvent } from '../../../../telemetry';
import { EventName } from '../../../../telemetry/constants';
Expand Down Expand Up @@ -126,7 +125,11 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
// If the list is refreshing, it's crucial to maintain sorting order at all
// times so that the visible items do not change.
const preserveOrderWhenFiltering = !!this.interpreterService.refreshPromise;
const suggestions = this.getItems(state.workspace);
const suggestions = this._getItems(state.workspace);
const refreshButton = {
iconPath: new ThemeIcon(ThemeIcons.Refresh),
tooltip: InterpreterQuickPickList.refreshInterpreterList,
};
state.path = undefined;
const currentInterpreterPathDisplay = this.pathUtils.getDisplayName(
this.configurationService.getSettings(state.workspace).pythonPath,
Expand All @@ -146,11 +149,21 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
matchOnDescription: true,
title: InterpreterQuickPickList.browsePath.openButtonLabel,
customButtonSetup: {
button: {
iconPath: getIcon(REFRESH_BUTTON_ICON),
tooltip: InterpreterQuickPickList.refreshInterpreterList,
button: refreshButton,
callback: (quickpickInput) => {
quickpickInput.buttons = [
{
iconPath: new ThemeIcon(ThemeIcons.SpinningLoader),
tooltip: InterpreterQuickPickList.refreshingInterpreterList,
},
];
this.interpreterService
.triggerRefresh()
.finally(() => {
quickpickInput.buttons = [refreshButton];
})
.ignoreErrors();
},
callback: () => this.interpreterService.triggerRefresh().ignoreErrors(),
},
initialize: () => {
// Note discovery is no longer guranteed to be auto-triggered on extension load, so trigger it when
Expand Down Expand Up @@ -186,7 +199,7 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
sendTelemetryEvent(EventName.SELECT_INTERPRETER_SELECTED, undefined, { action: 'escape' });
} else if (selection.label === this.manualEntrySuggestion.label) {
sendTelemetryEvent(EventName.SELECT_INTERPRETER_ENTER_OR_FIND);
return this._enterOrBrowseInterpreterPath(input, state, suggestions);
return this._enterOrBrowseInterpreterPath.bind(this);
} else if (selection.label === this.noPythonInstalled.label) {
this.commandManager.executeCommand(Commands.InstallPython).then(noop, noop);
this.wasNoPythonInstalledItemClicked = true;
Expand All @@ -199,7 +212,7 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
return undefined;
}

private getItems(resource: Resource) {
public _getItems(resource: Resource): QuickPickType[] {
const suggestions: QuickPickType[] = [this.manualEntrySuggestion];
const defaultInterpreterPathSuggestion = this.getDefaultInterpreterPathSuggestion(resource);
if (defaultInterpreterPathSuggestion) {
Expand Down Expand Up @@ -406,7 +419,6 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
public async _enterOrBrowseInterpreterPath(
input: IMultiStepInput<InterpreterStateArgs>,
state: InterpreterStateArgs,
suggestions: QuickPickType[],
): Promise<void | InputStep<InterpreterStateArgs>> {
const items: QuickPickItem[] = [
{
Expand All @@ -425,7 +437,7 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
// User entered text in the filter box to enter path to python, store it
sendTelemetryEvent(EventName.SELECT_INTERPRETER_ENTER_CHOICE, undefined, { choice: 'enter' });
state.path = selection;
this.sendInterpreterEntryTelemetry(selection, state.workspace, suggestions);
this.sendInterpreterEntryTelemetry(selection, state.workspace);
} else if (selection && selection.label === InterpreterQuickPickList.browsePath.label) {
sendTelemetryEvent(EventName.SELECT_INTERPRETER_ENTER_CHOICE, undefined, { choice: 'browse' });
const filtersKey = 'Executables';
Expand All @@ -439,9 +451,12 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
});
if (uris && uris.length > 0) {
state.path = uris[0].fsPath;
this.sendInterpreterEntryTelemetry(state.path!, state.workspace, suggestions);
this.sendInterpreterEntryTelemetry(state.path!, state.workspace);
} else {
return Promise.reject(InputFlowAction.resume);
}
}
return Promise.resolve();
}

@captureTelemetry(EventName.SELECT_INTERPRETER)
Expand Down Expand Up @@ -472,7 +487,8 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
* @param selection Intepreter path that was either entered manually or picked by browsing through the filesystem.
*/
// eslint-disable-next-line class-methods-use-this
private sendInterpreterEntryTelemetry(selection: string, workspace: Resource, suggestions: QuickPickType[]): void {
private sendInterpreterEntryTelemetry(selection: string, workspace: Resource): void {
const suggestions = this._getItems(workspace);
let interpreterPath = path.normalize(untildify(selection));

if (!path.isAbsolute(interpreterPath)) {
Expand Down
28 changes: 18 additions & 10 deletions src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,8 +759,10 @@ suite('Set Interpreter Command', () => {
.setup((i) => i.showQuickPick(TypeMoq.It.isAny()))
.returns(() => Promise.resolve(expectedEnterInterpreterPathSuggestion));

await setInterpreterCommand._pickInterpreter(multiStepInput.object, state);
const step = await setInterpreterCommand._pickInterpreter(multiStepInput.object, state);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
await step!(multiStepInput.object as any, state);
assert(
_enterOrBrowseInterpreterPath.calledOnceWith(multiStepInput.object, {
path: undefined,
Expand All @@ -782,6 +784,11 @@ suite('Set Interpreter Command', () => {
items,
acceptFilterBoxTextAsSelection: true,
};
let getItemsStub: sinon.SinonStub;
setup(() => {
getItemsStub = sinon.stub(SetInterpreterCommand.prototype, '_getItems').returns([]);
});
teardown(() => sinon.restore());

test('Picker should be displayed with expected items', async () => {
const state: InterpreterStateArgs = { path: 'some path', workspace: undefined };
Expand All @@ -791,7 +798,7 @@ suite('Set Interpreter Command', () => {
.returns(() => Promise.resolve((undefined as unknown) as QuickPickItem))
.verifiable(TypeMoq.Times.once());

await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state, []);
await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state);

multiStepInput.verifyAll();
});
Expand All @@ -803,7 +810,7 @@ suite('Set Interpreter Command', () => {
.setup((i) => i.showQuickPick(TypeMoq.It.isAny()))
.returns(() => Promise.resolve('enteredPath'));

await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state, []);
await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state);

expect(state.path).to.equal('enteredPath', '');
});
Expand All @@ -817,7 +824,7 @@ suite('Set Interpreter Command', () => {
.setup((a) => a.showOpenDialog(TypeMoq.It.isAny()))
.returns(() => Promise.resolve([expectedPathUri]));

await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state, []);
await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state);

expect(state.path).to.equal(expectedPathUri.fsPath, '');
});
Expand All @@ -840,7 +847,7 @@ suite('Set Interpreter Command', () => {
.verifiable(TypeMoq.Times.once());
platformService.setup((p) => p.isWindows).returns(() => true);

await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state, []);
await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state).ignoreErrors();

appShell.verifyAll();
});
Expand All @@ -858,7 +865,7 @@ suite('Set Interpreter Command', () => {
appShell.setup((a) => a.showOpenDialog(expectedParams)).verifiable(TypeMoq.Times.once());
platformService.setup((p) => p.isWindows).returns(() => false);

await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state, []);
await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state).ignoreErrors();

appShell.verifyAll();
});
Expand Down Expand Up @@ -891,7 +898,7 @@ suite('Set Interpreter Command', () => {
.setup((i) => i.showQuickPick(TypeMoq.It.isAny()))
.returns(() => Promise.resolve('enteredPath'));

await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state, []);
await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state);
const existsTelemetry = telemetryEvents[1];

sinon.assert.callCount(sendTelemetryStub, 2);
Expand All @@ -915,7 +922,7 @@ suite('Set Interpreter Command', () => {
.returns(() => Promise.resolve([{ fsPath: 'browsedPath' } as Uri]));
platformService.setup((p) => p.isWindows).returns(() => false);

await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state, []);
await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state);
const existsTelemetry = telemetryEvents[1];

sinon.assert.callCount(sendTelemetryStub, 2);
Expand Down Expand Up @@ -975,8 +982,9 @@ suite('Set Interpreter Command', () => {
if (discovered) {
suggestions.push({ interpreter: { path: expandedPath } } as IInterpreterQuickPickItem);
}

await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state, suggestions);
getItemsStub.restore();
getItemsStub = sinon.stub(SetInterpreterCommand.prototype, '_getItems').returns(suggestions);
await setInterpreterCommand._enterOrBrowseInterpreterPath(multiStepInput.object, state);
return telemetryEvents[1];
};

Expand Down
2 changes: 1 addition & 1 deletion typings/extensions.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,5 @@ declare interface Promise<T> {
/**
* Catches task errors and ignores them.
*/
ignoreErrors(): void;
ignoreErrors(): Promise<void>;
}