Use options object in terminal methods (#9739)

* Use options object in terminal methods
* Fix code review comments
* Update VSC
* Address review comments
* Fix types
* Revert "Update VSC"
This reverts commit 1c38572.

Author: DonJayamanne

src/client/common/terminal/activator/base.ts

@@ -3,26 +3,26 @@
 
 'use strict';
 
-import { Terminal, Uri } from 'vscode';
+import { Terminal } from 'vscode';
 import { createDeferred, sleep } from '../../utils/async';
-import { ITerminalActivator, ITerminalHelper, TerminalShellType } from '../types';
+import { ITerminalActivator, ITerminalHelper, TerminalActivationOptions, TerminalShellType } from '../types';
 
 export class BaseTerminalActivator implements ITerminalActivator {
     private readonly activatedTerminals: Map<Terminal, Promise<boolean>> = new Map<Terminal, Promise<boolean>>();
     constructor(private readonly helper: ITerminalHelper) {}
-    public async activateEnvironmentInTerminal(terminal: Terminal, resource: Uri | undefined, preserveFocus: boolean = true) {
+    public async activateEnvironmentInTerminal(terminal: Terminal, options?: TerminalActivationOptions): Promise<boolean> {
         if (this.activatedTerminals.has(terminal)) {
             return this.activatedTerminals.get(terminal)!;
         }
         const deferred = createDeferred<boolean>();
         this.activatedTerminals.set(terminal, deferred.promise);
         const terminalShellType = this.helper.identifyTerminalShell(terminal);
 
-        const activationCommamnds = await this.helper.getEnvironmentActivationCommands(terminalShellType, resource);
+        const activationCommands = await this.helper.getEnvironmentActivationCommands(terminalShellType, options?.resource, options?.interpreter);
         let activated = false;
-        if (activationCommamnds) {
-            for (const command of activationCommamnds!) {
-                terminal.show(preserveFocus);
+        if (activationCommands) {
+            for (const command of activationCommands!) {
+                terminal.show(options?.preserveFocus);
                 terminal.sendText(command);
                 await this.waitForCommandToProcess(terminalShellType);
                 activated = true;

src/client/common/terminal/activator/index.ts

@@ -4,8 +4,8 @@
 'use strict';
 
 import { inject, injectable, multiInject } from 'inversify';
-import { Terminal, Uri } from 'vscode';
-import { ITerminalActivationHandler, ITerminalActivator, ITerminalHelper } from '../types';
+import { Terminal } from 'vscode';
+import { ITerminalActivationHandler, ITerminalActivator, ITerminalHelper, TerminalActivationOptions } from '../types';
 import { BaseTerminalActivator } from './base';
 
 @injectable()
@@ -14,9 +14,9 @@ export class TerminalActivator implements ITerminalActivator {
     constructor(@inject(ITerminalHelper) readonly helper: ITerminalHelper, @multiInject(ITerminalActivationHandler) private readonly handlers: ITerminalActivationHandler[]) {
         this.initialize();
     }
-    public async activateEnvironmentInTerminal(terminal: Terminal, resource: Uri | undefined, preserveFocus: boolean = true) {
-        const activated = await this.baseActivator.activateEnvironmentInTerminal(terminal, resource, preserveFocus);
-        this.handlers.forEach(handler => handler.handleActivation(terminal, resource, preserveFocus, activated).ignoreErrors());
+    public async activateEnvironmentInTerminal(terminal: Terminal, options?: TerminalActivationOptions): Promise<boolean> {
+        const activated = await this.baseActivator.activateEnvironmentInTerminal(terminal, options);
+        this.handlers.forEach(handler => handler.handleActivation(terminal, options?.resource, options?.preserveFocus === true, activated).ignoreErrors());
         return activated;
     }
     protected initialize() {

src/client/common/terminal/factory.ts

@@ -3,13 +3,14 @@
 
 import { inject, injectable } from 'inversify';
 import { Uri } from 'vscode';
-import { IInterpreterService } from '../../interpreter/contracts';
+import { IInterpreterService, PythonInterpreter } from '../../interpreter/contracts';
 import { IServiceContainer } from '../../ioc/types';
 import { IWorkspaceService } from '../application/types';
 import { IFileSystem } from '../platform/types';
+import { isUri } from '../utils/misc';
 import { TerminalService } from './service';
 import { SynchronousTerminalService } from './syncTerminalService';
-import { ITerminalService, ITerminalServiceFactory } from './types';
+import { ITerminalService, ITerminalServiceFactory, TerminalCreationOptions } from './types';
 
 @injectable()
 export class TerminalServiceFactory implements ITerminalServiceFactory {
@@ -22,26 +23,39 @@ export class TerminalServiceFactory implements ITerminalServiceFactory {
     ) {
         this.terminalServices = new Map<string, TerminalService>();
     }
-    public getTerminalService(resource?: Uri, title?: string): ITerminalService {
+    public getTerminalService(options?: TerminalCreationOptions): ITerminalService;
+    public getTerminalService(resource?: Uri, title?: string): ITerminalService;
+    public getTerminalService(arg1?: Uri | TerminalCreationOptions, arg2?: string): ITerminalService {
+        const resource = isUri(arg1) ? arg1 : undefined;
+        const title = isUri(arg1) ? undefined : arg1?.title || arg2;
         const terminalTitle = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python';
-        const id = this.getTerminalId(terminalTitle, resource);
+        const interpreter = isUri(arg1) ? undefined : arg1?.interpreter;
+        const env = isUri(arg1) ? undefined : arg1?.env;
+
+        const options: TerminalCreationOptions = {
+            env,
+            interpreter,
+            resource,
+            title
+        };
+        const id = this.getTerminalId(terminalTitle, resource, interpreter);
         if (!this.terminalServices.has(id)) {
-            const terminalService = new TerminalService(this.serviceContainer, resource, terminalTitle);
+            const terminalService = new TerminalService(this.serviceContainer, options);
             this.terminalServices.set(id, terminalService);
         }
 
         // Decorate terminal service with the synchronous service.
-        return new SynchronousTerminalService(this.fs, this.interpreterService, this.terminalServices.get(id)!);
+        return new SynchronousTerminalService(this.fs, this.interpreterService, this.terminalServices.get(id)!, interpreter);
     }
     public createTerminalService(resource?: Uri, title?: string): ITerminalService {
-        const terminalTitle = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python';
-        return new TerminalService(this.serviceContainer, resource, terminalTitle);
+        title = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python';
+        return new TerminalService(this.serviceContainer, { resource, title });
     }
-    private getTerminalId(title: string, resource?: Uri): string {
-        if (!resource) {
+    private getTerminalId(title: string, resource?: Uri, interpreter?: PythonInterpreter): string {
+        if (!resource && !interpreter) {
             return title;
         }
-        const workspaceFolder = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService).getWorkspaceFolder(resource!);
-        return workspaceFolder ? `${title}:${workspaceFolder.uri.fsPath}` : title;
+        const workspaceFolder = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService).getWorkspaceFolder(resource || undefined);
+        return `${title}:${workspaceFolder?.uri.fsPath || ''}:${interpreter?.path}`;
     }
 }

src/client/common/terminal/helper.ts

@@ -47,9 +47,9 @@ export class TerminalHelper implements ITerminalHelper {
         const commandPrefix = isPowershell ? '& ' : '';
         return `${commandPrefix}${command.fileToCommandArgument()} ${args.join(' ')}`.trim();
     }
-    public async getEnvironmentActivationCommands(terminalShellType: TerminalShellType, resource?: Uri): Promise<string[] | undefined> {
+    public async getEnvironmentActivationCommands(terminalShellType: TerminalShellType, resource?: Uri, interpreter?: PythonInterpreter): Promise<string[] | undefined> {
         const providers = [this.pipenv, this.pyenv, this.bashCShellFish, this.commandPromptAndPowerShell];
-        const promise = this.getActivationCommands(resource || undefined, undefined, terminalShellType, providers);
+        const promise = this.getActivationCommands(resource || undefined, interpreter, terminalShellType, providers);
         this.sendTelemetry(resource, terminalShellType, EventName.PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL, promise).ignoreErrors();
         return promise;
     }

src/client/common/terminal/service.ts

@@ -2,15 +2,15 @@
 // Licensed under the MIT License.
 
 import { inject, injectable } from 'inversify';
-import { CancellationToken, Disposable, Event, EventEmitter, Terminal, Uri } from 'vscode';
+import { CancellationToken, Disposable, Event, EventEmitter, Terminal } from 'vscode';
 import '../../common/extensions';
 import { IInterpreterService } from '../../interpreter/contracts';
 import { IServiceContainer } from '../../ioc/types';
 import { captureTelemetry } from '../../telemetry';
 import { EventName } from '../../telemetry/constants';
 import { ITerminalManager } from '../application/types';
 import { IConfigurationService, IDisposableRegistry } from '../types';
-import { ITerminalActivator, ITerminalHelper, ITerminalService, TerminalShellType } from './types';
+import { ITerminalActivator, ITerminalHelper, ITerminalService, TerminalCreationOptions, TerminalShellType } from './types';
 
 @injectable()
 export class TerminalService implements ITerminalService, Disposable {
@@ -23,7 +23,7 @@ export class TerminalService implements ITerminalService, Disposable {
     public get onDidCloseTerminal(): Event<void> {
         return this.terminalClosed.event.bind(this.terminalClosed);
     }
-    constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer, private resource?: Uri, private title: string = 'Python') {
+    constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer, private readonly options?: TerminalCreationOptions) {
         const disposableRegistry = this.serviceContainer.get<Disposable[]>(IDisposableRegistry);
         disposableRegistry.push(this);
         this.terminalHelper = this.serviceContainer.get<ITerminalHelper>(ITerminalHelper);
@@ -56,12 +56,16 @@ export class TerminalService implements ITerminalService, Disposable {
             return;
         }
         this.terminalShellType = this.terminalHelper.identifyTerminalShell(this.terminal);
-        this.terminal = this.terminalManager.createTerminal({ name: this.title });
+        this.terminal = this.terminalManager.createTerminal({ name: this.options?.title || 'Python', env: this.options?.env });
 
         // Sometimes the terminal takes some time to start up before it can start accepting input.
         await new Promise(resolve => setTimeout(resolve, 100));
 
-        await this.terminalActivator.activateEnvironmentInTerminal(this.terminal!, this.resource, preserveFocus);
+        await this.terminalActivator.activateEnvironmentInTerminal(this.terminal!, {
+            resource: this.options?.resource,
+            preserveFocus,
+            interpreter: this.options?.interpreter
+        });
 
         this.terminal!.show(preserveFocus);
 
@@ -75,8 +79,8 @@ export class TerminalService implements ITerminalService, Disposable {
     }
 
     private async sendTelemetry() {
-        const pythonPath = this.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(this.resource).pythonPath;
-        const interpreterInfo = await this.serviceContainer.get<IInterpreterService>(IInterpreterService).getInterpreterDetails(pythonPath);
+        const pythonPath = this.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(this.options?.resource).pythonPath;
+        const interpreterInfo = this.options?.interpreter || (await this.serviceContainer.get<IInterpreterService>(IInterpreterService).getInterpreterDetails(pythonPath));
         const pythonVersion = interpreterInfo && interpreterInfo.version ? interpreterInfo.version.raw : undefined;
         const interpreterType = interpreterInfo ? interpreterInfo.type : undefined;
         captureTelemetry(EventName.TERMINAL_CREATE, { terminal: this.terminalShellType, pythonVersion, interpreterType });

src/client/common/terminal/syncTerminalService.ts

@@ -7,7 +7,7 @@ import { inject } from 'inversify';
 import * as path from 'path';
 import { CancellationToken, Disposable, Event } from 'vscode';
 import { EXTENSION_ROOT_DIR } from '../../constants';
-import { IInterpreterService } from '../../interpreter/contracts';
+import { IInterpreterService, PythonInterpreter } from '../../interpreter/contracts';
 import { Cancellation } from '../cancellation';
 import { traceVerbose } from '../logger';
 import { IFileSystem, TemporaryFile } from '../platform/types';
@@ -97,7 +97,8 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
     constructor(
         @inject(IFileSystem) private readonly fs: IFileSystem,
         @inject(IInterpreterService) private readonly interpreter: IInterpreterService,
-        public readonly terminalService: TerminalService
+        public readonly terminalService: TerminalService,
+        private readonly pythonInterpreter?: PythonInterpreter
     ) {}
     public dispose() {
         this.terminalService.dispose();
@@ -121,7 +122,7 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
         const lockFile = await this.createLockFile();
         const state = new ExecutionState(lockFile.filePath, this.fs, [command, ...args]);
         try {
-            const pythonExec = await this.interpreter.getActiveInterpreter(undefined);
+            const pythonExec = this.pythonInterpreter || (await this.interpreter.getActiveInterpreter(undefined));
             await this.terminalService.sendCommand(pythonExec?.path || 'python', [
                 shellExecFile.fileToCommandArgument(),
                 command.fileToCommandArgument(),

src/client/common/terminal/types.ts

@@ -6,7 +6,7 @@
 import { CancellationToken, Event, Terminal, Uri } from 'vscode';
 import { PythonInterpreter } from '../../interpreter/contracts';
 import { IEventNamePropertyMapping } from '../../telemetry/index';
-import { Resource } from '../types';
+import { IDisposable, Resource } from '../types';
 
 export enum TerminalActivationProviders {
     bashCShellFish = 'bashCShellFish',
@@ -31,7 +31,7 @@ export enum TerminalShellType {
     other = 'other'
 }
 
-export interface ITerminalService {
+export interface ITerminalService extends IDisposable {
     readonly onDidCloseTerminal: Event<void>;
     sendCommand(command: string, args: string[], cancel?: CancellationToken): Promise<void>;
     sendText(text: string): Promise<void>;
@@ -40,6 +40,31 @@ export interface ITerminalService {
 
 export const ITerminalServiceFactory = Symbol('ITerminalServiceFactory');
 
+export type TerminalCreationOptions = {
+    /**
+     * Object with environment variables that will be added to the Terminal.
+     */
+    env?: { [key: string]: string | null };
+    /**
+     * Resource identifier. E.g. used to determine python interpreter that needs to be used or environment variables or the like.
+     *
+     * @type {Uri}
+     */
+    resource?: Uri;
+    /**
+     * Title.
+     *
+     * @type {string}
+     */
+    title?: string;
+    /**
+     * Associated Python Interpreter.
+     *
+     * @type {PythonInterpreter}
+     */
+    interpreter?: PythonInterpreter;
+};
+
 export interface ITerminalServiceFactory {
     /**
      * Gets a terminal service with a specific title.
@@ -50,6 +75,15 @@ export interface ITerminalServiceFactory {
      * @memberof ITerminalServiceFactory
      */
     getTerminalService(resource?: Uri, title?: string): ITerminalService;
+    /**
+     * Gets a terminal service.
+     * If one exists with the same information, that is returned else a new one is created.
+     *
+     * @param {TerminalCreationOptions} [options]
+     * @returns {ITerminalService}
+     * @memberof ITerminalServiceFactory
+     */
+    getTerminalService(options?: TerminalCreationOptions): ITerminalService;
     createTerminalService(resource?: Uri, title?: string): ITerminalService;
 }
 
@@ -59,13 +93,18 @@ export interface ITerminalHelper {
     createTerminal(title?: string): Terminal;
     identifyTerminalShell(terminal?: Terminal): TerminalShellType;
     buildCommandForTerminal(terminalShellType: TerminalShellType, command: string, args: string[]): string;
-    getEnvironmentActivationCommands(terminalShellType: TerminalShellType, resource?: Uri): Promise<string[] | undefined>;
+    getEnvironmentActivationCommands(terminalShellType: TerminalShellType, resource?: Uri, interpreter?: PythonInterpreter): Promise<string[] | undefined>;
     getEnvironmentActivationShellCommands(resource: Resource, shell: TerminalShellType, interpreter?: PythonInterpreter): Promise<string[] | undefined>;
 }
 
 export const ITerminalActivator = Symbol('ITerminalActivator');
+export type TerminalActivationOptions = {
+    resource?: Resource;
+    preserveFocus?: boolean;
+    interpreter?: PythonInterpreter;
+};
 export interface ITerminalActivator {
-    activateEnvironmentInTerminal(terminal: Terminal, resource: Uri | undefined, preserveFocus?: boolean): Promise<boolean>;
+    activateEnvironmentInTerminal(terminal: Terminal, options?: TerminalActivationOptions): Promise<boolean>;
 }
 
 export const ITerminalActivationCommandProvider = Symbol('ITerminalActivationCommandProvider');

src/client/common/utils/misc.ts

@@ -40,3 +40,21 @@ export function isResource(resource?: InterpreterUri): resource is Resource {
     const uri = resource as Uri;
     return typeof uri.path === 'string' && typeof uri.scheme === 'string';
 }
+
+/**
+ * Checking whether something is a Uri.
+ * Using `instanceof Uri` doesn't always work as the object is not an instance of Uri (at least not in tests).
+ * That's why VSC too has a helper method `URI.isUri` (though not public).
+ *
+ * @export
+ * @param {InterpreterUri} [resource]
+ * @returns {resource is Uri}
+ */
+// tslint:disable-next-line: no-any
+export function isUri(resource?: Uri | any): resource is Uri {
+    if (!resource) {
+        return false;
+    }
+    const uri = resource as Uri;
+    return typeof uri.path === 'string' && typeof uri.scheme === 'string';
+}

src/client/common/variables/environmentVariablesProvider.ts

@@ -44,11 +44,7 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid
     }
     @cacheResourceSpecificInterpreterData('getEnvironmentVariables', cacheDuration)
     public async getEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables> {
-        const settings = this.configurationService.getSettings(resource);
-        const workspaceFolderUri = this.getWorkspaceFolderUri(resource);
-        this.trackedWorkspaceFolders.add(workspaceFolderUri ? workspaceFolderUri.fsPath : '');
-        this.createFileWatcher(settings.envFile, workspaceFolderUri);
-        let mergedVars = await this.envVarsService.parseFile(settings.envFile, this.process.env);
+        let mergedVars = await this.getCustomEnvironmentVariables(resource);
         if (!mergedVars) {
             mergedVars = {};
         }
@@ -63,6 +59,13 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid
         }
         return mergedVars;
     }
+    public async getCustomEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables | undefined> {
+        const settings = this.configurationService.getSettings(resource);
+        const workspaceFolderUri = this.getWorkspaceFolderUri(resource);
+        this.trackedWorkspaceFolders.add(workspaceFolderUri ? workspaceFolderUri.fsPath : '');
+        this.createFileWatcher(settings.envFile, workspaceFolderUri);
+        return this.envVarsService.parseFile(settings.envFile, this.process.env);
+    }
     public configurationChanged(e: ConfigurationChangeEvent) {
         this.trackedWorkspaceFolders.forEach(item => {
             const uri = item && item.length > 0 ? Uri.file(item) : undefined;

src/client/common/variables/types.ts

@@ -38,4 +38,5 @@ export const IEnvironmentVariablesProvider = Symbol('IEnvironmentVariablesProvid
 export interface IEnvironmentVariablesProvider {
     onDidEnvironmentVariablesChange: Event<Uri | undefined>;
     getEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables>;
+    getCustomEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables | undefined>;
 }