From ca854b88bfd4cef0d0ca3b627aaaec89f1a943fe Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 21 May 2018 10:18:36 -0700 Subject: [PATCH] Ensure the prompt to install missing packages is displayed only once (#1649) * Ensure the prompt to install missing packages is displayed only once * Add missing dependency * Fixes #980 --- news/2 Fixes/980 | 1 + .../common/installer/productInstaller.ts | 54 +++++++++++------ .../linters/errorHandlers/notInstalled.ts | 1 - src/test/common/installer.test.ts | 6 +- src/test/common/installer/installer.test.ts | 59 ++++++++++++++++++- 5 files changed, 99 insertions(+), 22 deletions(-) create mode 100644 news/2 Fixes/980 diff --git a/news/2 Fixes/980 b/news/2 Fixes/980 new file mode 100644 index 000000000000..0dcbcaacd8ff --- /dev/null +++ b/news/2 Fixes/980 @@ -0,0 +1 @@ +Ensure the prompt to install missing packages is not displayed more than once. diff --git a/src/client/common/installer/productInstaller.ts b/src/client/common/installer/productInstaller.ts index 9659ef0debad..8e3bde192995 100644 --- a/src/client/common/installer/productInstaller.ts +++ b/src/client/common/installer/productInstaller.ts @@ -2,11 +2,12 @@ import { inject, injectable, named } from 'inversify'; import * as os from 'os'; import * as path from 'path'; import * as vscode from 'vscode'; +import '../../common/extensions'; import { IFormatterHelper } from '../../formatters/types'; import { IServiceContainer } from '../../ioc/types'; import { ILinterManager } from '../../linters/types'; import { ITestsHelper } from '../../unittests/common/types'; -import { IApplicationShell } from '../application/types'; +import { IApplicationShell, IWorkspaceService } from '../application/types'; import { STANDARD_OUTPUT_CHANNEL } from '../constants'; import { IPlatformService } from '../platform/types'; import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types'; @@ -28,16 +29,34 @@ enum ProductType { } // tslint:disable-next-line:max-classes-per-file -abstract class BaseInstaller { +export abstract class BaseInstaller { + private static readonly PromptPromises = new Map>(); protected appShell: IApplicationShell; protected configService: IConfigurationService; + private readonly workspaceService: IWorkspaceService; constructor(protected serviceContainer: IServiceContainer, protected outputChannel: vscode.OutputChannel) { this.appShell = serviceContainer.get(IApplicationShell); this.configService = serviceContainer.get(IConfigurationService); + this.workspaceService = serviceContainer.get(IWorkspaceService); } - public abstract promptToInstall(product: Product, resource?: vscode.Uri): Promise; + public promptToInstall(product: Product, resource?: vscode.Uri): Promise { + // If this method gets called twice, while previous promise has not been resolved, then return that same promise. + // E.g. previous promise is not resolved as a message has been displayed to the user, so no point displaying + // another message. + const workspaceFolder = resource ? this.workspaceService.getWorkspaceFolder(resource) : undefined; + const key = `${product}${workspaceFolder ? workspaceFolder.uri.fsPath : ''}`; + if (BaseInstaller.PromptPromises.has(key)) { + return BaseInstaller.PromptPromises.get(key)!; + } + const promise = this.promptToInstallImplementation(product, resource); + BaseInstaller.PromptPromises.set(key, promise); + promise.then(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors(); + promise.catch(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors(); + + return promise; + } public async install(product: Product, resource?: vscode.Uri): Promise { if (product === Product.unittest) { @@ -83,22 +102,17 @@ abstract class BaseInstaller { .catch(() => false); } } - + protected abstract promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise; protected getExecutableNameFromSettings(product: Product, resource?: vscode.Uri): string { throw new Error('getExecutableNameFromSettings is not supported on this object'); } } -class CTagsInstaller extends BaseInstaller { +export class CTagsInstaller extends BaseInstaller { constructor(serviceContainer: IServiceContainer, outputChannel: vscode.OutputChannel) { super(serviceContainer, outputChannel); } - public async promptToInstall(product: Product, resource?: vscode.Uri): Promise { - const item = await this.appShell.showErrorMessage('Install CTags to enable Python workspace symbols?', 'Yes', 'No'); - return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore; - } - public async install(product: Product, resource?: vscode.Uri): Promise { if (this.serviceContainer.get(IPlatformService).isWindows) { this.outputChannel.appendLine('Install Universal Ctags Win32 to enable support for Workspace Symbols'); @@ -115,6 +129,10 @@ class CTagsInstaller extends BaseInstaller { } return InstallerResponse.Ignore; } + protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise { + const item = await this.appShell.showErrorMessage('Install CTags to enable Python workspace symbols?', 'Yes', 'No'); + return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore; + } protected getExecutableNameFromSettings(product: Product, resource?: vscode.Uri): string { const settings = this.configService.getSettings(resource); @@ -122,8 +140,8 @@ class CTagsInstaller extends BaseInstaller { } } -class FormatterInstaller extends BaseInstaller { - public async promptToInstall(product: Product, resource?: vscode.Uri): Promise { +export class FormatterInstaller extends BaseInstaller { + protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise { // Hard-coded on purpose because the UI won't necessarily work having // another formatter. const formatters = [Product.autopep8, Product.black, Product.yapf]; @@ -159,8 +177,8 @@ class FormatterInstaller extends BaseInstaller { } // tslint:disable-next-line:max-classes-per-file -class LinterInstaller extends BaseInstaller { - public async promptToInstall(product: Product, resource?: vscode.Uri): Promise { +export class LinterInstaller extends BaseInstaller { + protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise { const productName = ProductNames.get(product)!; const install = 'Install'; const disableAllLinting = 'Disable linting'; @@ -188,8 +206,8 @@ class LinterInstaller extends BaseInstaller { } // tslint:disable-next-line:max-classes-per-file -class TestFrameworkInstaller extends BaseInstaller { - public async promptToInstall(product: Product, resource?: vscode.Uri): Promise { +export class TestFrameworkInstaller extends BaseInstaller { + protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise { const productName = ProductNames.get(product)!; const item = await this.appShell.showErrorMessage(`Test framework ${productName} is not installed. Install?`, 'Yes', 'No'); return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore; @@ -208,8 +226,8 @@ class TestFrameworkInstaller extends BaseInstaller { } // tslint:disable-next-line:max-classes-per-file -class RefactoringLibraryInstaller extends BaseInstaller { - public async promptToInstall(product: Product, resource?: vscode.Uri): Promise { +export class RefactoringLibraryInstaller extends BaseInstaller { + protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise { const productName = ProductNames.get(product)!; const item = await this.appShell.showErrorMessage(`Refactoring library ${productName} is not installed. Install?`, 'Yes', 'No'); return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore; diff --git a/src/client/linters/errorHandlers/notInstalled.ts b/src/client/linters/errorHandlers/notInstalled.ts index c3fbd9447296..c7c56c66e7a9 100644 --- a/src/client/linters/errorHandlers/notInstalled.ts +++ b/src/client/linters/errorHandlers/notInstalled.ts @@ -1,5 +1,4 @@ import { OutputChannel, Uri } from 'vscode'; -import { isNotInstalledError } from '../../common/helpers'; import { IPythonExecutionFactory } from '../../common/process/types'; import { ExecutionInfo, Product } from '../../common/types'; import { IServiceContainer } from '../../ioc/types'; diff --git a/src/test/common/installer.test.ts b/src/test/common/installer.test.ts index 09b0385eaca2..ea1fd200b983 100644 --- a/src/test/common/installer.test.ts +++ b/src/test/common/installer.test.ts @@ -1,7 +1,7 @@ import * as path from 'path'; import * as TypeMoq from 'typemoq'; import { ConfigurationTarget, Uri } from 'vscode'; -import { IApplicationShell } from '../../client/common/application/types'; +import { IApplicationShell, IWorkspaceService } from '../../client/common/application/types'; import { ConfigurationService } from '../../client/common/configuration/service'; import { EnumEx } from '../../client/common/enumUtils'; import { createDeferred } from '../../client/common/helpers'; @@ -58,6 +58,10 @@ suite('Installer', () => { ioc.serviceManager.addSingletonInstance(IApplicationShell, TypeMoq.Mock.ofType().object); ioc.serviceManager.addSingleton(IConfigurationService, ConfigurationService); + const workspaceService = TypeMoq.Mock.ofType(); + workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isAny())).returns(() => undefined); + ioc.serviceManager.addSingletonInstance(IWorkspaceService, workspaceService.object); + ioc.registerMockProcessTypes(); ioc.serviceManager.addSingletonInstance(IsWindows, false); } diff --git a/src/test/common/installer/installer.test.ts b/src/test/common/installer/installer.test.ts index b1c37cf6bb85..69f88f2c21ea 100644 --- a/src/test/common/installer/installer.test.ts +++ b/src/test/common/installer/installer.test.ts @@ -1,11 +1,16 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +// tslint:disable:max-func-body-length no-invalid-this + import { expect, use } from 'chai'; import * as chaiAsPromised from 'chai-as-promised'; import * as TypeMoq from 'typemoq'; -import { Disposable, OutputChannel, Uri } from 'vscode'; +import { Disposable, OutputChannel, Uri, WorkspaceFolder } from 'vscode'; +import { IApplicationShell, IWorkspaceService } from '../../../client/common/application/types'; import { EnumEx } from '../../../client/common/enumUtils'; +import '../../../client/common/extensions'; +import { createDeferred, Deferred } from '../../../client/common/helpers'; import { ProductInstaller } from '../../../client/common/installer/productInstaller'; import { IInstallationChannelManager, IModuleInstaller } from '../../../client/common/installer/types'; import { IDisposableRegistry, ILogger, InstallerResponse, ModuleNamePurpose, Product } from '../../../client/common/types'; @@ -13,7 +18,6 @@ import { IServiceContainer } from '../../../client/ioc/types'; use(chaiAsPromised); -// tslint:disable-next-line:max-func-body-length suite('Module Installer', () => { [undefined, Uri.file('resource')].forEach(resource => { EnumEx.getNamesAndValues(Product).forEach(product => { @@ -22,7 +26,11 @@ suite('Module Installer', () => { let installationChannel: TypeMoq.IMock; let moduleInstaller: TypeMoq.IMock; let serviceContainer: TypeMoq.IMock; + let app: TypeMoq.IMock; + let promptDeferred: Deferred; + let workspaceService: TypeMoq.IMock; setup(() => { + promptDeferred = createDeferred(); serviceContainer = TypeMoq.Mock.ofType(); const outputChannel = TypeMoq.Mock.ofType(); @@ -33,6 +41,10 @@ suite('Module Installer', () => { installationChannel = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInstallationChannelManager), TypeMoq.It.isAny())).returns(() => installationChannel.object); + app = TypeMoq.Mock.ofType(); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell), TypeMoq.It.isAny())).returns(() => app.object); + workspaceService = TypeMoq.Mock.ofType(); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IWorkspaceService), TypeMoq.It.isAny())).returns(() => workspaceService.object); moduleInstaller = TypeMoq.Mock.ofType(); // tslint:disable-next-line:no-any @@ -41,6 +53,8 @@ suite('Module Installer', () => { installationChannel.setup(i => i.getInstallationChannel(TypeMoq.It.isAny())).returns(() => Promise.resolve(moduleInstaller.object)); }); teardown(() => { + // This must be resolved, else all subsequent tests will fail (as this same promise will be used for other tests). + promptDeferred.resolve(); disposables.forEach(disposable => { if (disposable) { disposable.dispose(); @@ -92,6 +106,47 @@ suite('Module Installer', () => { moduleInstaller.verify(m => m.installModule(TypeMoq.It.isValue(moduleName), TypeMoq.It.isValue(resource)), TypeMoq.Times.once()); } }); + test(`Ensure the prompt is displayed only once, untill the prompt is closed, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async function () { + if (product.value === Product.unittest) { + return this.skip(); + } + workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!))) + .returns(() => TypeMoq.Mock.ofType().object) + .verifiable(TypeMoq.Times.exactly(resource ? 5 : 0)); + 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())) + .returns(() => promptDeferred.promise) + .verifiable(TypeMoq.Times.once()); + + // Display first prompt. + installer.promptToInstall(product.value, resource).ignoreErrors(); + + // Display a few more prompts. + installer.promptToInstall(product.value, resource).ignoreErrors(); + installer.promptToInstall(product.value, resource).ignoreErrors(); + installer.promptToInstall(product.value, resource).ignoreErrors(); + installer.promptToInstall(product.value, resource).ignoreErrors(); + + app.verifyAll(); + workspaceService.verifyAll(); + }); + test(`Ensure the prompt is displayed again when previous prompt has been closed, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async function () { + if (product.value === Product.unittest) { + return this.skip(); + } + workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!))) + .returns(() => TypeMoq.Mock.ofType().object) + .verifiable(TypeMoq.Times.exactly(resource ? 3 : 0)); + 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())) + .returns(() => Promise.resolve(undefined)) + .verifiable(TypeMoq.Times.exactly(3)); + + await installer.promptToInstall(product.value, resource); + await installer.promptToInstall(product.value, resource); + await installer.promptToInstall(product.value, resource); + + app.verifyAll(); + workspaceService.verifyAll(); + }); } } });