Skip to content

Commit

Permalink
Revert: Run in dedicated terminal feature due to regressions (#21427)
Browse files Browse the repository at this point in the history
For #21393
  • Loading branch information
karthiknadig authored Jun 13, 2023
1 parent ddc44f2 commit 13fb281
Show file tree
Hide file tree
Showing 11 changed files with 55 additions and 159 deletions.
19 changes: 0 additions & 19 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -305,12 +305,6 @@
"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 @@ -1641,13 +1635,6 @@
"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 @@ -1806,12 +1793,6 @@
"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: 0 additions & 1 deletion package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
"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: 0 additions & 1 deletion src/client/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ 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: 5 additions & 16 deletions src/client/common/terminal/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

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 @@ -24,17 +23,13 @@ export class TerminalServiceFactory implements ITerminalServiceFactory {
) {
this.terminalServices = new Map<string, TerminalService>();
}
public getTerminalService(options: TerminalCreationOptions & { newTerminalPerFile?: boolean }): ITerminalService {
public getTerminalService(options: TerminalCreationOptions): ITerminalService {
const resource = options?.resource;
const title = options?.title;
let terminalTitle = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python';
const terminalTitle = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python';
const interpreter = options?.interpreter;
const id = this.getTerminalId(terminalTitle, resource, interpreter, options.newTerminalPerFile);
const id = this.getTerminalId(terminalTitle, resource, interpreter);
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 @@ -51,19 +46,13 @@ 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,
newTerminalPerFile?: boolean,
): string {
private getTerminalId(title: string, resource?: Uri, interpreter?: PythonEnvironment): string {
if (!resource && !interpreter) {
return title;
}
const workspaceFolder = this.serviceContainer
.get<IWorkspaceService>(IWorkspaceService)
.getWorkspaceFolder(resource || undefined);
const fileId = resource && newTerminalPerFile ? resource.fsPath : '';
return `${title}:${workspaceFolder?.uri.fsPath || ''}:${interpreter?.path}:${fileId}`;
return `${title}:${workspaceFolder?.uri.fsPath || ''}:${interpreter?.path}`;
}
}
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 & { newTerminalPerFile?: boolean }): ITerminalService;
getTerminalService(options: TerminalCreationOptions): ITerminalService;
createTerminalService(resource?: Uri, title?: string): ITerminalService;
}

Expand Down
6 changes: 0 additions & 6 deletions src/client/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -825,12 +825,6 @@ 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: 21 additions & 35 deletions src/client/terminals/codeExecution/codeExecutionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,31 +36,25 @@ export class CodeExecutionManager implements ICodeExecutionManager {
}

public registerCommands() {
[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,
[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');
})
.then(() => {
if (this.shouldTerminalFocusOnStart(file))
this.commandManager.executeCommand('workbench.action.terminal.focus');
})
.catch((ex) => traceError('Failed to execute file in terminal', ex));
}),
);
},
);
.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 @@ -93,16 +87,8 @@ export class CodeExecutionManager implements ICodeExecutionManager {
),
);
}
private async executeFileInTerminal(
file: Resource,
trigger: 'command' | 'icon',
options?: { newTerminalPerFile: boolean },
): Promise<void> {
sendTelemetryEvent(EventName.EXECUTION_CODE, undefined, {
scope: 'file',
trigger,
newTerminalPerFile: options?.newTerminalPerFile,
});
private async executeFileInTerminal(file: Resource, trigger: 'command' | 'icon') {
sendTelemetryEvent(EventName.EXECUTION_CODE, undefined, { scope: 'file', trigger });
const codeExecutionHelper = this.serviceContainer.get<ICodeExecutionHelper>(ICodeExecutionHelper);
file = file instanceof Uri ? file : undefined;
let fileToExecute = file ? file : await codeExecutionHelper.getFileToExecute();
Expand All @@ -124,7 +110,7 @@ export class CodeExecutionManager implements ICodeExecutionManager {
}

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

@captureTelemetry(EventName.EXECUTION_CODE, { scope: 'selection' }, false)
Expand Down
34 changes: 18 additions & 16 deletions src/client/terminals/codeExecution/terminalCodeExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ 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 @@ -29,13 +30,13 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
@inject(IInterpreterService) protected readonly interpreterService: IInterpreterService,
) {}

public async executeFile(file: Uri, options?: { newTerminalPerFile: boolean }) {
public async executeFile(file: Uri) {
await this.setCwdForFileExecution(file);
const { command, args } = await this.getExecuteFileArgs(file, [
file.fsPath.fileToCommandArgumentForPythonExt(),
]);

await this.getTerminalService(file, options).sendCommand(command, args);
await this.getTerminalService(file).sendCommand(command, args);
}

public async execute(code: string, resource?: Uri): Promise<void> {
Expand All @@ -47,23 +48,17 @@ 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 terminalService.show();
await this._terminalService.show();
return;
}
this.replActive = new Promise<boolean>(async (resolve) => {
const replCommandArgs = await this.getExecutableInfo(resource);
terminalService.sendCommand(replCommandArgs.command, replCommandArgs.args);
await this.getTerminalService(resource).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 @@ -81,12 +76,19 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
public async getExecuteFileArgs(resource?: Uri, executeArgs: string[] = []): Promise<PythonExecInfo> {
return this.getExecutableInfo(resource, executeArgs);
}
private getTerminalService(resource?: Uri, options?: { newTerminalPerFile: boolean }): ITerminalService {
return this.terminalServiceFactory.getTerminalService({
resource,
title: this.terminalTitle,
newTerminalPerFile: options?.newTerminalPerFile,
});
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 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, options?: { newTerminalPerFile: boolean }): Promise<void>;
executeFile(file: Uri): Promise<void>;
initializeRepl(resource?: Uri): Promise<void>;
}

Expand Down
47 changes: 1 addition & 46 deletions 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 different resources from the same workspace', () => {
test('Ensure same terminal is returned when using 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,49 +140,4 @@ 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: 8 additions & 17 deletions src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,12 @@ suite('Terminal - Code Execution Manager', () => {
executionManager.registerCommands();

const sorted = registered.sort();
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(),
);
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,
]);
});

test('Ensure executeFileInterTerminal will do nothing if no file is avialble', async () => {
Expand Down Expand Up @@ -138,10 +135,7 @@ 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.It.isAny()),
TypeMoq.Times.once(),
);
executionService.verify(async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute)), TypeMoq.Times.once());
});

test('Ensure executeFileInterTerminal will use active file', async () => {
Expand Down Expand Up @@ -170,10 +164,7 @@ suite('Terminal - Code Execution Manager', () => {
.returns(() => executionService.object);

await commandHandler!(fileToExecute);
executionService.verify(
async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute), TypeMoq.It.isAny()),
TypeMoq.Times.once(),
);
executionService.verify(async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute)), TypeMoq.Times.once());
});

async function testExecutionOfSelectionWithoutAnyActiveDocument(commandId: string, executionSericeId: string) {
Expand Down

0 comments on commit 13fb281

Please sign in to comment.