From 895dbf4dfd09ab5f36317ba5c0042b44133a737b Mon Sep 17 00:00:00 2001 From: meganrogge Date: Wed, 2 Jun 2021 14:59:41 -0700 Subject: [PATCH 1/4] fix #125069 --- .../terminal/browser/terminalInstance.ts | 94 ++++++++++++------- 1 file changed, 58 insertions(+), 36 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index 858a6486cdc..ee6ad2b6afb 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -31,7 +31,7 @@ import { TerminalLinkManager } from 'vs/workbench/contrib/terminal/browser/links import { IAccessibilityService } from 'vs/platform/accessibility/common/accessibility'; import { ITerminalInstanceService, ITerminalInstance, ITerminalExternalLinkProvider, IRequestAddInstanceToGroupEvent } from 'vs/workbench/contrib/terminal/browser/terminal'; import { TerminalProcessManager } from 'vs/workbench/contrib/terminal/browser/terminalProcessManager'; -import type { Terminal as XTermTerminal, IBuffer, ITerminalAddon, RendererType, ITheme } from 'xterm'; +import { Terminal as XTermTerminal, IBuffer, ITerminalAddon, RendererType, ITheme } from 'xterm'; import type { SearchAddon, ISearchOptions } from 'xterm-addon-search'; import type { Unicode11Addon } from 'xterm-addon-unicode11'; import type { WebglAddon } from 'xterm-addon-webgl'; @@ -60,6 +60,7 @@ import { Schemas } from 'vs/base/common/network'; import { DataTransfers } from 'vs/base/browser/dnd'; import { DragAndDropObserver, IDragAndDropObserverCallbacks } from 'vs/workbench/browser/dnd'; import { getColorClass } from 'vs/workbench/contrib/terminal/browser/terminalIcon'; +import { IDialogService } from 'vs/platform/dialogs/common/dialogs'; // How long in milliseconds should an average frame take to render for a notification to appear // which suggests the fallback DOM-based renderer @@ -251,7 +252,8 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { @IViewDescriptorService private readonly _viewDescriptorService: IViewDescriptorService, @IProductService private readonly _productService: IProductService, @IQuickInputService private readonly _quickInputService: IQuickInputService, - @IWorkbenchEnvironmentService workbenchEnvironmentService: IWorkbenchEnvironmentService + @IWorkbenchEnvironmentService workbenchEnvironmentService: IWorkbenchEnvironmentService, + @IDialogService private readonly _dialogService: IDialogService ) { super(); @@ -932,46 +934,66 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._terminalAltBufferActiveContextKey.set(!!(this._xterm && this._xterm.buffer.active === this._xterm.buffer.alternate)); } - override dispose(immediate?: boolean): void { - this._logService.trace(`terminalInstance#dispose (instanceId: ${this.instanceId})`); - dispose(this._linkManager); - this._linkManager = undefined; - dispose(this._commandTrackerAddon); - this._commandTrackerAddon = undefined; - dispose(this._widgetManager); + override dispose(immediate?: boolean, confirmed?: boolean): void { + if (this._configHelper.config.confirmOnExit && !confirmed) { + this._disposeAsync(); + } else { + this._logService.trace(`terminalInstance#dispose (instanceId: ${this.instanceId})`); + dispose(this._linkManager); + this._linkManager = undefined; + dispose(this._commandTrackerAddon); + this._commandTrackerAddon = undefined; + dispose(this._widgetManager); - if (this._xterm && this._xterm.element) { - this._hadFocusOnExit = this._xterm.element.classList.contains('focus'); - } - if (this._wrapperElement) { - if (this._wrapperElement.xterm) { - this._wrapperElement.xterm = undefined; + if (this._xterm && this._xterm.element) { + this._hadFocusOnExit = this._xterm.element.classList.contains('focus'); } - if (this._wrapperElement.parentElement && this._container) { - this._container.removeChild(this._wrapperElement); + if (this._wrapperElement) { + if (this._wrapperElement.xterm) { + this._wrapperElement.xterm = undefined; + } + if (this._wrapperElement.parentElement && this._container) { + this._container.removeChild(this._wrapperElement); + } + } + if (this._xterm) { + const buffer = this._xterm.buffer; + this._sendLineData(buffer.active, buffer.active.baseY + buffer.active.cursorY); + this._xterm.dispose(); } - } - if (this._xterm) { - const buffer = this._xterm.buffer; - this._sendLineData(buffer.active, buffer.active.baseY + buffer.active.cursorY); - this._xterm.dispose(); - } - if (this._pressAnyKeyToCloseListener) { - this._pressAnyKeyToCloseListener.dispose(); - this._pressAnyKeyToCloseListener = undefined; - } + if (this._pressAnyKeyToCloseListener) { + this._pressAnyKeyToCloseListener.dispose(); + this._pressAnyKeyToCloseListener = undefined; + } - this._processManager.dispose(immediate); - // Process manager dispose/shutdown doesn't fire process exit, trigger with undefined if it - // hasn't happened yet - this._onProcessExit(undefined); + this._processManager.dispose(immediate); + // Process manager dispose/shutdown doesn't fire process exit, trigger with undefined if it + // hasn't happened yet + this._onProcessExit(undefined); - if (!this._isDisposed) { - this._isDisposed = true; - this._onDisposed.fire(this); + if (!this._isDisposed) { + this._isDisposed = true; + this._onDisposed.fire(this); + } + super.dispose(); } - super.dispose(); + } + + private async _disposeAsync(): Promise { + const result = await this._showTerminalCloseConfirmation(); + if (result) { + return this.dispose(undefined, true); + } + } + + private async _showTerminalCloseConfirmation(): Promise { + const message = nls.localize('terminalService.terminalCloseConfirmationSingular', "This is an active terminal session, do you want to kill it?"); + const res = await this._dialogService.confirm({ + message, + type: 'warning', + }); + return res.confirmed; } detachFromProcess(): void { @@ -1242,7 +1264,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { } }); } else { - this.dispose(); + this.dispose(undefined, true); if (exitCodeMessage) { const failedDuringLaunch = this._processManager.processState === ProcessState.KilledDuringLaunch; if (failedDuringLaunch || this._configHelper.config.showExitAlert) { From b295408c3037457f3c739f66e2c3c5c0f37eb1fe Mon Sep 17 00:00:00 2001 From: meganrogge Date: Wed, 2 Jun 2021 15:07:04 -0700 Subject: [PATCH 2/4] tweak wording --- src/vs/workbench/contrib/terminal/browser/terminalInstance.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index ee6ad2b6afb..276eb63b049 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -936,7 +936,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { override dispose(immediate?: boolean, confirmed?: boolean): void { if (this._configHelper.config.confirmOnExit && !confirmed) { - this._disposeAsync(); + this._confirmDisposal(); } else { this._logService.trace(`terminalInstance#dispose (instanceId: ${this.instanceId})`); dispose(this._linkManager); @@ -980,7 +980,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { } } - private async _disposeAsync(): Promise { + private async _confirmDisposal(): Promise { const result = await this._showTerminalCloseConfirmation(); if (result) { return this.dispose(undefined, true); From 6ef81b303709dee03ecab759e35ec063a8988882 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Wed, 2 Jun 2021 15:29:32 -0700 Subject: [PATCH 3/4] add safeDisposeTerminal to terminal service --- .../contrib/terminal/browser/terminal.ts | 1 + .../terminal/browser/terminalActions.ts | 6 +- .../terminal/browser/terminalInstance.ts | 100 +++++++----------- .../terminal/browser/terminalService.ts | 10 ++ .../terminal/browser/terminalTabsList.ts | 2 +- 5 files changed, 54 insertions(+), 65 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.ts b/src/vs/workbench/contrib/terminal/browser/terminal.ts index 5d3c6bfb77d..31cac615fef 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.ts @@ -214,6 +214,7 @@ export interface ITerminalService { getEditableData(instance: ITerminalInstance): IEditableData | undefined; setEditable(instance: ITerminalInstance, data: IEditableData | null): Promise; instanceIsSplit(instance: ITerminalInstance): boolean; + safeDisposeTerminal(instance: ITerminalInstance): Promise; } export interface IRemoteTerminalService extends IOffProcessTerminalService { diff --git a/src/vs/workbench/contrib/terminal/browser/terminalActions.ts b/src/vs/workbench/contrib/terminal/browser/terminalActions.ts index 29675f981cb..8c39384e207 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalActions.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalActions.ts @@ -1627,10 +1627,10 @@ export function registerTerminalActions() { if (!selectedInstances) { return; } - for (const instance of selectedInstances) { - instance.dispose(true); - } const terminalService = accessor.get(ITerminalService); + for (const instance of selectedInstances) { + terminalService.safeDisposeTerminal(instance); + } if (terminalService.terminalInstances.length > 0) { terminalService.focusTabs(); focusNext(accessor); diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index 276eb63b049..858a6486cdc 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -31,7 +31,7 @@ import { TerminalLinkManager } from 'vs/workbench/contrib/terminal/browser/links import { IAccessibilityService } from 'vs/platform/accessibility/common/accessibility'; import { ITerminalInstanceService, ITerminalInstance, ITerminalExternalLinkProvider, IRequestAddInstanceToGroupEvent } from 'vs/workbench/contrib/terminal/browser/terminal'; import { TerminalProcessManager } from 'vs/workbench/contrib/terminal/browser/terminalProcessManager'; -import { Terminal as XTermTerminal, IBuffer, ITerminalAddon, RendererType, ITheme } from 'xterm'; +import type { Terminal as XTermTerminal, IBuffer, ITerminalAddon, RendererType, ITheme } from 'xterm'; import type { SearchAddon, ISearchOptions } from 'xterm-addon-search'; import type { Unicode11Addon } from 'xterm-addon-unicode11'; import type { WebglAddon } from 'xterm-addon-webgl'; @@ -60,7 +60,6 @@ import { Schemas } from 'vs/base/common/network'; import { DataTransfers } from 'vs/base/browser/dnd'; import { DragAndDropObserver, IDragAndDropObserverCallbacks } from 'vs/workbench/browser/dnd'; import { getColorClass } from 'vs/workbench/contrib/terminal/browser/terminalIcon'; -import { IDialogService } from 'vs/platform/dialogs/common/dialogs'; // How long in milliseconds should an average frame take to render for a notification to appear // which suggests the fallback DOM-based renderer @@ -252,8 +251,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { @IViewDescriptorService private readonly _viewDescriptorService: IViewDescriptorService, @IProductService private readonly _productService: IProductService, @IQuickInputService private readonly _quickInputService: IQuickInputService, - @IWorkbenchEnvironmentService workbenchEnvironmentService: IWorkbenchEnvironmentService, - @IDialogService private readonly _dialogService: IDialogService + @IWorkbenchEnvironmentService workbenchEnvironmentService: IWorkbenchEnvironmentService ) { super(); @@ -934,66 +932,46 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._terminalAltBufferActiveContextKey.set(!!(this._xterm && this._xterm.buffer.active === this._xterm.buffer.alternate)); } - override dispose(immediate?: boolean, confirmed?: boolean): void { - if (this._configHelper.config.confirmOnExit && !confirmed) { - this._confirmDisposal(); - } else { - this._logService.trace(`terminalInstance#dispose (instanceId: ${this.instanceId})`); - dispose(this._linkManager); - this._linkManager = undefined; - dispose(this._commandTrackerAddon); - this._commandTrackerAddon = undefined; - dispose(this._widgetManager); + override dispose(immediate?: boolean): void { + this._logService.trace(`terminalInstance#dispose (instanceId: ${this.instanceId})`); + dispose(this._linkManager); + this._linkManager = undefined; + dispose(this._commandTrackerAddon); + this._commandTrackerAddon = undefined; + dispose(this._widgetManager); - if (this._xterm && this._xterm.element) { - this._hadFocusOnExit = this._xterm.element.classList.contains('focus'); - } - if (this._wrapperElement) { - if (this._wrapperElement.xterm) { - this._wrapperElement.xterm = undefined; - } - if (this._wrapperElement.parentElement && this._container) { - this._container.removeChild(this._wrapperElement); - } - } - if (this._xterm) { - const buffer = this._xterm.buffer; - this._sendLineData(buffer.active, buffer.active.baseY + buffer.active.cursorY); - this._xterm.dispose(); - } - - if (this._pressAnyKeyToCloseListener) { - this._pressAnyKeyToCloseListener.dispose(); - this._pressAnyKeyToCloseListener = undefined; - } - - this._processManager.dispose(immediate); - // Process manager dispose/shutdown doesn't fire process exit, trigger with undefined if it - // hasn't happened yet - this._onProcessExit(undefined); - - if (!this._isDisposed) { - this._isDisposed = true; - this._onDisposed.fire(this); - } - super.dispose(); + if (this._xterm && this._xterm.element) { + this._hadFocusOnExit = this._xterm.element.classList.contains('focus'); } - } - - private async _confirmDisposal(): Promise { - const result = await this._showTerminalCloseConfirmation(); - if (result) { - return this.dispose(undefined, true); + if (this._wrapperElement) { + if (this._wrapperElement.xterm) { + this._wrapperElement.xterm = undefined; + } + if (this._wrapperElement.parentElement && this._container) { + this._container.removeChild(this._wrapperElement); + } + } + if (this._xterm) { + const buffer = this._xterm.buffer; + this._sendLineData(buffer.active, buffer.active.baseY + buffer.active.cursorY); + this._xterm.dispose(); } - } - private async _showTerminalCloseConfirmation(): Promise { - const message = nls.localize('terminalService.terminalCloseConfirmationSingular', "This is an active terminal session, do you want to kill it?"); - const res = await this._dialogService.confirm({ - message, - type: 'warning', - }); - return res.confirmed; + if (this._pressAnyKeyToCloseListener) { + this._pressAnyKeyToCloseListener.dispose(); + this._pressAnyKeyToCloseListener = undefined; + } + + this._processManager.dispose(immediate); + // Process manager dispose/shutdown doesn't fire process exit, trigger with undefined if it + // hasn't happened yet + this._onProcessExit(undefined); + + if (!this._isDisposed) { + this._isDisposed = true; + this._onDisposed.fire(this); + } + super.dispose(); } detachFromProcess(): void { @@ -1264,7 +1242,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { } }); } else { - this.dispose(undefined, true); + this.dispose(); if (exitCodeMessage) { const failedDuringLaunch = this._processManager.processState === ProcessState.KilledDuringLaunch; if (failedDuringLaunch || this._configHelper.config.showExitAlert) { diff --git a/src/vs/workbench/contrib/terminal/browser/terminalService.ts b/src/vs/workbench/contrib/terminal/browser/terminalService.ts index 9f13ba2cfc8..2f5caacd51c 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalService.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalService.ts @@ -220,6 +220,16 @@ export class TerminalService implements ITerminalService { this._refreshAvailableProfiles(); } + async safeDisposeTerminal(instance: ITerminalInstance): Promise { + if (this.configHelper.config.confirmOnExit) { + const confirmed = await this._showTerminalCloseConfirmation(); + if (!confirmed) { + return; + } + } + instance.dispose(); + } + private _setConnected() { this._connectionState = TerminalConnectionState.Connected; this._onDidChangeConnectionState.fire(); diff --git a/src/vs/workbench/contrib/terminal/browser/terminalTabsList.ts b/src/vs/workbench/contrib/terminal/browser/terminalTabsList.ts index 2893dacb848..b67adc0272c 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalTabsList.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalTabsList.ts @@ -309,7 +309,7 @@ class TerminalTabsRenderer implements IListRenderer { e.stopImmediatePropagation(); if (e.button === 1/*middle*/) { - instance.dispose(); + this._terminalService.safeDisposeTerminal(instance); } })); From 8f3a47cc88c2745feed8c1506e93c8dae2082e62 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Wed, 2 Jun 2021 15:36:45 -0700 Subject: [PATCH 4/4] add singleTerminal argument --- .../workbench/contrib/terminal/browser/terminalService.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminalService.ts b/src/vs/workbench/contrib/terminal/browser/terminalService.ts index 2f5caacd51c..88b3bca9fe7 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalService.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalService.ts @@ -222,8 +222,8 @@ export class TerminalService implements ITerminalService { async safeDisposeTerminal(instance: ITerminalInstance): Promise { if (this.configHelper.config.confirmOnExit) { - const confirmed = await this._showTerminalCloseConfirmation(); - if (!confirmed) { + const notConfirmed = await this._showTerminalCloseConfirmation(true); + if (notConfirmed) { return; } } @@ -880,9 +880,9 @@ export class TerminalService implements ITerminalService { return terminalIndex; } - protected async _showTerminalCloseConfirmation(): Promise { + protected async _showTerminalCloseConfirmation(singleTerminal?: boolean): Promise { let message: string; - if (this.terminalInstances.length === 1) { + if (this.terminalInstances.length === 1 || singleTerminal) { message = nls.localize('terminalService.terminalCloseConfirmationSingular', "There is an active terminal session, do you want to kill it?"); } else { message = nls.localize('terminalService.terminalCloseConfirmationPlural', "There are {0} active terminal sessions, do you want to kill them?", this.terminalInstances.length);