Summary:
Added a unit test to cover the inconsistency exception catch we added to capture the crashes caused by the new collection view behaviour in iOS 16.4.
The test deliberately puts the collection view state and the model state out of alignment, and then tests that the exception correctly occurs as expected.
Reviewed By: fabiomassimo
Differential Revision: D50072956
fbshipit-source-id: 4097cc0451d55d1a148156c783fe42654821113c
Summary: This diff adds a small, essentially no-op test to add coverage to the `isInDataUpdateBlock` method of `IGListReloadDataUpdater`.
Differential Revision: D49906263
fbshipit-source-id: e59a04721d6af58cc2b6148cd688be8225bf3fb8
Summary:
It seems the under-the-hood changes made to `UICollectionView` from iOS 16.4 onwards has changed our expectations of how these tests work.
When initially testing, it seemed that the new behaviour was that the existing cell in the view would get updated, but any newly inserted cells wouldn't be added to the collection view yet.
Thinking this might simply be invalid behaviour when the collection view isn't added to a superview, I added the collection view to a new `UIWindow` instance. When I re-ran the tests again while the view has a superview, all of the views update correctly, and none of them were left in a partial state.
I'm not too sure what the original intent of testing the collection view cells for being in a partially updated state was, but I think we need to reconsider that for this new `UICollectionView` behaviour.
Differential Revision: D49906268
fbshipit-source-id: 7fdc7ba3a534bd49a8a0684888283d2d1eba5912
Summary:
This test started failing in Xcode 15. It looks like now, the default behaviour of `UICollectionView` is to not perform cell sizing until the next layout pass. As such, the sizing of the cells were invalid until `layoutIfNeeded` is first called.
Adding this line to the test brings it back to all of the expected values.
Differential Revision: D49906272
fbshipit-source-id: cc05cb2e105521b2576b2b2013d06e6285f4e8a4
Summary: Duplicates an existing test and tests the same circumstances without a section controller `transitionDelegate` set. This causes all the logic to no-op and return the same instance of the original layout attributes object.
Differential Revision: D49900561
fbshipit-source-id: 8f768a998308c1aff73e8195aacdfd70579be601
Summary:
This was another test I originally added to test moving items belonging to sections containing only 1 item as this triggers the codepath to move the whole section.
It turns out this test isn't needed since the codepath is being properly tested in other tests.
Differential Revision: D49900555
fbshipit-source-id: f2586a22ef0f529fcfa10c4ef7d6d065a9a89f7d
Summary:
I'm going through the IGListKit test suite with Xcode 15 and updating the tests that subsequently started failing with the new changes made to `UICollectionView` under the hood.
This was a test I originally added to help add to the code coverage, but it looks like my expectations about how it actually should work were incorrect, and it's now getting flagged by the new `UICollectionView` logic. I stepped through all the state with breakpoints and worked out how this is supposed to work.
Differential Revision: D49898019
fbshipit-source-id: fad21d14f0ae3a0ffbee8dde502114971f35fa1c
Summary:
Follow up to D47263265
## What's happening
We're calling `listAdapter.reloadObjects(...)` in the middle of the IGListKit's internal data update, so it's using the new section index path, instead of the old one. So we're going from a list of 2, insert at 0, and try to reload index 1, then we crash (it'll try to update index 2, which doesn't exist in the old array)
## Fix
Just like for `[IGListAdapter performBatchAnimated:updates:completion:]`, lets use the old index. To get this working, lets ask the updater if we're in an update, rather than `IGListAdapter` try to keep track of it.
Sorry the diff is a bit long, but in case this feels reasonable to pick, I might
Differential Revision: D47281472
fbshipit-source-id: 193153cfc15c87084b67f58c50e921db459d6800
Summary:
## Changes in this pull request
Xcode 14 changed the minimum deployment target to iOS/tvOS 11.0 and macOS 10.13. This PR moves up the deployment targets so that a warning isn't shown on every build.
I also silenced the development region and base localization warnings.
### 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/main/.github/CONTRIBUTING.md)
Pull Request resolved: https://github.com/Instagram/IGListKit/pull/1573
Differential Revision: D45223095
Pulled By: TimOliver
fbshipit-source-id: 57fbd284809c09f86a9731d0676332de35fbe0df
Summary:
I'm still not sure if there's an easier way to test throwing methods that work on both GitHub Actions and our internal build tooling (Since GitHub does throw at asserts, and our internal tools don't), but this way at least works.
Each statement has to be contained in its own separate `try` because the first throwing method will cancel execution in the rest of the code block.
This diff separates out each throwing test into its own `try` block
Reviewed By: candance
Differential Revision: D45147876
fbshipit-source-id: 95d587d5abe4a695b1ca1f76ebf3bda3984c6065
Summary: Added making a call to the supplementary view data source in IGTestDelegateController so that codepath gets the same test exposure as the regular cell mechanism.
Reviewed By: candance
Differential Revision: D45004843
fbshipit-source-id: 6d3140ebc59da5ba4b7a151f6f104cdbd63418c2
Summary: Adds additional functionality and coverage testing to the adapter debugger class
Reviewed By: candance
Differential Revision: D45004651
fbshipit-source-id: 40330f5f2fed2946cc5ead89b30cf4030b596b65
Summary: Adds additional test coverage to the collection view and collection view layout test suites
Reviewed By: candance
Differential Revision: D45004591
fbshipit-source-id: 21587df6f9595cb54481067322aae6443e089152
Summary: I reenabled a couple of tests that had previously been marked disabled, but work properly now, and I added additional tests to cover some of the coverage cases that had previously been missed.
Reviewed By: candance
Differential Revision: D45004625
fbshipit-source-id: dacf49b2f0f420f269e3071ad4fc76762c95365c
Summary:
There were several tests in the E2E test suite that had been deliberately disabled. I reenabled them and tested them, and they all passed correctly.
I've left them reenabled in this diff, but we can disable them down the line again if they start to fail again.
Differential Revision: D45004373
fbshipit-source-id: c0bc96e1d364d702d12e945059daedf2ae21cfa5
Summary: This diff renames `IGReloadDataUpdaterTests` to `IGListReloadDataUpdaterTests` to be consistent with the naming scheme of the other classes
Differential Revision: D45004236
fbshipit-source-id: 8ecc2ca60740ff6cca1028400621a22c7b1d3406
Summary: This diff adds several new test classes to IGListKit's test suite, covering a variety of objects and use cases that weren't covered before.
Reviewed By: candance
Differential Revision: D45004015
fbshipit-source-id: 5e6cc3f56e62328173657c09c955c320e6067936
Summary: Fills in the remaining gaps in the tests in order to bring the code coverage of IGListDiffKit (ie, the diffing engine portion of IGListKit) up to 100%.
Differential Revision: D45002512
fbshipit-source-id: b4795946400f7e5ead8882b5d0f5003fb1d17aed
Summary:
The standardized Meta copyright notice is "Copyright (c) Meta Platforms, Inc. and affiliates." and not "Copyright (c) Meta Platforms, Inc. and its affiliates." (Dropping the "its")
This diff updates the copyright notice in each source file to the correct this.
Reviewed By: willbailey
Differential Revision: D44737667
fbshipit-source-id: 643bf36df76723e70d9d826c53cf8f29b8a0c8cc
Summary:
Five model classes in IGListDiffKit: `IGListBatchUpdateData`, `IGListIndexPathResult`, `IGListIndexSetResult`, `IGListMoveIndex`, and `IGListMoveIndexPath` didn't have 100% test coverage because their `description` debug string methods weren't being tested.
I added a new unit test file specifically for testing description strings in IGListDiffKit wrote a basic test to verify each one.
This brings IGListDiffKit's test coverage back up to 100%.
Differential Revision: D44002058
fbshipit-source-id: ebb1bbb3c9a6d380540fc6bbedf5b8c5e5b254f1
Summary:
There were some codepaths in `IGListBatchUpdateData.mm` that weren't completely covered by our unit test suite. Namely the equality checks where the object addresses are compared, and when incompatible objects are compared.
This diff adds 2 extra tests to cover those cases, bringing `IGListBatchUpdateData.mm`'s test coverage up to 100%.
Differential Revision: D44001262
fbshipit-source-id: 516851ca6ebdcbf3a1a4e7e6e0bd2efcef69920a
Summary:
Returns the supplementary view in the collection at the specified index for the section controller.
param `elementKind` The element kind of the supplementary view.
param `index` The index of the desired cell.
param `sectionController` The section controller requesting this information.
return The collection reusable view, or `nil` if not found.
warning This method may return `nil` if the cell is offscreen.
Reviewed By: maxolls
Differential Revision: D42726375
fbshipit-source-id: d9873440bd94140b88e7d2d56c0737ecf94925a8
Summary:
A quick push to fix something I noticed while studying how IGListKit works. This simply replaces "Facebook, Inc" with "Meta Platforms, Inc" in all of the source files where the company copyright notice is posted. This should help bring our external facing projects more in line with our new corporate branding.
There's still a lot more references to "Facebook" as a company in the library (especially around linking to other Meta sponsored open source libraries), but this might need additional scrutiny and review on a case-by-case basis, so let's handle those ones separately.
Reviewed By: lorixx
Differential Revision: D41207363
fbshipit-source-id: 57cdbf5eb1023b41a5f32c0c05e01628686a19fe
Summary:
Pull Request resolved: https://github.com/Instagram/IGListKit/pull/1544
Refactored and added functions to IGListAdapter for getting index path or scroll position of first visible item in list.
This makes it possible to see which item is the current first visible item scrolled to in the list, and determine how far it is scrolled into (analogous to additionalOffset in scrollToObject:).
Reviewed By: lorixx
Differential Revision: D32196398
fbshipit-source-id: d809d5f96bb4e1d95055dbebe4e55c1a428a40ed
Summary:
## Changes in this pull request
A better version of https://github.com/Instagram/IGListKit/issues/1465 =)
- SPM support with script-based generations.
- added macOS Catalyst support
### Generate SPM layout
1. From **project's root** run:
`bash scripts/generate_spm_sources_layout.sh`
2. Commit Changes
Repeat those steps each time you delete/add the project's files. **Make sure** to have this CI step which will check that `generate_spm_sources_layout.sh` is not broken.
Issue fixed: https://github.com/Instagram/IGListKit/issues/1368#1406
### Checklist
- [ ] All tests pass. Demo project builds and runs.
- [ ] I added tests, an experiment, or detailed why my change isn't tested.
- [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
- [ ] 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/1487
Reviewed By: DimaVartanian, candance
Differential Revision: D30428297
Pulled By: lorixx
fbshipit-source-id: 655291ff03445dec9b0b8cd97916f0c88207e9a7
Summary:
## Changes in this pull request
- set header `IGListAdapterUpdaterCompatible.h` to - public which fixed Xcode 12 unit test iOS build.
- include `IGListTransitionData.m` to tvOS trget which fixed Xcode 12 tvOS framework build.
- Added GitHub workflow `CI`.
### Checklist
- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [ ] 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/1478
Reviewed By: candance
Differential Revision: D25504500
Pulled By: lorixx
fbshipit-source-id: 91f477838f176f662dd74928b75df670a4106968
Summary:
## Context
We want the post-tap headers to be sticky so that context is maintained as the user scrolls down the media chain.
## In this diff
When clicking on a media tile in a media module (rather than the See All CTA), the sticky header was covering part of the media post's header. We want to shift the scroll-to view down so that the sticky header is sitting on top of the media post's header.
- Create new version of IGListAdapter API's `scrollToObject` function that contains an additional parameter `additionalYOffset`
- Call new `scrollToObject` function with offset of 48.0f (height of header)
## In this stack
Add sticky headers for post-taps, excluding SERP
Reviewed By: maxolls
Differential Revision: D29961626
fbshipit-source-id: c8644cc868e8d7ec92cad5f6c7742d93d001f67d
Summary:
`IGListExperimentBackgroundDiffing` has been running on Instagram for a couple months now, and we're seeing nice scroll performance improvements (less frame drops) without increasing crashes. Lets move the experiment to a real property!
Should it be enabled by default?
* I'm leaning toward no, but I could be convinced otherwise. This optimization is good for large/complex applications with lots of diffing, but I'm not sure the more common apps would need it initially. It does make updates more complicated and the order of operation will be a bit different. For example, we've seen some places call `-performUpdatesAnimated` and almost immediately expect the `IGListAdapter` data to be updated, leading to missing views. This issue can still happen without background diffing, but less often.
Reviewed By: joetam
Differential Revision: D25884786
fbshipit-source-id: 3c8755a774f63868b7dfbc8e7a2e5c012a9e7e27
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary: Lets test that the new updater (`IGListExperimentalAdapterUpdater`) can handle a missing section-controller. The old adapater `IGListAdapterUpdater` would crash.
Reviewed By: patters
Differential Revision: D23145773
fbshipit-source-id: 4589dcb11ddef5c7400947aea4b6827f1b2caaa8
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
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
