Expanding Submenus contribution support #9371

EstherPerelman · 2021-04-18T15:49:59Z

Signed-off-by: Esther Perelman Esther.Perelman@sap.com

What it does

Enabling contribute submenus not only for "scm/title" menus, But also for all other available contribution menus

How to test

Contribute submenus in any extension package.json following this syntax
Duplicates and cycle submenus are prevented.

For the next code in Package.json:

 "contributes": {
 ...
    "submenus": [
      {
        "id": "ep.custosubmenu1",
        "label": "custom submenu 1"
      },
      {
        "id": "ep.custosubmenu2",
        "label": "custom submenu 2"
      }
    ],
    "menus": {
      "ep.custosubmenu1": [
        {
          "command": "git.publish"
        },
        {
          "submenu": "ep.custosubmenu2"
        }
      ],
      "ep.custosubmenu2": [
        {
          "command": "git.sync",
          "group": "0_ep"
        },
        {
          "submenu": "ep.custosubmenu1"
        }
      ],
      "explorer/context": [
        {
          "command": "git.sync"
        },
        {
          "submenu": "ep.custosubmenu1"
        }
      ],
      "editor/title": [
        {
          "submenu": "ep.custosubmenu1"
        },
        {
          "submenu": "ep.custosubmenu1"
        }
      ]
    }
  },

The result is:

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

EstherPerelman · 2021-04-19T17:35:56Z

@vince-fugnitto @kittaakos @akosyakov , Anyone else...
Can you review please?

colin-grant-work · 2021-04-21T22:18:52Z

I will take a look tomorrow morning.

colin-grant-work · 2021-04-22T14:12:02Z

@EstherPerelman, I'm reading over the code now. In your PR description, you say

For the next code in Package.json:

Do you have a plugin with which to test the code, or should it be possible to copy that code into any plugin's package.json and see the results?

colin-grant-work

I believe this is working well. I've confirmed that the JSON manifest provided produces the menus that it should. I have some questions about the code and some suggestions for how it might be made more legible. Let me know what you think of those ideas.

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts

EstherPerelman · 2021-04-25T08:37:40Z

@EstherPerelman, I'm reading over the code now. In your PR description, you say

For the next code in Package.json:

Do you have a plugin with which to test the code, or should it be possible to copy that code into any plugin's package.json and see the results?

Thank you for reviewing, It is possible to copy this code to any plugin's package.json,
I will go over the conversations as soon as possible, Thank's

EstherPerelman · 2021-04-27T12:15:20Z

@colin-grant-work I've updated code, Can you check if everything is OK?
Thanks.

colin-grant-work

Thanks for the changes. I'm now getting all of the items I expect, but there are some odd ordering effects. For example, the manifest I gave above to non-cyclical menu duplication now gives the following menus:

All of the menu items are present, but Custom Submenu 3 now appears before the other two items in custom submenu 1 even though it is listed after both of them. Are the items being sorted alphabetically (with capital letters preceding lower-case letters), and is that intentional?

There's also an odd effect that is likely beyond the scope of this PR where selecting certain submenus causes a flicker effect where items are highlighted from the menu root out to the leaf, and it can cause a failure when hovering a new submenu as focus is returned to the root menu and lost at the leaf.

You can see a couple of instances in that GIF where the focus is lost. It happened more frequently, but it's so brief that the GIF doesn't capture them all. The behavior occurs on master as well (for example in the Sample Menu > Sample sub menu in the example application in the electron version), so I don't believe it's a problem with this code.

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts

EstherPerelman · 2021-04-28T12:14:37Z

All of the menu items are present, but Custom Submenu 3 now appears before the other two items in custom submenu 1 even though it is listed after both of them. Are the items being sorted alphabetically (with capital letters preceding lower-case letters), and is that intentional?

Yes it is sorted, Look at this

colin-grant-work

This looks good to me. Thanks @EstherPerelman!

EstherPerelman · 2021-04-29T07:56:29Z

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts

+                const parentSubmenusIds = new Set<string>();
+
+                return menus.reduce((children: MenuTree[], menuItem) => {
+                    if (menuItem.submenu) {
+                        if (parentSubmenusIds.has(menuItem.submenu)) {


Hi @colin-grant-work I updated the code because parentSubmenusIds declaration was inside the reduce
so it didn't work as excepted, I found by chance that the menu was duplicated...

EstherPerelman · 2021-04-29T08:00:28Z

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts

+                menus.forEach(menu => {
+                    if (group) {
+                        // Adding previous group to the start of current menu group.
+                        menu.group = `${group}/${menu.group || '_'}`;


Also updated default value for group from '' to '_' (If there's no group the item should be at the end of the menu list - compared with VSCode)

amiramw · 2021-05-06T08:13:26Z

Any objection to merging this one? I see it is approved by @colin-grant-work

amiramw · 2021-05-11T13:27:37Z

we will merge it this week if there's no objection

amiramw · 2021-05-13T09:57:08Z

@vince-fugnitto @azatsarynnyy @vinokurig

vince-fugnitto

I verified the changes with the vscode-builtin-git plugin, as well as the contributes.menus described in the pull-request description and it seems to work as intended.

I also verified if there were any regressions with the different contributions points:

contribution points

vince-fugnitto · 2021-05-13T13:24:31Z

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts

+        const tree: MenuTree[] = [];
+
+        Object.keys(allMenus).forEach(location => {
+            // Don't build menus tree for a submenu decleration at root.


typo: decleration > declaration

Thank you @vince-fugnitto, Fixed it.

Signed-off-by: Esther Perelman <Esther.Perelman@sap.com>

vince-fugnitto · 2021-05-13T13:41:20Z

The changes should allow us to easily support new menu contribution points such as #9387 👍

EstherPerelman · 2021-05-13T13:48:39Z

I verified the changes with the vscode-builtin-git plugin, as well as the contributes.menus described in the pull-request description and it seems to work as intended.

I also verified if there were any regressions with the different contributions points:

contribution points

Thanks for testing & verifying, Is that says you approve this PR?

vince-fugnitto · 2021-05-13T13:56:11Z

Thanks for testing & verifying, Is that says you approve this PR?

Since Red Hat initially implemented the menu-contribution-handler I wanted to give them the opportunity to test, but so far there hasn't been a review. Functionality the code seems to work well, but I have not done a thorough review code-wise. @RomanNikitenko @tsmaeder @vinokurig

vinokurig

Tested according to the provided steps and with the vscode git plugin, works as expected.

EstherPerelman marked this pull request as draft April 18, 2021 15:50

EstherPerelman force-pushed the Submenus branch from 148a1e4 to 83702e4 Compare April 19, 2021 15:48

EstherPerelman marked this pull request as ready for review April 19, 2021 15:50

EstherPerelman force-pushed the Submenus branch from 83702e4 to 6fcf092 Compare April 19, 2021 17:16

EstherPerelman force-pushed the Submenus branch from 6fcf092 to 487d86b Compare April 20, 2021 07:29

EstherPerelman changed the title ~~Expand support in Submenus contribution~~ Expanding Submenus contribution support Apr 20, 2021

colin-grant-work self-requested a review April 21, 2021 22:18

colin-grant-work reviewed Apr 22, 2021

View reviewed changes

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts Outdated Show resolved Hide resolved

vince-fugnitto mentioned this pull request Apr 23, 2021

Support "debug/toolBar" command icons #9389

Open

EstherPerelman force-pushed the Submenus branch 2 times, most recently from 37fda8f to 16b3e36 Compare April 27, 2021 08:51

colin-grant-work reviewed Apr 27, 2021

View reviewed changes

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts Show resolved Hide resolved

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts Show resolved Hide resolved

colin-grant-work approved these changes Apr 28, 2021

View reviewed changes

vince-fugnitto added menus issues related to the menu vscode issues related to VSCode compatibility labels Apr 28, 2021

vince-fugnitto requested review from azatsarynnyy and vinokurig April 28, 2021 14:52

EstherPerelman marked this pull request as draft April 28, 2021 20:35

EstherPerelman force-pushed the Submenus branch from 16b3e36 to 00e56b0 Compare April 29, 2021 07:41

EstherPerelman commented Apr 29, 2021

View reviewed changes

EstherPerelman marked this pull request as ready for review April 29, 2021 08:01

EstherPerelman requested a review from colin-grant-work April 29, 2021 12:30

vince-fugnitto reviewed May 13, 2021

View reviewed changes

Expand support in Submenus contribution

c781e0f

Signed-off-by: Esther Perelman <Esther.Perelman@sap.com>

EstherPerelman force-pushed the Submenus branch from 00e56b0 to c781e0f Compare May 13, 2021 13:38

vinokurig approved these changes May 14, 2021

View reviewed changes

amiramw merged commit 36d6e04 into eclipse-theia:master May 18, 2021

vince-fugnitto added this to the 1.14.0 milestone May 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expanding Submenus contribution support #9371

Expanding Submenus contribution support #9371

EstherPerelman commented Apr 18, 2021 •

edited

Loading

EstherPerelman commented Apr 19, 2021 •

edited

Loading

colin-grant-work commented Apr 21, 2021

colin-grant-work commented Apr 22, 2021

colin-grant-work left a comment •

edited

Loading

EstherPerelman commented Apr 25, 2021

EstherPerelman commented Apr 27, 2021

colin-grant-work left a comment

EstherPerelman commented Apr 28, 2021 •

edited

Loading

colin-grant-work left a comment

EstherPerelman Apr 29, 2021

EstherPerelman Apr 29, 2021 •

edited

Loading

amiramw commented May 6, 2021

amiramw commented May 11, 2021

amiramw commented May 13, 2021

vince-fugnitto left a comment

vince-fugnitto May 13, 2021

EstherPerelman May 13, 2021

vince-fugnitto commented May 13, 2021

EstherPerelman commented May 13, 2021

vince-fugnitto commented May 13, 2021

vinokurig left a comment

Expanding Submenus contribution support #9371

Expanding Submenus contribution support #9371

Conversation

EstherPerelman commented Apr 18, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

EstherPerelman commented Apr 19, 2021 • edited Loading

colin-grant-work commented Apr 21, 2021

colin-grant-work commented Apr 22, 2021

colin-grant-work left a comment • edited Loading

Choose a reason for hiding this comment

EstherPerelman commented Apr 25, 2021

EstherPerelman commented Apr 27, 2021

colin-grant-work left a comment

Choose a reason for hiding this comment

EstherPerelman commented Apr 28, 2021 • edited Loading

colin-grant-work left a comment

Choose a reason for hiding this comment

EstherPerelman Apr 29, 2021

Choose a reason for hiding this comment

EstherPerelman Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

amiramw commented May 6, 2021

amiramw commented May 11, 2021

amiramw commented May 13, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto May 13, 2021

Choose a reason for hiding this comment

EstherPerelman May 13, 2021

Choose a reason for hiding this comment

vince-fugnitto commented May 13, 2021

EstherPerelman commented May 13, 2021

vince-fugnitto commented May 13, 2021

vinokurig left a comment

Choose a reason for hiding this comment

EstherPerelman commented Apr 18, 2021 •

edited

Loading

EstherPerelman commented Apr 19, 2021 •

edited

Loading

colin-grant-work left a comment •

edited

Loading

EstherPerelman commented Apr 28, 2021 •

edited

Loading

EstherPerelman Apr 29, 2021 •

edited

Loading