[plugin] Cache command arguments to safely pass the command over JSON-RPC #5961

vinokurig · 2019-08-16T15:05:15Z

What it does

Pass command to tree item in the tree-view-widget
Cache command arguments to safely pass the command over JSON-RPC:
Currently tree item command doesn't provide arguments at all, they are overwritten to id:
https://github.com/theia-ide/theia/blob/f718cb3461eaf1a7de90c679d31cdbeafe485708/packages/plugin-ext/src/plugin/tree/tree-views.ts#L247-L249
The converter caches the command in the plugin side so the tree-view passes the command with id in arguments. This prevents JSON circular structure error and real arguments are available when the command is executed in the plugin side.

How to test

Clone sample plugin: https://github.com/vinokurig/Test.git and compile it
Start the plugin as a Hosted Plugin
Open the tree-view (list icon on the left panel):
click the label item
See A message from argument's field is shown.

The argument object contains a reference to itself: https://github.com/vinokurig/Test/blob/cdd7548fd1d1d3f22ba263017a80ef8a6192ac52/src/nodeDependencies.ts#L18-L20
so the command was safely converted and passed over JSON-RPC without an error.

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

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

vinokurig · 2019-08-19T07:46:45Z

fixes eclipse-che/che#13956

tsmaeder

Only commenting, because I just did a code review.

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

tsmaeder · 2019-08-19T08:35:08Z

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

+     * Convert to a command that can be safely passed over JSON-RPC.
+     */
+    toSafeCommand(command: theia.Command, disposables: DisposableCollection): theia.Command {
+        if (!this.isCommandRegistered) {


"isSafeCommandRegistered" would make it clear it's not the command that is being converted that needs to be registered.

tsmaeder · 2019-08-19T08:40:12Z

@vinokurig is there an issue this PR fixes? I can't find a reference in the PR description.

vinokurig · 2019-08-19T08:52:28Z

@tsmaeder

@vinokurig is there an issue this PR fixes? I can't find a reference in the PR description.

#5961 (comment)

tsmaeder · 2019-08-19T09:25:50Z

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

@@ -34,10 +35,16 @@ export class CommandRegistryImpl implements CommandRegistryExt {
    private readonly commands = new Set<string>();
    private readonly handlers = new Map<string, Handler>();
    private readonly argumentProcessors: ArgumentProcessor[];
+    private readonly commandsConverter: CommandsConverter;


CommandRegistryImpl never does anything with the "commandsConverter". IMO, the converter should be owned by
"TreeViewExtImpl", not the command registry.

This can be used by other APIs.

akosyakov · 2019-08-20T10:43:48Z

@olexii4 Does your approve means that changes looks good and you tested that it behaves well? Please provide such comments to make it clear for other committers that PR is well reviewed already. 🙏 We included it in review guidelines for such reasons: https://github.com/theia-ide/theia/blob/master/doc/pull-requests.md#justifiying-approve

akosyakov · 2019-08-20T10:45:46Z

@vinokurig If @tsmaeder is fine with changes and @olexii4 approve means #5961 (comment) you can proceed

akosyakov · 2019-08-21T09:26:26Z

I cannot verify, it fails for me:

root ERROR Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at ServerResponse.setHeader (_http_outgoing.js:470:11)
    at ServerResponse.header (/workspace/theia/node_modules/express/lib/response.js:767:10)
    at ServerResponse.send (/workspace/theia/node_modules/express/lib/response.js:170:12)
    at /workspace/theia/packages/plugin-ext/lib/hosted/node/plugin-reader.js:112:37
    at /workspace/theia/node_modules/express/lib/response.js:430:22
    at SendStream.onend (/workspace/theia/node_modules/express/lib/response.js:1047:5)
    at SendStream.emit (events.js:189:13)
    at ReadStream.onend (/workspace/theia/node_modules/send/index.js:822:10)
    at ReadStream.emit (events.js:194:15)
    at endReadableNT (_stream_readable.js:1125:12)
root INFO [hosted-plugin: 1934] PLUGIN_HOST(1934): PluginManagerExtImpl/init()
root INFO [hosted-plugin: 1934] PLUGIN_HOST(1934): initializing(custom-view-samples@0.0.1 with /workspace/theia/packages/plugin-ext-vscode/lib/node/plugin-vscode-init.js)
root INFO [hosted-plugin: 1934] PLUGIN_HOST(1934): PluginManagerExtImpl/loadPlugin(/workspace/theia/plugins/Test/out/extension.js)
root ERROR Uncaught Exception:  Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
root ERROR Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at ServerResponse.setHeader (_http_outgoing.js:470:11)
    at ServerResponse.header (/workspace/theia/node_modules/express/lib/response.js:767:10)
    at ServerResponse.send (/workspace/theia/node_modules/express/lib/response.js:170:12)
    at /workspace/theia/packages/plugin-ext/lib/hosted/node/plugin-reader.js:112:37
    at /workspace/theia/node_modules/express/lib/response.js:430:22
    at SendStream.onend (/workspace/theia/node_modules/express/lib/response.js:1047:5)
    at SendStream.emit (events.js:189:13)
    at ReadStream.onend (/workspace/theia/node_modules/send/index.js:822:10)
    at ReadStream.emit (events.js:194:15)
    at endReadableNT (_stream_readable.js:1125:12)
root ERROR Uncaught Exception:  Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
root ERROR Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at ServerResponse.setHeader (_http_outgoing.js:470:11)
    at ServerResponse.header (/workspace/theia/node_modules/express/lib/response.js:767:10)
    at ServerResponse.send (/workspace/theia/node_modules/express/lib/response.js:170:12)
    at /workspace/theia/packages/plugin-ext/lib/hosted/node/plugin-reader.js:112:37
    at /workspace/theia/node_modules/express/lib/response.js:430:22
    at SendStream.onend (/workspace/theia/node_modules/express/lib/response.js:1047:5)
    at SendStream.emit (events.js:189:13)
    at ReadStream.onend (/workspace/theia/node_modules/send/index.js:822:10)
    at ReadStream.emit (events.js:194:15)
    at endReadableNT (_stream_readable.js:1125:12)

I don't think that it is caused by this PR. It seems to be something broken with plugin systems generally now. The same issue were observed in my Monaco PR and in #5967

akosyakov · 2019-08-21T09:32:15Z

I will do a PR promptly, found an issue.

akosyakov · 2019-08-21T09:44:36Z

@vinokurig please review #6002, after that we can rebase your PR and test it

vinokurig · 2019-08-21T13:50:48Z

@akosyakov Re-based the branch.
While testing I've discovered that If the tree has more than one node with nested children, the command will work only for children of currently expanded node. That's because the internal commands map refreshes on each children request:
https://github.com/theia-ide/theia/blob/191bb6d02a841fe1f448809bfa1c64f032d00edb/packages/plugin-ext/src/plugin/tree/tree-views.ts#L177
In the vscode command is registered only when the tree node is fetched so they can refresh the commands map on the refresh tree request. In Theia tree nodes are always requested by the tree, so the commands map is refreshed on the children request to not to overload the commands map. I think we need to change the behavior of the tree like in the vscode way: #6007

akosyakov · 2019-08-21T14:31:12Z

I see, please put it the description of #6007 as well.

akosyakov · 2019-08-21T14:44:53Z

click the label item
See A message from argument's field is shown

@vinokurig I'm trying but clicking on the label item does not do anything. Where should it appear?

akosyakov · 2019-08-21T14:49:10Z

It only works if i click Refresh in the view or reload the browser. It does not work for me on first start for some reasons. You can also reproduce it by removing the focus from the view (i.e. open the navigator) and then do Reset Workbench Layout command.

vinokurig · 2019-08-22T08:22:02Z

@akosyakov

It only works if i click Refresh in the view or reload the browser. It does not work for me on first start for some reasons. You can also reproduce it by removing the focus from the view (i.e. open the navigator) and then do Reset Workbench Layout command.

Fixed, it was a bug in the tree-view-widget.tsx

vinokurig · 2019-08-23T06:38:04Z

@akosyakov
You can check it with https://github.com/microsoft/vscode-extension-samples/tree/master/tree-view-sample as well.
The commands from the Node Dependencies tree works: the web page of the dependency is opened by clicking the tree node.
NOTE The commands works only from the latest opened parent, If you click on node from previously opened parent it won't work. Can we go with it and than fix it in the #6007 ?

akosyakov · 2019-08-23T06:40:44Z

packages/plugin-ext/src/main/browser/view/tree-view-widget.tsx

@@ -141,13 +141,14 @@ export class PluginTree extends TreeImpl {
            }, update);
        }
        if (TreeViewNode.is(node)) {
-            return Object.assign(node, update);
+            return Object.assign(node, update, { command: item.command });


@vinokurig command cannot be applied to composite elements? just checking

yes, according to previous logic that was lost, command is set to fileNode:
https://github.com/theia-ide/theia/blob/93205109fa4017286ec0a441b0250b74b36c6495/packages/plugin-ext/src/main/browser/view/tree-views-main.tsx#L206-L218
bit is not set to folderNode:
https://github.com/theia-ide/theia/blob/93205109fa4017286ec0a441b0250b74b36c6495/packages/plugin-ext/src/main/browser/view/tree-views-main.tsx#L189-L204

I meant whether it should be set on the composite according to VS Code expectations? If so it would be nice to adjust it as well. It could be done separately, but needs an issue at least.

Raised an issue with question #6025

changes should be tested to be approved and a comment should be provided about it

akosyakov

You can check it with https://github.com/microsoft/vscode-extension-samples/tree/master/tree-view-sample as well.

I'm trying, but nothing happens:

I've installed an extension.
And then click on items in dependency tree.
I thought that a notification should appear for each item, but nothing is shown.
Refresh and reload does not help.

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

akosyakov · 2019-08-23T07:17:08Z

Tested it with VS Code as well, let me try again.

akosyakov

I've managed to get it work, fault on my side!

…-RPC Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

akosyakov · 2019-08-26T07:22:24Z

@vinokurig please merge, let's tackle remaining 2 issues separately

olexii4 · 2019-08-26T17:45:49Z

I tested these changes inside Eclipse CHE

I built the new image "olexii4dockerid/che-theia:next"
https://github.com/eclipse/che-theia

./build.sh --build-args:GITHUB_TOKEN=${GITHUB_TOKEN},--tag:next --branch:che-13956 --git-ref:refs\\/heads\\/master

and push it to upstream.
Create a new workspace from the devfile
https://gist.githubusercontent.com/olexii4/99aa676393255e70e03f70b0cafe5bde/raw/6e838186630eb64c272d9bf698666569610d815a/devfile.yaml
Open 'my Workspace' and try open a link from the tree.

vinokurig requested a review from olexii4 August 16, 2019 15:05

vinokurig requested a review from a team as a code owner August 16, 2019 15:05

vinokurig requested review from akosyakov, benoitf and evidolob August 16, 2019 15:06

akosyakov reviewed Aug 16, 2019

View reviewed changes

packages/plugin-ext/src/plugin/command-registry.ts Show resolved Hide resolved

akosyakov added plug-in system issues related to the plug-in system tree issues related to the tree (ex: tree widget) labels Aug 16, 2019

vinokurig force-pushed the che-13956 branch from c3ea868 to f703994 Compare August 19, 2019 07:28

tsmaeder reviewed Aug 19, 2019

View reviewed changes

vinokurig force-pushed the che-13956 branch from f703994 to 191bb6d Compare August 19, 2019 09:08

tsmaeder reviewed Aug 19, 2019

View reviewed changes

olexii4 previously approved these changes Aug 19, 2019

View reviewed changes

vinokurig force-pushed the che-13956 branch from 191bb6d to 9feb526 Compare August 21, 2019 13:30

vinokurig mentioned this pull request Aug 21, 2019

[plugin] Save tree nodes to cache and fetch the nodes if they were changed #6007

Open

vinokurig force-pushed the che-13956 branch from 9feb526 to 24a7eea Compare August 22, 2019 08:21

akosyakov reviewed Aug 23, 2019

View reviewed changes

packages/plugin-ext/src/plugin/command-registry.ts Outdated Show resolved Hide resolved

akosyakov approved these changes Aug 23, 2019

View reviewed changes

[plugin] Cache command arguments to safely pass the command over JSON…

efaaf0f

…-RPC Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig force-pushed the che-13956 branch from 24a7eea to efaaf0f Compare August 23, 2019 07:40

akosyakov mentioned this pull request Aug 23, 2019

TreeItem on click commands don't work #5744

Closed

olexii4 approved these changes Aug 26, 2019

View reviewed changes

vinokurig merged commit 24bf9f2 into master Aug 27, 2019

vinokurig deleted the che-13956 branch August 27, 2019 07:16

vinokurig mentioned this pull request Aug 27, 2019

The My Workspace actions works incorrectly eclipse-che/che#13956

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[plugin] Cache command arguments to safely pass the command over JSON-RPC #5961

[plugin] Cache command arguments to safely pass the command over JSON-RPC #5961

vinokurig commented Aug 16, 2019 •

edited

Loading

vinokurig commented Aug 19, 2019

tsmaeder left a comment •

edited

Loading

tsmaeder Aug 19, 2019

vinokurig Aug 19, 2019

tsmaeder commented Aug 19, 2019

vinokurig commented Aug 19, 2019

tsmaeder Aug 19, 2019

vinokurig Aug 19, 2019

akosyakov commented Aug 20, 2019 •

edited

Loading

akosyakov commented Aug 20, 2019

akosyakov commented Aug 21, 2019

akosyakov commented Aug 21, 2019

akosyakov commented Aug 21, 2019

vinokurig commented Aug 21, 2019

akosyakov commented Aug 21, 2019

akosyakov commented Aug 21, 2019

akosyakov commented Aug 21, 2019 •

edited

Loading

vinokurig commented Aug 22, 2019

vinokurig commented Aug 23, 2019

akosyakov Aug 23, 2019

vinokurig Aug 23, 2019 •

edited

Loading

akosyakov Aug 23, 2019

vinokurig Aug 23, 2019

akosyakov left a comment •

edited

Loading

akosyakov commented Aug 23, 2019

akosyakov left a comment

akosyakov commented Aug 26, 2019

olexii4 commented Aug 26, 2019

[plugin] Cache command arguments to safely pass the command over JSON-RPC #5961

[plugin] Cache command arguments to safely pass the command over JSON-RPC #5961

Conversation

vinokurig commented Aug 16, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

vinokurig commented Aug 19, 2019

tsmaeder left a comment • edited Loading

Choose a reason for hiding this comment

tsmaeder Aug 19, 2019

Choose a reason for hiding this comment

vinokurig Aug 19, 2019

Choose a reason for hiding this comment

tsmaeder commented Aug 19, 2019

vinokurig commented Aug 19, 2019

tsmaeder Aug 19, 2019

Choose a reason for hiding this comment

vinokurig Aug 19, 2019

Choose a reason for hiding this comment

akosyakov commented Aug 20, 2019 • edited Loading

akosyakov commented Aug 20, 2019

akosyakov commented Aug 21, 2019

akosyakov commented Aug 21, 2019

akosyakov commented Aug 21, 2019

vinokurig commented Aug 21, 2019

akosyakov commented Aug 21, 2019

akosyakov commented Aug 21, 2019

akosyakov commented Aug 21, 2019 • edited Loading

vinokurig commented Aug 22, 2019

vinokurig commented Aug 23, 2019

akosyakov Aug 23, 2019

Choose a reason for hiding this comment

vinokurig Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

akosyakov Aug 23, 2019

Choose a reason for hiding this comment

vinokurig Aug 23, 2019

Choose a reason for hiding this comment

akosyakov left a comment • edited Loading

Choose a reason for hiding this comment

akosyakov commented Aug 23, 2019

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Aug 26, 2019

olexii4 commented Aug 26, 2019

vinokurig commented Aug 16, 2019 •

edited

Loading

tsmaeder left a comment •

edited

Loading

akosyakov commented Aug 20, 2019 •

edited

Loading

akosyakov commented Aug 21, 2019 •

edited

Loading

vinokurig Aug 23, 2019 •

edited

Loading

akosyakov left a comment •

edited

Loading