Skip to content

Feature: Improve loading animations on the home page #10318

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 3 commits into from
Oct 28, 2022

Conversation

QuaintMako
Copy link
Contributor

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

What has been done

  • Switching from BulkConcurrentObservableCollection to ObservableCollection.
  • Minor refactoring.

Validation
How did you test these changes?

  • Built and ran the app

Screenshots
AnimationHome

@QuaintMako
Copy link
Contributor Author

Setting this as a draft for several reasons :
1- The result on my computer looks a bit choppy. I'd appreciate if someone could test it on their machine.
2- Switching from BulkCollection to ObservableCollection fixed the issue. But why was it Bulk in the first place? Need to be investigated. Still wrapping my head around what a BulkCollection is.

@hez2010
Copy link
Member

hez2010 commented Oct 28, 2022

Still wrapping my head around what a BulkCollection is.

It's our own implementation of ObservableCollection which supports bulk inserting multiple items without need of trigger CollectionChanged event when every single item gets inserted, and it's implemented in a thread-safe manner. This is a significant optimization to avoid UI freeze while dealing with plenty of data.

We prefer to use BulkConcurrentObservableCollection rather than ObservableCollection in Files.

@QuaintMako
Copy link
Contributor Author

It's our own implementation of ObservableCollection which supports bulk inserting multiple items without need of trigger CollectionChanged event when every single item gets inserted, and it's implemented in a thread-safe manner. This is a significant optimization to avoid UI freeze while dealing with plenty of data.

We prefer to use BulkConcurrentObservableCollection rather than ObservableCollection in Files.

Thanks for the explanation, I see the utility then.

In the case of this issue, maybe it's viable to use ObservableCollection? We haven't a lot of data to deal with, only six elements. In not, we'd need to somehow get BulkCollection to be "usable by animations" on the UI.

@hez2010
Copy link
Member

hez2010 commented Oct 28, 2022

I think it's fine to switch it here.

@QuaintMako QuaintMako marked this pull request as ready for review October 28, 2022 12:09
@yaira2 yaira2 changed the title Feature: Better animation on Home for folder widget Feature: Improve loading animations on the home page Oct 28, 2022
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 the ready to merge Pull requests that are approved and ready to merge label Oct 28, 2022
@yaira2 yaira2 merged commit 710cc1c into files-community:main Oct 28, 2022
@QuaintMako QuaintMako deleted the 8441_BetterAnimationStartup branch November 2, 2022 10:41
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.

Feature: Improve loading animations on the home page
3 participants