Item reloads are not calculated correctly

[simba909]

Hello!

First and foremost; これを作ってくれて、ありがとうございます！助かりました 🙇🏻‍♂️

I have noticed a minor issue, which I have described below using the issue template. I've also forked this repo and applied a fix, which you can find here: https://github.com/simba909/DiffableDataSources

I would be happy to provide a pull request 👍🏻

Checklist

• [x] This is not a Apple's bug.
• [x] Reviewed the README and documents.
• [x] Searched existing issues for ensure not duplicated.

Expected Behavior

When using this library and applying two snapshots after one another, I expect the two snapshots to be correctly differentiated like in Apple's NSDiffableDataSource so that item updates are correctly reflected between the two snapshots. Below is some sample code that demonstrates the use-case:

struct MyItem: Hashable, Equatable {
    let id: Int
    var name: String

    func hash(into hasher: inout Hasher) {
        hasher.combine(id)
    }
}

let snapshot = DiffableDataSourceSnapshot<Int, MyItem>()
snapshot.appendSections([0])

let myItem = MyItem(id: 1, name: "Test")
snapshot.appendItems([myItem])

dataSource.apply(snapshot) // <-- This works great, and as expected

// Now, let's apply another snapshot that includes an update to the item
let anotherSnapshot = DiffableDataSourceSnapshot<Int, MyItem>()
anotherSnapshot.appendSections([0])

let updatedItem = MyItem(id: 1, name: "Test 2") // <-- Notice that the id is the same, but the content is different
anotherSnapshot.appendItems([myItem])

dataSource.apply(anotherSnapshot) // <-- This results in 1 deletion, 1 insertion when it should produce 1 reload

Current Behavior

Item reloads are not discovered in complex items, because an equality check is used on the SnapshotStructure.Item differenceIdentifier instead of the hash value.

Steps to Reproduce

1. Create an Xcode project using this library
2. Take the small sample pasted above and put it in the project
3. Run the project, and notice how items are deleted/inserted instead of reloaded

[ra1028]

Sorry for my late reply @simba909 .

Thanks for your reporting! I run your sample project and found it works beautifully under almost conditions. However, more closer investigation revealed that the official API also performed insert + delete instead of reload. The following custom table view can found the diff update performed by the official diffable data source.

class TableView: UITableView {
    override func reloadRows(at indexPaths: [IndexPath], with animation: RowAnimation) {
        super.reloadRows(at: indexPaths, with: animation)
        print("reloaded")
    }

    override func insertRows(at indexPaths: [IndexPath], with animation: RowAnimation) {
        super.insertRows(at: indexPaths, with: animation)
        print("inserted")
    }

    override func deleteRows(at indexPaths: [IndexPath], with animation: RowAnimation) {
        super.deleteRows(at: indexPaths, with: animation)
        print("deleted")
    }
}

My backport seems to have correct diff, although the glitches animation under certain conditions. The following workarounds are possible for fix glitching animations.

struct MyItem: Hashable, Equatable {
    let id: Int
    var name: String

    func hash(into hasher: inout Hasher) {
        hasher.combine(id)
    }

+    static func == (lhs: MyItem, rhs: MyItem) -> Bool {
+        return lhs.id == rhs.id
+    }
}

let snapshot = DiffableDataSourceSnapshot<Int, MyItem>()
snapshot.appendSections([0])

let myItem = MyItem(id: 1, name: "Test 2")
snapshot.appendItems([myItem])
+ snapshot.reloadItems([myItem])

The official sample code also doesn't seem to compare anything other than id with the == operator. I hope this will help you.

[simba909]

Hi @ra1028 , and thank you for your response! Now it's my turn to excuse my late answer 😅

You are right, I tested the code you provided and the official solution indeed seem to not actually perform any reloads (just as you said). I also noticed that the animations are different, and it seems that the official solution will not use the UITableView.RowAnimation.automatic for inserts + deletes but rather .left and .right respectively (even though .automatic is the default) 🤔

With that said, doing the equality check only on ids seem wrong to me. This might have changed since your reply but when I tested the above code the official sample code does use the equality check, just without a reload. So I think a better solution might be to adjust the animations that this library uses by default. I've implemented the following solution in TableViewDiffableDataSource, which seems to do the trick:

public func apply(_ snapshot: DiffableDataSourceSnapshot<SectionIdentifierType, ItemIdentifierType>, animatingDifferences: Bool = true) {
    let rowInsertAnimation: UITableView.RowAnimation
    let rowDeleteAnimation: UITableView.RowAnimation

    switch defaultRowAnimation {
    case .automatic:
        rowInsertAnimation = .left
        rowDeleteAnimation = .right
    default:
        rowInsertAnimation = defaultRowAnimation
        rowDeleteAnimation = defaultRowAnimation
    }

    core.apply(
        snapshot,
        view: tableView,
        animatingDifferences: animatingDifferences,
        performUpdates: { tableView, changeset, setSections in
            tableView.reload(
                using: changeset,
                deleteSectionsAnimation: .automatic,
                insertSectionsAnimation: .automatic,
                reloadSectionsAnimation: .automatic,
                deleteRowsAnimation: rowDeleteAnimation,
                insertRowsAnimation: rowInsertAnimation,
                reloadRowsAnimation: self.defaultRowAnimation,
                setData: setSections)
    })
}

This still does not look 100% like the official solution, but close enough IMHO. Also this doesn't require any changes to any implementations of this library 🙂 If you are OK with it, I would be more than happy to provide a PR 👍

EDIT: Just realised that we might want to add some if #available checks for iOS 13 around the above code so that it still looks like it should for iOS 12 and earlier.

[ra1028]

@simba909 Thank you for explaining your thoughts clearly.

With that said, doing the equality check only on ids seem wrong to me.

True. Your opinions are overall make sense. Anyway, the handling id in original UITableViewDiffableDataSource isn't fair, IMHO. It's also true that sometimes animation can glitches in this library. However, I intend to match the official API as match as possible for now.

[simba909]

That's fair. Thank you for considering it! 🙂