Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix global menu params to align to vscode #10198

Closed
wants to merge 1 commit into from
Closed

fix global menu params to align to vscode #10198

wants to merge 1 commit into from

Conversation

shuyaqian
Copy link
Contributor

@shuyaqian shuyaqian commented Sep 30, 2021 •
edited by msujew
Loading

Signed-off-by: shuyaqian 717749594@qq.com

What it does

This pr solves when I need to register a menu in "explorer/context" and want to execute the appropriate command when particular files are selected. Currently, the URIs of all files cannot be obtained when multiple files are selected in the Theia. However, the URIs can be obtained in the vscode. When a command is registered, the second parameter of the callback function obtains the URIs of all files.

How to test

  1. deploy a plugin with 'explorer/context' in package.json
"menus": {
            "explorer/context": [
                {
                    "when": "resourceExtname == .js || resourceExtname == .py",
                    "command": "fastrun.explorer.run",
                    "group": "fast-run"
                }
            ]
        },
  1. registerCommand in plugin
cloudide.commands.registerCommand('fastrun.explorer.run', (uri, uris) => {
            
 });
  1. select multiple files in the explorer， and select the registered menu
  2. the second param returns the URI array of all selected files, but theia has only the first param.

Review checklist

Reminder for reviewers

msujew
msujew reviewed Sep 30, 2021
View reviewed changes
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Hi @shuyaqian, thank you for your contribution.

Do you mind explaining a bit more (in the What it does section of your PR) what issue your PR actully fixes? Perhaps even link an issue that already describes this problem.

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts Outdated Show resolved Hide resolved
@vince-fugnitto vince-fugnitto added the 🤔 needs more info issues that require more info from the author label Sep 30, 2021
@shuyaqian
Copy link
Contributor Author

Hi @shuyaqian, thank you for your contribution.

Do you mind explaining a bit more (in the What it does section of your PR) what issue your PR actully fixes? Perhaps even link an issue that already describes this problem.

This pr solves when I need to register a menu in "explorer/context" and want to execute the appropriate command when particular files are selected. Currently, the URIs of all files cannot be obtained when multiple files are selected in the Theia. However, the URIs can be obtained in the vscode. When a command is registered, the second parameter of the callback function obtains the URIs of all files.

@msujew msujew added vscode issues related to VSCode compatibility and removed 🤔 needs more info issues that require more info from the author labels Oct 8, 2021
@shuyaqian
fix global menu params to align vscode
c7092f2 
Signed-off-by: shuyaqian 717749594@qq.com
msujew
msujew reviewed Nov 1, 2021
View reviewed changes
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I finally had some time to test this PR. The changes seem to work well, however, I'm a bit concerned with the type safety in there.

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts
const selection = this.selectionService.selection;
const resourceParams = [];
Copy link
Member

Choose a reason for hiding this comment

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

This is currently implicitely typed as being any[]. Can you add an explicit type here (even if it's any[])?

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts
const uri = this.resourceContextKey.get();
return uri ? uri['codeUri'] : undefined;
if (TreeWidgetSelection.is(selection) && selection.source instanceof FileTreeWidget && selection.length > 1) {
resourceParams.push(selection.map((item: any) => item.uri && item.uri.codeUri));
Copy link
Member

Choose a reason for hiding this comment

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

This seems to work, but why can we assume that the selection items always contain a uri? Can we add an in check for the uri? This seems to be quite unsafe to me.

@vince-fugnitto
Copy link
Member

@shuyaqian do you still plan on working on the pull-request?

@JonasHelming JonasHelming added the decision_needed issues that require decisions on ways forward (ex: if they should be updated or closed) label May 6, 2022
This was referenced Jul 14, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msujew msujew msujew left review comments

Assignees

@msujew msujew

Labels
decision_needed issues that require decisions on ways forward (ex: if they should be updated or closed) vscode issues related to VSCode compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shuyaqian @vince-fugnitto @msujew @JonasHelming