Don't reveal the focused element when updating the tree rows #13703
Conversation
Fixes eclipse-theia#13461 Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
There was a problem hiding this comment.
I played around a bit and I also think it is a smoother experience if we avoid the scroll as suggested by this change, at least in the case reported in #13461.
There were just two minor undesirable whitespace changes (see inline suggestions).
| @@ -1575,9 +1575,9 @@ export namespace TreeWidget { | |||
| }} | |||
| totalCount={rows.length} | |||
| itemContent={index => this.props.renderNodeRow(rows[index])} | |||
| width={width} | |||
| width={width} | |||
There was a problem hiding this comment.
| width={width} | |
| width={width} |
| height={height} | ||
| // This is a pixel value, it will scan 200px to the top and bottom of the current view | ||
| // This is a pixel value, it will scan 200px to the top and bottom of the current view |
There was a problem hiding this comment.
| // This is a pixel value, it will scan 200px to the top and bottom of the current view | |
| // This is a pixel value, it will scan 200px to the top and bottom of the current view |
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
|
For documentation: I believe the correct way to implement this is to not change the set of visible items when updating the rows unless it's necessary: This means we keep the first visible item the same. If there are added or removed above the first item, the first visible item stays the same. If items below the first visible item are removed, the first item stays the same, unless there are not enough items below the first visible to fill the view: then we scroll up until the view is filled. Only if the first visible item is removed do we need to adjust the first visible item: a sane policy might be to move the first item that was visible before to be the first item. We'll need to think about corner cases if we ever implement something like this, obviously. I haven't implemented something like this because I can't figure out what the first visible item is with the |
What it does
Before this change, every time the rows in a tree widget were updated, the currently focused item in the tree would be revealed in the tree, i.e. the tree would be scrolled to make the item visible. Since the focused element does not change upon scrolling, this would lead to the tree scrolling up when the rows were updated.
This change stops doing that: the consequence will be that when elements are added or removed before the currently focused item, the item will be scrolled out of view. I would argue this is better than the current behaviour, which is super-annyoing. I guess we'll just have to give it a go and see whether we run into pathological cases.
Fixes #13461
Contributed on behalf of STMicroelectronics
How to test
Compare the behavior with and without the fix in the package in the explorer view.
Follow-ups
Review checklist
Reminder for reviewers