Summary: Similar to the previous diff, this diff updates some of the code styling and syntax in IGListKit's internal classes to make it much easier to provide test coverage to each method in there.
Reviewed By: candance
Differential Revision: D45003471
fbshipit-source-id: 1267901fc7a917bf1c30783654a8fd0da941bff1
Summary:
While testing the existing code coverage, I discovered several code patterns in some of IGListKit's public classes that were literally impossible to cover. These included things like bailing out of blocks when `weakSelf` is `nil`, or exiting early inside `switch` statements.
This diff reimplements some of the existing logic in several of the public facing IGListKit classes, so that all of these classes can be 100% covered by unit tests.
Reviewed By: candance
Differential Revision: D45002889
fbshipit-source-id: 6b87ea6338b9f33bed7955d6cc797d116b533085
Summary:
While auditing all of the classes in IGListKit, I discovered that `IGListBatchUpdates` is completely unused. It looks like in the documentation that a new class was introduced to replace it, and it looks like this transition has already completed.
Since it doesn't make sense to cover an unused class, let's delete it. :)
Differential Revision: D45002736
fbshipit-source-id: 45e39f37bf00dc6c4867e9df49cfdfce1252604f
Summary: We keep receiving must fixes for this assert that effectively are not followed up since they are hard to debug and seems do not affect the app. Demoting.
Reviewed By: candance
Differential Revision: D44934015
fbshipit-source-id: a65b8a5980a1e2a8ab48201987e90fdff646c498
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:
This diff imports and refines the PR made by cntrump on GitHub.
The PR introduces the following:
* Sample apps now use SPM instead of CocoaPods to import IGListKit.
* Adds Mac Catalyst as an example target.
* Adds C++ flags to the CocoaPods specs.
* Fixes a script issue that was discovered when regenerating the symlinks.
The PR originally aimed to remove the need for symlinked references to the IGListKit and IGListDiffKit source files, but in testing, I couldn't get it working. It's possible SPM being too strict [on where the headers can be placed to be discovered](https://forums.swift.org/t/how-do-i-specify-the-headers-directory-for-a-objc-target-in-swift-package-managers-package-swift/58531/3).
Additionally, another issue was that the original PR changed all of the `#import` statements to the the modular `import` statements, which is fine for the sample apps, but ended up breaking compatibility for any apps that had modules disabled.
## Changes in this pull request
Improve SwiftPM support:
Build module `IGListDiffKit` and `IGListKit` as Objective-C++.
module `IGListDiffKit`:
- Source/IGListDiffKit
- module defined in `Source/IGListDiffKit/modulemap/module.modulemap`
- requires `-fmodules` and `-fcxx-modules`
module `IGListKit`:
- depend on `IGListDiffKit`, use `import IGListDiffKit;`
- Source/IGListKit
- module defined in `Source/IGListKit/modulemap/module.modulemap`
- requires `-fmodules` and `-fcxx-modules`
module `IGListSwiftKit`:
- depend on `IGListKit`, use `import IGListKit`
- Source/IGListSwiftKit
Deleted `spm/` and `scripts/generate_spm_sources_layout.sh`, it is unnecessary.
Updated `.podspec`, add `'OTHER_CFLAGS' => '-fmodules'` and `'OTHER_CPLUSPLUSFLAGS' => '-fcxx-modules'`.
Add missing swift files for `IGListSwiftKit` in `IGListKit.xcodeproj`
### How to use
Replace `#import <IGListDiffKit/IGListDiffKit.h>` with `import IGListDiffKit;`, because `IGListDiffKit.h` isn't exist in `modulemap/`, Or create a symbol link by `ln -sf ../IGListDiffKit.h` for support it ?
### Examples
Use SwiftPM for building examples.
### 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/1546
Test Plan: Test PR showing running tests: https://github.com/TimOliver/IGListKit/actions/runs/4339956050/jobs/7578047058
Reviewed By: lorixx
Differential Revision: D33592395
Pulled By: TimOliver
fbshipit-source-id: 8f7b1873f2b1c6a80908bb55b123e31bea13bb0c
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:
Coming from the discussion in D40925763. It seems the reference to importing `IGListCollectionContext.` in `IGListAdapter` is unused. I was wondering if there was a deliberate design decision here to expose the `IGListCollectionContext` in here publicly at some point that never made it through. After consulting with some other iOS engineers, we agreed it's best not to expand the public facing surface area without some kind of tangible benefit, so lets remove this import to remove any more confusion.
I'll do a proper audit later when I have more time to see how many other unused header references there are in this library.
Differential Revision: D41179480
fbshipit-source-id: 2a96c199bd4357310e2517be8f0f154e7af91948
Summary: Almost all of the [warnings](https://fburl.com/scuba/errorreporting_instagram_ios_assertions/s2odhkpd) at `[IGListAdapter visibleObjects]` shown the section index is `9223372036854775807`, which is `NSNotFound`. When the section is `NSNotFound`, there is no need to get object though `[self objectAtSection:section];`, instead we should skip all the operations.
Differential Revision: D39294533
fbshipit-source-id: 3e481c76c0a2dd53b219800e590395b067ae2db8
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:
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:
I was assuming the delegate would always be an IGListAdapterProxy, but this only happens if the client sets either the `collectionViewDelegate` or `scrollViewDelegate` properties on IGListAdapter. Otherwise the delegate will just be the IGListAdapter. Change the debug code to handle this case.
We were starting to see some debug logs roll in with the delegate class listed as IGListAdapter. Hopefully this will return the correct class now.
Differential Revision: D31690438
fbshipit-source-id: bf6dde57756fee4fe617944d45e152cae734b4f1
Summary:
Trying to track down the causes of this assert. Unfortunately the assert does not tell us which class is returning the bad info. Add some HACKY TEMP code to print out the class name in some cases. Hopefully this will catch all the issues.
I put the code in a static function called within the assert, so it should be compiled out of prod builds.
Differential Revision: D31482081
fbshipit-source-id: bef0274c0e35b90d5f77b0cfd337e3f9a772e8b8
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:
## 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:
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:
## 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:
**In this diff**
Add `containerContentOffset` to `IGListCollectionContext`
- In this stack, I check this value from the product tile and content tile section controllers to determine how far the user has scrolled down the containing scroll view
**In this stack**
Add `scroll_logging_info` to product card and content tile events to track scroll depth on the Shop Tab homepage
- Project plan: https://fb.quip.com/mF9qAnGPE8CI
- Logging spec: https://fb.quip.com/6wp7AO63d8Wb#LPWACADjpNK
Differential Revision: D27793071
fbshipit-source-id: 22123511b0f85fd636d82981671ccda0b0d7db7d
Summary: Adds an init override if the generic type is a Swift value conforming to `ListIdentifiable`.
Reviewed By: natestedman
Differential Revision: D27013270
fbshipit-source-id: 44f5c45e6396643d251c23bfedb6cd0a482e3913
Summary:
Silences warning:
```
Source/IGListSwiftKit/IGListCollectionContext+Refinements.swift:90:71: warning: string interpolation produces a debug description for an optional value; did you mean to make this explicit?
fatalError("A nib named \"\(nibName)\" was not found in \(bundle)")
^~~~~~
Source/IGListSwiftKit/IGListCollectionContext+Refinements.swift:90:71: note: use 'String(describing:)' to silence this warning
fatalError("A nib named \"\(nibName)\" was not found in \(bundle)")
^~~~~~
String(describing: )
Source/IGListSwiftKit/IGListCollectionContext+Refinements.swift:90:71: note: provide a default value to avoid this warning
fatalError("A nib named \"\(nibName)\" was not found in \(bundle)")
^~~~~~
```
Reviewed By: natestedman
Differential Revision: D26320097
fbshipit-source-id: a5fa87fbe2cdf2ad8b55f985a904cf0d387debad
Summary: This API is dependent on dynamic types right now, making it awkward and unsafe to use in Swift. By providing a generic wrapper, we can make it safer to use.
Reviewed By: joetam
Differential Revision: D26057302
fbshipit-source-id: ec5d5ed202900f6171761214900fde1f9615e7f5
Summary: We don't need these synthesize anymore.
Reviewed By: joetam
Differential Revision: D26052888
fbshipit-source-id: eedb9acea68846af1a23fea15a44f94351eb999a
Summary: While building the example project, I noticed this warning
Reviewed By: lorixx
Differential Revision: D25962436
fbshipit-source-id: c686e07ac82551f658b40a72280dac29a026cb62
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: Now that the experimental updater is done, we can remove this protocol.
Reviewed By: iperry90
Differential Revision: D25884776
fbshipit-source-id: e7ce962f166aecf73ca1e8fdfa41404bc794696e
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: 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
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:
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
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
Summary: `NSIndexPath` is in UIKit, so we should be importing its header.
Reviewed By: lorixx
Differential Revision: D25334717
fbshipit-source-id: 5a134a0ddbe88fddb33adfa182d7aaf437bc47d0
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:
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
