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.

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

Involve selection service in tree view on plugin side #4609

Merged
merged 1 commit into from
Mar 21, 2019
Merged

Conversation

vzhukovs
Copy link
Contributor

This proposal changes provides an ability to use selection service in tree widget, which constructs on the plugin side.

The problem appears in mechanism that delegates calls of the context menu items into calls of commands in command service. The problem was in that, in selection service the last selected file resource was stored and when we want to call a command from the context menu on tree, provided from plugin, we couldn't pass currently selected node as an argument into actual plugin.

Such problem was related with kubernetes plugin, where tree node should have a metadata information about object that displayed in the tree(cluster configuration, resources, attributes etc). Such metadata is used to controlling the kubernetes cluster.

Related issue: #3882

Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com

@vzhukovs vzhukovs added plug-in system issues related to the plug-in system Team: Che-Plugins issues related to the che-plugins team labels Mar 19, 2019
if (is(selection)) {
return selection.metadata;
}
if (Array.isArray(selection) && is(selection[0])) {
Copy link
Member

Choose a reason for hiding this comment

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

Does VS Code allow multi selection in contributed tree views?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have no idea, looking for it now. But meantime, we're allowing to create tree widgets that allow use multi selection. (via tree properties)

Copy link
Member

@akosyakov akosyakov Mar 20, 2019

Choose a reason for hiding this comment

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

Just checked, that they allow, but in the context menu only the last touched node is provided

Copy link
Member

@akosyakov akosyakov Mar 20, 2019

Choose a reason for hiding this comment

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

we're allowing to create tree widgets that allow use multi selection. (via tree properties)

I cannot find it. For me multi-selection in the tree widget does not work in Theia, but in VS Code does. I check with Node Dependencies tree from here: https://github.com/akosyakov/vscode-trees (you need this PR, otherwise samples does not work) Could it be that multi-selection should be enabled by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure about it, but I made this conclusion based on this property, which pass into tree widget constructor here, this just need to be checked.

Could it be that multi-selection should be enabled by default?

I think it depends on configuration for particular use case. For example in kubernetes plugin there is a single selection option, but in file navigator a multi selection enabled. So I think, that multi selection might be enabled by default.

@akosyakov
Copy link
Member

@vzhukovskii Do you have simple VS Code extension to run against Theia and VS Code and see that expectations match? Otherwise it is not possible to verify correctness.

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

I've just tested it with the Kubernetes extension.
Works well for the commands that rely on node's metadata.
To resolve the #3882 completely we also need to fix that issue for the commands that use a node object itself.

@akosyakov
Copy link
Member

@azatsarynnyy Cool! Found this sample extension: https://github.com/Microsoft/vscode-extension-samples/tree/master/tree-view-sample checking that events are fired similarly with it...

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It works nicely. Please set the global selection though only when the widget has focus and clean it when it is destroyed. Otherwise it could still the selection from focused widget or leak the instance correspondingly.

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@vzhukovs vzhukovs merged commit 824fbfc into master Mar 21, 2019
@vzhukovs vzhukovs deleted the theia#3882 branch March 21, 2019 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system Team: Che-Plugins issues related to the che-plugins team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants