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.

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

Fix buttons in the interpreter quickpick list #19611

Merged
merged 8 commits into from
Aug 4, 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
5 changes: 5 additions & 0 deletions src/client/common/constants.ts
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
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
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
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
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
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
8 changes: 8 additions & 0 deletions src/test/mocks/vsc/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ export function escapeCodicons(text: string): string {
return text.replace(escapeCodiconsRegex, (match, escaped) => (escaped ? match : `\\${match}`));
}

export class ThemeIcon {
static readonly File: ThemeIcon;

static readonly Folder: ThemeIcon;

constructor(public readonly id: string, public readonly color?: ThemeColor) {}
}

export class ThemeColor {
constructor(public readonly id: string) {}
}
Expand Down
1 change: 1 addition & 0 deletions src/test/vscode-mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export function initialize() {
};
}

mockedVSCode.ThemeIcon = vscodeMocks.ThemeIcon;
mockedVSCode.ThemeColor = vscodeMocks.ThemeColor;
mockedVSCode.MarkdownString = vscodeMocks.MarkdownString;
mockedVSCode.Hover = vscodeMocks.Hover;
Expand Down
2 changes: 1 addition & 1 deletion typings/extensions.d.ts
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>;
}