Skip to content

Commit ca854b8

Browse files
DonJayamanneDonJayamanne
authored
Ensure the prompt to install missing packages is displayed only once (microsoft#1649)
* Ensure the prompt to install missing packages is displayed only once * Add missing dependency * Fixes microsoft#980
1 parent 67a6a4c commit ca854b8

File tree

5 files changed

+99
-22
lines changed

5 files changed

+99
-22
lines changed

‎news/2 Fixes/980‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Ensure the prompt to install missing packages is not displayed more than once.

‎src/client/common/installer/productInstaller.ts‎

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,12 @@ import { inject, injectable, named } from 'inversify';
22
import * as os from 'os';
33
import * as path from 'path';
44
import * as vscode from 'vscode';
5+
import '../../common/extensions';
56
import { IFormatterHelper } from '../../formatters/types';
67
import { IServiceContainer } from '../../ioc/types';
78
import { ILinterManager } from '../../linters/types';
89
import { ITestsHelper } from '../../unittests/common/types';
9-
import { IApplicationShell } from '../application/types';
10+
import { IApplicationShell, IWorkspaceService } from '../application/types';
1011
import { STANDARD_OUTPUT_CHANNEL } from '../constants';
1112
import { IPlatformService } from '../platform/types';
1213
import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types';
@@ -28,16 +29,34 @@ enum ProductType {
2829
}
2930

3031
// tslint:disable-next-line:max-classes-per-file
31-
abstract class BaseInstaller {
32+
export abstract class BaseInstaller {
33+
private static readonly PromptPromises = new Map<string, Promise<InstallerResponse>>();
3234
protected appShell: IApplicationShell;
3335
protected configService: IConfigurationService;
36+
private readonly workspaceService: IWorkspaceService;
3437

3538
constructor(protected serviceContainer: IServiceContainer, protected outputChannel: vscode.OutputChannel) {
3639
this.appShell = serviceContainer.get<IApplicationShell>(IApplicationShell);
3740
this.configService = serviceContainer.get<IConfigurationService>(IConfigurationService);
41+
this.workspaceService = serviceContainer.get<IWorkspaceService>(IWorkspaceService);
3842
}
3943

40-
public abstract promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse>;
44+
public promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
45+
// If this method gets called twice, while previous promise has not been resolved, then return that same promise.
46+
// E.g. previous promise is not resolved as a message has been displayed to the user, so no point displaying
47+
// another message.
48+
const workspaceFolder = resource ? this.workspaceService.getWorkspaceFolder(resource) : undefined;
49+
const key = `${product}${workspaceFolder ? workspaceFolder.uri.fsPath : ''}`;
50+
if (BaseInstaller.PromptPromises.has(key)) {
51+
return BaseInstaller.PromptPromises.get(key)!;
52+
}
53+
const promise = this.promptToInstallImplementation(product, resource);
54+
BaseInstaller.PromptPromises.set(key, promise);
55+
promise.then(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors();
56+
promise.catch(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors();
57+
58+
return promise;
59+
}
4160

4261
public async install(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
4362
if (product === Product.unittest) {
@@ -83,22 +102,17 @@ abstract class BaseInstaller {
83102
.catch(() => false);
84103
}
85104
}
86-
105+
protected abstract promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse>;
87106
protected getExecutableNameFromSettings(product: Product, resource?: vscode.Uri): string {
88107
throw new Error('getExecutableNameFromSettings is not supported on this object');
89108
}
90109
}
91110

92-
class CTagsInstaller extends BaseInstaller {
111+
export class CTagsInstaller extends BaseInstaller {
93112
constructor(serviceContainer: IServiceContainer, outputChannel: vscode.OutputChannel) {
94113
super(serviceContainer, outputChannel);
95114
}
96115

97-
public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
98-
const item = await this.appShell.showErrorMessage('Install CTags to enable Python workspace symbols?', 'Yes', 'No');
99-
return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
100-
}
101-
102116
public async install(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
103117
if (this.serviceContainer.get<IPlatformService>(IPlatformService).isWindows) {
104118
this.outputChannel.appendLine('Install Universal Ctags Win32 to enable support for Workspace Symbols');
@@ -115,15 +129,19 @@ class CTagsInstaller extends BaseInstaller {
115129
}
116130
return InstallerResponse.Ignore;
117131
}
132+
protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
133+
const item = await this.appShell.showErrorMessage('Install CTags to enable Python workspace symbols?', 'Yes', 'No');
134+
return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
135+
}
118136

119137
protected getExecutableNameFromSettings(product: Product, resource?: vscode.Uri): string {
120138
const settings = this.configService.getSettings(resource);
121139
return settings.workspaceSymbols.ctagsPath;
122140
}
123141
}
124142

125-
class FormatterInstaller extends BaseInstaller {
126-
public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
143+
export class FormatterInstaller extends BaseInstaller {
144+
protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
127145
// Hard-coded on purpose because the UI won't necessarily work having
128146
// another formatter.
129147
const formatters = [Product.autopep8, Product.black, Product.yapf];
@@ -159,8 +177,8 @@ class FormatterInstaller extends BaseInstaller {
159177
}
160178

161179
// tslint:disable-next-line:max-classes-per-file
162-
class LinterInstaller extends BaseInstaller {
163-
public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
180+
export class LinterInstaller extends BaseInstaller {
181+
protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
164182
const productName = ProductNames.get(product)!;
165183
const install = 'Install';
166184
const disableAllLinting = 'Disable linting';
@@ -188,8 +206,8 @@ class LinterInstaller extends BaseInstaller {
188206
}
189207

190208
// tslint:disable-next-line:max-classes-per-file
191-
class TestFrameworkInstaller extends BaseInstaller {
192-
public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
209+
export class TestFrameworkInstaller extends BaseInstaller {
210+
protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
193211
const productName = ProductNames.get(product)!;
194212
const item = await this.appShell.showErrorMessage(`Test framework ${productName} is not installed. Install?`, 'Yes', 'No');
195213
return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
@@ -208,8 +226,8 @@ class TestFrameworkInstaller extends BaseInstaller {
208226
}
209227

210228
// tslint:disable-next-line:max-classes-per-file
211-
class RefactoringLibraryInstaller extends BaseInstaller {
212-
public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
229+
export class RefactoringLibraryInstaller extends BaseInstaller {
230+
protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
213231
const productName = ProductNames.get(product)!;
214232
const item = await this.appShell.showErrorMessage(`Refactoring library ${productName} is not installed. Install?`, 'Yes', 'No');
215233
return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;

‎src/client/linters/errorHandlers/notInstalled.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import { OutputChannel, Uri } from 'vscode';
2-
import { isNotInstalledError } from '../../common/helpers';
32
import { IPythonExecutionFactory } from '../../common/process/types';
43
import { ExecutionInfo, Product } from '../../common/types';
54
import { IServiceContainer } from '../../ioc/types';

‎src/test/common/installer.test.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import * as path from 'path';
22
import * as TypeMoq from 'typemoq';
33
import { ConfigurationTarget, Uri } from 'vscode';
4-
import { IApplicationShell } from '../../client/common/application/types';
4+
import { IApplicationShell, IWorkspaceService } from '../../client/common/application/types';
55
import { ConfigurationService } from '../../client/common/configuration/service';
66
import { EnumEx } from '../../client/common/enumUtils';
77
import { createDeferred } from '../../client/common/helpers';
@@ -58,6 +58,10 @@ suite('Installer', () => {
5858
ioc.serviceManager.addSingletonInstance<IApplicationShell>(IApplicationShell, TypeMoq.Mock.ofType<IApplicationShell>().object);
5959
ioc.serviceManager.addSingleton<IConfigurationService>(IConfigurationService, ConfigurationService);
6060

61+
const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
62+
workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isAny())).returns(() => undefined);
63+
ioc.serviceManager.addSingletonInstance<IWorkspaceService>(IWorkspaceService, workspaceService.object);
64+
6165
ioc.registerMockProcessTypes();
6266
ioc.serviceManager.addSingletonInstance<boolean>(IsWindows, false);
6367
}

‎src/test/common/installer/installer.test.ts‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,23 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4+
// tslint:disable:max-func-body-length no-invalid-this
5+
46
import { expect, use } from 'chai';
57
import * as chaiAsPromised from 'chai-as-promised';
68
import * as TypeMoq from 'typemoq';
7-
import { Disposable, OutputChannel, Uri } from 'vscode';
9+
import { Disposable, OutputChannel, Uri, WorkspaceFolder } from 'vscode';
10+
import { IApplicationShell, IWorkspaceService } from '../../../client/common/application/types';
811
import { EnumEx } from '../../../client/common/enumUtils';
12+
import '../../../client/common/extensions';
13+
import { createDeferred, Deferred } from '../../../client/common/helpers';
914
import { ProductInstaller } from '../../../client/common/installer/productInstaller';
1015
import { IInstallationChannelManager, IModuleInstaller } from '../../../client/common/installer/types';
1116
import { IDisposableRegistry, ILogger, InstallerResponse, ModuleNamePurpose, Product } from '../../../client/common/types';
1217
import { IServiceContainer } from '../../../client/ioc/types';
1318

1419
use(chaiAsPromised);
1520

16-
// tslint:disable-next-line:max-func-body-length
1721
suite('Module Installer', () => {
1822
[undefined, Uri.file('resource')].forEach(resource => {
1923
EnumEx.getNamesAndValues<Product>(Product).forEach(product => {
@@ -22,7 +26,11 @@ suite('Module Installer', () => {
2226
let installationChannel: TypeMoq.IMock<IInstallationChannelManager>;
2327
let moduleInstaller: TypeMoq.IMock<IModuleInstaller>;
2428
let serviceContainer: TypeMoq.IMock<IServiceContainer>;
29+
let app: TypeMoq.IMock<IApplicationShell>;
30+
let promptDeferred: Deferred<string>;
31+
let workspaceService: TypeMoq.IMock<IWorkspaceService>;
2532
setup(() => {
33+
promptDeferred = createDeferred<string>();
2634
serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
2735
const outputChannel = TypeMoq.Mock.ofType<OutputChannel>();
2836

@@ -33,6 +41,10 @@ suite('Module Installer', () => {
3341

3442
installationChannel = TypeMoq.Mock.ofType<IInstallationChannelManager>();
3543
serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInstallationChannelManager), TypeMoq.It.isAny())).returns(() => installationChannel.object);
44+
app = TypeMoq.Mock.ofType<IApplicationShell>();
45+
serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell), TypeMoq.It.isAny())).returns(() => app.object);
46+
workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
47+
serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IWorkspaceService), TypeMoq.It.isAny())).returns(() => workspaceService.object);
3648

3749
moduleInstaller = TypeMoq.Mock.ofType<IModuleInstaller>();
3850
// tslint:disable-next-line:no-any
@@ -41,6 +53,8 @@ suite('Module Installer', () => {
4153
installationChannel.setup(i => i.getInstallationChannel(TypeMoq.It.isAny())).returns(() => Promise.resolve(moduleInstaller.object));
4254
});
4355
teardown(() => {
56+
// This must be resolved, else all subsequent tests will fail (as this same promise will be used for other tests).
57+
promptDeferred.resolve();
4458
disposables.forEach(disposable => {
4559
if (disposable) {
4660
disposable.dispose();
@@ -92,6 +106,47 @@ suite('Module Installer', () => {
92106
moduleInstaller.verify(m => m.installModule(TypeMoq.It.isValue(moduleName), TypeMoq.It.isValue(resource)), TypeMoq.Times.once());
93107
}
94108
});
109+
test(`Ensure the prompt is displayed only once, untill the prompt is closed, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async function () {
110+
if (product.value === Product.unittest) {
111+
return this.skip();
112+
}
113+
workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!)))
114+
.returns(() => TypeMoq.Mock.ofType<WorkspaceFolder>().object)
115+
.verifiable(TypeMoq.Times.exactly(resource ? 5 : 0));
116+
app.setup(a => a.showErrorMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
117+
.returns(() => promptDeferred.promise)
118+
.verifiable(TypeMoq.Times.once());
119+
120+
// Display first prompt.
121+
installer.promptToInstall(product.value, resource).ignoreErrors();
122+
123+
// Display a few more prompts.
124+
installer.promptToInstall(product.value, resource).ignoreErrors();
125+
installer.promptToInstall(product.value, resource).ignoreErrors();
126+
installer.promptToInstall(product.value, resource).ignoreErrors();
127+
installer.promptToInstall(product.value, resource).ignoreErrors();
128+
129+
app.verifyAll();
130+
workspaceService.verifyAll();
131+
});
132+
test(`Ensure the prompt is displayed again when previous prompt has been closed, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async function () {
133+
if (product.value === Product.unittest) {
134+
return this.skip();
135+
}
136+
workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!)))
137+
.returns(() => TypeMoq.Mock.ofType<WorkspaceFolder>().object)
138+
.verifiable(TypeMoq.Times.exactly(resource ? 3 : 0));
139+
app.setup(a => a.showErrorMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
140+
.returns(() => Promise.resolve(undefined))
141+
.verifiable(TypeMoq.Times.exactly(3));
142+
143+
await installer.promptToInstall(product.value, resource);
144+
await installer.promptToInstall(product.value, resource);
145+
await installer.promptToInstall(product.value, resource);
146+
147+
app.verifyAll();
148+
workspaceService.verifyAll();
149+
});
95150
}
96151
}
97152
});

0 commit comments

Comments
 (0)