Repository navigation
Fix inspector Auto Scroll - #4283
Conversation
This reverts commit a5f7819.
Spent time really debugging why the indexes were mismatched. Fixed the row index and height calculations, so that they will work properly and give precise values. Also removed inflation because it ended up bringing us to the wrong row when asking to scroll to it
| // a node is selected, its properties will also be selected as by | ||
| // convention properties are the first children of a node and properties | ||
| // typically do not have children and are never expanded by default. | ||
| for (InspectorTreeNode child in node.children) { |
There was a problem hiding this comment.
why was this logic removed? If the problem is it does bad things with too many properties, we can tweak the logic to only require some small number of children.
There was a problem hiding this comment.
While updating the scrolling code, I ended up changing the behaviour so that scrolling always snaps the focused item to the top left of the viewport. So adding any children to the bounding box doesn't change the result of the scrolling.
I am also changing the behaviour, so that we don't distinguish between properties and their parents. The reason for this is that sometimes I would be scrolling the properties with my arrow keys and the focus would go off the screen but the scroll would not update since the parent would be on the screen. I figured that simplifying the whole process to "Just make sure whatever I'm focused on is visible" would help make the experience more consistent.
There was a problem hiding this comment.
Scrolling to the top left corner of the screen will be jarring for cases like going through a list of search results.
For example image stepping through search results and you get one that is slightly off the screen but now you scroll ALL the way up till it is at the top of the screen.
I would try to maintain the previous functionality which is pretty much industry standard for how you should scroll to bring some new content to be visible on the screen. Instead I would special case so we abandon making the parent visible when it is not possible as the list of properties is larger than the height of the screen.
There was a problem hiding this comment.
We chatted about this during a 1:1 and decided together that scrolling to the center of the screen is a happy medium we can agree on.
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Looks good with a few questions.
| final approximateIconWidth = calculateTextSpanWidth( | ||
| const TextSpan( | ||
| text: 'A', | ||
| ), | ||
| ); |
There was a problem hiding this comment.
why not use the actual icon width here?
|
Tested on Macos and Web with:
|
Fixes #3561
This change fixes scrollToRect and related areas so that autoscrolling will always snap to the top left of the screen ( with 3 rows of padding).
If the focused node is already on the screen, no scrolling is done.