Prevent stale adapter:collectionView corruptions #517
Conversation
|
@rnystrom updated the pull request - view changes |
|
Importing b/c our CI is faster than Travis. |
|
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
lol |
Source/IGListAdapter.m
Outdated
| @@ -62,13 +62,23 @@ - (IGListCollectionView *)collectionView { | |||
| return (IGListCollectionView *)_collectionView; | |||
| } | |||
|
|
|||
| static void *kIGListCollectionViewAdapterKey = &kIGListCollectionViewAdapterKey; | |||
There was a problem hiding this comment.
where is this used? 🤔
Source/IGListAdapter.m
Outdated
| - (void)setCollectionView:(IGListCollectionView *)collectionView { | ||
| IGAssertMainThread(); | ||
| IGParameterAssert([collectionView isKindOfClass:[IGListCollectionView class]]); |
There was a problem hiding this comment.
unrelated to this PR? 🤔
There was a problem hiding this comment.
Have to remove so we can set to nil
|
|
||
| // if collection view has been used by a different list adapter, treat it as if we were using a new collection view | ||
| // this happens when embedding a IGListCollectionView inside a UICollectionViewCell that is reused | ||
| if (_collectionView != collectionView || _collectionView.dataSource != self) { | ||
| // if the collection view was being used with another IGListAdapter (e.g. cell reuse) | ||
| // destroy the previous association so the old adapter doesn't update the wrong collection view | ||
| static NSMapTable<IGListCollectionView *, IGListAdapter *> *globalCollectionViewAdapterMap = nil; |
There was a problem hiding this comment.
we're main-thread-only, but should we still consider using dispatch_once ?
There was a problem hiding this comment.
Totally fine if main-thread affined, dispatch_once will add unused overhead
|
💯 |
|
@rnystrom updated the pull request - view changes - changes since last import |
|
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
|
Changes in this pull request
This becomes an issue pretty easily in embedded
IGListAdapterenvironments (lists in lists). IGListKit creates a singleIGListSectionControllerinstance per object, but cells are reused withinIGListCollectionView. If you assignadapter.collectionView = cell.collectionView(like we recommend in our example), then you can have many adapters with a weak ref to the same collection view.Obviously when updates happen across all embedded lists, adapters could potentially update the same collection view and apply a corrupted batch update.
I proposed using
<objc/runtime.h>and using associated objects, but sinceOBJC_ASSOCIATION_ASSIGNis not zerod-on-release reference (its justunsafe_unretained) I'd have to make a weak wrapped object, which is overkill.Instead I think a global weak:weak map is fine. We don't have to worry about locking b/c of the main-thread affinity of the framework. Please push back on me if you have a better idea.
Issue fixed: #503
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.