Differ
Differ copied to clipboard
Revision of UIKit Overlay's API
I would like to revise Differ's public API in terms of its UIKit overlay. The changes I would like to make are the following:
1. Optional completion handlers for methods on UITableView
The UICollectionView
already has this parameter and it seems reasonable to support this on UITableView
as well. The intuitive approach to implement this would be using performBatchUpdates(_:completion:)
instead of beginUpdates()
/endUpdates()
. This method, however, is only available on iOS 11+.
Alternatively, we could keep the old methods and add extra @available(iOS 11.0, *)
methods with completion handlers.
Another idea would be to hook into CATransaction
, wrapping beginUpdates()
/endUpdates()
in CATransaction.begin()
/CATransaction.commit()
and registering the completion handler using CATransaction.setCompletionBlock(_:)
. I highly doubt that this would have the same semantics as using performBatchUpdates(_:completion:)
's completion handler.
Personally, I would make this a major version bump (see other changes below), drop support for iOS 10 and earlier, and simply use the modern performBatchUpdates(_:completion:)
API.
The update block passed to performBatchUpdates(_:completion:)
is also optional, meaning it is implicitly escaping, requiring us to annotate things like indexPathTransform
as @escaping
as well. I don't think that's a huge deal, though. I'm not sure if the block even escapes; we could try using withoutActuallyEscaping(_:do)
if there are strong reasons to not annotate the closures as @escaping
, though I'd try to avoid that if possible in case the block actually does escape.
2. Mandatory updateData
parameter for UITableView
Again, this is a feature we already have for UICollectionView
, and we should port it to UITableView
as well. Currently, it is very easy to use the Differ/UITableView
/UICollectionView
APIs incorrectly, as #62 shows. Adding this parameter alone is not going to solve these issues, but it at least tells users of the library at which point they are supposed to update the data source.
Unfortunately, the intuitive way of using Differ calls things like animateRowChanges(oldData:newData:)
when the underlying data has already changed, e.g., from inside a didSet
property observer, which is incorrect. This causes crashes when a UITableView
/UICollectionView
has not queried its data source or the cache has been invalidated prior to scheduling an animation. In these cases, the UITableView/UICollectionView query its data twice, before and after the changes specified by the diff, but ends up querying the new data twice, thus crashing with an NSInternalInconsistencyException
. Problems like this may be anchored deeply within an app's architecture and it might not be feasible to call Differ before the data changes and update the data source only once Differ tells you to.
We could try to prevent the crashes by checking if a diff is compatible with the current state of a UITableView
/UICollectionView
before applying it, and calling reloadData()
instead if it is not. I'm not sure how this interacts with nested calls to performBatchUpdates(_:completion:)
and will need to investigate a bit more. If you have any other ideas to address this issue, please let me know.
Even with this safeguard in place, I believe we should make the parameter mandatory to force users to think about when they update the data source. This way, users would need to opt in to using the APIs incorrectly.
Personally, I have no experience with making breaking changes in a library. Do you think we should keep the old signatures around and annotate them as @available(*, deprecated, message: "")
to make migration to 2.0 easier?
3. Optional animated: Bool
parameter for UITableView
and UICollectionView
.
Sometimes, we may want to update the UITableView
's data without animation, for example, if the UITableView
is currently offscreen or if the user has the "Reduce Motion" accessibility feature enabled. In these cases, updating the relevant parts of the UITableView
via performBatchUpdates(_:completion:)
is much more efficient than simply calling reloadData()
, which causes all cells to be reloaded. reloadData()
also has the nasty side effect of cancelling touches if the user is currently interacting with the cells.
If animated
is false
, we could simply wrap the call to performBatchUpdates(_:completion:)
in UIView.performWithoutAnimation(_:)
.
In summary, here are the changes I would like to make to the UIKit overlay. Analogous changes to the animateRowChanges
, animateRowAndSectionChanges
, animateItemChanges
, and animateItemAndSectionChanges
families of methods are intentionally left out here. Having the methods begin with "animated" and then having an animated: Bool
parameter might be a bit strange — maybe we should rename the methods to "applyRowChanges" or "transitionRows" or something else?
extension UITableView {
func apply(
_ diff: ExtendedDiff,
deletionAnimation: DiffRowAnimation = .automatic,
insertionAnimation: DiffRowAnimation = .automatic,
- indexPathTransform: (IndexPath) -> IndexPath = { $0 },
+ indexPathTransform: @escaping (IndexPath) -> IndexPath = { $0 },
+ updateData: () -> Void,
+ animated: Bool = true,
+ completion: ((_ finished: Bool) -> Void)? = nil,
)
func apply(
_ diff: NestedExtendedDiff,
rowDeletionAnimation: DiffRowAnimation = .automatic,
rowInsertionAnimation: DiffRowAnimation = .automatic,
sectionDeletionAnimation: DiffRowAnimation = .automatic,
sectionInsertionAnimation: DiffRowAnimation = .automatic,
- indexPathTransform: (IndexPath) -> IndexPath,
+ indexPathTransform: @escaping (IndexPath) -> IndexPath,
- sectionTransform: (Int) -> Int,
+ sectionTransform: @escaping (Int) -> Int,
+ updateData: () -> Void,
+ animated: Bool = true,
+ completion: ((_ finished: Bool) -> Void)? = nil,
)
}
extension UICollectionView {
func apply(
_ diff: ExtendedDiff,
updateData: () -> Void,
+ animated: Bool = true,
completion: ((Bool) -> Void)? = nil,
indexPathTransform: @escaping (IndexPath) -> IndexPath = { $0 },
)
func apply(
_ diff: NestedExtendedDiff,
indexPathTransform: @escaping (IndexPath) -> IndexPath = { $0 },
sectionTransform: @escaping (Int) -> Int = { $0 },
updateData: () -> Void,
+ animated: Bool = true,
completion: ((Bool) -> Void)? = nil,
)
}