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

Ensure full refresh icon removes any environment which isn't relevant and remove hard refresh icon in quickpick #19806

Merged
merged 6 commits into from
Sep 9, 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
6 changes: 1 addition & 5 deletions src/client/apiTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,6 @@ export interface ActiveEnvironmentChangedParams {
resource?: Uri;
}

export interface RefreshEnvironmentsOptions {
clearCache?: boolean;
}

export interface IProposedExtensionAPI {
environment: {
/**
Expand Down Expand Up @@ -203,7 +199,7 @@ export interface IProposedExtensionAPI {
* * clearCache : When true, this will clear the cache before environment refresh
* is triggered.
*/
refreshEnvironment(options?: RefreshEnvironmentsOptions): Promise<EnvPathType[] | undefined>;
refreshEnvironment(): Promise<EnvPathType[] | undefined>;
/**
* Tracks discovery progress for current list of known environments, i.e when it starts, finishes or any other relevant
* stage. Note the progress for a particular query is currently not tracked or reported, this only indicates progress of
Expand Down
1 change: 0 additions & 1 deletion src/client/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ export namespace Octicons {
* to change the icons.
*/
export namespace ThemeIcons {
export const ClearAll = 'clear-all';
export const Refresh = 'refresh';
export const SpinningLoader = 'loading~spin';
}
Expand Down
4 changes: 0 additions & 4 deletions src/client/common/utils/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,10 +317,6 @@ export namespace InterpreterQuickPickList {
'InterpreterQuickPickList.refreshInterpreterList',
'Refresh Interpreter list',
);
export const clearAllAndRefreshInterpreterList = localize(
'InterpreterQuickPickList.clearAllAndRefreshInterpreterList',
'Clear all and Refresh Interpreter list',
);
export const refreshingInterpreterList = localize(
'InterpreterQuickPickList.refreshingInterpreterList',
'Refreshing Interpreter list...',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,6 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
tooltip: InterpreterQuickPickList.refreshInterpreterList,
};

private readonly hardRefreshButton = {
iconPath: new ThemeIcon(ThemeIcons.ClearAll),
tooltip: InterpreterQuickPickList.clearAllAndRefreshInterpreterList,
};

private readonly noPythonInstalled: ISpecialQuickPickItem = {
label: `${Octicons.Error} ${InterpreterQuickPickList.noPythonInstalled}`,
detail: InterpreterQuickPickList.clickForInstructions,
Expand Down Expand Up @@ -155,16 +150,10 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
matchOnDescription: true,
title: InterpreterQuickPickList.browsePath.openButtonLabel,
customButtonSetups: [
{
button: this.hardRefreshButton,
callback: (quickpickInput) => {
this.refreshButtonCallback(quickpickInput, true);
},
},
{
button: this.refreshButton,
callback: (quickpickInput) => {
this.refreshButtonCallback(quickpickInput, false);
this.refreshButtonCallback(quickpickInput);
},
},
],
Expand Down Expand Up @@ -418,17 +407,17 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
}
}

private refreshButtonCallback(input: QuickPick<QuickPickItem>, clearCache: boolean) {
private refreshButtonCallback(input: QuickPick<QuickPickItem>) {
input.buttons = [
{
iconPath: new ThemeIcon(ThemeIcons.SpinningLoader),
tooltip: InterpreterQuickPickList.refreshingInterpreterList,
},
];
this.interpreterService
.triggerRefresh(undefined, { clearCache })
.triggerRefresh()
.finally(() => {
input.buttons = [this.hardRefreshButton, this.refreshButton];
input.buttons = [this.refreshButton];
})
.ignoreErrors();
}
Expand Down
2 changes: 1 addition & 1 deletion src/client/interpreter/interpreterService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ export class InterpreterService implements Disposable, IInterpreterService {
traceLog('Conda envs without Python are known to not work well; fixing conda environment...');
const promise = installer.install(Product.python, await this.getInterpreterDetails(pythonPath));
shell.withProgress(progressOptions, () => promise);
promise.then(() => this.triggerRefresh(undefined, { clearCache: true }).ignoreErrors());
promise.then(() => this.triggerRefresh().ignoreErrors());
}
}
}
5 changes: 2 additions & 3 deletions src/client/proposedApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
EnvironmentDetailsOptions,
EnvironmentsChangedParams,
IProposedExtensionAPI,
RefreshEnvironmentsOptions,
} from './apiTypes';
import { arePathsSame } from './common/platform/fs-paths';
import { IInterpreterPathService, Resource } from './common/types';
Expand Down Expand Up @@ -101,8 +100,8 @@ export function buildProposedApi(
setActiveEnvironment(path: string, resource?: Resource): Promise<void> {
return interpreterPathService.update(resource, ConfigurationTarget.WorkspaceFolder, path);
},
async refreshEnvironment(options?: RefreshEnvironmentsOptions) {
await discoveryApi.triggerRefresh(undefined, options ? { clearCache: options.clearCache } : undefined);
async refreshEnvironment() {
await discoveryApi.triggerRefresh();
const paths = discoveryApi.getEnvs().map((e) => getEnvPath(e.executable.filename, e.location));
return Promise.resolve(paths);
},
Expand Down
4 changes: 0 additions & 4 deletions src/client/pythonEnvironments/base/locator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,6 @@ export interface GetRefreshEnvironmentsOptions {
}

export type TriggerRefreshOptions = {
/**
* Trigger a fresh refresh.
*/
clearCache?: boolean;
/**
* Only trigger a refresh if it hasn't already been triggered for this session.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

import { Event } from 'vscode';
import { isTestExecution } from '../../../../common/constants';
import { traceInfo } from '../../../../logging';
import { reportInterpretersChanged } from '../../../../proposedApi';
import { arePathsSame, getFileInfo, pathExists } from '../../../common/externalDependencies';
Expand Down Expand Up @@ -52,13 +53,9 @@ export interface IEnvsCollectionCache {
/**
* Removes invalid envs from cache. Note this does not check for outdated info when
* validating cache.
* @param latestListOfEnvs Carries list of latest envs for this workspace session if known.
*/
validateCache(): Promise<void>;

/**
* Clears the in-memory cache. This can be used for hard refresh.
*/
clearCache(): Promise<void>;
validateCache(latestListOfEnvs?: PythonEnvInfo[]): Promise<void>;
}

export type PythonEnvLatestInfo = { hasLatestInfo?: boolean } & PythonEnvInfo;
Expand All @@ -79,15 +76,35 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionCha
super();
}

public async validateCache(): Promise<void> {
public async validateCache(latestListOfEnvs?: PythonEnvInfo[]): Promise<void> {
/**
* We do check if an env has updated as we already run discovery in background
* which means env cache will have up-to-date envs eventually. This also means
* we avoid the cost of running lstat. So simply remove envs which no longer
* exist.
* we avoid the cost of running lstat. So simply remove envs which are no longer
* valid.
*/
const areEnvsValid = await Promise.all(
this.envs.map((e) => (e.executable.filename === 'python' ? true : pathExists(e.executable.filename))),
this.envs.map(async (cachedEnv) => {
const { path } = getEnvPath(cachedEnv.executable.filename, cachedEnv.location);
if (await pathExists(path)) {
if (latestListOfEnvs) {
/**
* Only consider a cached env to be valid if it's relevant. That means:
* * It is either reported in the latest complete refresh for this session.
* * Or it is relevant for some other workspace folder which is not opened currently.
*/
if (cachedEnv.searchLocation) {
return true;
}
if (latestListOfEnvs.some((env) => cachedEnv.id === env.id)) {
return true;
}
} else {
return true;
}
}
return false;
}),
);
const invalidIndexes = areEnvsValid
.map((isValid, index) => (isValid ? -1 : index))
Expand Down Expand Up @@ -194,6 +211,15 @@ async function validateInfo(env: PythonEnvInfo) {
export async function createCollectionCache(storage: IPersistentStorage): Promise<PythonEnvInfoCache> {
const cache = new PythonEnvInfoCache(storage);
await cache.clearAndReloadFromStorage();
await cache.validateCache();
await validateCache(cache);
return cache;
}

async function validateCache(cache: PythonEnvInfoCache) {
if (isTestExecution()) {
// For purposes for test execution, block on validation so that we can determinally know when it finishes.
return cache.validateCache();
}
// Validate in background so it doesn't block on returning the API object.
return cache.validateCache().ignoreErrors();
}
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,12 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
// Do not trigger another refresh if a refresh has previously finished.
return Promise.resolve();
}
refreshPromise = this.startRefresh(query, options);
refreshPromise = this.startRefresh(query);
}
return refreshPromise.then(() => this.sendTelemetry(query, stopWatch));
}

private startRefresh(query: PythonLocatorQuery | undefined, options?: TriggerRefreshOptions): Promise<void> {
if (options?.clearCache) {
this.cache.clearCache();
}
private startRefresh(query: PythonLocatorQuery | undefined): Promise<void> {
this.createProgressStates(query);
const promise = this.addEnvsToCacheForQuery(query);
return promise
Expand Down Expand Up @@ -176,7 +173,8 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
this.cache.addEnv(env);
}
await updatesDone.promise;
await this.cache.validateCache();
// If query for all envs is done, `seen` should contain the list of all envs.
await this.cache.validateCache(query === undefined ? seen : undefined);
this.cache.flush().ignoreErrors();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
WorkspaceFolder,
} from 'vscode';
import { cloneDeep } from 'lodash';
import { anything, instance, mock, when } from 'ts-mockito';
import { anything, instance, mock, when, verify } from 'ts-mockito';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../../../client/common/application/types';
import { PathUtils } from '../../../../client/common/platform/pathUtils';
import { IPlatformService } from '../../../../client/common/platform/types';
Expand Down Expand Up @@ -80,6 +80,7 @@ suite('Set Interpreter Command', () => {
workspace = TypeMoq.Mock.ofType<IWorkspaceService>();
interpreterService = mock<IInterpreterService>();
when(interpreterService.refreshPromise).thenReturn(undefined);
when(interpreterService.triggerRefresh()).thenResolve();
when(interpreterService.triggerRefresh(anything(), anything())).thenResolve();
workspace.setup((w) => w.rootPath).returns(() => 'rootPath');

Expand Down Expand Up @@ -568,17 +569,11 @@ suite('Set Interpreter Command', () => {
expect(actualParameters).to.not.equal(undefined, 'Parameters not set');
const refreshButtons = actualParameters!.customButtonSetups;
expect(refreshButtons).to.not.equal(undefined, 'Callback not set');
expect(refreshButtons?.length).to.equal(1);

expect(refreshButtons?.length).to.equal(2);
let arg;
when(interpreterService.triggerRefresh(undefined, anything())).thenCall((_, _arg) => {
arg = _arg;
return Promise.resolve();
});
await refreshButtons![0].callback!({} as QuickPick<QuickPickItem>); // Invoke callback, meaning that the refresh button is clicked.
expect(arg).to.deep.equal({ clearCache: true });
await refreshButtons![1].callback!({} as QuickPick<QuickPickItem>); // Invoke callback, meaning that the refresh button is clicked.
expect(arg).to.deep.equal({ clearCache: false });

verify(interpreterService.triggerRefresh()).once();
});

test('Events to update quickpick updates the quickpick accordingly', async () => {
Expand Down
Loading