electron: update rendering of menu items

[vince-fugnitto]

What it does

The following pull-request updates the rendering of electron context-menu items based on a command's isEnabled. In electron, menu items cannot be disabled (grayed-out) like the browser so we must rely on hiding elements instead of disabling them. With this change, menu items in electron are not rendered if their isEnabled is not true.

For example, language-server commands (monaco) are always populated in electron menus which does not make sense since such commands (ex: 'go to definition') do not make sense when they are no-op. Omitting the rendering of these items also aligns with the menu behavior visible from vscode.

Master:

Pull Request:

How to test

1. execute the command Reset Workbench Layout
2. verify that the main-menu successfully displays the menu items (electron: update rendering of menu items #7869 (comment))
3. open a typescript file and trigger the context-menu
   (language feature menu items should be available)
4. open a markdown, image, textfile and trigger the context-menu
   (language feature menu items should not be available)

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

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

[kittaakos]

Two remarks:

• VS Code always shows application menu items even if they are not enabled. I assume, if we do this for the context menu, we will do the same for the app menu; do we want to diverge from VS Code's behavior?
• If we decide to hide not enabled items, we have to adjust this code too.

[kittaakos]

There is another side-effect of this PR, we do not show non-enabled menu items on start, so for instance, the Selection menu will contain only Select All. Since we do not support dynamic menu items in electron, the Selection menu content won't ever get updated unless the window has a blur then focus event:

I tend to not accept this PR with the current state. I prefer to see the menu items even if they're NOOP than not seeing them.

[vince-fugnitto]

Thank you for your feedback @kittaakos!

• VS Code always shows application menu items even if they are not enabled. I assume, if we do this for the context menu, we will do the same for the app menu; do we want to diverge from VS Code's behavior?

We will need to find a solution which does not affect the main-menu, but rather only the context-menu (due to the dynamic menu issue). In VS Code, the context-menu only displays enabled menu items, it will not display no-op menu items (similarly to this pull-request).

• If we decide to hide not enabled items, we have to adjust this code too.

The idea is to hide context-menu items only.

I tend to not accept this PR with the current state. I prefer to see the menu items even if they're NOOP than not seeing them.

I agree, I'll need to dive deeper to see if it's possible only to modify the context-menu and not the main-menu.

There is another side-effect of this PR, we do not show non-enabled menu items on start, so for instance, the Selection menu will contain only Select All.

I wasn't able to reproduce this issue with my changes, do you have the steps to reproduce?

[kittaakos]

do you have the steps to reproduce?

You can try resetting the workbench layout.

[kittaakos]

In VS Code, the context-menu only displays enabled menu items

The idea is to hide context-menu items only.

Do you want to align the browser behavior with electron? AFAIK, we disable but show the context menu items in browser env.

[vince-fugnitto]

In VS Code, the context-menu only displays enabled menu items

The idea is to hide context-menu items only.

Do you want to align the browser behavior with electron? AFAIK, we disable but show the context menu items in browser env.

@kittaakos I was only planning on updating the electron target to hide disabled menu items (similarly to vscode) and not update the browser which does not have this limitation. In the browser, disabled menu items are properly decorated (grayed-out).

[kittaakos]

@kittaakos I was only planning on updating the electron target to hide disabled menu items (similarly to vscode) and not update the browser which does not have this limitation. In the browser, disabled menu items are properly decorated (grayed-out).

👍 I am fine with it, I just wanted to know what to verify once it's ready.

[vince-fugnitto]

@kittaakos I modified the code to only affect context-menus and not the main-menu.
The behavior is now aligned with vscode (hiding disabled menu items from the context-menu).

[spoenemann]

👍

[vince-fugnitto]

@kittaakos @akosyakov thank you for the feedback, I've updated and retested the code based on your feedback :)

[kittaakos]

I have checked it with the electron example; it worked as expected. Thank you! 👍