Don't listen on menu changed, method 2

[alexr00]

This time, instead of only updating the tree elements who's actions are visible (selected, focused, focusing), update all tree items that the context change applies to, as before. However, the context key change event is now per tree renderer, not per menu.
Fixes #213145

[alexr00]

@bpasero, are you the right person to review menuService changes?

[bpasero]

Yielding for Jo.

[jrieken]

nerd snipped

[jrieken]

I like this but I would suggest to move it out of IMenu and into IMenuService, like the following

export interface IMenuService {
    // ... existing stuff

  getMenuContexts(id: MenuId): ReadonlySet<string>;
}

The implementation would be like yours: create, read and dispose the actual menu. We could then complement this with IMenuService#getMenuActions(id: MenudId, options?: IMenuActionOptions): [string, Array<MenuItemAction | SubmenuItemAction>][];) which would be the equivalent of: create menu, get actions, dispose menu.

Those two methods would be the "dead" counter-part of IMenu (which is listening and emitting). We could also add IMenuService#getMenuData which would encapsulate those two concepts: context and action groups but no eventing. So, maybe alltogether

interface IMenuData {
  contexts(): ReadonlySet<string>;
  actions: [string, Array<MenuItemAction | SubmenuItemAction>][];
}

export interface IMenuService {
    // ... existing stuff

  getMenuData(id: MenuId): IMenuData;
}

IMO this would encapsulate things nicer and clearly separate live IMenu-instances from just menu data which you to handle yourselves.

[alexr00]

Love it. I'll make the change.

[alexr00]

Looking at this further, I'm not sure we should separate the contexts from the menu. At the time menu is created it has a specific set of contexts. If you create that menu again later with a different scoped context key service you could have different contexts and different actions. I would propose that we have the IMenuData, and then have 2 additions to IMenuService:

/**
 * The "dead" counterpart to `createMenu`.
 * For other components to use.
 **/
getMenuActions(id: MenudId, contextKeyService: IContextKeyService, options?: IMenuActionOptions): [string, Array<MenuItemAction | SubmenuItemAction>][];

/**
 * This is what the tree view will use to create a "dead" menu and get the contexts
 **/
getMenuData(id: MenudId, contextKeyService: IContextKeyService, options?: IMenuActionOptions): IMenuData;

[jrieken]

At the time menu is created it has a specific set of contexts. If you create that menu again later with a different scoped context key service you could have different contexts and different actions.

Not contexts but context evaluations. The former are statically defined when you register menus items (mostly via Action2) and those (keys) should be enough to know what to listen on. The menu does exactly that which in convenient but wasteful when you have very many menu instances for the same menu-id.

So, likely we need getMenuActions with the signature you have outlined above and then getMenuData is without the CKS-argument but just returning the context keys (for listening).

[alexr00]

Hmm. I think I'm missing something still then since I think we still need the CKS for getMenuData. The items that get added to the menu depend on the CKS that is passed in. In the tree, we even make an overlay:

vscode/src/vs/workbench/browser/parts/views/treeView.ts

Lines 1701 to 1704 in 31493f3

	const contextKeyService = this.contextKeyService.createOverlay([
	['view', this.id],
	['viewItem', element.contextValue]
	]);

[jrieken]

1. IMenuService#getMenuAction(menuId, CKS, options?) -> update DOM
2. IMenuService#getMenuContext(menuId) -> Set -> CKS -> listen and check with set

getMenuContextKeys(id: MenudId): Set<string>

[alexr00]

@jrieken, this is ready for review again. I will adop the new getMenuActions method in places where it's obvious in a follow up PR.