Encapsulate argument adapter behavior in menu nodes

[colin-grant-work]

What it does

Fixes #16132, fixes #16082

An alternative to #16148

#11290 introduced the requirement that participants in the menu system pass around menu paths, labeled effectiveMenuPath, in a rather opaque way as an aid to recovering the behavior expected by commands contributed to particular menus by VSCode extensions. #14676 has introduced the possibility to handle that requirement better through better encapsulation of the argument adapter behavior in the relevant menu nodes and remove the very widespread passing of menu nodes for the benefit of a tiny subset of commands. This PR refactors the extension menu handling logic to better encapsulate the expected behavior and then does its best to remove all obsolete MenuPath passing.

How to test

1. The Git actions exposed through the tabbar should work, fixing Cannot "Commit Staged (Signed Off)" at all #16132.
2. The logic from Ensure context menu arguments are propagated to submenus #16083 is restored, fixing Custom context menu arguments are not passed to submenus #16082
3. Application-contributed menu and toolbar items should work.
4. Extension-contributed menu and toolbar items should work.

Follow-ups

Breaking changes

• This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[tsmaeder]

Hmh...I'm not really sure this PR is conceptually all that much different from the other one: in the end, we need to keep track of where a command is called for, here, it's done by wrapping the items inside the submenu link and passing down the argument adapter, whereas in the other PR it's done by passing down the "effective menu path". It kinda feels par for the course.
Maybe the right approach would be to give menu items access to the parent chain they are invoked from: actions could then walk up the parent chain to find an annotation on the nodes like an argument adapter or a contribution point. That might get rid of all the wrapping steps.

[tsmaeder]

I have a prototype I can share on Monday.

[colin-grant-work]

we need to keep track of where a command is called for, here, it's done by wrapping the items inside the submenu link and passing down the argument adapter, whereas in the other PR it's done by passing down the "effective menu path". It kinda feels par for the course.

To me, the key difference is where it's known. Here it needs to be known at the point that we register something where we know the VSCode contribution point, and it's encapsulated in that registration and passed down to any children. With the 'effective menu path' approach, it has to be known when handle something that we expect to have a specific adapter, which means that the menu path has to be passed around to all potential handlers, and that's both verbose and error-prone, because most people won't know why they're passing it at all, or exactly what they need to pass to get the 'right' behavior. To me, that's the key difference between the system before and after the menu refactor: the refactor moved a lot of the work we were doing to recover desired behavior at handler time up to registration time through behavior of menu nodes.

[tsmaeder]

Here it needs to be known at the point that we register something where we know the VSCode contribution point, and it's encapsulated in that registration and passed down to any children.

Sorry, but I don't agree with this point. The "effective menu path" is the context we're calling a menu item in. We need the whole wrapping because we're losing that context when we wrap menu items into toolbar items. At least that's a general concept, whereas the "argumentAdapter" is more specific to the plugin contribution handler and it leaks outside of the plugin-ext package.
I feel that if I knew nothing about the menu contribution handler, "here's the menu path your action was called from" is easier to make sense of than "here's a command adapter we need to carry around".

[colin-grant-work]

At least that's a general concept, whereas the "argumentAdapter" is more specific to the plugin contribution handler and it leaks outside of the plugin-ext package.

The trouble is it isn't a general concept: it was required exclusively for commands from plugins. The benefit of the adapter menu node code is that it's local: you can see looking at the menu node that it has an adapter, and that it makes sure any children it reports get that adapter. You also don't have to do anything if you don't care about the adapter: the person who registered the node already took care of everything. In the case of the effective menu path, everyone who dealt with menus at all had to pass it around without knowing why unless they tracked down the specific plugin-ext code that actually used it for something. That's why we ended up with a bug in the first place: even you, more familiar with Theia's code than just about anybody, didn't realize what you needed to pass or why. This approach is also better traceable: you can easily follow the call hierarchy for the node constructor and see where they're created and why; to figure out why the menu path was passed, you would have had to find all command registrations, then check every handler to see if it received (and actually used!) something like a menu path.

[ndoschek]

As discussed, we close this one in favour of #16148 for now.

Edit: Reopened, it seems clear there is value in this approach and in keeping the discussion active.