Open florianldt opened 4 years ago
Some improvement here.
After new tests, I saw that the crash occurred only when the previous value is empty.
Everything is working fine with with changing the code like this:
extension ViewController: InteractorDelegate {
func viewModelDidChange(_ old: ViewModel?, _ new: ViewModel) {
DispatchQueue.main.async {
guard old != nil && !old!.viewModels.isEmpty else {
self.tableView.reloadData()
return
}
self.tableView.animateRowChanges(oldData: old!.viewModels, newData: new.viewModels)
}
}
}
Same success when putting back RxSwift instead of the delegate.
Even if it is working as expected now. I am still questioning why the diffing isn't working when the array is empty. Having an previous value empty and a new value with 2 elements should be analyzed as 2 inserts no?
Hi @florianldt, it is difficult to tell what exactly is going on with just the code you posted here. Could you maybe provide a self-contained sample app showcasing the issue?
By the looks of the error message you are getting, I have faced a similar issue in the past. I believe this can happen if you don't use the API correctly and you schedule a transition before the UITableView
had a chance to query its data source. I'm going to try to reproduce this issue in a sample app.
When the underlying data has changed, you call animateRowChanges(oldData:newData:)
.
This diffs the data and determines where rows need to be inserted, deleted, and moved — in your case simply 7 insertions. animateRowChanges(oldData:newData:)
then delegates to beginUpdates()
and endUpdates()
. In the scenario where the UITableView
hasn't queried its data source before, beginUpdates()
does just that and finds out that the UITableView
is currently showing 7 rows. But after inserting 7 extra rows and calling endUpdates()
, the data source still reports only 7 rows, hence the internal inconsistency exception.
Scheduling the transition when the underlying data has already changed is simply too late.
A didSet
property observer or your method viewModelDidChange(_:_:)
is not the place to do so. Really, scheduling the transition when the data has already changed is always wrong and only usually works because the UITableView
caches its numberOfSections
etc.
So how can you work around this issue?
The correct way to use these APIs is updating your data source between beginUpdates()
and endUpdates()
or in the closure passed to performBatchUpdates(_:completion:)
, not prior to these calls. The UICollectionView
documentation also states that
[i]f the collection view's layout is not up to date before you call this method, a reload may occur. To avoid problems, you should update your data model inside the updates block or ensure the layout is updated before you call
performBatchUpdates(_:completion:)
.
Differ supports this since release 1.4.3 in the form of an additional updateData:
parameters on the UICollectionView
family of the transitioning methods, but not on the UITableView
family. I'm working on a pull request to bring this feature to UITableView
as well.
Once that feature lands, you should be able to resolve your issue by changing the viewModelDidChange(_:_:)
callback to something like viewModelWillChange(from:to:updateData:)
. You should probably also make var viewModel
a private(set)
because as described above, the didSet
property observer is too late to call these methods. Instead, you'll want to add a setter method that calls the delegate like so:
func setViewModel(_ newValue: ViewModel) {
if let delegate = self.delegate {
delegate?.viewModelWillChange(from: viewModel, to: newValue, updateData: { self.viewModel = newValue })
} else {
self.viewModel = newValue
}
}
Your implementation of InteractorDelegate
could then look like so:
func viewModelWillChange(from oldValue: ViewModel?, to newValue: ViewModel, updateData: () -> Void) {
self.tableView.animateRowChanges(oldData: oldValue.viewModels ?? [], newData: newValue.viewModels ?? [], updateData: updateData)
}
Alternatively, you can write a custom wrapper to animateRowChanges(oldData:newData:)
today that checks if oldData
is compatible with the current state of the UITableView
by comparing to its numberOfSections
and numberOfItems(inSection:)
. If oldData
is compatible, you can delegate to Differ, otherwise you could call reloadData()
.
@tonyarnold Considering the APIs appear to be widely used incorrectly, do you think we could deprecate the non-updateData
methods? Admittedly, it is not trivial to migrate to the correct way to do UITableView
and UICollectionView
animations, especially if the wrong way is deeply anchored within an app's architecture.
Hi and thank you for this library. I am new to diffing. I tested the example project and I am now trying to use Diffing with MMVM.
Here is the simple MVVM code I am using to try it:
Here for the
Controller
part:Here is what I got:
I first posted the question on stack overflow but maybe this is where it should be asked.
Is there something wrong with
Equatable
or am I missing something? Thank you.