[ASCollectionView] Deleting first item - inconsistency with associated supplementary node header

[tungvoduc]

I have a supplementary header node in the collection view and when I delete the first node, after that scroll it upward. I get crash immediately.

[hannahmbanana]

@tungvoduc: Thanks for reporting this! Would it be possible to submit a sample project showing this behavior? Command + D on the project folder will create a copy of the project. Feel free to email it to AsyncDisplayKit@gmail.com if you don't want to post it here.

[tungvoduc]

@hannahmbanana , I actually repro with the sample project CustomCollectionView which is using MosaicCollectionViewLayout.

Easy way to make is crash to to put these lines into the view controller implementation file:
-(void)collectionView:(UICollectionView *)collectionView didSelectItemAtIndexPath:(NSIndexPath *)indexPath { [[_sections objectAtIndex:indexPath.section] removeObjectAtIndex:indexPath.row]; [_collectionView deleteItemsAtIndexPaths:[NSArray arrayWithObjects:indexPath, nil]]; }

Then tapping some random item to delete it and scroll up and down and it will crash.

[hannahmbanana]

@tungvoduc: I confirmed that this is a regression after merging PR #1629 (cc @erichoracek).

@maicki: Are you familiar with this change?

[erichoracek]

@hannahmbanana I would get a crash after any item-level mutation when the collection view had supplementary nodes for each row (not just deletes of the last item) before my pull request. Section-level mutations worked without issue. This crash was already documented in #1278 and occurred in the same place as is documented here. ("Supplementary node should exist.")

If this behavior is reverted (#1772), we'd be trading crashes in some scenarios for crashes in other scenarios. Perhaps there is a logical error in the recalculation code I wrote? I'll attempt to clone down the example project and see if I can find out anything.

Apologies for introducing a regression!

[appleguy]

Hey Eric - thanks for your note! Since the patch is fairly large, I doubt we will actually push a revert, but it is concerning that this scenario (easy to reproduce) is indeed an item-level mutation that did not crash previously.

I think this does highlight that the Supplementary node implementation has some issues that the normal cell / data controller code paths do not, and really does need an overhaul. I’m hoping we can find a surgical fix, or at least a specific cause, of what behavior in the new code results in this issue so that the author of the fix on our team has enough context to pursue.

On Jun 18, 2016, at 2:55 PM, Eric Horacek notifications@github.com wrote:

@hannahmbanana https://github.com/hannahmbanana I would get an assertion after any item-level mutation when the collection view had supplementary nodes for each row (not just deletes of the last item) before my pull request. Section-level mutations worked without issue. This crash was already documented in #1278 #1278 and occurred in the same place as is documented here. ("Supplementary node should exist.")

If this behavior is reverted (#1772 #1772), we'd be trading crashes in some scenarios for crashes in other scenarios. Perhaps there is a logical error in the recalculation code I wrote? I'll attempt to clone down the example project and see if I can find out anything.

Apologies for introducing a regression!

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub #1771 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAigA7vcDVWh5XtvtU7Am6GMfIbDMqQiks5qNGk7gaJpZM4I4_U1.

[erichoracek]

@appleguy After digging into it, it looks like this is because UICollectionView supplementary views can be displayed even if there is not an item at their index path. Flow layout headers are a good example: even if you delete the cell at index 0, the header supplementary view at index 0 remains visible. As such, I believe that we can fix this issue by defensively re-populating the supplementary nodes if the deleted index paths contains the 0 index. I'll go ahead and create a pull request with this fix.

Definitely agree on the supplementary node implementation having some issues on the whole as well. Hopefully a new implementation can solve some issues!

[hannahmbanana]

@erichoracek: many thanks for investigating this and putting up PR!