IGListKit: Out of bounds collectionview update (crash)

Ok I think it just clicked for me:

• Each section controller is a unique instance, each with its own unique adapter instance
• Cells are being reused, attaching adapter.collectionView = cell.collectionView when the cell is reused

However adapters off-screen still have weak references to collection views that aren’t theirs anymore!

I think I have a solution to fix this:

We introduce an associated object on IGListCollectionView for the adapter. When setting the collection view, check if there is an adapter associated. If it exists, set its collection view to nil.

I took your example and made it work by:

• Setting displayDelegate = self HorizontalSectionController.init()
• Add the IGListDisplayDelegate protocol
• In the listAdapter(listAdapter:didEndDisplaying sectionController:cell:at index:) set adapter.collectionView = nil
• Comment out this assert

@heumn IMO this is a bug in IGListKit that we need to fix. Unfortunately that means you’ll need to patch your project to get it working, which is a bummer. But the 3.0.0 milestone will include this fix.

cc @jessesquires for all this. It’s a good one.

Is this true? I thought 1 adapter powered a single list. Or is this only true for the “inception” (collection-view-inside-a-cell) case?

This is only for the inception setups that wraps “collection-view-inside-a-cell” 😃

@heumn thanks for reporting and including an example project! I’m going to spin through this and see what’s going on.

@jessesquires we’ll still need to do this even for #31 (in order to finish that w/out hacks) b/c how we use single section controller instances vs cell reuse (collection view gets shared between instances).

@heumn aha, I see your point about “resetting” the collection view! It’ll still be in a dirty state. Ok, great, I think I’m on the same page. I’m going to play with some ideas and report back.

The code you outlined (all the work in the setter) was added specifically to handle cell reuse w/ embedded collection views. This setup is how we use it in Instagram

Hehe… I understood that, but arent you still keeping the state/knowedge about the state of the UICollectionView indirectly in the NSMapTable? Thus the code outlined gives you a false sense of “clean slate” for the UICollectionView as far as I understood 😃 This seems like the root cause of the “bug” / “edge case”. Then when the performUpdatesAnimatedis called, it tries to “connect the dots” from the previous state (before the re-use) of the UICollectionView that now serves as UI layer for another adapter.

That is very very much by design, since the point of the adapter is to handle edge cases, updating, and provide APIs, all of which require the collection view to work in a specific way.

Agreed that there is a bit of magic, but being open source, I wouldn’t go as far as saying “black box” 😅.

Haha… Agree… Apologies for the wording again (Norwegians and their norweglish and weird translated sentences). What I was trying to articulate was rather the handoff of knowledge and control by doing adapter.collectionView instead of answering to some type of protocol

This was a #greatdebate when building IGListKit. Reusing SCs makes tracking state (e.g. “selected”) trickier b/c you have to map sections/indices/cells to state vs just a @property BOOL selected; on the SC.

Got it. I fully understand there is a lot of discussions / pros / cons weighted in when designing something like this that I am missing 😃 Adds some limitations, but has some great benefits(!)

We actually don’t update and fallback to reload when there is no window, but I think cells that are in the reuse pool are still subviews but have hidden = YES. We should do digging here, maybe adding || collectionView.hidden == YES to this.

I think that would only partially solve the problem because that would only be true for the UICollectionView that is not re-used yet and still in the pool. 🤔 The re-used ones will still crash (as they are not hidden). The old adapter will try to update a UICollectionView that is now “owned” by another adapter

Can you expand on what visibility means here? Not sure I follow.

That was a bad wording (again). What I meant was knowledge about reused / not reused.

We’re planning on getting rid of IGListCollectionView in #409, just lower priority. @amonshiz even mentioned adding UITableView support! But that’s a ways off.

That sounds like a great idea 👌

What I propose for now is that I research how I can to stop the adapters from updating a re-used UICollectionView they shouldnt update any more

@heumn ack, so everything is correct, but the batch update is using the wrong IGListCollectionView!

That’s because when the update is queued inside HorizontalSectionController.didUpdate(to object:).

Here’s some proof of the issue:

I’m still rooting to fully understand the chain of events, but the biggest issue is the adapter.performUpdates(...) call within didUpdate(to object:), and the fact that IGListAdapter uses the current collection view when updates is called.

Will try to come up w/ an elegant solution that doesn’t require a bit update, might require updating like #494… Also lights a 🔥 that we need to get a stock embedded section controller built to help avoid these situations!

Hang in there @heumn, we’ll get this! cc @PhilCai1993 too in case you have any ideas.