Menu Extensibility

[colin-grant-work]

At the moment, there are a number of cases in our menu system that refer to two concrete implementations of the MenuNode interface, ActionMenuNode and CompositeMenuNode, and pass off any other MenuNode to a default handler. For example:

theia/packages/core/src/browser/menu/browser-menu-plugin.ts

Lines 121 to 134 in 7445e23

	protected registerMenu(menuCommandRegistry: MenuCommandRegistry, menu: CompositeMenuNode, args: any[]): void {
	for (const child of menu.children) {
	if (child instanceof ActionMenuNode) {
	menuCommandRegistry.registerActionMenu(child, args);
	if (child.altNode) {
	menuCommandRegistry.registerActionMenu(child.altNode, args);
	}
	} else if (child instanceof CompositeMenuNode) {
	this.registerMenu(menuCommandRegistry, child, args);
	} else {
	this.handleDefault(menuCommandRegistry, child, args);
	}
	}
	}

That's an odd sort of universal special-casing, and puts us in an awkward position, because if we want to introduce any new kind of behavior, we have to continue to special-case or inherit from those exact classes - downstream applications (may) rely on the fact that anything else, regardless of interface shape, is passed into the default handler. For example, if we want to have a different kind of menu node with children (as I do in this PR with CompositeMenuNodeWrapper), we can't use a check for .children and then handle it as equivalent to a CompositeMenuNode because that might break a hypothetical DownstreamCompositeMenuNode that has .children but doesn't inherit from CompositeMenuNode and so goes into the default handler. Instead we'd have to implement a class OtherUpstreamCompositeMenuNode and check node instanceof CompositeMenuNode || node instanceof OtherUpstreamCompositeMenuNode. It's not impossible to maintain, but it's inconvenient and points to the fragility of relying on concrete implementations and instanceof checks. It also violates the principle that any downstream override should have higher priority than the upstream implementation: here the upstream implementation of registerMenu (and the other methods with this structure) runs almost in its entirety before the method intended to serve as the override runs at all.

@kittaakos, it looks like you did some of the work that added the handleDefault option. Can you give a sense of the concrete use cases you were aiming to support and whether there might be other ways to get there?

For example, rather than overriding the defaultHandler, would it be possible to achieve the same result with something like (for the method above)

protected override registerMenu(menuCommandRegistry: MenuCommandRegistry, menu: CompositeMenuNode, args: any[]): void {
    if (/* ...check for downstream use cases */) {
        // handle downstream use cases...
    } else {
         super.registerMenu(menuCommandRegistry, menu, args);
    }
}

[kittaakos]

Thanks for the heads-up, @colin-grant-work.

In the Arduino IDE, we want to preserve the menu structure of the previous IDE. That is, there are "label-only" menu items for grouping.

From the previous IDE:

Arduino IDE:

I added an example to Theia here: 9a07349.

For example, rather than overriding the defaultHandler, would it be possible to achieve the same result with something like (for the method above)

We have to try it out with vanilla Theia. If it works and makes the Theia customization more convenient, I do not have anything against breaking the current APIs. I wanted to avoid registering a command and making it permanently disabled to show a label-only menu item. If you have any concrete proposal I can try out, I am more than happy to do it.

[colin-grant-work]

@kittaakos, thanks for clarifying what the intent there was. I've made the relevant methods fully recursive so that they're called on all menu nodes, not just compound menu nodes, and refactored the sample contributions accordingly. I think they're working as you intended, and I think the new code is easier to override for a wider variety of downstream use cases. I'd be happy to hear what you think.

[kittaakos]

Thank you for updating the sample, @colin-grant-work. I looked into your PR, and it looks great. 😍 Once it is available from NPM, I will try the integration.