Instagram / IGListKit

A data-driven UICollectionView framework for building fast and flexible lists.
https://instagram.github.io/IGListKit/
MIT License
12.87k stars 1.54k forks source link

IGListReloadDataUpdater and Mantle #490

Closed PhilCai1993 closed 7 years ago

PhilCai1993 commented 7 years ago

Recently I came across a trouble using IGListReloadDataUpdater with Mantle 😂

When I'm fetching data from server, for example, searching with a keyword. Everytime when I got a JSON array, it's mapped using MTLModel in Mantle.

@interface Man:MTLModel<IGListDiffable>
@property NSString *name;
@property int age;
@end

JSON:

[
{name:a,age:1},
{name:b,age:2},
]

If keywords are equal, the JSON is equal. But when mapped to subclass ofMTLModel, the results are only semantically equal, with different pointers.

But MTLModel has implemented its own hash and isEqual:. IGListReloadDataUpdater uses NSPointerFunctionsObjectPersonality as objectLookupPointerFunctions. So sectionControllerForObject: in IGListSectionMap doesn't return nil, it returns something.

For example: First time: I got [Man<0x140fadd30>, Man<0x140fadd31>], then performupdate..., collectionview refresh.✅ Second time: I got [Man<0x140fadd32>, Man<0x140fadd33>].then performupdate..., because [<0x140fadd30> isEqual:<0x140fadd32>] and [<0x140fadd31> isEqual:<0x140fadd33>](hashes are equal too). When -[IGListAdapter updateObjects:dataSource:] , this line might return some sectionController, and [dataSource listAdapter:self sectionControllerForObject:object] will not be called.

This behavior is a little unexpected. I subclassed IGListReloadDataUpdater and override - objectLookupPointerFunctions using NSPointerFunctionsObjectPointerPersonality as a workaround, instead of NSPointerFunctionsObjectPersonality.(Actually I copied almost all codes from IGListReloadDataUpdater due to subclassing restriction)

Because I really want to recreate a sectioncontroller when performupdate... Is it a right way to do that?

Thanks~

rnystrom commented 7 years ago

@PhilCai1993 hmm good point. To be honest, the reload updater is there basically just for unit tests, and predates the diffing stuff being a core part of IGListKit (used to be its own library).

Ya we should update the map used by the reload updater to account for diffing identifier and diffable equality. In fact, we should just remove that API entirely from the IGListAdapterUpdater and create the NSMapTable w/ lookup functions in IGListSectionMap.

PhilCai1993 commented 7 years ago

I'm not saying it's a bug. But since you say reload updater is basically for unit tests, I'm wondering whether it could be used just for reloadData in production use?

PhilCai1993 commented 7 years ago

I resolved this by checking this issue.

@interface ManToken:NSObject<IGListDiffable>
@property Man *man;
@end

And using self as diffIdentifier and NSObject's default isEqual: as isEqualToDiffableObject:. This avoids Mantle's equality and hash functions.