Instagram / Instagram/IGListKit

[RFC] Change IGListDiff result type to generic object with array of operations

Open
#694 2 comments 0 reactions 0 assignees View on GitHub
proposal
Dominant language
Objective-C
Stars
13.1k
Forks
1.5k
PR merge metrics
No merged PRs in 30d

Description

Had a shower-thought this morning: what if we change the result produced by `IGListDiff(...)` to be a generic result object with an array of operations (delete/insert/reload/move)?

### Why?

There are currently too many things doing essentially the same stuff:

- [IGListIndexSetResult](https://github.com/Instagram/IGListKit/blob/master/Source/Common/IGListIndexSetResult.h) and [IGListIndexPathResult](https://github.com/Instagram/IGListKit/blob/master/Source/Common/IGListIndexPathResult.h)
- `NSIndexSet` or `NSArray`
- Have multiple delete, insert, update, and moves
- Keep a map tracking from/to for identifiers
- [IGListMoveIndex](https://github.com/Instagram/IGListKit/blob/master/Source/Common/IGListMoveIndex.h) and [IGListMoveIndexPath](https://github.com/Instagram/IGListKit/blob/master/Source/Common/IGListMoveIndexPath.h)
- `NSInteger` or `NSIndexPath`
- Has a to/from
- [IGListReloadIndexPath](https://github.com/Instagram/IGListKit/blob/master/Source/Internal/IGListReloadIndexPath.h)
- `NSIndexPath`
- Has a to/from

There's a lot of duplication going on here. I think we can clean it up:

### Proposal

Create a generic "operation" object:

```objc
@interface IGListOperation: NSObject
@property (nonatomic, strong, readonly) ObjectType from;
@property (nonatomic, strong, readonly) ObjectType to;
@property (nonatomic, assign, readonly) IGListOperationType type; // delete, insert, move, update
@end
```

Then we create a new result object:

```objc
@interface IGListDiffResult: NSObject
@property (nonatomic, strong, readonly) NSArray *> *operations;
@end
```

I'd also love to restore diff statistics (like clocking the time it takes to diff). We used to collect this data.

We could also add back APIs like `-[IGListDiffResult inserts]` that just query an internal map of operation types to operations. Might be convenient?

### Considerations

- We [merge diff results](https://github.com/Instagram/IGListKit/blob/master/Source/IGListAdapterUpdater.m#L254-L302) with other coalesced operations. This still has to happen.
- Operations should implement `isEqual:` and `hash` so we can add and unique them in `NSSet`s

### Questions

- Can we intermix `NSIndexPath` and `NSInteger` with generic types?
- I don't think so, and if even if we could, I don't think we could right a proper `hash` and `isEqual` for the operation
- Is unpacking `NSNumber` with `integerValue` when enumerating operations a concern at all? The perf overhead should be minuscule, but it does exist

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.