Skip to content
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

Improve folder enumeration speed #4922

Merged
merged 9 commits into from
May 23, 2021
Merged

Conversation

gave92
Copy link
Member

@gave92 gave92 commented May 15, 2021

Resolved / Related Issues
For #4180

Details of Changes

  • Removes caching feature
  • Perf tweaks to folder loading code: removed calls to StorageFolder methods when not strictly needed
  • Fixes an issue for which icons are not loaded when browsing mtp devices

Warning
These changes creates issues with the "current" details view, I find the app crashing every 5 folders.
This only happens with the WCT datagrid layout mode, the "new" details view works good.
This actually looks like a bug with WCT and has been known for a while (#4304) but these changes seem to make the crash more common.
Depending on how we plan to ship the next version (new or old details view) it may make sense to wait before doing this change, but I'd love to get some feedbacks on how well this PR work for you.

Validation
How did you test these changes?

  • Built and ran the app
  • Tested loading folders from disk, network, mtp (phone)

@yaira2
Copy link
Member

yaira2 commented May 16, 2021

Depending on how we plan to ship the next version (new or old details view) it may make sense to wait before doing this change, but I'd love to get some feedbacks on how well this PR work for you.

@gave92 The new details layout will replace the old one in v2.

@gave92 gave92 marked this pull request as ready for review May 16, 2021 12:00
@lukeblevins lukeblevins self-requested a review May 16, 2021 14:31
lukeblevins
lukeblevins previously approved these changes May 16, 2021
Copy link
Contributor

@lukeblevins lukeblevins left a comment

Choose a reason for hiding this comment

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

This looks like a good improvement as long as we remove the old details layout before v2

@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label May 16, 2021
@yaira2
Copy link
Member

yaira2 commented May 16, 2021

@gave92 It seems like the status bar isn't updating anymore. Another issue is that everything seems very choppy when loading large directories.

@yaira2 yaira2 added needs - code review and removed ready to merge Pull requests that are approved and ready to merge labels May 16, 2021
@gave92
Copy link
Member Author

gave92 commented May 16, 2021

@yaichenbaum is status bar working in main for you?

@yaira2
Copy link
Member

yaira2 commented May 16, 2021

@yaichenbaum is status bar working in main for you?

It looks like it's an issue in main as well.

@gave92
Copy link
Member Author

gave92 commented May 23, 2021

Another issue is that everything seems very choppy when loading large directories.

@yaichenbaum is this still an issue? If so could you clarify? I would not expect it to be "more choppy" than the current.

@yaira2
Copy link
Member

yaira2 commented May 23, 2021

Another issue is that everything seems very choppy when loading large directories.

@yaichenbaum is this still an issue? If so could you clarify? I would not expect it to be "more choppy" than the current.

It appears to be working better now, I noticed a different issue where the app seems to scroll down after loading the system 32 directory in the details layout.

@gave92
Copy link
Member Author

gave92 commented May 23, 2021

It appears to be working better now, I noticed a different issue where the app seems to scroll down after loading the system 32 directory in the details layout.

Could you get me a screen recording of this? I have trouble reproducing

@yaira2
Copy link
Member

yaira2 commented May 23, 2021

It appears to be working better now, I noticed a different issue where the app seems to scroll down after loading the system 32 directory in the details layout.

Could you get me a screen recording of this? I have trouble reproducing

I reset Files and I can't reproduce it anymore 😂

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.

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels May 23, 2021
@yaira2 yaira2 merged commit ea7d908 into files-community:main May 23, 2021
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.

3 participants