Address review comments

- remove indirection from TerminalService - change shellPath as getter rather than property - use instanceof instead of is()
eclipse-theia · Nov 28, 2023 · 31725fb · 31725fb
1 parent 3de96bb
commit 31725fb
Show file tree

Hide file tree

Showing 5 changed files with 11 additions and 21 deletions.
diff --git a/packages/plugin-ext/src/main/browser/terminal-main.ts b/packages/plugin-ext/src/main/browser/terminal-main.ts
@@ -18,6 +18,7 @@ import { interfaces } from '@theia/core/shared/inversify';
 import { ApplicationShell, WidgetOpenerOptions } from '@theia/core/lib/browser';
 import { TerminalEditorLocationOptions, TerminalOptions } from '@theia/plugin';
 import { TerminalLocation, TerminalWidget } from '@theia/terminal/lib/browser/base/terminal-widget';
+import { TerminalProfileService } from '@theia/terminal/lib/browser/terminal-profile-service';
 import { TerminalService } from '@theia/terminal/lib/browser/base/terminal-service';
 import { TerminalServiceMain, TerminalServiceExt, MAIN_RPC_CONTEXT } from '../../common/plugin-api-rpc';
 import { RPCProtocol } from '../../common/rpc-protocol';
@@ -36,6 +37,7 @@ import { HostedPluginSupport } from '../../hosted/browser/hosted-plugin';
 export class TerminalServiceMainImpl implements TerminalServiceMain, TerminalLinkProvider, Disposable {
 
     private readonly terminals: TerminalService;
+    private readonly terminalProfileService: TerminalProfileService;
     private readonly pluginTerminalRegistry: PluginTerminalRegistry;
     private readonly hostedPluginSupport: HostedPluginSupport;
     private readonly shell: ApplicationShell;
@@ -47,6 +49,7 @@ export class TerminalServiceMainImpl implements TerminalServiceMain, TerminalLin
 
     constructor(rpc: RPCProtocol, container: interfaces.Container) {
         this.terminals = container.get(TerminalService);
+        this.terminalProfileService = container.get(TerminalProfileService);
         this.pluginTerminalRegistry = container.get(PluginTerminalRegistry);
         this.hostedPluginSupport = container.get(HostedPluginSupport);
         this.shell = container.get(ApplicationShell);
@@ -65,7 +68,7 @@ export class TerminalServiceMainImpl implements TerminalServiceMain, TerminalLin
 
         container.bind(TerminalLinkProvider).toDynamicValue(() => this);
 
-        this.toDispose.push(this.terminals.onDidChangeShell(shell => {
+        this.toDispose.push(this.terminalProfileService.onDidChangeDefaultShell(shell => {
             this.extProxy.$setShell(shell);
         }));
     }

diff --git a/packages/terminal/src/browser/base/terminal-service.ts b/packages/terminal/src/browser/base/terminal-service.ts
@@ -50,8 +50,6 @@ export interface TerminalService {
      */
     getDefaultShell(): Promise<string>;
 
-    readonly onDidChangeShell: Event<string>
-
     readonly onDidCreateTerminal: Event<TerminalWidget>;
 
     readonly currentTerminal: TerminalWidget | undefined;

diff --git a/packages/terminal/src/browser/shell-terminal-profile.ts b/packages/terminal/src/browser/shell-terminal-profile.ts
@@ -14,16 +14,18 @@
 // SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
 // *****************************************************************************
 
-import { URI, isObject } from '@theia/core';
+import { URI } from '@theia/core';
 import { TerminalService } from './base/terminal-service';
 import { TerminalWidget, TerminalWidgetOptions } from './base/terminal-widget';
 import { TerminalProfile } from './terminal-profile-service';
 
 export class ShellTerminalProfile implements TerminalProfile {
 
-    readonly shellPath: string;
+    get shellPath(): string | undefined {
+        return this.options.shellPath;
+    }
 
-    constructor(protected readonly terminalService: TerminalService, protected readonly options: TerminalWidgetOptions) { this.shellPath = options.shellPath!; }
+    constructor(protected readonly terminalService: TerminalService, protected readonly options: TerminalWidgetOptions) { }
 
     async start(): Promise<TerminalWidget> {
         const widget = await this.terminalService.newTerminal(this.options);
@@ -41,9 +43,3 @@ export class ShellTerminalProfile implements TerminalProfile {
         return new ShellTerminalProfile(this.terminalService, { ...this.options, ...options });
     }
 }
-
-export namespace ShellTerminalProfile {
-    export function is(candidate: unknown): candidate is ShellTerminalProfile {
-        return isObject(candidate) && 'shellPath' in candidate;
-    }
-}
diff --git a/packages/terminal/src/browser/terminal-frontend-contribution.ts b/packages/terminal/src/browser/terminal-frontend-contribution.ts
@@ -220,9 +220,6 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu
     protected readonly onDidChangeCurrentTerminalEmitter = new Emitter<TerminalWidget | undefined>();
     readonly onDidChangeCurrentTerminal: Event<TerminalWidget | undefined> = this.onDidChangeCurrentTerminalEmitter.event;
 
-    protected readonly onDidChangeShellEmitter = new Emitter<string>();
-    readonly onDidChangeShell: Event<string> = this.onDidChangeShellEmitter.event;
-
     @inject(ContextKeyService)
     protected readonly contextKeyService: ContextKeyService;
 
@@ -256,9 +253,6 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu
                 }
             });
         });
-        this.profileService.onDidChangeDefaultShell(async shell => {
-            this.onDidChangeShellEmitter.fire(shell !== '' ? shell : await this.getDefaultShell());
-        });
     }
 
     get terminalHideSearch(): boolean {
@@ -1027,7 +1021,6 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu
         }
 
         this.preferenceService.set(`terminal.integrated.defaultProfile.${OS.backend.type().toLowerCase()}`, result[0], PreferenceScope.User);
-        this.profileService.setDefaultProfile(result[0]);
     }
 
     protected async openActiveWorkspaceTerminal(options?: ApplicationShell.WidgetOptions): Promise<void> {

diff --git a/packages/terminal/src/browser/terminal-profile-service.ts b/packages/terminal/src/browser/terminal-profile-service.ts
@@ -149,8 +149,8 @@ export class DefaultTerminalProfileService implements TerminalProfileService {
         }
         this.defaultProfileIndex = this.order.indexOf(id);
 
-        if (ShellTerminalProfile.is(profile) && profile.shellPath) {
-            this.onDidChangeDefaultShellEmitter.fire(profile.shellPath!);
+        if (profile instanceof ShellTerminalProfile && profile.shellPath) {
+            this.onDidChangeDefaultShellEmitter.fire(profile.shellPath);
         } else {
             this.onDidChangeDefaultShellEmitter.fire('');
         }