vscode built-in menus by akosyakov · Pull Request #4173 · eclipse-theia/theia

akosyakov · 2019-01-28T09:47:59Z

fix #4004
VS Code docs: https://code.visualstudio.com/api/references/contribution-points#contributes.menus

This PR alignes Theia menus with VS Code built-in menus:

navigator menu group is always sorted to the top
each menu command receives a current selected resource as first argument, Theia core URIs translated to VS Code URIs
supported menus:
- explorer/context
- editor/context
- editor/title
- debug/callstack/context
- view/item/context

Out of scope, since there are not corresponding implementations in Theia:

SCM menus: support scm menu contributions #4171
editor/title/context menu: [tabbar-toolbar] support context menus #4161
view/title support view title menu contributions #4174
ALT commands [menus] alternative command support #4162

Testing:
One can test against this VS Code extension: https://github.com/akosyakov/vscode-menus

There is no API breaking changes, but menu constants were renamed. Is it a case for major release? @marcdumais-work @svenefftinge

Location of built-in explorer groups in Theia and VS Code:

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

benoitf · 2019-01-28T10:52:27Z

packages/plugin-ext/src/common/plugin-protocol.ts

+    command: string;
+    title: string;
+    category?: string;
+    icon?: string | { light: string; dark: string; };


hello again: why here do we have icon light and icon dark ?

Should we ignore and don't parse from package.json? https://github.com/Microsoft/vscode/blob/366bfe96bf2fe4cff37de895c3fa2ccb776eb621/extensions/markdown-language-features/package.json#L29-L36

What is your suggestion?

hello, my question is why it's not IconUrl that you introduced there

export type IconUrl = string | { light: string; dark: string; };

PluginPackageCommand is a format of raw command as it is defined by VS Code, with relative paths for icons. In order to download them, paths are converted into URLs served by hostedPlugin entry point. A new type is introduced to make it clear that a string is a url, not a raw path.

shouldn't be then URI instead of string for iconUrl ?

I thought about it, but then we need to transfer such urls from backend to frontend, and decided to stick with strings as for other remote APIs.

benoitf · 2019-01-28T11:26:52Z

packages/plugin-ext/src/main/browser/command-registry-main.ts

                execute: (...args: any[]) => {
-                    this.proxy.$executeCommand(command.id, ...args);
+                    // plugin command handlers cannot handle Theia URI, only VS Code URI
+                    const resolvedArgs = (args || []).map(arg => arg instanceof URI ? arg['codeUri'] : arg);


it's not conflicting with https://github.com/theia-ide/theia/blob/master/packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts#L46 ?

looks like there is something not so neat around URI. AFAIK consumer should just use the object

it's not conflicting with https://github.com/theia-ide/theia/blob/master/packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts#L46 ?

I don't think so. vscode.open is called by a plugin, so URI should be converted from VS Code to Theia format.

I'm trying to handle the reverse case: when Theia calls a plugin command. In this case, plugin command handlers expect only URIs in VS Code format, so Theia URIs should be always converted.

Does it make sense? Do you know the better place for conversion?

thx @akosyakov for the reply. The location of the convert is still looking strange to me here.
AFAIK If someone is calling a plug-in then this is that part that need to convert to theia URI format, not theia plug-in checking if format that it is receiving is in a vscode format.

I'm afraid that we miss several converters. Here it is for Uri but what if there is another type, etc.
should it be inside RPC ? or is it mangled there https://github.com/theia-ide/theia/pull/4062/files

AFAIK If someone is calling a plug-in then this is that part that need to convert to theia URI format, not theia plug-in checking if format that it is receiving is in a vscode format.

The core does not know that it calls a plug-in, it calls a menu command handler and pass URI of a selected resource to it. Usually it is fine. With plugin-in it is not, because they cannot handle such URIs.

Code in command-registry-main is a menu command handler from plugin-ins. Could you be more specific and suggest where it should be done if not here?

Do you suggest to register a synthetic command for menu items that translates URIs and then delegate to a plugin-in command handler?

do you have an example of failing test for this usecase ?
is it only for vs code extensions or for theia plug-ins ?

@akosyakov yes

ok, moved to menus-contribution-handler: https://github.com/theia-ide/theia/pull/4173/files#diff-c111d7d3381b06ddff0ddb201236bec1

akosyakov · 2019-01-29T08:01:25Z

@azatsarynnyy mentioned that there are duplicate menu items with these changes, for example for https://github.com/Azure/vscode-kubernetes-tools/blob/master/package.json#L579-L588 even though when context should ensure their uniqueness... i'm looking into it

akosyakov · 2019-01-29T10:11:25Z

@azatsarynnyy could you try again, i've removed a limitation to have only one menu item per a command. Since we have different contexts it does not make sense: 265c0e0

azatsarynnyy · 2019-01-29T10:28:06Z

@akosyakov I've tested the PR with the latest version of the vscode k8s extension and it works well. I see no issues.

akosyakov · 2019-01-29T12:45:37Z

@svor Could you check please that registering code lenses from the vscode extensions still work?

benoitf

LGTM, thx

svor

Tested with vscode java extension 0.37.0 and it works without unexpected problems

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

…rent contexts Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov · 2019-01-31T09:56:42Z

update Changelog with breaking changes

svenefftinge

LGTM

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov · 2019-01-31T11:22:34Z

CHANGELOG.md

 # Change Log

-## v0.3.20
+## v0.4.0


cc @marcdumais-work

akosyakov requested review from benoitf and evidolob as code owners January 28, 2019 09:48

benoitf reviewed Jan 28, 2019

View reviewed changes

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

benoitf reviewed Jan 28, 2019

View reviewed changes

akosyakov requested review from azatsarynnyy, kittaakos and svenefftinge and removed request for azatsarynnyy January 28, 2019 12:00

akosyakov changed the title ~~[WIP] vscode built-in menus~~ vscode built-in menus Jan 28, 2019

akosyakov force-pushed the ak/vscode_built_in_menus branch 2 times, most recently from 05e62e2 to 4f5e0ef Compare January 28, 2019 12:14

benoitf reviewed Jan 28, 2019

View reviewed changes

akosyakov mentioned this pull request Jan 28, 2019

[node-debug] fixes for Toggle skipping this file command #4177

Merged

akosyakov requested a review from azatsarynnyy January 29, 2019 08:03

akosyakov force-pushed the ak/vscode_built_in_menus branch from 265c0e0 to 3aaa72b Compare January 29, 2019 12:33

akosyakov requested a review from svor January 29, 2019 12:45

akosyakov force-pushed the ak/vscode_built_in_menus branch from 3aaa72b to 1eee41e Compare January 29, 2019 12:46

benoitf approved these changes Jan 29, 2019

View reviewed changes

svor approved these changes Jan 29, 2019

View reviewed changes

akosyakov added 7 commits January 30, 2019 10:36

[vscode/menu] sort navigation group always to the top top of a menu

c39ee29

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

[vscode] explorer/context menu support

18a007a

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

[vscode] editor/context menu support

f80d6ec

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

[vscode] editor/title context menu support

abdce91

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

[vscode] debug/callstack/context menu support

f0ddc38

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

[browser] allow multiple menu items for the same command but in diffe…

145b687

…rent contexts Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

fix #4151: register vscode commands/handlers properly

627c974

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

[vscode/menu] provide current selected resource as a command argument

e5c8ff9

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the ak/vscode_built_in_menus branch from 1eee41e to e5c8ff9 Compare January 30, 2019 11:09

svenefftinge approved these changes Jan 31, 2019

View reviewed changes

update Change Log with renamed menu groups

06b4b4f

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov commented Jan 31, 2019

View reviewed changes

CHANGELOG.md

# Change Log

## v0.3.20

## v0.4.0

Copy link

Member Author

akosyakov Jan 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @marcdumais-work

akosyakov merged commit d552d2a into master Jan 31, 2019

akosyakov deleted the ak/vscode_built_in_menus branch January 31, 2019 12:34

Conversation

akosyakov commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benoitf Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosyakov Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benoitf Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosyakov Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosyakov Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosyakov commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akosyakov commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azatsarynnyy commented Jan 29, 2019

Uh oh!

akosyakov commented Jan 29, 2019

Uh oh!

benoitf left a comment

Choose a reason for hiding this comment

Uh oh!

svor left a comment

Choose a reason for hiding this comment

Uh oh!

akosyakov commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svenefftinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

akosyakov commented Jan 28, 2019 •

edited

Loading

benoitf Jan 28, 2019 •

edited

Loading

akosyakov Jan 28, 2019 •

edited

Loading

benoitf Jan 28, 2019 •

edited

Loading

akosyakov Jan 29, 2019 •

edited

Loading

akosyakov Jan 29, 2019 •

edited

Loading

akosyakov commented Jan 29, 2019 •

edited

Loading

akosyakov commented Jan 29, 2019 •

edited

Loading

akosyakov commented Jan 31, 2019 •

edited

Loading