Merge pull request #158307 from microsoft/joh/calm-alligator

`IAction` must not be disposable.

src/vs/base/common/actions.ts

@@ -25,7 +25,7 @@ export type WorkbenchActionExecutedEvent = {
 	from: string;
 };
 
-export interface IAction extends IDisposable {
+export interface IAction {
 	readonly id: string;
 	label: string;
 	tooltip: string;
@@ -242,12 +242,6 @@ export class SubmenuAction implements IAction {
 		this._actions = actions;
 	}
 
-	dispose(): void {
-		// there is NOTHING to dispose and the SubmenuAction should
-		// never have anything to dispose as it is a convenience type
-		// to bridge into the rendering world.
-	}
-
 	async run(): Promise<void> { }
 }
 
@@ -268,7 +262,6 @@ export function toAction(props: { id: string; label: string; enabled?: boolean;
 		enabled: props.enabled ?? true,
 		checked: props.checked ?? false,
 		run: async () => props.run(),
-		tooltip: props.label,
-		dispose: () => { }
+		tooltip: props.label
 	};
 }

src/vs/base/common/lifecycle.ts

@@ -156,6 +156,14 @@ export function dispose<T extends IDisposable>(arg: T | IterableIterator<T> | un
 	}
 }
 
+export function disposeIfDisposable<T extends IDisposable | object>(disposables: Array<T>): Array<T> {
+	for (const d of disposables) {
+		if (isDisposable(d)) {
+			d.dispose();
+		}
+	}
+	return [];
+}
 
 export function combinedDisposable(...disposables: IDisposable[]): IDisposable {
 	const parent = toDisposable(() => dispose(disposables));

src/vs/editor/contrib/contextmenu/browser/contextmenu.ts

@@ -274,8 +274,7 @@ export class ContextMenuController implements IEditorContribution {
 				class: undefined,
 				enabled: (typeof opts.enabled === 'undefined' ? true : opts.enabled),
 				checked: opts.checked,
-				run: opts.run,
-				dispose: () => null
+				run: opts.run
 			};
 		};
 		const createSubmenuAction = (label: string, actions: IAction[]): SubmenuAction => {

src/vs/platform/actions/browser/menuEntryActionViewItem.ts

@@ -11,7 +11,7 @@ import { ActionRunner, IAction, IRunEvent, Separator, SubmenuAction } from 'vs/b
 import { Event } from 'vs/base/common/event';
 import { UILabelProvider } from 'vs/base/common/keybindingLabels';
 import { KeyCode } from 'vs/base/common/keyCodes';
-import { combinedDisposable, DisposableStore, IDisposable, MutableDisposable, toDisposable } from 'vs/base/common/lifecycle';
+import { combinedDisposable, MutableDisposable, toDisposable } from 'vs/base/common/lifecycle';
 import { isLinux, isWindows, OS } from 'vs/base/common/platform';
 import 'vs/css!./menuEntryActionViewItem';
 import { localize } from 'vs/nls';
@@ -28,34 +28,21 @@ import { isDark } from 'vs/platform/theme/common/theme';
 import { IHoverDelegate } from 'vs/base/browser/ui/iconLabel/iconHoverDelegate';
 import { assertType } from 'vs/base/common/types';
 
-export function createAndFillInContextMenuActions(menu: IMenu, options: IMenuActionOptions | undefined, target: IAction[] | { primary: IAction[]; secondary: IAction[] }, primaryGroup?: string): IDisposable {
+export function createAndFillInContextMenuActions(menu: IMenu, options: IMenuActionOptions | undefined, target: IAction[] | { primary: IAction[]; secondary: IAction[] }, primaryGroup?: string): void {
 	const groups = menu.getActions(options);
 	const modifierKeyEmitter = ModifierKeyEmitter.getInstance();
 	const useAlternativeActions = modifierKeyEmitter.keyStatus.altKey || ((isWindows || isLinux) && modifierKeyEmitter.keyStatus.shiftKey);
 	fillInActions(groups, target, useAlternativeActions, primaryGroup ? actionGroup => actionGroup === primaryGroup : actionGroup => actionGroup === 'navigation');
-	return asDisposable(groups);
 }
 
-export function createAndFillInActionBarActions(menu: IMenu, options: IMenuActionOptions | undefined, target: IAction[] | { primary: IAction[]; secondary: IAction[] }, primaryGroup?: string | ((actionGroup: string) => boolean), primaryMaxCount?: number, shouldInlineSubmenu?: (action: SubmenuAction, group: string, groupSize: number) => boolean, useSeparatorsInPrimaryActions?: boolean): IDisposable {
+export function createAndFillInActionBarActions(menu: IMenu, options: IMenuActionOptions | undefined, target: IAction[] | { primary: IAction[]; secondary: IAction[] }, primaryGroup?: string | ((actionGroup: string) => boolean), primaryMaxCount?: number, shouldInlineSubmenu?: (action: SubmenuAction, group: string, groupSize: number) => boolean, useSeparatorsInPrimaryActions?: boolean): void {
 	const groups = menu.getActions(options);
 	const isPrimaryAction = typeof primaryGroup === 'string' ? (actionGroup: string) => actionGroup === primaryGroup : primaryGroup;
 
 	// Action bars handle alternative actions on their own so the alternative actions should be ignored
 	fillInActions(groups, target, false, isPrimaryAction, primaryMaxCount, shouldInlineSubmenu, useSeparatorsInPrimaryActions);
-	return asDisposable(groups);
 }
 
-function asDisposable(groups: ReadonlyArray<[string, ReadonlyArray<MenuItemAction | SubmenuItemAction>]>): IDisposable {
-	const disposables = new DisposableStore();
-	for (const [, actions] of groups) {
-		for (const action of actions) {
-			disposables.add(action);
-		}
-	}
-	return disposables;
-}
-
-
 function fillInActions(
 	groups: ReadonlyArray<[string, ReadonlyArray<MenuItemAction | SubmenuItemAction>]>, target: IAction[] | { primary: IAction[]; secondary: IAction[] },
 	useAlternativeActions: boolean,

src/vs/platform/actions/common/actions.ts

@@ -442,12 +442,6 @@ export class MenuItemAction implements IAction {
 		}
 	}
 
-	dispose(): void {
-		// there is NOTHING to dispose and the MenuItemAction should
-		// never have anything to dispose as it is a convenience type
-		// to bridge into the rendering world.
-	}
-
 	run(...args: any[]): Promise<void> {
 		let runArgs: any[] = [];
 

src/vs/platform/actions/common/menuService.ts

@@ -260,7 +260,6 @@ class Menu implements IMenu {
 					} else {
 						action = new SubmenuItemAction(item, this._menuService, this._contextKeyService, options);
 						if (action.actions.length === 0) {
-							action.dispose();
 							action = undefined;
 						}
 					}

src/vs/platform/quickinput/browser/commandsQuickAccess.ts

@@ -55,10 +55,10 @@ export abstract class AbstractCommandsQuickAccessProvider extends PickerQuickAcc
 		this.options = options;
 	}
 
-	protected async _getPicks(filter: string, disposables: DisposableStore, token: CancellationToken): Promise<Array<ICommandQuickPick | IQuickPickSeparator>> {
+	protected async _getPicks(filter: string, _disposables: DisposableStore, token: CancellationToken): Promise<Array<ICommandQuickPick | IQuickPickSeparator>> {
 
 		// Ask subclass for all command picks
-		const allCommandPicks = await this.getCommandPicks(disposables, token);
+		const allCommandPicks = await this.getCommandPicks(token);
 
 		if (token.isCancellationRequested) {
 			return [];
@@ -176,7 +176,7 @@ export abstract class AbstractCommandsQuickAccessProvider extends PickerQuickAcc
 	/**
 	 * Subclasses to provide the actual command entries.
 	 */
-	protected abstract getCommandPicks(disposables: DisposableStore, token: CancellationToken): Promise<Array<ICommandQuickPick>>;
+	protected abstract getCommandPicks(token: CancellationToken): Promise<Array<ICommandQuickPick>>;
 }
 
 interface ISerializedCommandHistory {

src/vs/workbench/api/browser/mainThreadMessageService.ts

@@ -5,15 +5,13 @@
 
 import * as nls from 'vs/nls';
 import Severity from 'vs/base/common/severity';
-import { Action, IAction } from 'vs/base/common/actions';
+import { IAction, toAction } from 'vs/base/common/actions';
 import { MainThreadMessageServiceShape, MainContext, MainThreadMessageOptions } from '../common/extHost.protocol';
 import { extHostNamedCustomer, IExtHostContext } from 'vs/workbench/services/extensions/common/extHostCustomers';
 import { IDialogService } from 'vs/platform/dialogs/common/dialogs';
 import { INotificationService } from 'vs/platform/notification/common/notification';
 import { Event } from 'vs/base/common/event';
 import { ICommandService } from 'vs/platform/commands/common/commands';
-import { dispose } from 'vs/base/common/lifecycle';
-import { ExtensionIdentifier } from 'vs/platform/extensions/common/extensions';
 
 @extHostNamedCustomer(MainContext.MainThreadMessageService)
 export class MainThreadMessageService implements MainThreadMessageServiceShape {
@@ -43,28 +41,15 @@ export class MainThreadMessageService implements MainThreadMessageServiceShape {
 
 		return new Promise<number | undefined>(resolve => {
 
-			const primaryActions: MessageItemAction[] = [];
-
-			class MessageItemAction extends Action {
-				constructor(id: string, label: string, handle: number) {
-					super(id, label, undefined, true, () => {
-						resolve(handle);
-						return Promise.resolve();
-					});
-				}
-			}
-
-			class ManageExtensionAction extends Action {
-				constructor(id: ExtensionIdentifier, label: string, commandService: ICommandService) {
-					super(id.value, label, undefined, true, () => {
-						return commandService.executeCommand('_extensions.manage', id.value);
-					});
+			const primaryActions: IAction[] = commands.map(command => toAction({
+				id: `_extension_message_handle_${command.handle}`,
+				label: command.title,
+				enabled: true,
+				run: () => {
+					resolve(command.handle);
+					return Promise.resolve();
 				}
-			}
-
-			commands.forEach(command => {
-				primaryActions.push(new MessageItemAction('_extension_message_handle_' + command.handle, command.title, command.handle));
-			});
+			}));
 
 			let source: string | { label: string; id: string } | undefined;
 			if (options.source) {
@@ -80,7 +65,13 @@ export class MainThreadMessageService implements MainThreadMessageServiceShape {
 
 			const secondaryActions: IAction[] = [];
 			if (options.source) {
-				secondaryActions.push(new ManageExtensionAction(options.source.identifier, nls.localize('manageExtension', "Manage Extension"), this._commandService));
+				secondaryActions.push(toAction({
+					id: options.source.identifier.value,
+					label: nls.localize('manageExtension', "Manage Extension"),
+					run: () => {
+						return this._commandService.executeCommand('_extensions.manage', options.source!.identifier.value);
+					}
+				}));
 			}
 
 			const messageHandle = this._notificationService.notify({
@@ -93,8 +84,6 @@ export class MainThreadMessageService implements MainThreadMessageServiceShape {
 			// if promise has not been resolved yet, now is the time to ensure a return value
 			// otherwise if already resolved it means the user clicked one of the buttons
 			Event.once(messageHandle.onDidClose)(() => {
-				dispose(primaryActions);
-				dispose(secondaryActions);
 				resolve(undefined);
 			});
 		});

src/vs/workbench/browser/actions.ts

@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { IAction } from 'vs/base/common/actions';
-import { Disposable, DisposableStore, IDisposable, MutableDisposable } from 'vs/base/common/lifecycle';
+import { Disposable, DisposableStore, IDisposable } from 'vs/base/common/lifecycle';
 import { Emitter, Event } from 'vs/base/common/event';
 import { MenuId, IMenuService, IMenu, SubmenuItemAction, IMenuActionOptions } from 'vs/platform/actions/common/actions';
 import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
@@ -43,7 +43,7 @@ class MenuActions extends Disposable {
 		this.disposables.clear();
 		this._primaryActions = [];
 		this._secondaryActions = [];
-		this.disposables.add(createAndFillInActionBarActions(this.menu, this.options, { primary: this._primaryActions, secondary: this._secondaryActions }));
+		createAndFillInActionBarActions(this.menu, this.options, { primary: this._primaryActions, secondary: this._secondaryActions });
 		this.disposables.add(this.updateSubmenus([...this._primaryActions, ...this._secondaryActions], {}));
 		this._onDidChange.fire();
 	}
@@ -66,7 +66,6 @@ class MenuActions extends Disposable {
 export class CompositeMenuActions extends Disposable {
 
 	private readonly menuActions: MenuActions;
-	private readonly contextMenuActionsDisposable = this._register(new MutableDisposable());
 
 	private _onDidChange = this._register(new Emitter<void>());
 	readonly onDidChange: Event<void> = this._onDidChange.event;
@@ -98,7 +97,7 @@ export class CompositeMenuActions extends Disposable {
 
 		if (this.contextMenuId) {
 			const menu = this.menuService.createMenu(this.contextMenuId, this.contextKeyService);
-			this.contextMenuActionsDisposable.value = createAndFillInActionBarActions(menu, this.options, { primary: [], secondary: actions });
+			createAndFillInActionBarActions(menu, this.options, { primary: [], secondary: actions });
 			menu.dispose();
 		}
 

src/vs/workbench/browser/parts/activitybar/activitybarActions.ts

@@ -191,11 +191,9 @@ class MenuActivityActionViewItem extends ActivityActionViewItem {
 		}
 	}
 
-	protected async resolveMainMenuActions(menu: IMenu, disposables: DisposableStore): Promise<IAction[]> {
+	protected async resolveMainMenuActions(menu: IMenu, _disposable: DisposableStore): Promise<IAction[]> {
 		const actions: IAction[] = [];
-
-		disposables.add(createAndFillInActionBarActions(menu, undefined, { primary: [], secondary: actions }));
-
+		createAndFillInActionBarActions(menu, undefined, { primary: [], secondary: actions });
 		return actions;
 	}
 
@@ -276,7 +274,7 @@ export class AccountsActivityActionViewItem extends MenuActivityActionViewItem {
 						providerSubMenuActions.push(signOutAction);
 					}
 
-					const providerSubMenu = disposables.add(new SubmenuAction('activitybar.submenu', `${accountName} (${providerDisplayName})`, providerSubMenuActions));
+					const providerSubMenu = new SubmenuAction('activitybar.submenu', `${accountName} (${providerDisplayName})`, providerSubMenuActions);
 					menus.push(providerSubMenu);
 				});
 			} else {

src/vs/workbench/browser/parts/compositeBarActions.ts

@@ -7,7 +7,7 @@ import { localize } from 'vs/nls';
 import { Action, IAction, Separator } from 'vs/base/common/actions';
 import { $, addDisposableListener, append, clearNode, EventHelper, EventType, getDomNodePagePosition, hide, show } from 'vs/base/browser/dom';
 import { ICommandService } from 'vs/platform/commands/common/commands';
-import { dispose, toDisposable, MutableDisposable, DisposableStore } from 'vs/base/common/lifecycle';
+import { toDisposable, MutableDisposable, DisposableStore, disposeIfDisposable } from 'vs/base/common/lifecycle';
 import { IContextMenuService } from 'vs/platform/contextview/browser/contextView';
 import { IThemeService, IColorTheme, ThemeIcon } from 'vs/platform/theme/common/themeService';
 import { TextBadge, NumberBadge, IBadge, IconBadge, ProgressBadge } from 'vs/workbench/services/activity/common/activity';
@@ -472,7 +472,7 @@ export class CompositeOverflowActivityActionViewItem extends ActivityActionViewI
 
 	showMenu(): void {
 		if (this.actions) {
-			dispose(this.actions);
+			disposeIfDisposable(this.actions);
 		}
 
 		this.actions = this.getActions();
@@ -481,7 +481,7 @@ export class CompositeOverflowActivityActionViewItem extends ActivityActionViewI
 			getAnchor: () => this.container,
 			getActions: () => this.actions,
 			getCheckedActionsRepresentation: () => 'radio',
-			onHide: () => dispose(this.actions)
+			onHide: () => disposeIfDisposable(this.actions)
 		});
 	}
 
@@ -512,7 +512,7 @@ export class CompositeOverflowActivityActionViewItem extends ActivityActionViewI
 		super.dispose();
 
 		if (this.actions) {
-			this.actions = dispose(this.actions);
+			this.actions = disposeIfDisposable(this.actions);
 		}
 	}
 }

src/vs/workbench/browser/parts/editor/editorGroupView.ts

@@ -26,7 +26,7 @@ import { IEditorProgressService } from 'vs/platform/progress/common/progress';
 import { EditorProgressIndicator } from 'vs/workbench/services/progress/browser/progressIndicator';
 import { localize } from 'vs/nls';
 import { coalesce, firstOrDefault } from 'vs/base/common/arrays';
-import { combinedDisposable, dispose, MutableDisposable, toDisposable } from 'vs/base/common/lifecycle';
+import { MutableDisposable, toDisposable } from 'vs/base/common/lifecycle';
 import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
 import { DeferredPromise, Promises, RunOnceWorker } from 'vs/base/common/async';
 import { EventType as TouchEventType, GestureEvent } from 'vs/base/browser/touch';
@@ -341,18 +341,15 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
 		const updateContainerToolbar = () => {
 			const actions: { primary: IAction[]; secondary: IAction[] } = { primary: [], secondary: [] };
 
-			this.containerToolBarMenuDisposable.value = combinedDisposable(
+			// Clear old actions
+			this.containerToolBarMenuDisposable.value = toDisposable(() => containerToolbar.clear());
 
-				// Clear old actions
-				toDisposable(() => containerToolbar.clear()),
-
-				// Create new actions
-				createAndFillInActionBarActions(
-					containerToolbarMenu,
-					{ arg: { groupId: this.id }, shouldForwardArgs: true },
-					actions,
-					'navigation'
-				)
+			// Create new actions
+			createAndFillInActionBarActions(
+				containerToolbarMenu,
+				{ arg: { groupId: this.id }, shouldForwardArgs: true },
+				actions,
+				'navigation'
 			);
 
 			for (const action of [...actions.primary, ...actions.secondary]) {
@@ -385,15 +382,14 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
 
 		// Fill in contributed actions
 		const actions: IAction[] = [];
-		const actionsDisposable = createAndFillInContextMenuActions(menu, undefined, actions);
+		createAndFillInContextMenuActions(menu, undefined, actions);
 
 		// Show it
 		this.contextMenuService.showContextMenu({
 			getAnchor: () => anchor,
 			getActions: () => actions,
 			onHide: () => {
 				this.focus();
-				dispose(actionsDisposable);
 			}
 		});
 	}