Summary:
I'm looking at updating IGKitKit's test suite for Xcode 15, but when I went to do a build, it failed at the `IGWarnAssert` macro. Turns out that because we set the condition in the macro to always return `(NO)`, there's no need to expose `frmt` to test for the condition.
This diff removes `frmt` from the define, allowing the tests to build again.
Reviewed By: fabiomassimo
Differential Revision:
D49628618
Privacy Context Container: L1203725
fbshipit-source-id: b0456a149bf5be96c8401a21866880ccd72ebbcb
Summary: This assertion has been firing for at least the [last 3 months](https://fburl.com/scuba/errorreporting_instagram_ios_assertions/vfxo4h2g) and we have shipped the app anyways so it is definitely not launch blocking.
Reviewed By: fabiomassimo
Differential Revision: D49281317
fbshipit-source-id: d525f88fe676e813b8413a2e0f21b6b12728b359
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
Issue fixed: #
### 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/1299
Reviewed By: m3rlin45
Differential Revision: D18703628
Pulled By: TimOliver
fbshipit-source-id: d347ed9c59c62ccdb73bd3abd14e289b7c06f3f6
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:
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:
Chatted with maxolls about this assertion - it fires when a user of IGListKit accidentally implements a UICV layout (which IGListKit will ignore). Definitely not ideal, but not a launch blocking issue so demoting this assert to a warning.
It's currently the second highest firing mustfix in alpha 239.
https://fburl.com/scuba/errorreporting_instagram_ios_assertions/yap7opjy
Reviewed By: maxolls
Differential Revision: D36983661
fbshipit-source-id: 00497637cffea7553495c837bec267acca7cd3d8
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:
In T97824645 a test that was failing for other reason was fixed and started
failing because it could not compile IGListKit.
The problem is that IGListKit was not prepared to be compiled for Mac Catalyst
since it was only testing for iOS platforms to include UIKit. These changes
modify the header to work in Mac Catalyst.
Differential Revision: D30410680
fbshipit-source-id: f886826b9c485acfb039a9e681afd85f9186c344
Summary:
This was accidentally introduced in D25334717 (9cb16b0f31), at the time I didn't pay close attention to the impact there. The diff actually broke the CI for IGListKit in open source community since IGListDiffKit was meant to be compatible in both MacOS and iOS.
The right approach is to rely on the IGListCompatibility.h to resolve the linked code here.
Reviewed By: rzito
Differential Revision: D30344334
fbshipit-source-id: 842ca560a1bfe6d54e7d90466575e4ed35cd9b87
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:
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: 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
Summary: `NSIndexPath` is in UIKit, so we should be importing its header.
Reviewed By: lorixx
Differential Revision: D25334717
fbshipit-source-id: 5a134a0ddbe88fddb33adfa182d7aaf437bc47d0
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
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:
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
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
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
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
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
Summary: Actually this won't work and causes even more crashes. For example, if we reload cell 0 and insert at 0, we end up with just a single delete & insert at 0, which causes an inconsistency exception. `IGListApplyUpdatesToCollectionView` already takes care of avoiding multiple reloads of the same `NSIndexPath` by using a `NSMutableSet` when collecting the `reloadDeletePaths`.
Reviewed By: apadalko
Differential Revision: D21822502
fbshipit-source-id: c2382951ffe013f82c1de2492286d2911f40444d
Summary: Ship this experiment. `IGListCollectionViewLayout` should get the section/index counts via `UICollectionView` to stay in sync, instead of the `dataSource`.
Reviewed By: apadalko
Differential Revision: D21789871
fbshipit-source-id: 9fe069bd793b36680cd8562e69c7367d69a11ec7
Summary: This allows us to potentially enable `-Wundef` in the future. Behavior is the same, undefined macros are false.
Differential Revision: D20906603
fbshipit-source-id: d5798861f0c9dfcd7a87936d575621d52c0ef7c7
Summary:
**Context**
Recently an IGListKit experiment was introduced, `IGListExperimentPerformUpdatesWithoutDeferringCATransactionCommit`. When enabled, we opt to no longer defer a call to `[CATransaction commit]`, because it could feasibly end a different transaction than intended. In practice, this was leading to issues with `UIViewPropertyAnimator`, where deffered commits were ending in-progress animators.
**This Change**
The results we have seen from enabling this fix show no changes to performance and stability, so this seems safe to roll out. This change removes the experiment, and enables the new, non-deffered behavior.
Reviewed By: lorixx
Differential Revision: D20120169
fbshipit-source-id: 0473652020a250d67b02b860fb74c73e43615aef
Summary:
**Context**
When performing non-animated updates, `IGListKit` does so by beginning a `CATransaction` before `performBatchUpdates:`, and ending it in the completion block. While this disables animations for the collection view updates, it is feasible that some other changes might be bundled into the same transaction, given that we have no guarantee of the completion block being called inline.
It seems that this is happening in practice and occasionally disrupting `UIViewPropertyAnimator` animations, forcing the completion immediately.
**This Change**
To fix this issue, we can commit the transaction inline. Here we use `performWithoutAnimation:`, although it's possible that this is just a wrapper around `CATransaction` calls anyway.
Because the change may have unknown side effects, this change is gated by an experiment.
Reviewed By: maxolls
Differential Revision: D19150307
fbshipit-source-id: f7cff46ac7141609f42caafaf69bad71a980ad29
Summary:
Pull Request resolved: https://github.com/Instagram/IGListKit/pull/1377
This splits the diffing-only components from `IGListKit` out into a separate library, `IGListDiffKit`, allowing them to be used independently of the rest of `IGListKit`. We've found that the diffing components of the library are useful independently of the rest of the `UICollectionView` infrastructure, and separating the libraries allows apps to take advantage of those components without paying the full binary size cost of `IGListKit`. The diffing components are available as a subspec in Cocoapods, and as an independent framework in Carthage and direct Xcode project import (i.e. Git submodules).
This is a breaking change, necessitating a major version bump, although it should have only minor impact for projects using the umbrella header `IGListKit.h`, as that header continues to import the diffing utilities. In particular, for a project using Cocoapods and importing `IGListKit.h`, there should be no code changes required.
Reviewed By: calimarkus
Differential Revision: D18114249
fbshipit-source-id: 363b5a427e32800bbc6e82f897230963b4167245
