Skip to content

Commit

Permalink
Revert "Revert: Run in dedicated terminal feature due to regressions (#…
Browse files Browse the repository at this point in the history
…21418)"

This reverts commit cb859ca.
  • Loading branch information
Kartik Raj authored Jul 18, 2023
1 parent c144200 commit 3ed0c76
Show file tree
Hide file tree
Showing 11 changed files with 159 additions and 55 deletions.
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
34 changes: 16 additions & 18 deletions src/client/terminals/codeExecution/terminalCodeExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,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<void> {
Expand All @@ -48,17 +47,23 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
await this.getTerminalService(resource).sendText(code);
}
public async initializeRepl(resource?: Uri) {
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?: Uri, 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',
);
});
});
25 changes: 17 additions & 8 deletions src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 3ed0c76

Please sign in to comment.