Refactor PluginMenuContributionHandler

[colin-grant-work]

What it does

Fixes #11264 by introducing support for when clauses in submenu contributions.

Also refactors the PluginMenuContributionHandler and allows for straightforward contribution of menu items to tabbar toolbars.

The refactor was not strictly necessary to allow when clauses in submenus but was motivated by the complexity of the PluginMenuHandler class, which had scattered in it (1) knowledge about matching contribution points to menus, (2) elaborate additional machinery for building a tree to handle all of the places a given submenu might occur, (3) knowledge of how to transform the arguments passed in various contexts to the arguments expected by plugins, and (4) knowledge about how to manage toolbar items contributed from menus. Completely missing from that implementation were (1) handling of icon fields of submenus and (2) the ability to contribute a submenu to a titlebar's `navigation' group and get an item that opens that menu.

In place of that monolith, I've implemented a system that divides those responsibilities in the following ways.

• (1) Since the mapping of contribution points to menus is pretty static, I've just put that in a map. It could be moved to be generated in a method of some injectable class if we believe it should be more easily extensible.
• (2) In place of front-loading the matching of submenus to menus, I've added machinery in the menu system to contribute a submenu and then add it to an existing menu later.
• (3) To handle the need for specific handling of specific commands in specific menus, I've implemented a contributable MenuCommandAdapterRegistry system that can delegate to MenuCommandAdapters to execute specific commands, and a PluginMenuCommandAdapter to handle the argument transformations for particular contexts.
• (4) In place of individual contributions by the MenuContributionHandler to the toolbar system, I've added the ability to add an entire menu to the tabbar system.

In addition, I've added:

• handling for icons from submenu contributions
• handling of toolbar items that should open menus
• integration of toolbar items from menus into the MenuCommandAdapter system
• passing of HTML element contexts into the context menu system for accurate handling of when contexts.
• passing of the top-level menu path into the context menu building system to allow delegation to the MenuCommandAdapter system

Some remaining troublesome bits:

• The code that builds menus is duplicated pretty much line-for-line in the BrowserMenuFactory and ElectronMainMenuFactory.
• The menu system did a lot of instanceof checks for CompositeMenuNode and ActionMenuNode, both of which diverged quite a bit from the MenuNode interface. That makes the system (including downstream customizations) pretty fragile, because it can't easily accept anything not in the class hierarchy of those two kinds. I've tried to expand the interface of MenuNode to include many of the fields present on those implementations and then rely less on instanceof checks
• The code to handle menus in the toolbar system is a bit odd (specific undocumented format for submenus of the more context menu, e.g.), and my code mostly adds special cases on top.

How to test

We support these menu contribution points:

[
    'comments/comment/context', // Description of comment
    'comments/comment/title', // Description of comment
    'comments/commentThread/context', // Description of comment thread
    'debug/callstack/context', // Serialized stack frame
    'editor/context', // URI
    'editor/title', // URI
    'editor/title/context', // URI
    'explorer/context', // URI
    'scm/resourceFolder/context', // Description of SCM resource, but only if using a plugin-contributed SCM provider.
    'scm/resourceGroup/context',  // Description of SCM resource, but only if using a plugin-contributed SCM provider.
    'scm/resourceState/context',  // Description of SCM resource, but only if using a plugin-contributed SCM provider.
    'scm/title',  // Description of SCM resource, but only if using a plugin-contributed SCM provider.
    'timeline/item/context', 
    'view/item/context', // Serialized version of the tree node.
    'view/title' // Nothing
]

Details on their location can be found here (though not as nicely alphabetized :-)).

0. Follow the steps of Plugin-contributed view context menu items pollute other plugins' views #11264. Observe that you don't see GitLens contributions on the NPM view.
1. Install menu-contribution-test-0.0.1.zip. Source here.
2. For every contribution point, one menu item / toolbar item should appear with the label / tooltip Appears in: <contribution point>.
3. Running the associated action should produce a backend log with the form 'SENTINEL FOR THE ARGS WE GET FOR', contributionPoint, '\n', ...args
4. Those messages the args should correspond to (in order of priority of match)
   • What you see in VSCode in the corresponding position.
   • What you see in the same location on master.
   • The prose descriptions above.
5. In addition:
   • On plugin views, you should see a thumbsup/thumbsdown icon. Clicking it should reverse the direction.

   Proves that when clauses are respected for tabbar contributions. Cf. Improve plugin-view toolbar lifecycle #10546

   • In the explorer, the context menu for .ts files should include a Test Submenu; other files should not.

   Proves that when clauses are respected for submenus. Cf. Plugin-contributed view context menu items pollute other plugins' views #11264

   • In the tabbar toolbar for editor widgets, an item with the beaker-stop icon should appear for .ts files only. Clicking on that item should reveal a menu with the label Appears in: menu-contribution-test.submenu.

   Proves that submenus can be registered for /title contribution points, and their when are respected.

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

[msujew]

Looks quite good, at least all menu contribution points mentioned in the testing steps seem to work as expected with the provided test extension.

By the way, while installing Gitlens as a test for this, I noticed that webviews don't display for me in web environments for some reason. I always get a 404 not found error while loading them, even fairly old versions of Theia (tested with 1.26.0, 1.25.0 and 1.20.0). Can you reproduce this issue @colin-grant-work? I didn't want to open a separate issue for this, if it only related to my environment.

[msujew]

Suggestion: I'm usually all in for keeping the code extensible for downstream users. However, I don't believe that it'd be necessary in this case. How about a CodeEditorWidget namespace instead?

[colin-grant-work]

I'd agree, but this class already existed in this form - I just moved it out of the menus-contribution-handler file. Happy to deprecate / remove in favor of a namespace, though.

[colin-grant-work]

@msujew, thanks for taking a look. I'm currently working on a more thorough refactor to support submenu registration better without forcing all plugin items into a single group, since that isn't the behavior intended by VSCode. I'll address your comments - and remove all of my random console logs :-) - as I work through that.

[colin-grant-work]

@planger, would you mind taking a look at the Playwright failures here? The failures have to do with menus, and the PR has to do with menus, so it seems likely that the changes have caused the failures, but I took a bit of a look and couldn't figure out where things are going wrong: the menus that the tests claim to be looking for are present, and the PR doesn't change any of the HTML rendering that they hook onto, as far as I can tell.

[planger]

@colin-grant-work Sure, I'm happy to have a look! I'm currently on vacation though (including next week), but can look into it right after. Is that soon enough? If it can't wait I'll ask someone in our team to have a look.

[colin-grant-work]

@planger, I was able to track down the problem. My code had added trailing separators to menus, and Phosphor was automatically hiding those using a p-mod-collapsed class. That was preventing the label() method from returning, as the selector was never visible. I have added handling to the MenuItem class that checks for that class name and returns empty for its label and its shortcut if that class is found. That way, the code can handle invisible items but correctly treats it as empty / invisible for any checks of the item's status. But I didn't mean to have trailing separators, so I've also removed those in a separate commit.

[planger]

@planger, I was able to track down the problem. My code had added trailing separators to menus, and Phosphor was automatically hiding those using a p-mod-collapsed class. That was preventing the label() method from returning, as the selector was never visible. I have added handling to the MenuItem class that checks for that class name and returns empty for its label and its shortcut if that class is found. That way, the code can handle invisible items but correctly treats it as empty / invisible for any checks of the item's status. But I didn't mean to have trailing separators, so I've also removed those in a separate commit.

@colin-grant-work Great, thanks for making the page object more resilient!

[vince-fugnitto]

LGTM 👍 I confirmed the latest changes work well.