NSInternalInconsistencyException using MVVM approach

[florianldt]

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:

protocol InteractorDelegate: class {
    func viewModelDidChange(_ old: ViewModel?, _ new: ViewModel)
}

class Interactor {

    weak var delegate: InteractorDelegate?

    var items = [
            [1,5,6,7,4,6,7,1,5],
            [1,5,2,1,0,6,7],
    ]

    var viewModel: ViewModel? {
        didSet {
            delegate?.viewModelDidChange(oldValue, viewModel!)
        }
    }

    var currentObjects: Int = 0 {
        didSet {
            viewModel = .init(with: .loaded(items[currentObjects]))
        }
    }

    init() {
        viewModel = .init(with: .initialized)
    }

    func fetchValue() {
        currentObjects = currentObjects == 0 ? 1 : 0
    }
}

struct ViewModel {

    enum ViewModelType: Equatable {
        case cell(CellViewModel)
    }

    enum State {
        case initialized
        case loaded([Int])
    }

    let state: State
    let viewModels: [ViewModelType]

    init(with state: State) {
        self.state = state
        switch state {
        case .initialized: viewModels = []
        case .loaded(let values):
            viewModels = CellViewModel.from(values).map(ViewModelType.cell)
        }
    }
}

extension ViewModel: Equatable {

    static func ==(left: ViewModel, right: ViewModel) -> Bool {
        return left.state == left.state
    }
}

extension ViewModel.State: Equatable {

    static func ==(left: ViewModel.State, right: ViewModel.State) -> Bool {
        switch (left, right) {
        case (.initialized, .initialized): return true
        case let (.loaded(l), .loaded(r)): return l == r
        default: return false
        }
    }
}

struct CellViewModel {
    let description: String
}

extension CellViewModel {

    static func from(_ values: [Int]) -> [CellViewModel] {
        return values.map { CellViewModel(description: String($0)) }
    }
}

extension CellViewModel: Equatable {

    static func ==(left: CellViewModel, right: CellViewModel) -> Bool {
        return left.description == right.description
    }
}

Here for the Controller part:

import UIKit
import Differ

class ViewController: UIViewController {

    ...

    override func viewDidLoad() {
        super.viewDidLoad()
        ...

        interactor.fetchValue()
    }

    @objc
    func onRefresh() {
        interactor.fetchValue()
    }
}

extension ViewController: UITableViewDataSource {
    func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return interactor.viewModel.value.viewModels.count
    }

    func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
        let cellViewModel = interactor.viewModel.value.viewModels[indexPath.row]
        switch cellViewModel {
        case .cell(let viewModel):
            let cell = tableView.dequeueReusableCell(withIdentifier: "cell", for: indexPath)
            cell.textLabel?.text = viewModel.description
            return cell
        }
    }
}

extension ViewController: InteractorDelegate {

    func viewModelDidChange(_ old: ViewModel?, _ new: ViewModel) {

        if let prev = old {
            print("Previous => State: \(prev.state) | ViewModelType.count: \(prev.viewModels.count)")
        } else {
            print("Previous => State: nil | ViewModelType.count: nil")
        }
        print("Current => State: \(new.state) | ViewModelType.count: \(new.viewModels.count)")
        DispatchQueue.main.async {
            self.tableView.animateRowChanges(oldData: old?.viewModels ?? [], newData: new.viewModels)
        }
    }
}

Here is what I got:

Previous => State: initialized | ViewModelType.count: 0
Current => State: loaded([1, 5, 2, 1, 0, 6, 7]) | ViewModelType.count: 7
2019-10-29 13:45:56.636678+0900 TestDiffer[93631:21379549] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Invalid update: invalid number of rows in section 0. The number of rows contained in an existing section after the update (7) must be equal to the number of rows contained in that section before the update (7), plus or minus the number of rows inserted or deleted from that section (7 inserted, 0 deleted) and plus or minus the number of rows moved into or out of that section (0 moved in, 0 moved out).'

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.

[florianldt]

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?

[jenox]

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.