From 327690a3cb97e20b9698ed6cbd822936a976e0e9 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 17 May 2021 07:25:20 -0700 Subject: [PATCH 1/3] Unsplit terminals Fixes #121278 --- .../contrib/terminal/browser/terminal.ts | 4 ++ .../terminal/browser/terminalActions.ts | 15 +++++++ .../contrib/terminal/browser/terminalGroup.ts | 45 +++++++++++++++++-- .../terminal/browser/terminalInstance.ts | 17 ++++++- .../terminal/browser/terminalService.ts | 18 ++++++++ .../contrib/terminal/common/terminal.ts | 1 + 6 files changed, 95 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.ts b/src/vs/workbench/contrib/terminal/browser/terminal.ts index e09e62b186868..b852fc3ebe58e 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.ts @@ -72,6 +72,7 @@ export interface ITerminalGroup { resizePanes(relativeSizes: number[]): void; setActiveInstanceByIndex(index: number): void; attachToElement(element: HTMLElement): void; + removeInstance(instance: ITerminalInstance): void; setVisible(visible: boolean): void; layout(width: number, height: number): void; addDisposable(disposable: IDisposable): void; @@ -143,6 +144,7 @@ export interface ITerminalService { getActiveOrCreateInstance(): ITerminalInstance; splitInstance(instance: ITerminalInstance, shell?: IShellLaunchConfig, cwd?: string | URI): ITerminalInstance | null; splitInstance(instance: ITerminalInstance, profile: ITerminalProfile): ITerminalInstance | null; + unsplitInstance(instance: ITerminalInstance): void; /** * Perform an action with the active terminal instance, if the terminal does @@ -524,6 +526,8 @@ export interface ITerminalInstance { */ attachToElement(container: HTMLElement): Promise | void; + detachFromElement(): void; + /** * Configure the dimensions of the terminal instance. * diff --git a/src/vs/workbench/contrib/terminal/browser/terminalActions.ts b/src/vs/workbench/contrib/terminal/browser/terminalActions.ts index afe0da7fd0ee1..6c1b44b85baad 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalActions.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalActions.ts @@ -1395,6 +1395,21 @@ export function registerTerminalActions() { return undefined; } }); + registerAction2(class extends Action2 { + constructor() { + super({ + id: TerminalCommandId.Unsplit, + title: { value: localize('workbench.action.terminal.unsplit', "Unsplit Terminal"), original: 'Unsplit Terminal' }, + f1: true, + category, + precondition: KEYBINDING_CONTEXT_TERMINAL_PROCESS_SUPPORTED + }); + } + async run(accessor: ServicesAccessor) { + const terminalService = accessor.get(ITerminalService); + await terminalService.doWithActiveInstance(async t => terminalService.unsplitInstance(t)); + } + }); registerAction2(class extends Action2 { constructor() { super({ diff --git a/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts b/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts index f8d63f6930351..7a75fdb2c3a3c 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts @@ -5,7 +5,7 @@ import { TERMINAL_VIEW_ID } from 'vs/workbench/contrib/terminal/common/terminal'; import { Event, Emitter } from 'vs/base/common/event'; -import { IDisposable, Disposable, DisposableStore } from 'vs/base/common/lifecycle'; +import { IDisposable, Disposable, DisposableStore, dispose } from 'vs/base/common/lifecycle'; import { SplitView, Orientation, IView, Sizing } from 'vs/base/browser/ui/splitview/splitview'; import { IWorkbenchLayoutService, Parts, Position } from 'vs/workbench/services/layout/browser/layoutService'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; @@ -233,6 +233,7 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { private _groupElement: HTMLElement | undefined; private _panelPosition: Position = Position.BOTTOM; private _terminalLocation: ViewContainerLocation = ViewContainerLocation.Panel; + private _instanceDisposables: Map = new Map(); private _activeInstanceIndex: number; private _isVisible: boolean = false; @@ -317,8 +318,10 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { } private _initInstanceListeners(instance: ITerminalInstance): void { - instance.addDisposable(instance.onDisposed(instance => this._onInstanceDisposed(instance))); - instance.addDisposable(instance.onFocused(instance => this._setActiveInstance(instance))); + this._instanceDisposables.set(instance.instanceId, [ + instance.onDisposed(instance => this._onInstanceDisposed(instance)), + instance.onFocused(instance => this._setActiveInstance(instance)) + ]); } private _onInstanceDisposed(instance: ITerminalInstance): void { @@ -408,6 +411,42 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { this.setVisible(this._isVisible); } + removeInstance(instance: ITerminalInstance): void { + const index = this._terminalInstances.indexOf(instance); + if (index === -1) { + return; + } + + const wasActiveInstance = instance === this.activeInstance; + this._terminalInstances.splice(index, 1); + instance.detachFromElement(); + + // TODO: Share code with onInstanceDisposed + // Adjust focus if the instance was active + if (wasActiveInstance && this._terminalInstances.length > 0) { + const newIndex = index < this._terminalInstances.length ? index : this._terminalInstances.length - 1; + this.setActiveInstanceByIndex(newIndex); + // TODO: Only focus the new instance if the group had focus? + if (this.activeInstance) { + this.activeInstance.focus(true); + } + } else if (index < this._activeInstanceIndex) { + // Adjust active instance index if needed + this._activeInstanceIndex--; + } + + this._splitPaneContainer?.remove(instance); + + // Dispose listeners + const disposables = this._instanceDisposables.get(instance.instanceId); + if (disposables) { + dispose(disposables); + this._instanceDisposables.delete(instance.instanceId); + } + + this._onInstancesChanged.fire(); + } + get title(): string { if (this._terminalInstances.length === 0) { // Normally consumers should not call into title at all after the group is disposed but diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index 8591578a436db..2a40774f4946b 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -604,6 +604,16 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._container.appendChild(this._wrapperElement); } + detachFromElement(): void { + if (!this._wrapperElement) { + return; + } + this._container?.removeChild(this._wrapperElement); + this._wrapperElement = undefined; + + // TODO: Don't allow reattach init of xterm + } + private async _attachToElement(container: HTMLElement): Promise { if (this._wrapperElement) { throw new Error('The terminal instance has already been attached to a container'); @@ -900,7 +910,6 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { override dispose(immediate?: boolean): void { this._logService.trace(`terminalInstance#dispose (instanceId: ${this.instanceId})`); - dispose(this._linkManager); this._linkManager = undefined; dispose(this._commandTrackerAddon); @@ -921,7 +930,11 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { if (this._xterm) { const buffer = this._xterm.buffer; this._sendLineData(buffer.active, buffer.active.baseY + buffer.active.cursorY); - this._xterm.dispose(); + try { + this._xterm.dispose(); + } catch { + // No op, exception may get thrown from removeChild + } } if (this._pressAnyKeyToCloseListener) { diff --git a/src/vs/workbench/contrib/terminal/browser/terminalService.ts b/src/vs/workbench/contrib/terminal/browser/terminalService.ts index a91032a284afd..86e6c28143eb2 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalService.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalService.ts @@ -621,6 +621,23 @@ export class TerminalService implements ITerminalService { return instance; } + unsplitInstance(instance: ITerminalInstance): void { + const oldGroup = this.getGroupForInstance(instance); + if (!oldGroup || oldGroup.terminalInstances.length < 2) { + return; + } + + oldGroup.removeInstance(instance); + + const newGroup = this._instantiationService.createInstance(TerminalGroup, this._terminalContainer, instance); + newGroup.onPanelOrientationChanged((orientation) => this._onPanelOrientationChanged.fire(orientation)); + this._terminalGroups.push(newGroup); + + newGroup.addDisposable(newGroup.onDisposed(this._onGroupDisposed.fire, this._onGroupDisposed)); + newGroup.addDisposable(newGroup.onInstancesChanged(this._onInstancesChanged.fire, this._onInstancesChanged)); + this._onInstancesChanged.fire(); + } + protected _initInstanceListeners(instance: ITerminalInstance): void { instance.addDisposable(instance.onDisposed(this._onInstanceDisposed.fire, this._onInstanceDisposed)); instance.addDisposable(instance.onTitleChanged(this._onInstanceTitleChanged.fire, this._onInstanceTitleChanged)); @@ -969,6 +986,7 @@ export class TerminalService implements ITerminalService { instance.shellLaunchConfig.hideFromUser = false; const terminalGroup = this._instantiationService.createInstance(TerminalGroup, this._terminalContainer, instance); this._terminalGroups.push(terminalGroup); + terminalGroup.onPanelOrientationChanged((orientation) => this._onPanelOrientationChanged.fire(orientation)); terminalGroup.addDisposable(terminalGroup.onDisposed(this._onGroupDisposed.fire, this._onGroupDisposed)); terminalGroup.addDisposable(terminalGroup.onInstancesChanged(this._onInstancesChanged.fire, this._onInstancesChanged)); if (this.terminalInstances.length === 1) { diff --git a/src/vs/workbench/contrib/terminal/common/terminal.ts b/src/vs/workbench/contrib/terminal/common/terminal.ts index 2db99f8b26908..6ec4d1f8ab704 100644 --- a/src/vs/workbench/contrib/terminal/common/terminal.ts +++ b/src/vs/workbench/contrib/terminal/common/terminal.ts @@ -397,6 +397,7 @@ export const enum TerminalCommandId { Split = 'workbench.action.terminal.split', SplitInstance = 'workbench.action.terminal.splitInstance', SplitInActiveWorkspace = 'workbench.action.terminal.splitInActiveWorkspace', + Unsplit = 'workbench.action.terminal.unsplit', Relaunch = 'workbench.action.terminal.relaunch', FocusPreviousPane = 'workbench.action.terminal.focusPreviousPane', ShowTabs = 'workbench.action.terminal.showTabs', From 6353aaac13a9165b1390bcd016bbd6b77030df4b Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 17 May 2021 07:32:59 -0700 Subject: [PATCH 2/3] Share code between dispose instance and remove instance --- .../contrib/terminal/browser/terminal.ts | 2 - .../contrib/terminal/browser/terminalGroup.ts | 68 +++++++------------ .../terminal/browser/terminalInstance.ts | 10 --- 3 files changed, 23 insertions(+), 57 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.ts b/src/vs/workbench/contrib/terminal/browser/terminal.ts index b852fc3ebe58e..495bbcffcdb98 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.ts @@ -526,8 +526,6 @@ export interface ITerminalInstance { */ attachToElement(container: HTMLElement): Promise | void; - detachFromElement(): void; - /** * Configure the dimensions of the terminal instance. * diff --git a/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts b/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts index 7a75fdb2c3a3c..a074a70acd284 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts @@ -324,14 +324,31 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { ]); } - private _onInstanceDisposed(instance: ITerminalInstance): void { - // Get the index of the instance and remove it from the list + private _onInstanceDisposed(instance: ITerminalInstance) { + this._removeInstance(instance); + } + + removeInstance(instance: ITerminalInstance): void { + this._removeInstance(instance); + + // Dispose instance event listeners + const disposables = this._instanceDisposables.get(instance.instanceId); + if (disposables) { + dispose(disposables); + this._instanceDisposables.delete(instance.instanceId); + } + } + + private _removeInstance(instance: ITerminalInstance) { const index = this._terminalInstances.indexOf(instance); - const wasActiveInstance = instance === this.activeInstance; - if (index !== -1) { - this._terminalInstances.splice(index, 1); + if (index === -1) { + return; } + const wasActiveInstance = instance === this.activeInstance; + this._terminalInstances.splice(index, 1); + + // TODO: Share code with onInstanceDisposed // Adjust focus if the instance was active if (wasActiveInstance && this._terminalInstances.length > 0) { const newIndex = index < this._terminalInstances.length ? index : this._terminalInstances.length - 1; @@ -345,10 +362,7 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { this._activeInstanceIndex--; } - // Remove the instance from the split pane if it has been created - if (this._splitPaneContainer) { - this._splitPaneContainer.remove(instance); - } + this._splitPaneContainer?.remove(instance); // Fire events and dispose group if it was the last instance if (this._terminalInstances.length === 0) { @@ -411,42 +425,6 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { this.setVisible(this._isVisible); } - removeInstance(instance: ITerminalInstance): void { - const index = this._terminalInstances.indexOf(instance); - if (index === -1) { - return; - } - - const wasActiveInstance = instance === this.activeInstance; - this._terminalInstances.splice(index, 1); - instance.detachFromElement(); - - // TODO: Share code with onInstanceDisposed - // Adjust focus if the instance was active - if (wasActiveInstance && this._terminalInstances.length > 0) { - const newIndex = index < this._terminalInstances.length ? index : this._terminalInstances.length - 1; - this.setActiveInstanceByIndex(newIndex); - // TODO: Only focus the new instance if the group had focus? - if (this.activeInstance) { - this.activeInstance.focus(true); - } - } else if (index < this._activeInstanceIndex) { - // Adjust active instance index if needed - this._activeInstanceIndex--; - } - - this._splitPaneContainer?.remove(instance); - - // Dispose listeners - const disposables = this._instanceDisposables.get(instance.instanceId); - if (disposables) { - dispose(disposables); - this._instanceDisposables.delete(instance.instanceId); - } - - this._onInstancesChanged.fire(); - } - get title(): string { if (this._terminalInstances.length === 0) { // Normally consumers should not call into title at all after the group is disposed but diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index 2a40774f4946b..2d23256e06ecf 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -604,16 +604,6 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._container.appendChild(this._wrapperElement); } - detachFromElement(): void { - if (!this._wrapperElement) { - return; - } - this._container?.removeChild(this._wrapperElement); - this._wrapperElement = undefined; - - // TODO: Don't allow reattach init of xterm - } - private async _attachToElement(container: HTMLElement): Promise { if (this._wrapperElement) { throw new Error('The terminal instance has already been attached to a container'); From a1646e35c3684542ffc3da25ee8de9977c79b308 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 17 May 2021 07:37:07 -0700 Subject: [PATCH 3/3] Remove try catch, clean up --- src/vs/workbench/contrib/terminal/browser/terminalGroup.ts | 1 - .../workbench/contrib/terminal/browser/terminalInstance.ts | 6 +----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts b/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts index a074a70acd284..d91b24482257b 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts @@ -348,7 +348,6 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { const wasActiveInstance = instance === this.activeInstance; this._terminalInstances.splice(index, 1); - // TODO: Share code with onInstanceDisposed // Adjust focus if the instance was active if (wasActiveInstance && this._terminalInstances.length > 0) { const newIndex = index < this._terminalInstances.length ? index : this._terminalInstances.length - 1; diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index 2d23256e06ecf..9d47112572dcd 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -920,11 +920,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { if (this._xterm) { const buffer = this._xterm.buffer; this._sendLineData(buffer.active, buffer.active.baseY + buffer.active.cursorY); - try { - this._xterm.dispose(); - } catch { - // No op, exception may get thrown from removeChild - } + this._xterm.dispose(); } if (this._pressAnyKeyToCloseListener) {