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

Preemptive caching #2944

Merged
merged 33 commits into from
Feb 14, 2021
Merged

Preemptive caching #2944

merged 33 commits into from
Feb 14, 2021

Conversation

jakoss
Copy link
Contributor

@jakoss jakoss commented Jan 12, 2021

Adds preemptive caching to #1769
Fixes #3169

I had to do some refactoring to implement this feature. I had to make the "enumeration" functions more independent of ItemViewModel. I moved out some methods out of viewmodel, just for sake of readability and just to be a little more SOLID. I managed to lower ItemViewModel line count by 200.

There's one question from me. What is the reason to keep Win32 files enumeration along with Windows.Storage? I measured a little bit and i used application with only Windows.Storage enumeration and i didn't found significant performance difference. All gains from using direct C++ apis are minimal compared to other required operations in EnumerateItemsFromStandardFolderAsync. So i believe that using that introduces a lot of complicated and doubled logic without any real gain. @yaichenbaum @gave92 What do you think about that?

I'll tackle the "double loading" issue in next Pull Request.

@gave92
Copy link
Member

gave92 commented Jan 12, 2021

Windows.Storage does not list shortcut files and hidden items
And Win32 does not work on phone storage and network paths

@jakoss
Copy link
Contributor Author

jakoss commented Jan 12, 2021

Windows.Storage does not list shortcut files and hidden items
And Win32 does not work on phone storage (and network?)

Ok, so removing that is not an option

@gave92
Copy link
Member

gave92 commented Jan 12, 2021

And none of those work on recycle bin xD yeah it's complicated

@yaira2 yaira2 requested a review from gave92 January 12, 2021 20:11
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.

@jakoss It seems like the NumberBox Header is skipped over by the screen reader, can you check that out?

Files/Views/SettingsPages/Experimental.xaml Outdated Show resolved Hide resolved
yaira2
yaira2 previously approved these changes Jan 14, 2021
Copy link
Member

@gave92 gave92 left a comment

Choose a reason for hiding this comment

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

I love the ItemViewModel refactoring :)

An issue I'm seeing is that with this option enabled folder switching is sometimes delayed (I'll try to post a video). Is it just me?

Files/ViewModels/SettingsViewModel.cs Outdated Show resolved Hide resolved
@yaira2
Copy link
Member

yaira2 commented Jan 24, 2021

Sorry, I had busy week. I need to merge hez2010 changes in here and that will take me some time. I'll try to do this sometime next week

It's no rush, thank you for the update.

@tsvietOK tsvietOK marked this pull request as draft January 24, 2021 22:02
@jakoss
Copy link
Contributor Author

jakoss commented Jan 31, 2021

@yaichenbaum @gave92 I merged the changes from main branch, it was quite a ride after @hez2010 PR. But i think i got it right. Not sure about the end result after all. It does speed up things on my computer, but i have only nvme drives to test that on. And even on those the speedup is not really that mindblowing. Try it out on your machines.

Also there are minor changes to PersistentSQLiteCacheAdapter that should resolve the #3169

I also made the adjustement to cache only the first 32 loaded items, since it's all we need to "visually" improve loading time.

@jakoss jakoss marked this pull request as ready for review January 31, 2021 20:02
yaira2
yaira2 previously approved these changes Feb 3, 2021
@yaira2 yaira2 requested a review from gave92 February 3, 2021 20:29
gave92
gave92 previously approved these changes Feb 14, 2021
@gave92 gave92 requested a review from yaira2 February 14, 2021 20:29
@gave92
Copy link
Member

gave92 commented Feb 14, 2021

@jakoss I updated this PR with the latest changes from main. I hope I haven't screwed anything up. Small changes mostly to PersistentSQLiteCacheAdapter. Should be ready for merging.

Files/ViewModels/SettingsViewModel.cs Outdated Show resolved Hide resolved
@yaira2 yaira2 merged commit 22c55c3 into files-community:main Feb 14, 2021
@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label Feb 14, 2021
@yaira2 yaira2 mentioned this pull request Feb 14, 2021
@yaira2
Copy link
Member

yaira2 commented Feb 14, 2021

@jakoss Thank you!

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.

Crashes Upon Load
4 participants