Fix NSTableView animations

[hannesoid]

NSTableView

• Now based on patches, because NSTableView needs moves to be executed in order
• Changed Interface to use rowIndices, because there are not IndexPaths involved in the row animations of it
• I deprecated the previous methods for compatibility

UITableView

• prevent beginUpdates/endUpdates for empty diff removed after discussion

[jenox]

I don't think we should drop the calls to beginUpdates() and endUpdates() if the diff is empty. beginUpdates() and endUpdates() also give the visible cells a chance to resize with animation, which users of this library might rely on. Keep in mind that an empty diff does not imply equal or substitutable data: Diffing is generally done based on record identity (Identifiable), and not based on substitutability (Equatable).

For example, an item in your shopping cart might change its price. Differ would leave this cell as is when diffing based on the items' IDs, but the cell still needs to redraw and potentially requires more space to do so.

[hannesoid]

Thanks for the feedback, good point. For my use case I wanted it to exactly prevent the behaviour because I handled those needed resizes/redraws more specifically. But I understand now that people may rely on it and expect it.

Would it be alright to add a parameter defaulting to the always update behaviour like: updateForEmptyDiff: Bool = true or preventAnimationForEmptyDiff: Bool = false, or would you say in this case the callsite should be responsible for it (first creating diff then checking if empty and then calling or not?)?

[jenox]

Intuitively, I'd want to avoid a preventAnimationForEmptyDiff parameter. However, moving this responsibility to the call site isn't nice either, as we'd end up diffing twice.

Could you elaborate on what you are currently doing to handle the resizes for specifically?

[hannesoid]

I have a UITableView and NSTableView with "self-sizing" cells that involve text editing which loops through a model layer. In order to prevent loss of firstResponder, other potential glitches and for keeping the updates to the minimum necessary, I do the following when a new list of items "arrives": For items which have been in the old list as well and have changed (based on full equality check), and have a visible cell, I update their contentView directly and take note of the potential changed height, on iOS by wrapping in begin/endUpdates and on macOS by calling noteHeightOfRows(withIndexesChanged:).
After that I do an animated update using Differ with equality check being based on identifiers.

[jenox]

Personally I don't see a reason to use separate beginUpdates()/endUpdates() pairs for the record identity and equality checks. But then again I haven't worked much with firstResponders in dynamic UITableViews.

Here's how I usually update my table views. Maybe you can give that a try? In your case with dynamic row heights, you'd want to run the block to update the cell's models before calling out to Differ, such that they are configured correctly by the time endUpdates() is called and can compute the correct row height. Note that calling out to Differ from a didSet as in the linked project is generally a bad idea (#62), so you'd probably want something like this instead:

func transition(to newItems: [Item]) {
    // With the cells not being able to listen for changes — they simply
    // get a snapshot of an item at one point time — we need to make
    // sure that cells which haven't been inserted or deleted are up to
    // date.
    for indexPath in self.tableView.indexPathsForVisibleRows ?? [] {
        if let cell = self.tableView.cellForRow(at: indexPath) as? YourCellType {
            let newItem = newItems[indexPath.row]

            if self.items.containsDifferentSnapshot(of: newItem) {
                cell.item = newItem
            }
        }
    }

    // We diff based on _record identities_ here
    self.tableView.animateRowChanges(
        oldData: self.items,
        newData: newItems,
        isEqual: { $0.id == $1.id },
        updateData: { self.items = newItems } // See #62, coming soon™
    )
}

extension Collection where Element: Equatable & Identifiable {
    public func containsDifferentSnapshot(of element: Element) -> Bool {
        return contains(where: { $0.id == element.id && $0 != element })
    }
}

Please let me know if this works for you.

[hannesoid]

I do a similar approach in my project, but can't go into more detail now.
But as for this PR, I think I'll just leave out that new option parameter and use the current behavior, the scope then being just to fix NSTableView updates.

[hannesoid]

@tonyarnold fyi this is a the new PR after discarding #69

[hannesoid]

@jenox please have a look at the new code. By the way, I - and probably users who's compilation won't break :) - appreciate you taking the time to look at this in such detail!

[jenox]

@hannesoid Looks good in my opinion now!

I can't merge things here though, @tonyarnold will need to do that.

[tonyarnold]

I'm going to merge this into master now, but feel free to open new PRs to adjust anything that needs attention. I'll be away from my office for a bit this week, but I'd be keen to see work progress on #71 before cutting a new 2.0 release (just so we don't have to bump to v 3.0 due to further API breakage on the iOS side).

[hannesoid]

thanks @tonyarnold!