fix set selection on right click selected node

[amiramw]

Fixes #7146

Signed-off-by: Amiram Wingarten [email protected]

What it does

Perform selection on right click also if the node was already selected so that the global selection service is updated and the menu content is displayed with the correct context.

How to test

Scenario 1:

• Put a breakpoint on selection-service.ts on selection setter.
• Click on selected node in the tree stops in this breakpoint
• Right click on selected node in the tree and make sure the debugger stop on the same breakpoint

Scenario 2

• Select some package.json file
• Refresh theia
• Now that the file from before is selected right click on it
• There should be14 menu items and not only 8

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

[kittaakos]

Hey @amiramw, thank you for the contribution. I have checked the behavior with your changes, and it does not seem correct. I did the followings:

• open your PR in Gitpod, start the workspace with Theia, etc.
• select .gitpod.yml,
• press and hold ⌘ (obviously, this is Ctrl on Linux and Windows) and select .npmignore. (This point you have two selected items in the Explorer: .gitpod.yml and .npmignore)
• still holding ⌘ and right-click on .npmignore.
• you can see, .npmignore is not selected anymore. Expected: it is.

I want to keep both nodes as the selected ones (focus), not one of them. I cannot this PR as is.

Before:

With your changes:

[kittaakos]

BTW, the comment you have just deleted, tells the expected behavior:

// Keep the selection for the context menu, if the widget support multi-selection and the right click happens on an already selected node.

[amiramw]

Thanks @kittaakos, now I better understand the flow. Please see my latest change.

[kittaakos]

I debugged into your changes, and I cannot really understand, why calling updateGlobalSelection before rendering the context menu in the TreeWidget would make any difference.

I did the same as before except I have selected other files. When the two files are selected, and I hold down the Ctrl/Cmd then right-click, I can see the updateGlobalSelection is a NOOP, as the currentSelection is the "same" as the new selection value:

Call stack:

Before updating the selection in the SelectionService:

The currentSelection and the new selection:

[amiramw]

@kittaakos the issue I'm trying to solve is when focus change before right clicking a selected node. For example if you select the editor tab and then right click the same file in the tree then selection stays the one from the editor.

I think it shouldn't happen with multi select when there are more than one node selected. Each selected on top of the first one take global selection in that case. So everything is synced anyway.

[kittaakos]

For example if you select the editor tab and then right click the same file in the tree then selection stays the one from the editor.

OK, got it. I could not reproduce it with a CodeEditor but it behaves as you've described with another widget, such as the Preferences. This 👇, right?

[kittaakos]

The first thing that looks odd is that you ignore the this.props.globalSelection. If it is false and it is false by default, you cannot update the global selection service, so your change is not fully correct. Long story short, you need a guard, something like this:

if (this.props.globalSelection) {
  this.updateGlobalSelection();
}

The other thing is, why do not we activate the widget before rendering the context menu? (CC: @akosyakov) Can we have a contextmenu listener on the tree widget node and this would focus the widget, and would update the global selection if the tree is configured to do so before rendering the context menu.

protected onAfterAttach(msg: Message): void {
  // all the listeners we have now, plus the new one to activate the widget, and automtaically update the global selection if required before rendering the context menu.
  this.addEventListener(this.node, 'contextmenu', () => this.doFocus());
}

Can we have a contextmenu listener on the tree widget node

FYI, I have tried this approach, and it works too.

[akosyakov]

So a use case is that the navigator is not focused, but another widget, like editor, and then someone triggers context menu?

[akosyakov]

About contextmenu listener, I'm not sure that it guarantees timing-wise, since real focus and activation happens in another tick. It could be there will be a race between activation and contextmenu handler rendering the context menu.

[kittaakos]

So a use case is that the navigator is not focused, but another widget, like editor, and then someone triggers context menu?

That was my understanding.

I'm not sure that it guarantees timing-wise

Can you suggest another approach maybe? I did some quick testing locally, and the contextmenu listener seems to work. Also, I do not want to have quasi "get-selection set-selection" before rendering the context menu. It smells like a hack quick fix for a specific use-case.

[akosyakov]

Also, I do not want to have quasi "get-selection set-selection" before rendering the context menu. It smells like a hack quick fix for a specific use-case.

We actually don't set tree selection as global selection, but active widget. So we should only ensure that the navigator is focused before rendering a context menu, your suggestion to focus before sounds good. I think we could replace here update with doFocus:

theia/packages/core/src/browser/tree/tree-widget.tsx

Line 1119 in bc383db

this.update();

. Rendering of a context menu is ensured only after rendering on next tick by the code above and doFocus force triggers rendering.

[kittaakos]

We actually don't set tree selection as global selection

I meant with the proposed fix: https://github.com/eclipse-theia/theia/pull/7147/files#diff-415d8ea8ab3a316c90a0ca5e605de20eR1108

I think we could replace here update with doFocus:

Good idea, doFocus will update the widget anyways.

@amiramw, could you please try if the suggestion from @akosyakov fixes your use case?

[amiramw]

@amiramw, could you please try if the suggestion from @akosyakov fixes your use case?

It seems to work! I updated the PR.

[amiramw]

Can someone help with why travis is failing?

[akosyakov]

@amiramw I've restarted the build. The failure is not related to your changes. Preferences integration tests are failing because of some flakiness in our implementation. I'm looking into it.

[akosyakov]

@kittaakos Could you finish the review please?

[kittaakos]

Could you finish the review please?

👍

So a use case is that the navigator is not focused, but another widget, like editor, and then someone triggers context menu?

That was my understanding.

I have verified it; it works as expected.