Commit graph

372 commits

Author SHA1 Message Date
Maxime Ollivier
032e1b0b83 goodbye allowsBackgroundReloading
Summary:
Originally, `allowsBackgroundReloading` was added to improve performance, but ironically, it's causing lots of performance issues among other issues.
* Performance: Looking back, it's not too surprising that it causes perf issues. We're falling back to a full `-reloadData` if the view is not in the window, which can happen pretty often. For example, if a view-controller is within a `UINavigationController` stack but not on top, or within a `UITabBarController`. Because a full `-reloadData` will re-query the cells and re-create the entire layout, it's going to be more expensive than an incremental update via `-performUpdatesAnimated`. The proof is in the data and we have a few examples where this flag was the cause of significant UI stalls.
* Bugs: Because we might reload cells often, it can create strange animation artifacts. Specifically, it was breaking the `UIView` snapshots just before a transition, like the new zoom animator.

Overall, we ended disabling this feature and I think most apps will be in the same boat.

But what if this flag does improve my app's performance?
* File an issue and lets chat! I'd be curious to understand why that's the case. If a full `-reloadData` is more performant than an incremental `-performUpdatesAnimated`, than something odd is happening and I don't think this flag is the right solution.

Reviewed By: joetam

Differential Revision: D25884777

fbshipit-source-id: c4626a52082ef4c7b7300b21077529f26c551e70
2021-01-21 19:58:48 -08:00
Maxime Ollivier
6c48800ff5 remove protocol IGListAdapterUpdaterCompatible
Summary: Now that the experimental updater is done, we can remove this protocol.

Reviewed By: iperry90

Differential Revision: D25884776

fbshipit-source-id: e7ce962f166aecf73ca1e8fdfa41404bc794696e
2021-01-21 19:58:48 -08:00
Maxime Ollivier
9994d5abc2 clean up performExperimentalUpdateAnimated
Summary:
Lets clean up `-performExperimentalUpdateAnimated`
* Remove the "experimental"
* Re-order the params to match the other method
* Rename dataBlock -> sectionDataBlock to make it clear we mean sections
* Rename applyDataBlock -> applySectionDataBlock

Reviewed By: Haud

Differential Revision: D25884784

fbshipit-source-id: e24c54b43c08c02538c83ba044b1a547cd0f38ae
2021-01-21 19:58:48 -08:00
Maxime Ollivier
43af8838df merge IGListUpdatingDelegateExperimental into IGListUpdatingDelegate
Summary:
Now that the new updater has shipped, lets update `IGListUpdatingDelegate` with the new methods:
* `-performExperimentalUpdateAnimated` is the new section update method (renaming coming in the next diff)
* `-performDataSourceChange` lets us safely update the `UICollectionView` dataSource

Also, something I've been wanting to do for a long time, lets group related methods in `IGListUpdatingDelegate.h`.

Reviewed By: Haud

Differential Revision: D25884780

fbshipit-source-id: 5d9201ace8bf6b281d71ff03463cb7c911e7f967
2021-01-21 19:58:47 -08:00
Maxime Ollivier
247e7cac65 ship the new updater
Summary:
It's time to ship the new updater! `IGListExperimentalAdapterUpdater` has been running on Instagram for a couple months with better performance and stability. In this diff, we're renaming `IGListExperimentalAdapterUpdater` (and related classes) to `IGListAdapterUpdater`.

Here's a recap:

## Stability

* `[IGListAdapter setDataSource]` isn't safe
  * We're changing the underlying data without telling the `UICollectionView`.
  * Fix: Lets invalidate the `UICollectionView` data by changing its dataSource.
* `[IGListAdapter setCollectionView]` isn't safe
  * This is synchronous, but we might have pending or on-going updates. The `UICollectionView` might get synced before the pending update actually start executing, so the diff results will be off.
  * Fix: Lets wrap updates in a transaction that can be cancelled.
* Returning a nil `IGListSectionController` from `IGListAdapterDataSource` could crash
  * The `IGListAdapterUpdater` will still perform the diffing assuming that all the objects will have a section, which isn't the case.
  * Fix: Lets generate the `IGListSectionController` before the diffing.
* Other improvements
  * Lets ask for the `fromObject` just before diffing, instead of asking when scheduling the update.
  * If the `UICollectionView` section count doesn't match `fromObject`, lets fallback to a reload.

## Performance

* Re-test background diffing
  * `IGListExperimentBackgroundDiffing` and coalescing updates wasn't safe because of the issues mentioned above. The longer we wait, the more likely we'll end up in a race condition. Lets try re-testing with the stability improvements.
* Unblocks background layout calculation
  * This is a larger project, but these improvements are required to make background work safe.
* Only create the `backgroundView` if needed (although this doesn't really require the new updater)

## Other

* Transactions
  * `IGListAdapterUpdater` is the workhorse of `IGListKit` and has become a bit hard to follow over the years. We want to break it apart into simpler, more manageable parts.
* Avoid blocks
  * There's a lot of blocks flying around, making crash logs hard to read. Lets try to use methods/functions where possible.

Reviewed By: Haud, lorixx

Differential Revision: D25884782

fbshipit-source-id: 1357fa23513a239051d5b1766823effa3199f656
2021-01-21 19:58:47 -08:00
Maxime Ollivier
3b47aa27dc add debugDescriptionLines for new updater
Summary: The current `IGListAdapterUpdater` implements `-debugDescriptionLines` which is used by `IGListDebugger` to dump information about all the apaters, so lets also implement it for `IGListExperimentalAdapterUpdater`.

Reviewed By: lorixx

Differential Revision: D25884781

fbshipit-source-id: 86b227258f033f0079058af04915171d6a149241
2021-01-21 19:58:47 -08:00
Maxime Ollivier
329a4d300d ship IGListExperimentSectionCountValidation
Summary: This makes sure the `UICollectionView` section count matches what we expect before applying a diff results, if not, we fallback to a `reloadData`. This doesn't decrease the crash rate significantly, but it's nice to have a last line of defense. We do tradeoff crashes for performance issues, but it's better that an app works at all and we'll see the issue via asserts.

Reviewed By: lorixx

Differential Revision: D25884778

fbshipit-source-id: 5011d0907ce0f971ea3a0bf95c1549d52f615982
2021-01-21 19:58:47 -08:00
Maxime Ollivier
c0cf10d84c ship IGListExperimentArrayAndSetOptimization
Summary: This experiment shows a slight decrease in frame-drops on features that have more expensive hashing, so lets ship!

Reviewed By: lorixx

Differential Revision: D25884785

fbshipit-source-id: a5e33abe6f129166ab9b75de80636db801599b74
2021-01-21 19:58:46 -08:00
Zhisheng Huang
f1a7f70c24 Ship the enable inline update by default for IGListBindingSingleSectionController
Summary:
We had shipped the IGListBindingSingleSectionController and have been using it for direct inbox/message/reaction list for a while now and it's quite stable for the inline update infra, there is no need to keep this boolean around anymore.

Let's cleanup for good!

Differential Revision: D25871674

fbshipit-source-id: b6cc9de9308d2d3feb7e354040c1b49ce588a4ec
2021-01-11 14:05:37 -08:00
dirtmelon
057eaf3f14 Add shouldSelectItemAtIndex to IGListSectionController (#1479)
Summary:
## Changes in this pull request
Add `shouldSelectItemAtIndex:` to `IGListSectionController`.

Issue fixed: https://github.com/Instagram/IGListKit/issues/1033

### Checklist

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
- [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md)

Pull Request resolved: https://github.com/Instagram/IGListKit/pull/1479

Reviewed By: DimaVartanian

Differential Revision: D25562797

Pulled By: lorixx

fbshipit-source-id: 39e398914e35f2cff090c9b8bd9f32a345bc5d6f
2020-12-16 11:40:10 -08:00
Richard Zito
9cb16b0f31 Import UIKit for NSIndexPath usage
Summary: `NSIndexPath` is in UIKit, so we should be importing its header.

Reviewed By: lorixx

Differential Revision: D25334717

fbshipit-source-id: 5a134a0ddbe88fddb33adfa182d7aaf437bc47d0
2020-12-04 10:55:46 -08:00
Zhisheng Huang
2c519b0b10 Improve the visibleObjects performance
Summary:
Trying to improve my previous QE regression on all the perf: https://www.internalfb.com/intern/qe2/ig_ios_listkit_improvements_universe/ig_ios_listkit_improvements_skip_view_section_controller_map/review

My theory is that it's incresing the load in the -visibleObjects call, then we call -[UICollectionView cellForIndexPath:] which is expensive in the `_sectionControllerForCell:`

Thus my new approach is to optimize this -visibleObjects entirely and instead of dealing with visible cells, we can use the `indexPathsForVisibleItems` which can give us the indexpath instead and make it faster. It would be O(N) instead of O(N^2) compared to before.

Reviewed By: calimarkus

Differential Revision: D24663555

fbshipit-source-id: 69fcf2d0b44f8bc06351c92f96c3cbe227c22479
2020-11-02 11:18:54 -08:00
Zhisheng Huang
6311bbcfb4 Fix the visibleObjects inconsistency assertion
Summary:
Another source-of-truth inconsistency issue.

I kept getting this assertion T65423827 task.
It turned out the the root cause is that we use the `_viewSectionControllerMap` to keep track of the mapping between view and section controller.

Which is dangerous.
Because the UICV's source of truth is the `sectionMap`, now we are adding another source of truth between view <-> sectionController to be `_viewSectionControllerMap`, this is just waiting for bugs to happen.

We should always refers to sectionMap as the source of truth for everything here: since we can get the section index, we can replace all the `-sectionControllerForView` to use `-sectionControllerForSection:` instead, which is safer.

Multiple source of truth is the bug for this unexpected assertion.

Once we can cleanup this, then we dont need to use that weird: `- (void)mapView:(UICollectionReusableView *)view toSectionController:(IGListSectionController *)sectionController` API which we might forget to map, that can cause bug.

Reviewed By: elfredpagan

Differential Revision: D24173911

fbshipit-source-id: 6138cafc0b382fc569d17b14b13a6b50d85d342d
2020-10-17 02:05:24 -07:00
dirtmelon
b948982674 Replace if let with guard let (#1466)
Summary:
## Changes in this pull request

### Checklist

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
- [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md)

Pull Request resolved: https://github.com/Instagram/IGListKit/pull/1466

Reviewed By: bdotdub

Differential Revision: D24235336

Pulled By: lorixx

fbshipit-source-id: bfc5fab26ae28ff9306d1b889b82b49c6ae2a636
2020-10-12 14:29:11 -07:00
Zhisheng Huang
bee04ca961 Remove useless infra assert
Summary: Some product callsite just pass in nil object to this API thus keep causing the assertion. It's a bit too noisy and not much value here since we by default would still return `NSNotFound` here.

Reviewed By: stavash

Differential Revision: D24233480

fbshipit-source-id: 38b2eed3a9286bdd0a7eb0467894a13bbdca007a
2020-10-11 00:07:37 -07:00
Nate Stedman
5f3c7e0319 Add experimental Swift IGListKit APIs
Summary:
This diff introduces two experimental APIs to make `IGListKit` more usable with Swift values types:

- The `ListIdentifiable` protocol, which provides the equivalent of the diff identifier half of `ListDiffable`. For equality, we just use Swift's native `Equatable`.
- The `ListValueSectionController` base class, which provides a section controller interface for use with `ListIdentifiable` values.

These two APIs work together through the use of a private box class. I'd like to keep this detail hidden from callers, so that product code only deals with native Swift types.

Differential Revision: D23606413

fbshipit-source-id: 23718c508643f165c74550f1515d77448bd42d42
2020-09-22 08:00:06 -07:00
Maxime Ollivier
cbaa7a8dce fix invalidateLayout crash
Summary:
Pretty sure in `-invalidateLayout`, we should calculate the `NSIndexPaths` after the delay. Otherwise, those `NSIndexPaths` might be incorrect after the update.

Before this change, running `test_whenInvalidatingInsideBatchUpdate_andRemoveThatSectionController_thatCollectionViewDoesntCrash` crashes with:
> Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'attempting to invalidate an item at an invalid indexPath: <NSIndexPath: 0x9566e79c2077363c> {length = 2, path = 1 - 0} globalIndex: 1 numItems: 1'

Reviewed By: candance

Differential Revision: D23742819

fbshipit-source-id: 39496d04025ff3554673313df3c7516c33c305d4
2020-09-16 16:19:09 -07:00
Nate Stedman
a6526ce097 Expose ListGenericSectionController.object and ListSectionController.collectionContext as implicitly-unwrapped optionals
Summary:
These methods are currently correctly annotated as `nullable`, because they can return `nil`. However, this is not practical for writing actual Swift code that uses the APIs. Overrides of `cellForItem(at:)` //must// return a `UICollectionViewCell`. Without a collection context, there's no good way to get one, so callsites must either:

- Force-unwrap the `collectionContext`.
- `guard let`/`else fatalError` the `collectionContext`.
- Return a default `UICollectionViewCell`, which just hides the bug.

None of these are good: we don't want to encourage things force-unwraps and `fatalError` in idiomatic product code, because people will see them and get the idea that these patterns are okay to use elsewhere, and they are not. This also happens with `IGListGenericSectionController`'s `object` value: to use it when creating a cell, it must be explicitly unwrapped.

We could make these `nonnull`, since there's no enforcement of the annotations in Objective-C, that feels a bit too far, and it would break existing code that uses the optional API, because you can't force-unwrap (etc.) a non-optional value. Instead, we can use `null_resettable` explicitly. [While it's not covered by name in the Apple docs](https://developer.apple.com/documentation/swift/objective-c_and_c_code_customization/designating_nullability_in_objective-c_apis):

> Without a nullability annotation or with a null_resettable annotation—Imported as implicitly unwrapped optionals

...it bridges the property to Swift as an implicitly-unwrapped optional. I suppose it's implied as the equivalent of "without a nullability annotation".

This works even for `readonly` properties that can't be reset, and it solves both issues: it feels like less of a "100% `nonnull`" guarantee, and it's backwards-compatible with code using optionals.

To demonstrate that this is backwards-compatible with existing Swift code, this test code, which runs through various optional/non-optional APIs, compiles:

```
final class TestSectionController: ListSectionController {
    override func cellForItem(at index: Int) -> UICollectionViewCell {
        let cell = collectionContext.dequeueReusableCell(for: self, at: index)
        let optional = collectionContext?.dequeueReusableCell(for: self, at: index)
        if let _ = collectionContext {}
        return optional ?? cell
    }
}

final class TestGenericSectionController: ListGenericSectionController<NSString> {
    override func cellForItem(at index: Int) -> UICollectionViewCell {
        let label = UILabel() // imagine it's from a cell
        label.text = object as String
        label.text = (object as String) + "Suffix" // wouldn't work with optional
        label.text = object.map { $0 as String }
        if let object = self.object {
            label.text = object as String
        }
        return collectionContext.dequeueReusableCell(for: self, at: index)
    }
}
```

Reviewed By: patters, lorixx

Differential Revision: D23603716

fbshipit-source-id: e4750dcfe0072d482951dbf2e9efb1ee3de46884
2020-09-10 12:30:10 -07:00
Maxime Ollivier
02ffb1163b fix crash on missing cell
Summary: If we apply the data updates outside the `performBatchUpdates`'s `updates` block, we need to make sure the layout is up to date. Otherwise, `performBatchUpdates` will try to do a layout before calling `updates` and get the wrong cells. Calling `[self.collectionView numberOfSections]` will update the section/item count, but won't get the cells.

Reviewed By: Haud

Differential Revision: D23604257

fbshipit-source-id: 02bbf0484c46cf6e7a12ed02d45ededb3b84ac19
2020-09-09 12:41:21 -07:00
Nate Stedman
a64f9b809d Make IGListGenericSectionController's -didUpdateToObject: properly generic
Summary:
This is currently just `id`, so when you override it, you can't use the local without casting. We should declare it as `ObjectType` so that it gets the specialized type instead.

The implementation just assigns to the generic `_object` property-backing ivar, so no difference in type-safety.

Differential Revision: D23589584

fbshipit-source-id: 3784929f89580fd603fffcf940c7a5b502501bc5
2020-09-09 06:07:45 -07:00
Maxime Ollivier
29d4640997 create emptyViewForListAdapter only when needed
Summary: We don't need to create the `emptyViewForListAdapter` if it's hidden

Reviewed By: patters

Differential Revision: D23429238

fbshipit-source-id: 8d85964c177f53a0e0cc0867339c1cbf0db2ee4e
2020-09-08 09:11:12 -07:00
Maxime Ollivier
0693ca6be5 skip performBatchUpdates if possible
Summary: We don't actually need to call `[UICollectionVew performBatchUpdates ...]` if we don't have changes.

Reviewed By: patters

Differential Revision: D23386181

fbshipit-source-id: 469a22a35b9ead5181e95f36a8bc59ba36373bbe
2020-09-08 09:11:12 -07:00
Maxime Ollivier
fc8c4ba640 validate the section count before a performBatchUpdate
Summary:
We're seeing some crashes where the `UICollectionView`'s section count doesn't match the `fromObjets`. Ideally that shouldn't happen, but in case it does, lets fallback to a `reloadData` instead of crashing. Lets `IGFailAssert()` to keep an eye on it.

EDIT: Turns out `UICollectionView` will sometimes already do that for item-level inconsistencies! Example error message:
>  The number of items contained in an existing section after the update (9) must be equal to the number of items contained in that section before the update (10), plus or minus the number of items inserted or deleted from that section (0 inserted, 0 deleted) and plus or minus the number of items moved into or out of that section (0 moved in, 0 moved out). - will perform reloadData.

But it still throws a `NSInternalInconsistencyException` for section-level inconsistencies.

Reviewed By: patters

Differential Revision: D23386178

fbshipit-source-id: a9930b619701cffd37d0d6e2bcb1268ef7c9d0ce
2020-09-08 09:11:12 -07:00
Maxime Ollivier
c9144620e5 add dealloc tests
Summary:
It's easy to create a retain cycle or just forget to release a pointer, so lets test that the expendable parts of `IGListExperimentalAdapterUpdater` get deallocated after an update.

Lets also try to group related tests together. It's getting kind of hard to tell which tests already exist.

Reviewed By: patters

Differential Revision: D23386180

fbshipit-source-id: 90f0e6c7532287ee245e788dd752d45f368dc27e
2020-09-08 09:11:12 -07:00
Maxime Ollivier
4b4388d967 safer collectionView and dataSource changes
Summary:
Changing the `IGListAdapter`'s `collectionView` or `dataSource` isn't safe. For example:
  * If we change the `dataSource`, the `collectionView` will be out of sync with the `IGListAdapter`'s data and can easily crash.
  * If we change the `collectionView` or `dataSource` during background diffing, the diffing result will be out of date by the time we get back to to the main main thread.

To fix this:
* On `[IGListAdapter setDataSource: ...]`, lets also change the `UICollectionView`'s `dataSource` to invalidate any section/item counts. We don't have to call `[UICollectionView reloadData]` just yet, we just want to let the collectionView know that the counts might have changed.
* On `[IGListAdapter setDataSource: ...]` and `[IGListAdapter setCollectionView:...]`, we should cancel any pending or on-going updates. The tricky part is that we should still execute the `itemUpdateBlocks` and `completionBlocks` in the right order.

I kind of want to make the `collectionView` readonly, but I think it would be too large of a public API change at this point.

Reviewed By: patters

Differential Revision: D23151919

fbshipit-source-id: 61cf025127706acaf22f153ec148ac0ea575bc98
2020-09-08 09:11:12 -07:00
Maxime Ollivier
86f31e4017 clean up IGListBatchUpdateTransaction
Summary: Same as last diff, but for `IGListBatchUpdateTransaction`

Reviewed By: patters

Differential Revision: D23151920

fbshipit-source-id: 8941791b7870f34077a5e2beded2e913b453fbbf
2020-09-08 09:11:12 -07:00
Maxime Ollivier
d8a728ec7f clean up IGListReloadTransaction
Summary:
Using blocks can be nice, but here it causes a couple problems:
* The order of execution is a bit hard to follow. For example, in `-begin`, the very first thing we see defined is `executeCompletionBlocks` which is the last thing actually called. `IGListReloadTransaction` isn't too complex, but it gets even harder to follow in `IGListBatchUpdateTransaction`.
* It makes crash and assert reports hard to understand, because the stack includes mostly block names.

So lets turn blocks into methods.

Reviewed By: patters

Differential Revision: D23151918

fbshipit-source-id: 3ac4c77e9978c2700fd6744f084f624d27f0d300
2020-09-08 09:11:12 -07:00
Maxime Ollivier
13ad185227 create update transactions
Summary:
Lets move things to the right transaction:
* `performBatchUpdate` path to `IGListBatchUpdateTransaction`
* `reloadData` path to `IGListReloadTransaction`

Reviewed By: patters

Differential Revision: D23145770

fbshipit-source-id: e80fc05d2783e165354a147453083b449c92a61c
2020-09-08 09:11:12 -07:00
Maxime Ollivier
bba6b252a4 create IGListUpdateTransactionBuilder
Summary: Now, lets move pending changes to a separate object `IGListUpdateTransactionBuilder`, which can we discarded once the update actually starts.

Reviewed By: patters

Differential Revision: D23145774

fbshipit-source-id: e12de178de33497f476972a9ad89ebb4e8d413ab
2020-09-08 09:11:12 -07:00
Maxime Ollivier
8c1253246f replace IGListBatchUpdates with IGListItemUpdatesCollector
Summary:
Before adding `transactions`, lets clarify what information is collected before vs during the update. We can split apart `IGListBatchUpdates`.
* Before the update
  * Collect the update blocks in `itemUpdateBlocks` which will be executed later during the update.
  * Collect completion blocks in the same `completionBlocks` list as the other updates.
* During the update
  * Collect item updates in `IGListItemUpdatesCollector`, like inserts and deletes.
  * Collect new completion blocks (in case a section-controller schedules an update in -didUpdateToObject) in a separete list `inUpdateCompletionBlocks`

Reviewed By: patters

Differential Revision: D23145778

fbshipit-source-id: cae2c0689ac1ef0d93e3c04856b0495856e305b6
2020-09-08 09:11:12 -07:00
Maxime Ollivier
caf058c85a implement performExperimentalUpdateAnimated
Summary:
Lets implement `performExperimentalUpdateAnimated`. There's a couple of things happening:
* We're now using the `IGListTransitionData` container object
* We don't need `pendingTransitionToObjects` anymore because we query the `fromObjects` just before performing the diffing.
* We should update the unit tests to use `performExperimentalUpdateAnimated` since the regular `performUpdateAnimated` will now fail.

Reviewed By: patters

Differential Revision: D23145781

fbshipit-source-id: 0b896e918241e709c5eb23f56a6b7323521a2222
2020-09-08 09:11:11 -07:00
Maxime Ollivier
088d932707 duplicate IGListAdapterUpdater to begin refactor
Summary:
There's a few issues with `IGListAdapterUpdater` which would be difficult to safely fix "in place", so lets create a duplicate updater that we can test separatly.

Ugh, copy paste?
* The alternative would be to change `IGListAdapterUpdater` directly and have lots of `if/else` branching. I've tried it and it's hard to follow and easy to break. By creating a separate class, we do run the risk of having 2 diverging updaters, but I think it's worth the risk since it rarely gets changed. I'll try to ship (or burn) this new updater quickly.

Why duplicate `IGListAdapterUpdater` rather then starting from scratch?
* I want to take advantage of the existing unit tests. I can make small incremental changes and make sure the tests still pass.
* There's a lot going on in `IGListAdapterUpdater` and it's easier to refactor it by moving things around, rather then writing it from nothing.

Why does `IGListAdapterUpdater` need a clean up?
* Check out the first diff in the stack or task T74605897.

Reviewed By: patters

Differential Revision: D23145777

fbshipit-source-id: 360e980a89a5681ce38d1b5f6f1ca035eb1eb195
2020-09-08 09:11:11 -07:00
Maxime Ollivier
ad5171978e create IGListUpdatingDelegateExperimental
Summary:
For diffing, `IGListAdapterUpdater` uses the full `fromObjects` instead of `validObjects`, which can be different if we're missing `IGListSectionController`. The diffing results won't match what the `IGListAdapter` tells the `UICollectionView`, so we can crash.

To fix this, lets try to generate the `IGListSectionController` before performing the diffing, and keep them in a container object (`IGListTransitionData`) alonside the `fromObjects` and `toObjects`. This might also unblock other cool features in the future, like giving `IGListSectionControllers` time for background work, but that's for another day.

Since this change is relatively large, lets create a temporary protocol `IGListUpdatingDelegateExperimental` which will have a slightly different API from `IGListUpdatingDelegate`. When/if the new implementation ships, we can merge both protocols.

Why not edit `IGListAdapterUpdater` directly?
* I don't want to mess up exiting updaters just yet.

Reviewed By: patters

Differential Revision: D23145776

fbshipit-source-id: 21642cafa64e2a26a173e2ba683ef5c6cede17d7
2020-09-08 09:11:11 -07:00
Maxime Ollivier
38877cfc83 protocolize IGListAdapterUpdater
Summary: Create temporary protocol for `IGListAdapterUpdater` to test a different implementation. The idea is to ship (or burn) the new implementation before the next 5.0.0 version release.

Reviewed By: patters

Differential Revision: D23145772

fbshipit-source-id: ede312c4a1289a34f73c14415a9bca40033fe925
2020-09-08 09:11:11 -07:00
Maxime Ollivier
aca18c7470 ship IGListExperimentSkipLayoutBeforeUpdate
Summary: Experiment looks good, so lets ship.

Reviewed By: patters

Differential Revision: D23145775

fbshipit-source-id: 16da69f220a2247490bb99b4965bcba33f498ced
2020-09-08 09:11:11 -07:00
Maxime Ollivier
254c04196a remove unused experiment param
Summary: We pass `experiments` to places that don't need it, so lets clean that up.

Reviewed By: patters

Differential Revision: D23145782

fbshipit-source-id: affcfe0f38d1b8e544940af89e5692dbdc36dd76
2020-09-08 09:11:11 -07:00
Maxime Ollivier
0769da43df NSSet and NSArray optimization
Summary:
On `IGListAdapater` data update:
1) Pass the capacity count, so that arrays don't have to re-size.
2) Avoid using a set, so that we don't need to deal with hashes and equality. The updater should have dealt with duplicates already.

Reviewed By: patters

Differential Revision: D23145771

fbshipit-source-id: 2ed93231e15ddcd66cfe4d1f7384c563c77caa8e
2020-09-08 09:11:11 -07:00
Maxime Ollivier
af78523453 rename movesAsDeletesInserts to sectionMovesAsDeletesInserts
Summary: Lets clarify that we're taking about section moves

Reviewed By: Haud

Differential Revision: D22897102

fbshipit-source-id: ea77ef695ddae8e7b1df96d51002bc8c0cc7fab7
2020-08-03 07:19:18 -07:00
Maxime Ollivier
56fa344508 check for a valid section NSInterger in IGListAdapter
Summary:
Issue: Invalidating the layout of a `IGListSectionController` that's been removed from the map crashes, because we're passing `NSNotFound` to `[UICollectionView numberOfItemsInSection:...]`. This can easily happen if the section-controller invalidates its layout in a `performBatchAnimated` completion block, but the associated object was deleted during the update or after a full reload.

Fix: Like the rest of `IGListAdapter`, lets check for a valid `NSInteger` and bail early if we don't have it.

Reviewed By: mariajayne

Differential Revision: D22588010

fbshipit-source-id: bf0c26f313795bb10682ef6b660d6ea8a7ce9f1e
2020-07-17 08:45:24 -07:00
Maxime Ollivier
d59c266108 avoid calling UICollectionView performBatch update directly from a block
Summary: Wrap `[UICollectionView performBatchUpdates ...]` so that in case it crashes, the first app symbol will not be a block. A block name includes the line number, which means if you change the block line number, it will be categorized as a different crash. This makes tracking crashes across multiple app-versions a pain.

Differential Revision: D22398750

fbshipit-source-id: 90643e7eafe76b07e7022111f183e4734fd8a347
2020-07-07 07:26:54 -07:00
Maxime Ollivier
c9e9a4b7d4 fix param order
Summary: Noob mistake

Reviewed By: Haud

Differential Revision: D22393044

fbshipit-source-id: e27dc0d94d76df82f4aee5dc7575b2564177bc7d
2020-07-06 10:34:30 -07:00
Maxime Ollivier
d5c7076063 add IGListAdapterUpdaterDelegate calls to measure diffing performance
Summary: To prepare testing `Foundation` vs `IGListKit` diffing, lets measure how long it takes.

Reviewed By: Haud

Differential Revision: D22295546

fbshipit-source-id: 8023717f66ea68cbc24981272e8c134dd893274a
2020-07-02 13:07:03 -07:00
Nate Stedman
aa5b229ba2 Fix import in IGListKit
Summary: This header refers to `UICollectionViewCell` but does not import UIKit.

Reviewed By: candance

Differential Revision: D22340609

fbshipit-source-id: 8d03a48d363ecf3b00b29f89828d83ab44615fd4
2020-07-01 17:41:03 -07:00
Maxime Ollivier
97b0a13a59 add animated to IGListAdapterUpdaterDelegate
Summary: The duration of an update depends a lot of if it's animated, so lets pass that info to the delegate.

Reviewed By: lorixx

Differential Revision: D22265405

fbshipit-source-id: 4d164447384b84eb6f41c7f8365d0b28670e1372
2020-06-30 10:13:03 -07:00
Maxime Ollivier
e3e26c4b4b test skipping layout before performing updates
Summary: We call `[collectionView layoutIfNeeded]` before performing updates, but it's not clear why. It leads to some strange animation issues when performing updates while scrolling. So lets try to skip it?

Reviewed By: Haud

Differential Revision: D22244053

fbshipit-source-id: 0c01a73860ebc16f430fed04d5c29a5bde038286
2020-06-26 10:07:14 -07:00
Maxime Ollivier
86ecc60085 clean up IGListExperimentReloadDataFallback
Summary: This experiment shipped so we can remove it. We still need a way to disable it with `allowsReloadingOnTooManyUpdates` in case we don't want to resign the first responder on large updates.

Reviewed By: Haud

Differential Revision: D22219238

fbshipit-source-id: 98e78f3ed040809db6c4b4c8da8fd0e976aca0a2
2020-06-26 10:07:13 -07:00
Maxime Ollivier
7fc4384a78 clean up IGListExperimentDeferredToObjectCreation
Summary: Experiment shipped.

Reviewed By: Haud, lorixx

Differential Revision: D22219240

fbshipit-source-id: 9299d1371b87593fcbece4557cf760cb9c42e1fe
2020-06-26 10:07:13 -07:00
Maxime Ollivier
34c935c1a5 clean up IGListExperimentGetCollectionViewAtUpdate
Summary: Experiment shipped.

Reviewed By: Haud, lorixx

Differential Revision: D22219242

fbshipit-source-id: 60dbb5f008656ae67b02294ef16138703ebe392f
2020-06-26 10:07:13 -07:00
Maxime Ollivier
a70d2d70ae fix update queueing when using reload fallback
Summary: If we fallback to `reloadData`, we don't queue the next update. That means if you call `performUpdate` multiple times quickly, there's a chance that the last update doesn't take effect.

Reviewed By: Haud

Differential Revision: D22219237

fbshipit-source-id: 167e3428859a0194f8f8c57624504edd531480df
2020-06-26 10:07:13 -07:00
Maxime Ollivier
29bf582f47 fix IGListAdapterUpdaterDelegate
Summary:
Issue: There's a couple of situations where `IGListAdapterUpdater` updates the `collectionView` without notifying the `IGListAdapterUpdater`.
* If we call `reloadDataFallback()` because of a missing window, there's no delegate calls.
* If we call `reloadDataFallback()` because of too many diff updates, we call `willPerformBatchUpdatesWithCollectionView` but not `didPerformBatchUpdates`.

Fix: Lets clean up the delegate calls
* If we `[UICollectioView performBatchUpdates:]` lets call `willPerformBatchUpdatesWithCollectionView` & `didPerformBatchUpdates`
* If we fallback to `[UICollectionView reloadData]` lets call `willReloadDataWithCollectionView` & `didReloadDataWithCollectionView`
* If we fallback to doing nothing, lets call `didFinishWithoutUpdates`

Reviewed By: Haud

Differential Revision: D22219236

fbshipit-source-id: 1afb4df8dbbaf4725424027bb52c1a5cc5100b30
2020-06-26 10:07:13 -07:00