From cf615d5700c64f4e4707b6753ca227803906e8e4 Mon Sep 17 00:00:00 2001 From: jeanp413 Date: Mon, 16 May 2022 02:45:49 -0500 Subject: [PATCH] Fixes #148983 --- .../terminal/browser/media/terminal.css | 7 +-- .../contrib/terminal/browser/terminal.ts | 3 +- .../terminal/browser/terminalEditor.ts | 8 +-- .../contrib/terminal/browser/terminalGroup.ts | 5 -- .../terminal/browser/terminalGroupService.ts | 18 +++++-- .../terminal/browser/terminalInstance.ts | 51 ++++++++++--------- .../terminal/browser/terminalService.ts | 4 -- .../contrib/terminal/browser/terminalView.ts | 6 +-- .../test/browser/workbenchTestServices.ts | 1 + 9 files changed, 51 insertions(+), 52 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/media/terminal.css b/src/vs/workbench/contrib/terminal/browser/media/terminal.css index 945161cad7f48..73c2a11c08a12 100644 --- a/src/vs/workbench/contrib/terminal/browser/media/terminal.css +++ b/src/vs/workbench/contrib/terminal/browser/media/terminal.css @@ -39,7 +39,7 @@ .monaco-workbench .editor-instance .terminal-wrapper, .monaco-workbench .pane-body.integrated-terminal .terminal-wrapper { - display: none; + display: block; height: 100%; box-sizing: border-box; } @@ -113,11 +113,6 @@ .xterm.xterm-cursor-pointer .xterm-screen { cursor: pointer; } .xterm.column-select.focus .xterm-screen { cursor: crosshair; } -.monaco-workbench .editor-instance .terminal-wrapper.active, -.monaco-workbench .pane-body.integrated-terminal .terminal-wrapper.active { - display: block; -} - .monaco-workbench .editor-instance .xterm { padding-left: 20px !important; } diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.ts b/src/vs/workbench/contrib/terminal/browser/terminal.ts index 6d1494b10d3e9..3368395091873 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.ts @@ -339,6 +339,7 @@ export interface ITerminalGroupService extends ITerminalInstanceHost, ITerminalF hidePanel(): void; focusTabs(): void; showTabs(): void; + updateVisibility(): void; } /** @@ -741,7 +742,7 @@ export interface ITerminalInstance { * * @param container The element to attach the terminal instance to. */ - attachToElement(container: HTMLElement): Promise | void; + attachToElement(container: HTMLElement): void; /** * Detaches the terminal instance from the terminal editor DOM element. diff --git a/src/vs/workbench/contrib/terminal/browser/terminalEditor.ts b/src/vs/workbench/contrib/terminal/browser/terminalEditor.ts index 71eeff2a7ace5..61412cfb781e7 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalEditor.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalEditor.ts @@ -31,6 +31,7 @@ import { INotificationService } from 'vs/platform/notification/common/notificati import { openContextMenu } from 'vs/workbench/contrib/terminal/browser/terminalContextMenu'; import { ICommandService } from 'vs/platform/commands/common/commands'; import { ACTIVE_GROUP } from 'vs/workbench/services/editor/common/editorService'; +import { IWorkbenchLayoutService, Parts } from 'vs/workbench/services/layout/browser/layoutService'; const findWidgetSelector = '.simple-find-part-wrapper'; @@ -68,7 +69,8 @@ export class TerminalEditor extends EditorPane { @IInstantiationService private readonly _instantiationService: IInstantiationService, @IContextMenuService private readonly _contextMenuService: IContextMenuService, @INotificationService private readonly _notificationService: INotificationService, - @ITerminalProfileService private readonly _terminalProfileService: ITerminalProfileService + @ITerminalProfileService private readonly _terminalProfileService: ITerminalProfileService, + @IWorkbenchLayoutService private readonly _workbenchLayoutService: IWorkbenchLayoutService ) { super(terminalEditorId, telemetryService, themeService, storageService); this._findState = new FindReplaceState(); @@ -86,7 +88,7 @@ export class TerminalEditor extends EditorPane { if (this._lastDimension) { this.layout(this._lastDimension); } - this._editorInput.terminalInstance?.setVisible(this.isVisible()); + this._editorInput.terminalInstance?.setVisible(this.isVisible() && this._workbenchLayoutService.isVisible(Parts.EDITOR_PART)); if (this._editorInput.terminalInstance) { // since the editor does not monitor focus changes, for ex. between the terminal // panel and the editors, this is needed so that the active instance gets set @@ -208,7 +210,7 @@ export class TerminalEditor extends EditorPane { override setVisible(visible: boolean, group?: IEditorGroup): void { super.setVisible(visible, group); - return this._editorInput?.terminalInstance?.setVisible(visible); + this._editorInput?.terminalInstance?.setVisible(visible && this._workbenchLayoutService.isVisible(Parts.EDITOR_PART)); } override getActionViewItem(action: IAction): IActionViewItem | undefined { diff --git a/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts b/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts index 357c121317f3c..7e70c6912192d 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalGroup.ts @@ -251,7 +251,6 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { private _instanceDisposables: Map = new Map(); private _activeInstanceIndex: number = -1; - private _isVisible: boolean = false; get terminalInstances(): ITerminalInstance[] { return this._terminalInstances; } @@ -313,8 +312,6 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { this._splitPaneContainer!.split(instance, parentIndex + 1); } - instance.setVisible(this._isVisible); - this._onInstancesChanged.fire(); } @@ -478,7 +475,6 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { this._initialRelativeSizes = undefined; } } - this.setVisible(this._isVisible); } get title(): string { @@ -511,7 +507,6 @@ export class TerminalGroup extends Disposable implements ITerminalGroup { } setVisible(visible: boolean): void { - this._isVisible = visible; if (this._groupElement) { this._groupElement.style.display = visible ? '' : 'none'; } diff --git a/src/vs/workbench/contrib/terminal/browser/terminalGroupService.ts b/src/vs/workbench/contrib/terminal/browser/terminalGroupService.ts index 4e057f1cb4d2f..a26f793e03305 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalGroupService.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalGroupService.ts @@ -5,7 +5,7 @@ import { Orientation } from 'vs/base/browser/ui/sash/sash'; import { timeout } from 'vs/base/common/async'; -import { Emitter } from 'vs/base/common/event'; +import { Emitter, Event } from 'vs/base/common/event'; import { Disposable } from 'vs/base/common/lifecycle'; import { URI } from 'vs/base/common/uri'; import { FindReplaceState } from 'vs/editor/contrib/find/browser/findState'; @@ -75,6 +75,8 @@ export class TerminalGroupService extends Disposable implements ITerminalGroupSe this.onDidChangeGroups(() => this._terminalGroupCountContextKey.set(this.groups.length)); this._findState = new FindReplaceState(); + + Event.any(this.onDidChangeActiveGroup, this.onDidChangeInstances)(() => this.updateVisibility()); } hidePanel(): void { @@ -271,7 +273,6 @@ export class TerminalGroupService extends Disposable implements ITerminalGroupSe const oldActiveGroup = this.activeGroup; this.activeGroupIndex = index; if (force || oldActiveGroup !== this.activeGroup) { - this.groups.forEach((g, i) => g.setVisible(i === this.activeGroupIndex)); this._onDidChangeActiveGroup.fire(this.activeGroup); this._onDidChangeActiveInstance.fire(this.activeInstance); } @@ -309,8 +310,6 @@ export class TerminalGroupService extends Disposable implements ITerminalGroupSe this.activeGroupIndex = instanceLocation.groupIndex; this._onDidChangeActiveGroup.fire(this.activeGroup); instanceLocation.group.setActiveInstanceByIndex(activeInstanceIndex, true); - this.groups.forEach((g, i) => g.setVisible(i === instanceLocation.groupIndex)); - } setActiveGroupToNext() { @@ -476,6 +475,17 @@ export class TerminalGroupService extends Disposable implements ITerminalGroupSe return `${index + 1}: ${group.title ? group.title : ''}`; }); } + + /** + * Visibility should be updated in the following cases: + * 1. Toggle `TERMINAL_VIEW_ID` visibility + * 2. Change active group + * 3. Change instances in active group + */ + updateVisibility() { + const visible = this._viewsService.isViewVisible(TERMINAL_VIEW_ID); + this.groups.forEach((g, i) => g.setVisible(visible && i === this.activeGroupIndex)); + } } interface IInstanceLocation { diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index 88b3e2d041d5b..995bb43a50771 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -552,8 +552,8 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { // The terminal panel needs to have been created to get the real view dimensions if (!this._container) { // Set the fallback dimensions if not - this._cols = 80; - this._rows = 30; + this._cols = Constants.DefaultCols; + this._rows = Constants.DefaultRows; return; } @@ -736,6 +736,11 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._pathService.userHome().then(userHome => { this._userHome = userHome.fsPath; }); + + if (this._isVisible) { + this._open(); + } + return xterm; } @@ -954,7 +959,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._container = undefined; } - attachToElement(container: HTMLElement): Promise | void { + attachToElement(container: HTMLElement): void { // The container did not change, do nothing if (this._container === container) { return; @@ -962,24 +967,28 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._attachBarrier.open(); - // Attach has not occurred yet - if (!this._wrapperElement) { - return this._attachToElement(container); - } - this.xterm?.attachToElement(this._wrapperElement); - // The container changed, reattach this._container = container; - this._container.appendChild(this._wrapperElement); + if (this._wrapperElement) { + this._container.appendChild(this._wrapperElement); + } setTimeout(() => this._initDragAndDrop(container)); } - private async _attachToElement(container: HTMLElement): Promise { - if (this._wrapperElement) { - throw new Error('The terminal instance has already been attached to a container'); + /** + * Opens the the terminal instance inside the parent DOM element previously set with + * `attachToElement`, you must ensure the parent DOM element is explicitly visible before + * invoking this function as it performs some DOM calculations internally + */ + private _open(): void { + if (this._wrapperElement || !this.xterm) { + return; + } + + if (!this._container || !this._container.isConnected) { + throw new Error('A container element needs to be set with `attachToElement` and be part of the DOM before calling `_open`'); } - this._container = container; this._wrapperElement = document.createElement('div'); this._wrapperElement.classList.add('terminal-wrapper'); const xtermElement = document.createElement('div'); @@ -987,7 +996,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._container.appendChild(this._wrapperElement); - const xterm = await this._xtermReadyPromise; + const xterm = this.xterm; // Attach the xterm object to the DOM, exposing it to the smoke tests this._wrapperElement.xterm = xterm.raw; @@ -1106,21 +1115,16 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._register(dom.addDisposableListener(xterm.raw.textarea, 'blur', () => this._setFocus(false))); this._register(dom.addDisposableListener(xterm.raw.textarea, 'focusout', () => this._setFocus(false))); - this._initDragAndDrop(container); + this._initDragAndDrop(this._container); this._widgetManager.attachToElement(screenElement); this._processManager.onProcessReady((e) => { this._linkManager?.setWidgetManager(this._widgetManager); }); - // const computedStyle = window.getComputedStyle(this._container); - // const computedStyle = window.getComputedStyle(this._container.parentElement!); - // const width = parseInt(computedStyle.getPropertyValue('width').replace('px', ''), 10); - // const height = parseInt(computedStyle.getPropertyValue('height').replace('px', ''), 10); if (this._lastLayoutDimensions) { this.layout(this._lastLayoutDimensions); } - this.setVisible(this._isVisible); this.updateConfig(); // If IShellLaunchConfig.waitOnExit was true and the process finished before the terminal @@ -1376,10 +1380,9 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { setVisible(visible: boolean): void { this._isVisible = visible; - if (this._wrapperElement) { - this._wrapperElement.classList.toggle('active', visible); - } + this._wrapperElement?.classList.toggle('active', visible); if (visible && this.xterm) { + this._open(); // Resize to re-evaluate dimensions, this will ensure when switching to a terminal it is // using the most up to date dimensions (eg. when terminal is created in the background // using cached dimensions of a split terminal). diff --git a/src/vs/workbench/contrib/terminal/browser/terminalService.ts b/src/vs/workbench/contrib/terminal/browser/terminalService.ts index 62d52e82cc942..f7ea738dde3af 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalService.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalService.ts @@ -764,8 +764,6 @@ export class TerminalService implements ITerminalService { group.addInstance(source); this.setActiveInstance(source); await this._terminalGroupService.showPanel(true); - // TODO: Shouldn't this happen automatically? - source.setVisible(true); if (target && side) { const index = group.terminalInstances.indexOf(target) + (side === 'after' ? 1 : 0); @@ -775,7 +773,6 @@ export class TerminalService implements ITerminalService { // Fire events this._onDidChangeInstances.fire(); this._onDidChangeActiveGroup.fire(this._terminalGroupService.activeGroup); - this._terminalGroupService.showPanel(true); this._onDidRequestHideFindWidget.fire(); } @@ -1033,7 +1030,6 @@ export class TerminalService implements ITerminalService { } shellLaunchConfig.parentTerminalId = parent.instanceId; instance = group.split(shellLaunchConfig); - this._terminalGroupService.groups.forEach((g, i) => g.setVisible(i === this._terminalGroupService.activeGroupIndex)); } return instance; } diff --git a/src/vs/workbench/contrib/terminal/browser/terminalView.ts b/src/vs/workbench/contrib/terminal/browser/terminalView.ts index d450e1d66098c..f2a8e3a5aaede 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalView.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalView.ts @@ -171,12 +171,8 @@ export class TerminalViewPane extends ViewPane { // defer focusing the panel to the focus() call // to prevent overriding preserveFocus for extensions this._terminalGroupService.showPanel(false); - if (hadTerminals) { - this._terminalGroupService.activeGroup?.setVisible(visible); - } - } else { - this._terminalGroupService.activeGroup?.setVisible(false); } + this._terminalGroupService.updateVisibility(); })); this.layoutBody(this._parentDomElement.offsetHeight, this._parentDomElement.offsetWidth); } diff --git a/src/vs/workbench/test/browser/workbenchTestServices.ts b/src/vs/workbench/test/browser/workbenchTestServices.ts index 9a1025debd886..7973cf4475559 100644 --- a/src/vs/workbench/test/browser/workbenchTestServices.ts +++ b/src/vs/workbench/test/browser/workbenchTestServices.ts @@ -1830,6 +1830,7 @@ export class TestTerminalGroupService implements ITerminalGroupService { getFindState(): FindReplaceState { throw new Error('Method not implemented.'); } findNext(): void { throw new Error('Method not implemented.'); } findPrevious(): void { throw new Error('Method not implemented.'); } + updateVisibility(): void { throw new Error('Method not implemented.'); } } export class TestTerminalProfileService implements ITerminalProfileService {