Review usage/overrides of get.*Actions on ViewPanes

[sbatten]

With the introduction of View Menu Actions its possible for ViewPanes to accidentally override them. We should verify if this is happening and consider if we still want View Menu Actions with DnD for views already introduced.
/cc @eamodio

Updated Description

Currently ViewPanes and ViewPaneContainers can contribute Primary, Secondary and ContextMenu actions in following ways

• Primary actions

  • Provide using getActions
  • Register to ViewContainerTitle menu with navigation group
  • Register to ViewTitle menu with navigation group

• Secondary actions

  • Provide using getSecondaryActions
  • Register to ViewContainerTitle menu
  • Register to ViewTitle menu

• Context menu actions

  • Provide using getContextMenuActions
  • Register to ViewContainerTitleContext menu
  • Register to ViewTitleContext menu

We would like to move away from get*Actions and use Menu registry because

• All actions shall be command driven so that user can assign keybindings
• Facilitates providing customizable menus
• Improves code quality
  • One consistent way to register actions across workbench
  • Allows removing redundant code if the same action is used at multiple places
  • Helps in removing ambiguity of actions lifecycle and simplifies views as they do not need to maintain actions state

I will be removing the get*Actions method after everyone move away from it. I listed the existing consumers of this and assigned the respective owner. Please adopt using Menu registries.

Here are some tips that might help you while adopting

• If you have a custom UI for your action, you can override getActionViewItem like before and provide custom implementation.

Example:

vscode/src/vs/workbench/contrib/markers/browser/markersView.ts

Lines 852 to 857 in af29768

	public getActionViewItem(action: IAction): IActionViewItem | undefined {
	if (action.id === `workbench.actions.treeView.${this.id}.filter`) {
	return this.instantiationService.createInstance(MarkersFilterActionViewItem, action, this);
	}
	return super.getActionViewItem(action);
	}

• If your action needs access to your view's or view container's state, you can use ViewAction or ViewPaneContainerAction. They will pass our view or viewPaneContainer as arg in run method

Example:

vscode/src/vs/workbench/contrib/outline/browser/outlinePane.ts

Lines 641 to 660 in 56e35cf

	registerAction2(class Collapse extends ViewAction<OutlinePane> {
	constructor() {
	super({
	viewId: OutlineViewId,
	id: 'outline.collapse',
	title: localize('collapse', "Collapse All"),
	f1: false,
	icon: Codicon.collapseAll,
	menu: {
	id: MenuId.ViewTitle,
	group: 'navigation',
	when: ContextKeyEqualsExpr.create('view', OutlineViewId)
	}
	});
	}
	runInView(_accessor: ServicesAccessor, view: OutlinePane) {
	view.collapseAll();
	}
	});

To Adopt

• Search - @roblourens

  • vscode/src/vs/workbench/contrib/search/browser/searchView.ts

    Line 1849 in 2ecb47d

    getActions(): IAction[] {

• Terminal - @Tyriar @meganrogge

  • vscode/src/vs/workbench/contrib/terminal/browser/terminalView.ts

    Line 157 in 0ef0d2d

    public getActions(): IAction[] {

[sbatten]

Looking a little more at this, there may be other uses?

[bpasero]

@sbatten @sandy081 I wanted to follow up on a related topic, not sure if it is the same as this issue: today, Composite has methods getActions and getSecondaryActions (even getContextMenuActions?). We probably have other view related pieces with this kind of API.

That API should be deprecated imho and removed in favour of registering commands to Menu locations, as we e.g. do with all editor related actions in the editor tool bar. There should be no reason that any piece of UI is required to return actions upfront from these methods and every location should be capable of receiving actions with proper menu contributions and commands.

Example for the proper way of registering actions:

vscode/src/vs/workbench/browser/parts/editor/editor.contribution.ts

Line 496 in 9f7b100

MenuRegistry.appendMenuItem(MenuId.EditorTitleContext, { command: { id: editorCommands.CLOSE_OTHER_EDITORS_IN_GROUP_COMMAND_ID, title: nls.localize('closeOthers', "Close Others"), precondition: EditorGroupEditorsCountContext.notEqualsTo('1') }, group: '1_close', order: 20 });

[sandy081]

@sbatten Not sure if I understand this debt item. Can you please elaborate more?

[sandy081]

@bpasero Yes, I completely agree.

Thanks to @jrieken who provided support for contributing menu actions to any view. I discussed with him then and discussed about the future plan of moving away from get*Actions to MenuRegistry.

We can use this item to migrate.

[sandy081]

I also quickly looked into this then and figured out that there are some actions which need ActionItem infrastructure. For eg:

• FilterAction in problems panel
• `Dropdowns in Terminal and Output panel etc.,

[eamodio]

FWIW, When adding commands to the Timeline view -- I first added them through the MenuRegistry and context keys, but I ultimately removed all of that and moved to get*Actions because it was so much easier, especially dealing with internal view state. Also the Timeline has a custom list of "filters" which are the labels of the sources registered, so hooking that into getSecondaryActions was quite trivial, where as to provide that with the MenuRegistry would require a lot more indirection and complexity.

[sandy081]

@eamodio Can you please provide an example of the complexity and indirection you mentioned. At the end you need to have a logical condition to show an action in get*Actions or through MenuRegistry. Is not it possible to compute the logical state in your view and set that as context?

[eamodio]

Ultimately it turned out not to be a big deal -- I converted Timeline back to using the MenuRegistry via registerAction2

[sbatten]

If I understand the desired action from @bpasero and @sandy081, this is the list of usages that should be migrated. Perhaps we can have view owners migrate during upcoming debt week.

vscode/src/vs/workbench/contrib/comments/browser/commentsView.ts

Lines 122 to 129 in 15c02dc

	public getActions(): IAction[] {
	if (!this.collapseAllAction) {
	this.collapseAllAction = new Action('vs.tree.collapse', nls.localize('collapseAll', "Collapse All"), 'collapse-all', true, () => this.tree ? new CollapseAllAction<any, any>(this.tree, true).run() : Promise.resolve());
	this._register(this.collapseAllAction);
	}
	return [this.collapseAllAction];
	}

vscode/src/vs/workbench/contrib/debug/browser/breakpointsView.ts

Lines 231 to 237 in c650757

	public getActions(): IAction[] {
	return [
	new AddFunctionBreakpointAction(AddFunctionBreakpointAction.ID, AddFunctionBreakpointAction.LABEL, this.debugService, this.keybindingService),
	new ToggleBreakpointsActivatedAction(ToggleBreakpointsActivatedAction.ID, ToggleBreakpointsActivatedAction.ACTIVATE_LABEL, this.debugService, this.keybindingService),
	new RemoveAllBreakpointsAction(RemoveAllBreakpointsAction.ID, RemoveAllBreakpointsAction.LABEL, this.debugService, this.keybindingService)
	];
	}

vscode/src/vs/workbench/contrib/debug/browser/callStackView.ts

Lines 182 to 188 in 7653d83

	getActions(): IAction[] {
	if (this.pauseMessage.hidden) {
	return [new CollapseAction(() => this.tree, true, 'explorer-action codicon-collapse-all')];
	}
	return [];
	}

vscode/src/vs/workbench/contrib/debug/browser/repl.ts

Lines 446 to 456 in e3a36fa

	getActions(): IAction[] {
	const result: IAction[] = [];
	if (this.debugService.getModel().getSessions(true).filter(s => s.hasSeparateRepl() && !sessionsToIgnore.has(s)).length > 1) {
	result.push(this.selectReplAction);
	}
	result.push(this.clearReplAction);
	result.forEach(a => this._register(a));
	return result;
	}

vscode/src/vs/workbench/contrib/debug/browser/variablesView.ts

Lines 153 to 155 in 9a2062f

	getActions(): IAction[] {
	return [new CollapseAction(() => this.tree, true, 'explorer-action codicon-collapse-all')];
	}

vscode/src/vs/workbench/contrib/debug/browser/watchExpressionsView.ts

Lines 153 to 159 in de3e5a4

	getActions(): IAction[] {
	return [
	new AddWatchExpressionAction(AddWatchExpressionAction.ID, AddWatchExpressionAction.LABEL, this.debugService, this.keybindingService),
	new CollapseAction(() => this.tree, true, 'explorer-action codicon-collapse-all'),
	new RemoveAllWatchExpressionsAction(RemoveAllWatchExpressionsAction.ID, RemoveAllWatchExpressionsAction.LABEL, this.debugService, this.keybindingService)
	];
	}

vscode/src/vs/workbench/contrib/extensions/browser/extensionsViews.ts

Lines 923 to 930 in 86161ff

	getActions(): IAction[] {
	if (this.extensionManagementServerService.remoteExtensionManagementServer && this.extensionManagementServerService.localExtensionManagementServer === this.server) {
	const installLocalExtensionsInRemoteAction = this._register(this.instantiationService.createInstance(InstallLocalExtensionsInRemoteAction));
	installLocalExtensionsInRemoteAction.class = 'codicon codicon-cloud-download';
	return [installLocalExtensionsInRemoteAction];
	}
	return [];
	}

vscode/src/vs/workbench/contrib/extensions/browser/extensionsViews.ts

Lines 1020 to 1029 in 86161ff

	getActions(): IAction[] {
	if (!this.installAllAction) {
	this.installAllAction = this._register(this.instantiationService.createInstance(InstallWorkspaceRecommendedExtensionsAction, InstallWorkspaceRecommendedExtensionsAction.ID, InstallWorkspaceRecommendedExtensionsAction.LABEL, []));
	this.installAllAction.class = 'codicon codicon-cloud-download';
	}
	const configureWorkspaceFolderAction = this._register(this.instantiationService.createInstance(ConfigureWorkspaceFolderRecommendedExtensionsAction, ConfigureWorkspaceFolderRecommendedExtensionsAction.ID, ConfigureWorkspaceFolderRecommendedExtensionsAction.LABEL));
	configureWorkspaceFolderAction.class = 'codicon codicon-pencil';
	return [this.installAllAction, configureWorkspaceFolderAction];
	}

vscode/src/vs/workbench/contrib/files/browser/views/explorerView.ts

Lines 287 to 298 in 187dedb

	getActions(): IAction[] {
	if (!this.actions) {
	this.actions = [
	this.instantiationService.createInstance(NewFileAction),
	this.instantiationService.createInstance(NewFolderAction),
	this.instantiationService.createInstance(RefreshExplorerView, RefreshExplorerView.ID, RefreshExplorerView.LABEL),
	this.instantiationService.createInstance(CollapseExplorerView, CollapseExplorerView.ID, CollapseExplorerView.LABEL)
	];
	this.actions.forEach(a => this._register(a));
	}
	return this.actions;
	}

vscode/src/vs/workbench/contrib/files/browser/views/openEditorsView.ts

Lines 308 to 314 in 7653d83

	getActions(): IAction[] {
	return [
	this.instantiationService.createInstance(ToggleEditorLayoutAction, ToggleEditorLayoutAction.ID, ToggleEditorLayoutAction.LABEL),
	this.instantiationService.createInstance(SaveAllAction, SaveAllAction.ID, SaveAllAction.LABEL),
	this.instantiationService.createInstance(CloseAllEditorsAction, CloseAllEditorsAction.ID, CloseAllEditorsAction.LABEL)
	];
	}

vscode/src/vs/workbench/contrib/outline/browser/outlinePane.ts

Lines 411 to 417 in 7bd4ccb

	getActions(): IAction[] {
	return [
	new Action('collapse', localize('collapse', "Collapse All"), 'explorer-action codicon-collapse-all', true, () => {
	return new CollapseAction(() => this._tree, true, undefined).run();
	})
	];
	}

vscode/src/vs/workbench/contrib/remote/browser/tunnelView.ts

Lines 575 to 577 in 25e889a

	getActions(): IAction[] {
	return this.titleActions;
	}

vscode/src/vs/workbench/contrib/scm/browser/repositoryPane.ts

Lines 1062 to 1073 in 1a95f31

	getActions(): IAction[] {
	if (this.toggleViewModelModeAction) {
	return [
	this.toggleViewModelModeAction,
	...this.menus.getTitleActions()
	];
	} else {
	return this.menus.getTitleActions();
	}
	}

vscode/src/vs/workbench/contrib/search/browser/searchView.ts

Lines 1779 to 1787 in 79a1f5a

	getActions(): IAction[] {
	return [
	this.state === SearchUIState.SlowSearch ?
	this.cancelAction :
	this.refreshAction,
	...this.actions,
	this.toggleCollapseAction
	];
	}

vscode/src/vs/workbench/contrib/terminal/browser/terminalView.ts

Lines 131 to 145 in 8a56212

	public getActions(): IAction[] {
	if (!this._actions) {
	this._splitTerminalAction = this._instantiationService.createInstance(SplitTerminalAction, SplitTerminalAction.ID, SplitTerminalAction.LABEL);
	this._actions = [
	this._instantiationService.createInstance(SwitchTerminalAction, SwitchTerminalAction.ID, SwitchTerminalAction.LABEL),
	this._instantiationService.createInstance(CreateNewTerminalAction, CreateNewTerminalAction.ID, CreateNewTerminalAction.SHORT_LABEL),
	this._splitTerminalAction,
	this._instantiationService.createInstance(KillTerminalAction, KillTerminalAction.ID, KillTerminalAction.PANEL_LABEL)
	];
	this._actions.forEach(a => {
	this._register(a);
	});
	}
	return this._actions;
	}

vscode/src/vs/workbench/contrib/outline/browser/outlinePane.ts

Lines 419 to 436 in 7bd4ccb

	getSecondaryActions(): IAction[] {
	const group = this._register(new RadioGroup([
	new SimpleToggleAction(this._outlineViewState, localize('sortByPosition', "Sort By: Position"), () => this._outlineViewState.sortBy === OutlineSortOrder.ByPosition, _ => this._outlineViewState.sortBy = OutlineSortOrder.ByPosition),
	new SimpleToggleAction(this._outlineViewState, localize('sortByName', "Sort By: Name"), () => this._outlineViewState.sortBy === OutlineSortOrder.ByName, _ => this._outlineViewState.sortBy = OutlineSortOrder.ByName),
	new SimpleToggleAction(this._outlineViewState, localize('sortByKind', "Sort By: Category"), () => this._outlineViewState.sortBy === OutlineSortOrder.ByKind, _ => this._outlineViewState.sortBy = OutlineSortOrder.ByKind),
	]));
	const result = [
	new SimpleToggleAction(this._outlineViewState, localize('followCur', "Follow Cursor"), () => this._outlineViewState.followCursor, action => this._outlineViewState.followCursor = action.checked),
	new SimpleToggleAction(this._outlineViewState, localize('filterOnType', "Filter on Type"), () => this._outlineViewState.filterOnType, action => this._outlineViewState.filterOnType = action.checked),
	new Separator(),
	...group.actions,
	];
	for (const r of result) {
	this._register(r);
	}
	return result;
	}

vscode/src/vs/workbench/contrib/scm/browser/repositoryPane.ts

Lines 1074 to 1076 in 1a95f31

	getSecondaryActions(): IAction[] {
	return this.menus.getTitleSecondaryActions();
	}

[sandy081]

@isidorn Sorry that my previous list was missing one view (CallStacksView). Added it and assigned to you.

[isidorn]

@sandy081 somehow I missed that one. I just pushed a commit that tackles that. Thanks

[joaomoreno]

@eamodio FYI I started with this for SCM.

[joaomoreno]

Edit: I was convinced in ZH's standup to push this ahead. Please ignore my rant below.

---

@sbatten I started this for SCM: https://github.com/microsoft/vscode/compare/joao/scm-menu-actions

After 1 hour of work I'm not even 10% done, so I decided to pause it.

There are usually two reasons for forcing everyone to invest their time in large refactorings:

1. To benefit from fixes / new features
2. To eventually delete the old way of doing things

Today, actions in SCM work just fine and don't leak. Jumping to menus is a big investment since not only all actions need to be converted but all the view state they depend on needs to be accessible via context keys. This is considerable effort, even without the fixing of the eventual bugs which always come after a large refactoring. I can see that moving to menus could enable user customizable menus in the future. But until this is on the near future plan, I do not think it is reasonable to invest so much time in refactoring for no added immediate benefit.

How necessary is it that all parts of Code move and could we maintain both menus and getActions until we decide to implement a feature which actually depends on views only having actions via menus?

[joaomoreno]

Might just push this out to February's debt week, since it's hard to do a staged rollout of this.

[sandy081]

Thanks @joaomoreno - I also updated the description with the benefits and reasons behind this.

[Tyriar]

I started this but will also push it to Feb so it gets more testing before stable.

[joaomoreno]

@eamodio I've refactored the SCM world for this: e83180b I smoke tested it quite heavily, but still be on the lookout for upcoming issues surrounding menu items in SCM, feel free to assign them to me as they come.

[Tyriar]

I've done my piece here. It was a lot of changes so I'll also be on the lookout for issues but the terminal action code is in a much better shape as a result of this, thanks!