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

Use argument processor for the commands called from plugins #6631

Closed
wants to merge 1 commit into from
Closed

Use argument processor for the commands called from plugins #6631

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/plugin-ext/src/plugin/command-registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ export class CommandRegistryImpl implements CommandRegistryExt {

// tslint:disable:no-any
executeCommand<T>(id: string, ...args: any[]): PromiseLike<T | undefined> {
args = args.map(arg => this.argumentProcessors.reduce((r, p) => p.processArgument(r), arg));
Copy link
Member

@akosyakov akosyakov Nov 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We already do it in this.executeLocalCommand, so we will run it twice.

I don't understand why we will run it for main side commands?

An idea behind argument processors is to avoid sending data which is already in the plugin host for local commands not to transform something for the main side. You can do it by overriding services in the main side already, like CommandRegisry.execute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right, overriding service might be better solution in this case. I've updated code in following PR on che-theia side: https://github.com/eclipse/che-theia/pull/562/files.

The main problem I'm trying to solve is that, in our infrastructure each plugin runs in separate container. For example, there is the kubernetes plugin, which provides command to see the diff for the selected resource (opened in the editor). Right part for the diff takes directly from the opened editor. But left part - there is a resource stored to temporary file (in /tmp directory) in container, where plugin hosts. So we're having following situation. Kubernetes plugin calls vscode.diff command with two arguments: 1) path to the file, opened in the editor, everything is fine; 2) path to the file which doesn't exists in theia container, but stored in different container in /tmp directory. So we need to modify the file scheme for second argument. And with custom file content provider open this file in the diff viewer. Smth like that.

But I've tried to override the call of executeCommand method and that works for the che-theia use case. So this PR can be safely closed.

Copy link
Member

Choose a reason for hiding this comment

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

@vzhukovskii ok, glad that you find a way to resolve the issue!

if (this.handlers.has(id)) {
return this.executeLocalCommand(id, ...args);
} else if (KnownCommands.mapped(id)) {
Expand Down