Fix #10369: icons of custom tree view row are not visible on mouse hover

[EstherPerelman]

Signed-off-by: Esther Perelman [email protected]

What it does

Fix #10369 by replacing update to forceUpdateGrid call on mouseover event since the update request not activate the rendering of the grid by react (it doesn't pass the internal React.Grid _calculateChildrenToRender function because of old caches)

How to test (also for buggy result)

1. install tree-view-sample extension of vscode-extensions-samples on Theia (you can easily download my custom-view-sample.zip )
2. from the top menu select View -> Open View -> Packege Explorer
3. hover with the mouse on the rows
4. good result: see edit icon for each row on hover, bad result: see edit icon only on selecting row

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

[msujew]

Thanks @EstherPerelman for taking care of this.

While this works on a surface level, I noticed some issues with debouncing the forced update here. Especially when swiftly moving over the tree nodes (the hover marker will lag behind a few frames). I tried the change below and it seems to fix the issue as well. Are there any issues with this approach that I didn't notice?

[msujew]

@EstherPerelman I still feel like debouncing here is not necessary and only leads to artifacts when swiftly moving the cursor over the tree nodes. Notice how the icon always lags behind the cursor, while the background highlighting correctly moves with the cursor:

2021-11-04.16-26-39.mp4

When removing the debounce, the feature continues to work as expected, without any of the artifacts, at least in my tests. What speaks in favor of debouncing the forced update?

[EstherPerelman]

What speaks in favor of debouncing the forced update?

@msujew I added the debounce because in my test(without the debounce) the edit icon wasn't clickable (- didn't show the information message on the bottom right as happens now),
Also the icon not always appears onMouseOver event as you can see in the video:

vscode.Theia.Browser.Example.-.Google.Chrome.2021-11-05.00-54-23.mp4

Notice how the icon always lags behind the cursor, while the background highlighting correctly moves with the cursor

I changed the debounce wait value to 1ms, It seems to be better WDYS?

[msujew]

@EstherPerelman Alright, thank you for the explanation. That makes me believe that forcing an update here is the wrong direction. I have another idea:

1. Add this rule to the tree.css file:

.theia-TreeNode:not(:hover):not(.theia-mod-selected) .theia-tree-view-inline-action {
    display: none;
}

2. Always render the inline commands by removing these lines of code:
   

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

   Lines 331 to 333 in 6a376d3

   	if (this.model.selectedNodes.every(selected => selected.id !== node.id) && node.id !== this.hoverNodeId) {
   	return false;
   	}

3. Add the ACTION_ITEM class constant (imported from @theia/core/lib/browser/widgets/widget) to the inline command classes:
   

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

   Line 353 in 6a376d3

   const className = [TREE_NODE_SEGMENT_CLASS, TREE_NODE_TAIL_CLASS, icon, 'theia-tree-view-inline-action'].join(' ');

4. Remove the forced update on the setHoverNodeId method.

That way we always render the inline commands to html, but don't display them if it's not necessary. What do you think about that?

[EstherPerelman]

That way we always render the inline commands to html, but don't display them if it's not necessary. What do you think about that?

@msujew, in the first time I looked at this code area I wondered why the hover was not implemented with css... but preferred not to touch the existing code, with your suggestion it's working perfect and the code is more reasonable

[msujew]

@EstherPerelman Yeah, it seems to be some ancient code that's been there since the first version of the inline commands and has never been touched since. The new approach looks way better.

@colin-grant-work @vince-fugnitto Any possible regressions that you might see due to the CSS approach compared to using setHoverNodeId? If not, I would approve this PR soonish.

[vince-fugnitto]

I confirmed that the behavior works well on hover 👍

[vince-fugnitto]

@msujew should we prefix the entry with .theia-tree-view as not to potentially affect other views and keep the styling specific to the plugin-system?

Suggested change

	.theia-TreeNode:not(:hover):not(.theia-mod-selected) .theia-tree-view-inline-action {
	.theia-tree-view .theia-TreeNode:not(:hover):not(.theia-mod-selected) .theia-tree-view-inline-action {

[msujew]

Sure, sounds like a good idea 👍

[EstherPerelman]

Hi @vince-fugnitto @msujew I pushed the change, Can you merge it soonish?

[msujew]

LGTM! I just reconfirmed that everything works as expected.

I will probably merge this tomorrow, if there are no objections.