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

Implemented test for crash in ASCollectionView on reloadData#543

Merged
appleguy merged 1 commit intofacebookarchive:masterfrom
victormayorov:master
Jul 8, 2015
Merged

Implemented test for crash in ASCollectionView on reloadData#543
appleguy merged 1 commit intofacebookarchive:masterfrom
victormayorov:master

Conversation

@victormayorov
Copy link
Copy Markdown
Contributor

Test to show how ASCollectionViewController may crash. Looks like Apple bug. Simplest possible fix in ASDK is to modify ASCollectionView.mm:

- (void)reloadDataWithCompletion:(void (^)())completion
{
  ASDisplayNodeAssert(self.asyncDelegate, @"ASCollectionView's asyncDelegate property must be set.");
  ASDisplayNodePerformBlockOnMainThread(^{
    [super reloadData];
    [self layoutIfNeeded]; // Here is the FIX
  });
  [_dataController reloadDataWithAnimationOptions:kASCollectionViewAnimationNone completion:completion];
}

@appleguy
Copy link
Copy Markdown
Contributor

appleguy commented Jul 8, 2015

Very cool, thanks for creating this new Tests file. I have several ASCollectionView tests locally that I want to upload soon, both an examples project and adapting some of that code to run as part of the unit test target on every commit.

Any reason you chose to commit this DISABLED_ without including your -layoutIfNeeded fix? Do you have a sense for what is causing this and if the workaround / fix would need to be put in other methods besides -reloadData, such as -reloadSections or -reloadRows, or any other editing methods?

appleguy added a commit that referenced this pull request Jul 8, 2015
Implemented test for crash in ASCollectionView on reloadData
@appleguy appleguy merged commit 6703c95 into facebookarchive:master Jul 8, 2015
@victormayorov
Copy link
Copy Markdown
Contributor Author

@appleguy I don't like the workaround, may be you can find another solution? But this is the only way, which I know for now. It looks like Apple bug, and I haven't checked it on iOS 7. It means that the workaround theoretically can be used only for iOS 8, or all iOS older than 7. I need to investigate it. I'll do it in nearest days, and now you have possibility to look at it also.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants