Skip to content

fix: Prevent repetitive viewport analysis in PointerMoved #7366

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

Merged
merged 2 commits into from
Dec 28, 2021

Conversation

lukeblevins
Copy link
Contributor

@lukeblevins lukeblevins commented Dec 27, 2021

Resolved / Related Issues
Items resolved / related issues by this PR.

Details of Changes
Analyze the ListView viewport for item rectangles a single time, upfront, rather than every time the pointer is moved. Further, do this rather intensive work on selection rectangle y-axis dilation only when the scrolling gesture becomes determinate for 1 second.

English: Drag-to-select will no longer lag in very large folders (e.g. System32), and similar safeguards are put in place when attempting to select more items outside the bottom (or top) of the ListView.

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

Screenshots (optional)
N/A

yaira2
yaira2 previously approved these changes Dec 27, 2021
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

Nice work!

@yaira2 yaira2 requested a review from gave92 December 27, 2021 16:34
@gave92
Copy link
Member

gave92 commented Dec 27, 2021

If you select a lot of items by scrolling without stopping, items in the middle won't get selected (by the time you stop some items will be virtualized again / ContainerFromItem() will be null).

@lukeblevins
Copy link
Contributor Author

@gave92 This was part of the safeguard to ensure we don't lag when dilating the selection rectangle over the y axis. Otherwise, we would be adding every item in the viewport for each tiny vertical offset change. Waiting until the user stops scrolling seemed like a decent temporary solution until someone can figure out a better method, but considering how bad the behavior was before in large directories, we should go ahead with this imo

@gave92
Copy link
Member

gave92 commented Dec 27, 2021

Made a small change that should fix the above issue. It should not affect performance, could you confirm if it's ok?

@lukeblevins
Copy link
Contributor Author

@gave92 Not really sure how you did it, but the issue mentioned above is indeed fixed. Thanks!

@lukeblevins lukeblevins requested a review from yaira2 December 28, 2021 03:56
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Dec 28, 2021
@yaira2 yaira2 merged commit 9dca5bd into main Dec 28, 2021
@yaira2 yaira2 deleted the dev/lubl/slow-dragrect-fix branch December 28, 2021 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve rectangle selection performance
3 participants