Revert: Run in dedicated terminal feature due to regressions (#21427)

For #21393
microsoft · Jun 13, 2023 · 13fb281 · 13fb281
1 parent ddc44f2
commit 13fb281
Show file tree

Hide file tree

Showing 11 changed files with 55 additions and 159 deletions.
diff --git a/package.json b/package.json
@@ -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",
@@ -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",
@@ -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",

diff --git a/package.nls.json b/package.nls.json
@@ -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",

diff --git a/src/client/common/constants.ts b/src/client/common/constants.ts
@@ -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';

diff --git a/src/client/common/terminal/factory.ts b/src/client/common/terminal/factory.ts
@@ -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';
@@ -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);
         }
@@ -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}`;
     }
 }
diff --git 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 & { newTerminalPerFile?: boolean }): ITerminalService;
+    getTerminalService(options: TerminalCreationOptions): ITerminalService;
     createTerminalService(resource?: Uri, title?: string): ITerminalService;
 }
 

diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts
@@ -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.

diff --git a/src/client/terminals/codeExecution/codeExecutionManager.ts b/src/client/terminals/codeExecution/codeExecutionManager.ts
@@ -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);
@@ -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();
@@ -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)

diff --git a/src/client/terminals/codeExecution/terminalCodeExecution.ts b/src/client/terminals/codeExecution/terminalCodeExecution.ts
@@ -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,
@@ -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> {
@@ -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;
     }
@@ -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);

diff --git 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<void>;
-    executeFile(file: Uri, options?: { newTerminalPerFile: boolean }): Promise<void>;
+    executeFile(file: Uri): Promise<void>;
     initializeRepl(resource?: Uri): Promise<void>;
 }
 

diff --git 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 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');
@@ -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',
-        );
-    });
 });
diff --git a/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts b/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts
@@ -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 () => {
@@ -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 () => {
@@ -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) {