Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

Fix collection view data inconsistency #672

Merged

Conversation

Adlai-Holler
Copy link
Contributor

This resolves #654

We could remove the flag and just call [self layoutIfNeeded] immediately after [super reloadData]

  • + Reduces complexity of ASCollectionView
  • - Relies on implementation details of UICollectionView
  • - Performance cost

@@ -638,7 +648,7 @@ - (void)rangeControllerBeginUpdates:(ASRangeController *)rangeController {
- (void)rangeController:(ASRangeController *)rangeController endUpdatesAnimated:(BOOL)animated completion:(void (^)(BOOL))completion {
ASDisplayNodeAssertMainThread();

if (!self.asyncDataSource) {
if (!self.asyncDataSource || _pendingReloadData) {
if (completion) {
completion(NO);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we call completion(YES) if we suppressed the update due to a pending reload?

@eanagel
Copy link
Contributor

eanagel commented Sep 20, 2015

@adly-holler, this is some great detective work! The idea of simply calling layoutIfNeeded after our reloadData is very attractive indeed. (Would that actually drop this down to a 1-line commit?) You mention performance cost as a drawback to this approach, could you expand on that a bit?

@eanagel
Copy link
Contributor

eanagel commented Sep 20, 2015

Oh yes and another question, is this a potential problem for ASTableView as well? Have you explored that side of the equation?

@Adlai-Holler
Copy link
Contributor Author

@eanagel Thanks! It would indeed make this a 1-line commit, assuming that reloadData -> setNeedsLayout which I'd bet it does. The performance cost comes from laying out the cells when they don't necessarily need to be laid out. For a simple example, consider if you called reloadData 5 times during the same frame. The collection view cells would get laid out 5 times for one frame.

As for ASTableView, I don't think it behaves the same way but I'll check it tomorrow and post back on this thread either way.

appleguy added a commit that referenced this pull request Sep 20, 2015
@appleguy appleguy merged commit ca9b1ca into facebookarchive:master Sep 20, 2015
@appleguy
Copy link
Contributor

Will analyze more, merging as it does fix the test project submitted recently.

@Adlai-Holler
Copy link
Contributor Author

Note: Ported the sample project to table view and I'm not seeing the same issue.

nguyenhuy pushed a commit to nguyenhuy/AsyncDisplayKit that referenced this pull request Jan 25, 2016
Fix by using `_superIsPendingDataLoad` introduced in facebookarchive#672
nguyenhuy pushed a commit to nguyenhuy/AsyncDisplayKit that referenced this pull request Jan 25, 2016
Fix by using `_superIsPendingDataLoad` introduced in facebookarchive#672
nguyenhuy pushed a commit to nguyenhuy/AsyncDisplayKit that referenced this pull request Jan 25, 2016
Fix by using `_superIsPendingDataLoad` introduced in facebookarchive#672
nguyenhuy pushed a commit to nguyenhuy/AsyncDisplayKit that referenced this pull request Feb 2, 2016
Fix by using `_superIsPendingDataLoad` introduced in facebookarchive#672
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants