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

Bring back feature to Run Python file in dedicated terminal #21656

Merged
merged 4 commits into from
Jul 18, 2023
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
19 changes: 19 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions src/client/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
21 changes: 16 additions & 5 deletions src/client/common/terminal/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -23,13 +24,17 @@ export class TerminalServiceFactory implements ITerminalServiceFactory {
) {
this.terminalServices = new Map<string, TerminalService>();
}
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);
}
Expand All @@ -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>(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}`;
}
}
2 changes: 1 addition & 1 deletion src/client/common/terminal/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
6 changes: 6 additions & 0 deletions src/client/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
56 changes: 35 additions & 21 deletions src/client/terminals/codeExecution/codeExecutionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>(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>(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>(IInterpreterService);
Expand Down Expand Up @@ -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<void> {
sendTelemetryEvent(EventName.EXECUTION_CODE, undefined, {
scope: 'file',
trigger,
newTerminalPerFile: options?.newTerminalPerFile,
});
const codeExecutionHelper = this.serviceContainer.get<ICodeExecutionHelper>(ICodeExecutionHelper);
file = file instanceof Uri ? file : undefined;
let fileToExecute = file ? file : await codeExecutionHelper.getFileToExecute();
Expand All @@ -110,7 +124,7 @@ export class CodeExecutionManager implements ICodeExecutionManager {
}

const executionService = this.serviceContainer.get<ICodeExecutionService>(ICodeExecutionService, 'standard');
await executionService.executeFile(fileToExecute);
await executionService.executeFile(fileToExecute, options);
}

@captureTelemetry(EventName.EXECUTION_CODE, { scope: 'selection' }, false)
Expand Down
40 changes: 19 additions & 21 deletions src/client/terminals/codeExecution/terminalCodeExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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<boolean>;
constructor(
@inject(ITerminalServiceFactory) protected readonly terminalServiceFactory: ITerminalServiceFactory,
Expand All @@ -30,35 +29,41 @@ 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<void> {
if (!code || code.trim().length === 0) {
return;
}

await this.initializeRepl();
await this.initializeRepl(resource);
Copy link
Author

@karrtikr karrtikr Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was that we missed to pass resource here, hence it initialized/created the REPL for a different resource. When sending the code, it noticed there was no REPL and created another terminal, leading to 2 terminals.

Made this field mandatory and added tests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is a resource? I see its a Uri, but is it an identifier of what we are executing (such as specific line of a file)?

Copy link
Author

@karrtikr karrtikr Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that's exactly what it is, it carries the path to the file which is to be executed. Initialize REPL creates the REPL for that particular file, then the following line sends the code which is to be executed in that particular file.

Issue was that no REPL was created for the file whose code was sent.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... Little lost on: "When sending the code, it noticed there was no REPL and created another terminal, leading to 2 terminals." I understand there was no REPL created for the specific file, but it created another terminal, not a brand new REPL?

Is this because one REPL has to be instantiated from a one instance of terminal?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it created another terminal, not a brand new REPL?

Right. Brand new REPLs are only created by initializeRepl(). "Send code" section is only responsible for sending the command in whatever REPL/terminal is currently opened for the file. If it notices that nothing is opened, it creates a terminal, it cannot create REPL.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see I see! Thank you for explaining!

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<boolean>(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;
}
Expand All @@ -76,19 +81,12 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
public async getExecuteFileArgs(resource?: Uri, executeArgs: string[] = []): Promise<PythonExecInfo> {
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);
Expand Down
2 changes: 1 addition & 1 deletion src/client/terminals/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export const ICodeExecutionService = Symbol('ICodeExecutionService');

export interface ICodeExecutionService {
execute(code: string, resource?: Uri): Promise<void>;
executeFile(file: Uri): Promise<void>;
executeFile(file: Uri, options?: { newTerminalPerFile: boolean }): Promise<void>;
initializeRepl(resource?: Uri): Promise<void>;
}

Expand Down
47 changes: 46 additions & 1 deletion src/test/common/terminals/factory.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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<WorkspaceFolder>();
workspaceFolderA.setup((w) => w.uri).returns(() => workspaceUriA);
const workspaceFolderB = TypeMoq.Mock.ofType<WorkspaceFolder>();
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',
);
});
});
Loading