From 31725fbd5a73f4897b9a4f9ef9136d4b2ffeb9a3 Mon Sep 17 00:00:00 2001 From: Remi Schnekenburger Date: Tue, 28 Nov 2023 10:45:27 +0100 Subject: [PATCH] Address review comments - remove indirection from TerminalService - change shellPath as getter rather than property - use instanceof instead of is() --- .../plugin-ext/src/main/browser/terminal-main.ts | 5 ++++- .../terminal/src/browser/base/terminal-service.ts | 2 -- .../terminal/src/browser/shell-terminal-profile.ts | 14 +++++--------- .../src/browser/terminal-frontend-contribution.ts | 7 ------- .../src/browser/terminal-profile-service.ts | 4 ++-- 5 files changed, 11 insertions(+), 21 deletions(-) diff --git a/packages/plugin-ext/src/main/browser/terminal-main.ts b/packages/plugin-ext/src/main/browser/terminal-main.ts index 6ad478c6f7e02..fe1876c7bf8b4 100644 --- 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 index 785a9e5c188d4..02f8e17ec4e68 100644 --- 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; - readonly onDidChangeShell: Event - readonly onDidCreateTerminal: Event; readonly currentTerminal: TerminalWidget | undefined; diff --git a/packages/terminal/src/browser/shell-terminal-profile.ts b/packages/terminal/src/browser/shell-terminal-profile.ts index 962f1ffbae2ab..8d320320b72d6 100644 --- 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 { 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 index 10f362694ecbd..2263f1402e692 100644 --- 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(); readonly onDidChangeCurrentTerminal: Event = this.onDidChangeCurrentTerminalEmitter.event; - protected readonly onDidChangeShellEmitter = new Emitter(); - readonly onDidChangeShell: Event = 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 { diff --git a/packages/terminal/src/browser/terminal-profile-service.ts b/packages/terminal/src/browser/terminal-profile-service.ts index f39dfa06d9c70..2a3f9b4570835 100644 --- 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(''); }