Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix terminal disposable leaks, get dropdown menu to show for terminal editors #239742

Merged
merged 14 commits into from
Feb 6, 2025
Merged

fix terminal disposable leaks, get dropdown menu to show for terminal editors #239742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e2fa641
fix terminal tab disposable issues
meganrogge Feb 5, 2025
55d0316
clean up
meganrogge Feb 5, 2025
d53d5c3
fix onCwdChange, commandFinished
meganrogge Feb 5, 2025
28dbd97
fix onDidAddCapabilityType
meganrogge Feb 5, 2025
5bf403b
fix onDisposed
meganrogge Feb 5, 2025
582aa85
revert a change
meganrogge Feb 5, 2025
4a4e342
Merge branch 'merogge/disposables' into merogge/terminal-dispose
meganrogge Feb 5, 2025
122f563
clear disposables each time
meganrogge Feb 6, 2025
866cff4
fix #239810
meganrogge Feb 6, 2025
5051f76
add new onAnyInstanceAddedCapabilityType to terminal service
meganrogge Feb 6, 2025
7a26dd2
Merge branch 'merogge/disposables' into merogge/terminal-dispose
meganrogge Feb 6, 2025
0d0f380
Merge branch 'main' into merogge/terminal-dispose
meganrogge Feb 6, 2025
1c4d909
get rid of map, add dispose disposable to store and delete it when te…
meganrogge Feb 6, 2025
8a37dcc
fix issue
meganrogge Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/vs/workbench/contrib/terminal/browser/terminal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ export interface ITerminalService extends ITerminalInstanceHost {
readonly onAnyInstanceSelectionChange: Event<ITerminalInstance>;
readonly onAnyInstanceTitleChange: Event<ITerminalInstance>;
readonly onAnyInstanceShellTypeChanged: Event<ITerminalInstance>;
readonly onAnyInstanceAddedCapabilityType: Event<TerminalCapability>;

/**
* Creates a terminal.
Expand Down
35 changes: 17 additions & 18 deletions src/vs/workbench/contrib/terminal/browser/terminalEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import * as dom from '../../../../base/browser/dom.js';
import { IActionViewItem } from '../../../../base/browser/ui/actionbar/actionbar.js';
import { Action, IAction } from '../../../../base/common/actions.js';
import { IAction } from '../../../../base/common/actions.js';
import { CancellationToken } from '../../../../base/common/cancellation.js';
import { DropdownWithPrimaryActionViewItem } from '../../../../platform/actions/browser/dropdownWithPrimaryActionViewItem.js';
import { IMenu, IMenuService, MenuId, MenuItemAction } from '../../../../platform/actions/common/actions.js';
Expand All @@ -27,7 +27,8 @@ import { openContextMenu } from './terminalContextMenu.js';
import { ACTIVE_GROUP } from '../../../services/editor/common/editorService.js';
import { IWorkbenchLayoutService, Parts } from '../../../services/layout/browser/layoutService.js';
import { IBaseActionViewItemOptions } from '../../../../base/browser/ui/actionbar/actionViewItems.js';
import { DisposableStore } from '../../../../base/common/lifecycle.js';
import { DisposableStore, MutableDisposable } from '../../../../base/common/lifecycle.js';
import { ITerminalProfile, TerminalLocation } from '../../../../platform/terminal/common/terminal.js';

export class TerminalEditor extends EditorPane {

Expand All @@ -44,6 +45,8 @@ export class TerminalEditor extends EditorPane {

private _cancelContextMenu: boolean = false;

private readonly _newDropdown: MutableDisposable<DropdownWithPrimaryActionViewItem> = this._register(new MutableDisposable());

private readonly _disposableStore = this._register(new DisposableStore());

constructor(
Expand All @@ -65,6 +68,7 @@ export class TerminalEditor extends EditorPane {
super(terminalEditorId, group, telemetryService, themeService, storageService);
this._dropdownMenu = this._register(menuService.createMenu(MenuId.TerminalNewDropdownContext, contextKeyService));
this._instanceMenu = this._register(menuService.createMenu(MenuId.TerminalInstanceContext, contextKeyService));
this._register(this._terminalProfileService.onDidChangeAvailableProfiles(profiles => this._updateTabActionBar(profiles)));
}

override async setInput(newInput: TerminalEditorInput, options: IEditorOptions | undefined, context: IEditorOpenContext, token: CancellationToken) {
Expand Down Expand Up @@ -147,6 +151,11 @@ export class TerminalEditor extends EditorPane {
}));
}

private _updateTabActionBar(profiles: ITerminalProfile[]): void {
const actions = getTerminalActionBarArgs(TerminalLocation.Editor, profiles, this._getDefaultProfileName(), this._terminalProfileService.contributedProfiles, this._terminalService, this._dropdownMenu, this._disposableStore);
this._newDropdown.value?.update(actions.dropdownAction, actions.dropdownMenuActions);
}

layout(dimension: dom.Dimension): void {
const instance = this._editorInput?.terminalInstance;
if (instance) {
Expand All @@ -163,30 +172,20 @@ export class TerminalEditor extends EditorPane {

override getActionViewItem(action: IAction, options: IBaseActionViewItemOptions): IActionViewItem | undefined {
switch (action.id) {
case TerminalCommandId.CreateTerminalEditor: {
case TerminalCommandId.CreateTerminalEditorSameGroup: {
if (action instanceof MenuItemAction) {
const location = { viewColumn: ACTIVE_GROUP };
const actions = getTerminalActionBarArgs(location, this._terminalProfileService.availableProfiles, this._getDefaultProfileName(), this._terminalProfileService.contributedProfiles, this._terminalService, this._dropdownMenu);
this._registerDisposableActions(actions.dropdownAction, actions.dropdownMenuActions);
const button = this._instantiationService.createInstance(DropdownWithPrimaryActionViewItem, action, actions.dropdownAction, actions.dropdownMenuActions, actions.className, { hoverDelegate: options.hoverDelegate });
return button;
this._disposableStore.clear();
const actions = getTerminalActionBarArgs(location, this._terminalProfileService.availableProfiles, this._getDefaultProfileName(), this._terminalProfileService.contributedProfiles, this._terminalService, this._dropdownMenu, this._disposableStore);
this._newDropdown.value = this._instantiationService.createInstance(DropdownWithPrimaryActionViewItem, action, actions.dropdownAction, actions.dropdownMenuActions, actions.className, { hoverDelegate: options.hoverDelegate });
this._newDropdown.value?.update(actions.dropdownAction, actions.dropdownMenuActions);
return this._newDropdown.value;
}
}
}
return super.getActionViewItem(action, options);
}

/**
* Actions might be of type Action (disposable) or Separator or SubmenuAction, which don't extend Disposable
*/
private _registerDisposableActions(dropdownAction: IAction, dropdownMenuActions: IAction[]): void {
this._disposableStore.clear();
if (dropdownAction instanceof Action) {
this._disposableStore.add(dropdownAction);
}
dropdownMenuActions.filter(a => a instanceof Action).forEach(a => this._disposableStore.add(a));
}

private _getDefaultProfileName(): string {
let defaultProfileName;
try {
Expand Down
21 changes: 11 additions & 10 deletions src/vs/workbench/contrib/terminal/browser/terminalMenus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { TerminalCommandId, TERMINAL_VIEW_ID } from '../common/terminal.js';
import { TerminalContextKeys, TerminalContextKeyStrings } from '../common/terminalContextKey.js';
import { terminalStrings } from '../common/terminalStrings.js';
import { ACTIVE_GROUP, SIDE_GROUP } from '../../../services/editor/common/editorService.js';
import { DisposableStore } from '../../../../base/common/lifecycle.js';

const enum ContextMenuGroup {
Create = '1_create',
Expand Down Expand Up @@ -679,7 +680,7 @@ export function setupTerminalMenus(): void {
});
}

export function getTerminalActionBarArgs(location: ITerminalLocationOptions, profiles: ITerminalProfile[], defaultProfileName: string, contributedProfiles: readonly IExtensionTerminalProfile[], terminalService: ITerminalService, dropdownMenu: IMenu): {
export function getTerminalActionBarArgs(location: ITerminalLocationOptions, profiles: ITerminalProfile[], defaultProfileName: string, contributedProfiles: readonly IExtensionTerminalProfile[], terminalService: ITerminalService, dropdownMenu: IMenu, disposableStore: DisposableStore): {
dropdownAction: IAction;
dropdownMenuActions: IAction[];
className: string;
Expand All @@ -694,37 +695,37 @@ export function getTerminalActionBarArgs(location: ITerminalLocationOptions, pro
const options: ICreateTerminalOptions = { config: p, location };
const splitOptions: ICreateTerminalOptions = { config: p, location: splitLocation };
const sanitizedProfileName = p.profileName.replace(/[\n\r\t]/g, '');
dropdownActions.push(new Action(TerminalCommandId.NewWithProfile, isDefault ? localize('defaultTerminalProfile', "{0} (Default)", sanitizedProfileName) : sanitizedProfileName, undefined, true, async () => {
dropdownActions.push(disposableStore.add(new Action(TerminalCommandId.NewWithProfile, isDefault ? localize('defaultTerminalProfile', "{0} (Default)", sanitizedProfileName) : sanitizedProfileName, undefined, true, async () => {
const instance = await terminalService.createTerminal(options);
terminalService.setActiveInstance(instance);
await terminalService.focusActiveInstance();
}));
submenuActions.push(new Action(TerminalCommandId.Split, isDefault ? localize('defaultTerminalProfile', "{0} (Default)", sanitizedProfileName) : sanitizedProfileName, undefined, true, async () => {
})));
submenuActions.push(disposableStore.add(new Action(TerminalCommandId.Split, isDefault ? localize('defaultTerminalProfile', "{0} (Default)", sanitizedProfileName) : sanitizedProfileName, undefined, true, async () => {
const instance = await terminalService.createTerminal(splitOptions);
terminalService.setActiveInstance(instance);
await terminalService.focusActiveInstance();
}));
})));
}

for (const contributed of contributedProfiles) {
const isDefault = contributed.title === defaultProfileName;
const title = isDefault ? localize('defaultTerminalProfile', "{0} (Default)", contributed.title.replace(/[\n\r\t]/g, '')) : contributed.title.replace(/[\n\r\t]/g, '');
dropdownActions.push(new Action('contributed', title, undefined, true, () => terminalService.createTerminal({
dropdownActions.push(disposableStore.add(new Action('contributed', title, undefined, true, () => terminalService.createTerminal({
config: {
extensionIdentifier: contributed.extensionIdentifier,
id: contributed.id,
title
},
location
})));
submenuActions.push(new Action('contributed-split', title, undefined, true, () => terminalService.createTerminal({
}))));
submenuActions.push(disposableStore.add(new Action('contributed-split', title, undefined, true, () => terminalService.createTerminal({
config: {
extensionIdentifier: contributed.extensionIdentifier,
id: contributed.id,
title
},
location: splitLocation
})));
}))));
}

const defaultProfileAction = dropdownActions.find(d => d.label.endsWith('(Default)'));
Expand All @@ -746,6 +747,6 @@ export function getTerminalActionBarArgs(location: ITerminalLocationOptions, pro
submenuActions.unshift(defaultSubmenuProfileAction);
}

const dropdownAction = new Action('refresh profiles', localize('launchProfile', 'Launch Profile...'), 'codicon-chevron-down', true);
const dropdownAction = disposableStore.add(new Action('refresh profiles', localize('launchProfile', 'Launch Profile...'), 'codicon-chevron-down', true));
return { dropdownAction, dropdownMenuActions: dropdownActions, className: `terminal-tab-actions-${terminalService.resolveLocation(location)}` };
}
27 changes: 15 additions & 12 deletions src/vs/workbench/contrib/terminal/browser/terminalService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import * as cssValue from '../../../../base/browser/cssValue.js';
import { DeferredPromise, timeout } from '../../../../base/common/async.js';
import { debounce, memoize } from '../../../../base/common/decorators.js';
import { DynamicListEventMultiplexer, Emitter, Event, IDynamicListEventMultiplexer } from '../../../../base/common/event.js';
import { Disposable, dispose, IDisposable, toDisposable } from '../../../../base/common/lifecycle.js';
import { Disposable, DisposableStore, dispose, IDisposable, toDisposable } from '../../../../base/common/lifecycle.js';
import { Schemas } from '../../../../base/common/network.js';
import { isMacintosh, isWeb } from '../../../../base/common/platform.js';
import { URI } from '../../../../base/common/uri.js';
Expand Down Expand Up @@ -166,6 +166,7 @@ export class TerminalService extends Disposable implements ITerminalService {
@memoize get onAnyInstanceSelectionChange() { return this._register(this.createOnInstanceEvent(e => e.onDidChangeSelection)).event; }
@memoize get onAnyInstanceTitleChange() { return this._register(this.createOnInstanceEvent(e => e.onTitleChanged)).event; }
@memoize get onAnyInstanceShellTypeChanged() { return this._register(this.createOnInstanceEvent(e => Event.map(e.onDidChangeShellType, () => e))).event; }
@memoize get onAnyInstanceAddedCapabilityType() { return this._register(this.createOnInstanceEvent(e => e.capabilities.onDidAddCapabilityType)).event; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we not end up needing this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do need that and use it here 5051f76#diff-9ad94bfccb2c181ffea0b1abb3b85b7a630784ddc3de16f47560a374da26c7ccR130

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think I was looking at a partial diff somehow

constructor(
@IContextKeyService private _contextKeyService: IContextKeyService,
@ILifecycleService private readonly _lifecycleService: ILifecycleService,
Expand Down Expand Up @@ -836,17 +837,19 @@ export class TerminalService extends Disposable implements ITerminalService {
}

protected _initInstanceListeners(instance: ITerminalInstance): void {
const instanceDisposables: IDisposable[] = [
instance.onDimensionsChanged(() => {
this._onDidChangeInstanceDimensions.fire(instance);
if (this._terminalConfigurationService.config.enablePersistentSessions && this.isProcessSupportRegistered) {
this._saveState();
}
}),
instance.onDidFocus(this._onDidChangeActiveInstance.fire, this._onDidChangeActiveInstance),
instance.onRequestAddInstanceToGroup(async e => await this._addInstanceToGroup(instance, e))
];
instance.onDisposed(() => dispose(instanceDisposables));
const instanceDisposables = new DisposableStore();
instanceDisposables.add(instance.onDimensionsChanged(() => {
this._onDidChangeInstanceDimensions.fire(instance);
if (this._terminalConfigurationService.config.enablePersistentSessions && this.isProcessSupportRegistered) {
this._saveState();
}
}));
instanceDisposables.add(instance.onDidFocus(this._onDidChangeActiveInstance.fire, this._onDidChangeActiveInstance));
instanceDisposables.add(instance.onRequestAddInstanceToGroup(async e => await this._addInstanceToGroup(instance, e)));
const disposeListener = this._register(instance.onDisposed(() => {
instanceDisposables.dispose();
this._store.delete(disposeListener);
}));
}

private async _addInstanceToGroup(instance: ITerminalInstance, e: IRequestAddInstanceToGroupEvent): Promise<void> {
Expand Down
4 changes: 2 additions & 2 deletions src/vs/workbench/contrib/terminal/browser/terminalTabbedView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import { LayoutPriority, Orientation, Sizing, SplitView } from '../../../../base/browser/ui/splitview/splitview.js';
import { Disposable, dispose, IDisposable } from '../../../../base/common/lifecycle.js';
import { Disposable, DisposableStore, dispose, IDisposable } from '../../../../base/common/lifecycle.js';
import { IConfigurationService } from '../../../../platform/configuration/common/configuration.js';
import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js';
import { ITerminalConfigurationService, ITerminalGroupService, ITerminalInstance, ITerminalService, TerminalConnectionState } from './terminal.js';
Expand Down Expand Up @@ -89,7 +89,7 @@ export class TerminalTabbedView extends Disposable {
this._tabsListMenu = this._register(menuService.createMenu(MenuId.TerminalTabContext, contextKeyService));
this._tabsListEmptyMenu = this._register(menuService.createMenu(MenuId.TerminalTabEmptyAreaContext, contextKeyService));

this._tabList = this._register(this._instantiationService.createInstance(TerminalTabList, this._tabListElement));
this._tabList = this._register(this._instantiationService.createInstance(TerminalTabList, this._tabListElement, this._register(new DisposableStore())));

const terminalOuterContainer = $('.terminal-outer-container');
this._terminalContainer = $('.terminal-groups-container');
Expand Down
19 changes: 10 additions & 9 deletions src/vs/workbench/contrib/terminal/browser/terminalTabsList.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ export class TerminalTabList extends WorkbenchList<ITerminalInstance> {

constructor(
container: HTMLElement,
disposableStore: DisposableStore,
@IContextKeyService contextKeyService: IContextKeyService,
@IListService listService: IListService,
@IThemeService themeService: IThemeService,
@IConfigurationService private readonly _configurationService: IConfigurationService,
@ITerminalService private readonly _terminalService: ITerminalService,
@ITerminalGroupService private readonly _terminalGroupService: ITerminalGroupService,
Expand All @@ -89,7 +89,7 @@ export class TerminalTabList extends WorkbenchList<ITerminalInstance> {
getHeight: () => TerminalTabsListSizes.TabHeight,
getTemplateId: () => 'terminal.tabs'
},
[instantiationService.createInstance(TerminalTabsRenderer, container, instantiationService.createInstance(ResourceLabels, DEFAULT_LABELS_CONTAINER), () => this.getSelectedElements())],
[disposableStore.add(instantiationService.createInstance(TerminalTabsRenderer, container, instantiationService.createInstance(ResourceLabels, DEFAULT_LABELS_CONTAINER), () => this.getSelectedElements()))],
{
horizontalScrolling: false,
supportDynamicHeights: false,
Expand Down Expand Up @@ -242,7 +242,7 @@ export class TerminalTabList extends WorkbenchList<ITerminalInstance> {
}
}

class TerminalTabsRenderer implements IListRenderer<ITerminalInstance, ITerminalTabEntryTemplate> {
class TerminalTabsRenderer extends Disposable implements IListRenderer<ITerminalInstance, ITerminalTabEntryTemplate> {
templateId = 'terminal.tabs';

constructor(
Expand All @@ -259,6 +259,7 @@ class TerminalTabsRenderer implements IListRenderer<ITerminalInstance, ITerminal
@IThemeService private readonly _themeService: IThemeService,
@IContextViewService private readonly _contextViewService: IContextViewService
) {
super();
}

renderTemplate(container: HTMLElement): ITerminalTabEntryTemplate {
Expand Down Expand Up @@ -288,13 +289,13 @@ class TerminalTabsRenderer implements IListRenderer<ITerminalInstance, ITerminal

const actionsContainer = DOM.append(label.element, $('.actions'));

const actionBar = new ActionBar(actionsContainer, {
const actionBar = this._register(new ActionBar(actionsContainer, {
actionRunner: new TerminalContextActionRunner(),
actionViewItemProvider: (action, options) =>
action instanceof MenuItemAction
? this._instantiationService.createInstance(MenuEntryActionViewItem, action, { hoverDelegate: options.hoverDelegate })
: undefined
});
}));

return {
element,
Expand Down Expand Up @@ -495,14 +496,14 @@ class TerminalTabsRenderer implements IListRenderer<ITerminalInstance, ITerminal
fillActionBar(instance: ITerminalInstance, template: ITerminalTabEntryTemplate): void {
// If the instance is within the selection, split all selected
const actions = [
new Action(TerminalCommandId.SplitActiveTab, terminalStrings.split.short, ThemeIcon.asClassName(Codicon.splitHorizontal), true, async () => {
this._register(new Action(TerminalCommandId.SplitActiveTab, terminalStrings.split.short, ThemeIcon.asClassName(Codicon.splitHorizontal), true, async () => {
this._runForSelectionOrInstance(instance, async e => {
this._terminalService.createTerminal({ location: { parentTerminal: e } });
});
}),
new Action(TerminalCommandId.KillActiveTab, terminalStrings.kill.short, ThemeIcon.asClassName(Codicon.trashcan), true, async () => {
})),
this._register(new Action(TerminalCommandId.KillActiveTab, terminalStrings.kill.short, ThemeIcon.asClassName(Codicon.trashcan), true, async () => {
this._runForSelectionOrInstance(instance, e => this._terminalService.safeDisposeTerminal(e));
})
}))
];
// TODO: Cache these in a way that will use the correct instance
template.actionBar.clear();
Expand Down
Loading
Loading