From 81ae205e871d4326e7b549ee2667844c50e16c34 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 18 Jul 2023 16:34:58 -0700 Subject: [PATCH] Bring back feature to Run Python file in dedicated terminal (#21656) Closes https://github.com/microsoft/vscode-python/issues/21282 Closes https://github.com/microsoft/vscode-python/issues/21420 Closes https://github.com/microsoft/vscode-python/issues/21215 Reverts microsoft/vscode-python#21418 --- package.json | 19 +++++++ package.nls.json | 1 + src/client/common/constants.ts | 1 + src/client/common/terminal/factory.ts | 21 +++++-- src/client/common/terminal/types.ts | 2 +- src/client/telemetry/index.ts | 6 ++ .../codeExecution/codeExecutionManager.ts | 56 ++++++++++++------- .../codeExecution/terminalCodeExecution.ts | 40 +++++++------ src/client/terminals/types.ts | 2 +- .../common/terminals/factory.unit.test.ts | 47 +++++++++++++++- .../codeExecutionManager.unit.test.ts | 25 ++++++--- .../terminalCodeExec.unit.test.ts | 29 +++++++++- 12 files changed, 190 insertions(+), 59 deletions(-) diff --git a/package.json b/package.json index e38e6921e50a..abadcdb3bb0b 100644 --- a/package.json +++ b/package.json @@ -392,6 +392,12 @@ "icon": "$(play)", "title": "%python.command.python.execInTerminalIcon.title%" }, + { + "category": "Python", + "command": "python.execInDedicatedTerminal", + "icon": "$(play)", + "title": "%python.command.python.execInDedicatedTerminal.title%" + }, { "category": "Python", "command": "python.debugInTerminal", @@ -1818,6 +1824,13 @@ "title": "%python.command.python.execInTerminalIcon.title%", "when": "false" }, + { + "category": "Python", + "command": "python.execInDedicatedTerminal", + "icon": "$(play)", + "title": "%python.command.python.execInDedicatedTerminal.title%", + "when": "false" + }, { "category": "Python", "command": "python.debugInTerminal", @@ -1976,6 +1989,12 @@ "title": "%python.command.python.execInTerminalIcon.title%", "when": "resourceLangId == python && !isInDiffEditor && !virtualWorkspace && shellExecutionSupported" }, + { + "command": "python.execInDedicatedTerminal", + "group": "navigation@0", + "title": "%python.command.python.execInDedicatedTerminal.title%", + "when": "resourceLangId == python && !isInDiffEditor && !virtualWorkspace && shellExecutionSupported" + }, { "command": "python.debugInTerminal", "group": "navigation@1", diff --git a/package.nls.json b/package.nls.json index 84c920b7c22e..79609f02e83a 100644 --- a/package.nls.json +++ b/package.nls.json @@ -7,6 +7,7 @@ "python.command.python.execInTerminal.title": "Run Python File in Terminal", "python.command.python.debugInTerminal.title": "Debug Python File", "python.command.python.execInTerminalIcon.title": "Run Python File", + "python.command.python.execInDedicatedTerminal.title": "Run Python File in Dedicated Terminal", "python.command.python.setInterpreter.title": "Select Interpreter", "python.command.python.clearWorkspaceInterpreter.title": "Clear Workspace Interpreter Setting", "python.command.python.viewOutput.title": "Show Output", diff --git a/src/client/common/constants.ts b/src/client/common/constants.ts index b285667aaa6a..bea0ef9e235c 100644 --- a/src/client/common/constants.ts +++ b/src/client/common/constants.ts @@ -44,6 +44,7 @@ export namespace Commands { export const Enable_SourceMap_Support = 'python.enableSourceMapSupport'; export const Exec_In_Terminal = 'python.execInTerminal'; export const Exec_In_Terminal_Icon = 'python.execInTerminal-icon'; + export const Exec_In_Separate_Terminal = 'python.execInDedicatedTerminal'; export const Exec_Selection_In_Django_Shell = 'python.execSelectionInDjangoShell'; export const Exec_Selection_In_Terminal = 'python.execSelectionInTerminal'; export const GetSelectedInterpreterPath = 'python.interpreterPath'; diff --git a/src/client/common/terminal/factory.ts b/src/client/common/terminal/factory.ts index 3855cb6cee3c..39cc88c4b024 100644 --- a/src/client/common/terminal/factory.ts +++ b/src/client/common/terminal/factory.ts @@ -3,6 +3,7 @@ import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; +import * as path from 'path'; import { IInterpreterService } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; import { PythonEnvironment } from '../../pythonEnvironments/info'; @@ -23,13 +24,17 @@ export class TerminalServiceFactory implements ITerminalServiceFactory { ) { this.terminalServices = new Map(); } - public getTerminalService(options: TerminalCreationOptions): ITerminalService { + public getTerminalService(options: TerminalCreationOptions & { newTerminalPerFile?: boolean }): ITerminalService { const resource = options?.resource; const title = options?.title; - const terminalTitle = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python'; + let terminalTitle = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python'; const interpreter = options?.interpreter; - const id = this.getTerminalId(terminalTitle, resource, interpreter); + const id = this.getTerminalId(terminalTitle, resource, interpreter, options.newTerminalPerFile); if (!this.terminalServices.has(id)) { + if (resource && options.newTerminalPerFile) { + terminalTitle = `${terminalTitle}: ${path.basename(resource.fsPath).replace('.py', '')}`; + } + options.title = terminalTitle; const terminalService = new TerminalService(this.serviceContainer, options); this.terminalServices.set(id, terminalService); } @@ -46,13 +51,19 @@ export class TerminalServiceFactory implements ITerminalServiceFactory { title = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python'; return new TerminalService(this.serviceContainer, { resource, title }); } - private getTerminalId(title: string, resource?: Uri, interpreter?: PythonEnvironment): string { + private getTerminalId( + title: string, + resource?: Uri, + interpreter?: PythonEnvironment, + newTerminalPerFile?: boolean, + ): string { if (!resource && !interpreter) { return title; } const workspaceFolder = this.serviceContainer .get(IWorkspaceService) .getWorkspaceFolder(resource || undefined); - return `${title}:${workspaceFolder?.uri.fsPath || ''}:${interpreter?.path}`; + const fileId = resource && newTerminalPerFile ? resource.fsPath : ''; + return `${title}:${workspaceFolder?.uri.fsPath || ''}:${interpreter?.path}:${fileId}`; } } diff --git a/src/client/common/terminal/types.ts b/src/client/common/terminal/types.ts index 880bf0dd72fb..303188682378 100644 --- a/src/client/common/terminal/types.ts +++ b/src/client/common/terminal/types.ts @@ -97,7 +97,7 @@ export interface ITerminalServiceFactory { * @returns {ITerminalService} * @memberof ITerminalServiceFactory */ - getTerminalService(options: TerminalCreationOptions): ITerminalService; + getTerminalService(options: TerminalCreationOptions & { newTerminalPerFile?: boolean }): ITerminalService; createTerminalService(resource?: Uri, title?: string): ITerminalService; } diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 50d7ebb0f9b1..f0b57a4043d9 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -827,6 +827,12 @@ export interface IEventNamePropertyMapping { * @type {('command' | 'icon')} */ trigger?: 'command' | 'icon'; + /** + * Whether user chose to execute this Python file in a separate terminal or not. + * + * @type {boolean} + */ + newTerminalPerFile?: boolean; }; /** * Telemetry Event sent when user executes code against Django Shell. diff --git a/src/client/terminals/codeExecution/codeExecutionManager.ts b/src/client/terminals/codeExecution/codeExecutionManager.ts index ed671f2846a2..9f1ba6e90d90 100644 --- a/src/client/terminals/codeExecution/codeExecutionManager.ts +++ b/src/client/terminals/codeExecution/codeExecutionManager.ts @@ -36,25 +36,31 @@ export class CodeExecutionManager implements ICodeExecutionManager { } public registerCommands() { - [Commands.Exec_In_Terminal, Commands.Exec_In_Terminal_Icon].forEach((cmd) => { - this.disposableRegistry.push( - this.commandManager.registerCommand(cmd as any, async (file: Resource) => { - const interpreterService = this.serviceContainer.get(IInterpreterService); - const interpreter = await interpreterService.getActiveInterpreter(file); - if (!interpreter) { - this.commandManager.executeCommand(Commands.TriggerEnvironmentSelection, file).then(noop, noop); - return; - } - const trigger = cmd === Commands.Exec_In_Terminal ? 'command' : 'icon'; - await this.executeFileInTerminal(file, trigger) - .then(() => { - if (this.shouldTerminalFocusOnStart(file)) - this.commandManager.executeCommand('workbench.action.terminal.focus'); + [Commands.Exec_In_Terminal, Commands.Exec_In_Terminal_Icon, Commands.Exec_In_Separate_Terminal].forEach( + (cmd) => { + this.disposableRegistry.push( + this.commandManager.registerCommand(cmd as any, async (file: Resource) => { + const interpreterService = this.serviceContainer.get(IInterpreterService); + const interpreter = await interpreterService.getActiveInterpreter(file); + if (!interpreter) { + this.commandManager + .executeCommand(Commands.TriggerEnvironmentSelection, file) + .then(noop, noop); + return; + } + const trigger = cmd === Commands.Exec_In_Terminal ? 'command' : 'icon'; + await this.executeFileInTerminal(file, trigger, { + newTerminalPerFile: cmd === Commands.Exec_In_Separate_Terminal, }) - .catch((ex) => traceError('Failed to execute file in terminal', ex)); - }), - ); - }); + .then(() => { + if (this.shouldTerminalFocusOnStart(file)) + this.commandManager.executeCommand('workbench.action.terminal.focus'); + }) + .catch((ex) => traceError('Failed to execute file in terminal', ex)); + }), + ); + }, + ); this.disposableRegistry.push( this.commandManager.registerCommand(Commands.Exec_Selection_In_Terminal as any, async (file: Resource) => { const interpreterService = this.serviceContainer.get(IInterpreterService); @@ -87,8 +93,16 @@ export class CodeExecutionManager implements ICodeExecutionManager { ), ); } - private async executeFileInTerminal(file: Resource, trigger: 'command' | 'icon') { - sendTelemetryEvent(EventName.EXECUTION_CODE, undefined, { scope: 'file', trigger }); + private async executeFileInTerminal( + file: Resource, + trigger: 'command' | 'icon', + options?: { newTerminalPerFile: boolean }, + ): Promise { + sendTelemetryEvent(EventName.EXECUTION_CODE, undefined, { + scope: 'file', + trigger, + newTerminalPerFile: options?.newTerminalPerFile, + }); const codeExecutionHelper = this.serviceContainer.get(ICodeExecutionHelper); file = file instanceof Uri ? file : undefined; let fileToExecute = file ? file : await codeExecutionHelper.getFileToExecute(); @@ -110,7 +124,7 @@ export class CodeExecutionManager implements ICodeExecutionManager { } const executionService = this.serviceContainer.get(ICodeExecutionService, 'standard'); - await executionService.executeFile(fileToExecute); + await executionService.executeFile(fileToExecute, options); } @captureTelemetry(EventName.EXECUTION_CODE, { scope: 'selection' }, false) diff --git a/src/client/terminals/codeExecution/terminalCodeExecution.ts b/src/client/terminals/codeExecution/terminalCodeExecution.ts index 9261483b45e1..4c329e939599 100644 --- a/src/client/terminals/codeExecution/terminalCodeExecution.ts +++ b/src/client/terminals/codeExecution/terminalCodeExecution.ts @@ -10,7 +10,7 @@ import { IWorkspaceService } from '../../common/application/types'; import '../../common/extensions'; import { IPlatformService } from '../../common/platform/types'; import { ITerminalService, ITerminalServiceFactory } from '../../common/terminal/types'; -import { IConfigurationService, IDisposableRegistry } from '../../common/types'; +import { IConfigurationService, IDisposableRegistry, Resource } from '../../common/types'; import { IInterpreterService } from '../../interpreter/contracts'; import { buildPythonExecInfo, PythonExecInfo } from '../../pythonEnvironments/exec'; import { ICodeExecutionService } from '../../terminals/types'; @@ -19,7 +19,6 @@ import { ICodeExecutionService } from '../../terminals/types'; export class TerminalCodeExecutionProvider implements ICodeExecutionService { private hasRanOutsideCurrentDrive = false; protected terminalTitle!: string; - private _terminalService!: ITerminalService; private replActive?: Promise; constructor( @inject(ITerminalServiceFactory) protected readonly terminalServiceFactory: ITerminalServiceFactory, @@ -30,13 +29,13 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService { @inject(IInterpreterService) protected readonly interpreterService: IInterpreterService, ) {} - public async executeFile(file: Uri) { + public async executeFile(file: Uri, options?: { newTerminalPerFile: boolean }) { await this.setCwdForFileExecution(file); const { command, args } = await this.getExecuteFileArgs(file, [ file.fsPath.fileToCommandArgumentForPythonExt(), ]); - await this.getTerminalService(file).sendCommand(command, args); + await this.getTerminalService(file, options).sendCommand(command, args); } public async execute(code: string, resource?: Uri): Promise { @@ -44,21 +43,27 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService { return; } - await this.initializeRepl(); + await this.initializeRepl(resource); await this.getTerminalService(resource).sendText(code); } - public async initializeRepl(resource?: Uri) { + public async initializeRepl(resource: Resource) { + const terminalService = this.getTerminalService(resource); if (this.replActive && (await this.replActive)) { - await this._terminalService.show(); + await terminalService.show(); return; } this.replActive = new Promise(async (resolve) => { const replCommandArgs = await this.getExecutableInfo(resource); - await this.getTerminalService(resource).sendCommand(replCommandArgs.command, replCommandArgs.args); + terminalService.sendCommand(replCommandArgs.command, replCommandArgs.args); // Give python repl time to start before we start sending text. setTimeout(() => resolve(true), 1000); }); + this.disposables.push( + terminalService.onDidCloseTerminal(() => { + this.replActive = undefined; + }), + ); await this.replActive; } @@ -76,19 +81,12 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService { public async getExecuteFileArgs(resource?: Uri, executeArgs: string[] = []): Promise { return this.getExecutableInfo(resource, executeArgs); } - private getTerminalService(resource?: Uri): ITerminalService { - if (!this._terminalService) { - this._terminalService = this.terminalServiceFactory.getTerminalService({ - resource, - title: this.terminalTitle, - }); - this.disposables.push( - this._terminalService.onDidCloseTerminal(() => { - this.replActive = undefined; - }), - ); - } - return this._terminalService; + private getTerminalService(resource: Resource, options?: { newTerminalPerFile: boolean }): ITerminalService { + return this.terminalServiceFactory.getTerminalService({ + resource, + title: this.terminalTitle, + newTerminalPerFile: options?.newTerminalPerFile, + }); } private async setCwdForFileExecution(file: Uri) { const pythonSettings = this.configurationService.getSettings(file); diff --git a/src/client/terminals/types.ts b/src/client/terminals/types.ts index cf31f4ef1dd0..47ac16d9e08b 100644 --- a/src/client/terminals/types.ts +++ b/src/client/terminals/types.ts @@ -8,7 +8,7 @@ export const ICodeExecutionService = Symbol('ICodeExecutionService'); export interface ICodeExecutionService { execute(code: string, resource?: Uri): Promise; - executeFile(file: Uri): Promise; + executeFile(file: Uri, options?: { newTerminalPerFile: boolean }): Promise; initializeRepl(resource?: Uri): Promise; } diff --git a/src/test/common/terminals/factory.unit.test.ts b/src/test/common/terminals/factory.unit.test.ts index ef6b7d8f5b0f..5ad2da8e793a 100644 --- a/src/test/common/terminals/factory.unit.test.ts +++ b/src/test/common/terminals/factory.unit.test.ts @@ -105,7 +105,7 @@ suite('Terminal Service Factory', () => { expect(notSameAsThirdInstance).to.not.equal(true, 'Instances are the same'); }); - test('Ensure same terminal is returned when using resources from the same workspace', () => { + test('Ensure same terminal is returned when using different resources from the same workspace', () => { const file1A = Uri.file('1a'); const file2A = Uri.file('2a'); const fileB = Uri.file('b'); @@ -140,4 +140,49 @@ suite('Terminal Service Factory', () => { 'Instances should be different for different workspaces', ); }); + + test('When `newTerminalPerFile` is true, ensure different terminal is returned when using different resources from the same workspace', () => { + const file1A = Uri.file('1a'); + const file2A = Uri.file('2a'); + const fileB = Uri.file('b'); + const workspaceUriA = Uri.file('A'); + const workspaceUriB = Uri.file('B'); + const workspaceFolderA = TypeMoq.Mock.ofType(); + workspaceFolderA.setup((w) => w.uri).returns(() => workspaceUriA); + const workspaceFolderB = TypeMoq.Mock.ofType(); + workspaceFolderB.setup((w) => w.uri).returns(() => workspaceUriB); + + workspaceService + .setup((w) => w.getWorkspaceFolder(TypeMoq.It.isValue(file1A))) + .returns(() => workspaceFolderA.object); + workspaceService + .setup((w) => w.getWorkspaceFolder(TypeMoq.It.isValue(file2A))) + .returns(() => workspaceFolderA.object); + workspaceService + .setup((w) => w.getWorkspaceFolder(TypeMoq.It.isValue(fileB))) + .returns(() => workspaceFolderB.object); + + const terminalForFile1A = factory.getTerminalService({ + resource: file1A, + newTerminalPerFile: true, + }) as SynchronousTerminalService; + const terminalForFile2A = factory.getTerminalService({ + resource: file2A, + newTerminalPerFile: true, + }) as SynchronousTerminalService; + const terminalForFileB = factory.getTerminalService({ + resource: fileB, + newTerminalPerFile: true, + }) as SynchronousTerminalService; + + const terminalsAreSameForWorkspaceA = terminalForFile1A.terminalService === terminalForFile2A.terminalService; + expect(terminalsAreSameForWorkspaceA).to.equal(false, 'Instances are the same for Workspace A'); + + const terminalsForWorkspaceABAreDifferent = + terminalForFile1A.terminalService === terminalForFileB.terminalService; + expect(terminalsForWorkspaceABAreDifferent).to.equal( + false, + 'Instances should be different for different workspaces', + ); + }); }); diff --git a/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts b/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts index 3676834873a0..30f95c94d217 100644 --- a/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts +++ b/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts @@ -77,12 +77,15 @@ suite('Terminal - Code Execution Manager', () => { executionManager.registerCommands(); const sorted = registered.sort(); - expect(sorted).to.deep.equal([ - Commands.Exec_In_Terminal, - Commands.Exec_In_Terminal_Icon, - Commands.Exec_Selection_In_Django_Shell, - Commands.Exec_Selection_In_Terminal, - ]); + expect(sorted).to.deep.equal( + [ + Commands.Exec_In_Separate_Terminal, + Commands.Exec_In_Terminal, + Commands.Exec_In_Terminal_Icon, + Commands.Exec_Selection_In_Django_Shell, + Commands.Exec_Selection_In_Terminal, + ].sort(), + ); }); test('Ensure executeFileInterTerminal will do nothing if no file is avialble', async () => { @@ -135,7 +138,10 @@ suite('Terminal - Code Execution Manager', () => { const fileToExecute = Uri.file('x'); await commandHandler!(fileToExecute); helper.verify(async (h) => h.getFileToExecute(), TypeMoq.Times.never()); - executionService.verify(async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute)), TypeMoq.Times.once()); + executionService.verify( + async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute), TypeMoq.It.isAny()), + TypeMoq.Times.once(), + ); }); test('Ensure executeFileInterTerminal will use active file', async () => { @@ -164,7 +170,10 @@ suite('Terminal - Code Execution Manager', () => { .returns(() => executionService.object); await commandHandler!(fileToExecute); - executionService.verify(async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute)), TypeMoq.Times.once()); + executionService.verify( + async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute), TypeMoq.It.isAny()), + TypeMoq.Times.once(), + ); }); async function testExecutionOfSelectionWithoutAnyActiveDocument(commandId: string, executionSericeId: string) { diff --git a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts index 1f33b619fad0..9523f35a1040 100644 --- a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts +++ b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts @@ -25,7 +25,7 @@ import { TerminalCodeExecutionProvider } from '../../../client/terminals/codeExe import { ICodeExecutionService } from '../../../client/terminals/types'; import { PYTHON_PATH } from '../../common'; import * as sinon from 'sinon'; -import assert from 'assert'; +import { assert } from 'chai'; import { PythonEnvironment } from '../../../client/pythonEnvironments/info'; import { IInterpreterService } from '../../../client/interpreter/contracts'; @@ -390,6 +390,7 @@ suite('Terminal - Code Execution', () => { const env = await createCondaEnv(condaEnv, procService.object, fileSystem.object); if (!env) { assert(false, 'Should not be undefined for conda version 4.9.0'); + return; } const procs = createPythonProcessService(procService.object, env); const condaExecutionService = { @@ -509,6 +510,7 @@ suite('Terminal - Code Execution', () => { const env = await createCondaEnv(condaEnv, procService.object, fileSystem.object); if (!env) { assert(false, 'Should not be undefined for conda version 4.9.0'); + return; } const procs = createPythonProcessService(procService.object, env); const condaExecutionService = { @@ -650,6 +652,31 @@ suite('Terminal - Code Execution', () => { await executor.execute('cmd2'); terminalService.verify(async (t) => t.sendText('cmd2'), TypeMoq.Times.once()); }); + + test('Ensure code is sent to the same terminal for a particular resource', async () => { + const resource = Uri.file('a'); + terminalFactory.reset(); + terminalFactory + .setup((f) => f.getTerminalService(TypeMoq.It.isAny())) + .callback((options: TerminalCreationOptions) => { + assert.deepEqual(options.resource, resource); + }) + .returns(() => terminalService.object); + + const pythonPath = 'usr/bin/python1234'; + const terminalArgs = ['-a', 'b', 'c']; + platform.setup((p) => p.isWindows).returns(() => false); + interpreterService + .setup((s) => s.getActiveInterpreter(TypeMoq.It.isAny())) + .returns(() => Promise.resolve(({ path: pythonPath } as unknown) as PythonEnvironment)); + terminalSettings.setup((t) => t.launchArgs).returns(() => terminalArgs); + + await executor.execute('cmd1', resource); + terminalService.verify(async (t) => t.sendText('cmd1'), TypeMoq.Times.once()); + + await executor.execute('cmd2', resource); + terminalService.verify(async (t) => t.sendText('cmd2'), TypeMoq.Times.once()); + }); }); }); });