Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add update and move support #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

winstondu
Copy link

@winstondu winstondu commented Feb 21, 2020

Checklist

  • All tests are passed.
  • Added tests (N/A)
  • Documented the code using Xcode markup.
  • Searched existing pull requests for ensure not duplicated.

Description

The current code was not correctly leveraging DifferenceKit to address moves and updates. In particular, it was not distinguishing changes in content versus changes in the identifier.

This code change ensures that the difference identifier is no longer of the whole Item, but instead just the hashValue of the Item (as determined by the implemention of Item.)

For a visualization of the difference, please see the below gifs (the example iOS code in this repo was slightly modified to illustrate this).

For a clear conceptual example of the correction that this pull request sets out to accomplish, consider if Item was the following:

struct person: Hashable {
    var birthName : String  // Assume this is our unique identifier for a person.
    var age : Int
    func hash(into hasher: inout Hasher) {
            hasher.combine(birthName)
   }
}

And we had to compare the datasource diff of the following lists (being used as datasource updates for a collectionview):
Source: [(birthName: "Andy", age: 15), (birthName: "Kevin", age: 16)]
Target: [(birthName: "Kevin", age: 16)]

Without the change in this pull request, we get 2 deletions and 1 insertion (animated as such)

With this change in this PR , we get 1 deletion and 1 update (correctly animated).

Related Issue

N/A

Motivation and Context

The motivation was for a better, and more correct, diff animation.

Impact on Existing Code

None beyond what is described.

Screenshots

Notice the "move" animations that occur in after that do not happen in before

Before:
Before

After:
after

@winstondu
Copy link
Author

@ra1028

@ollitapa
Copy link

Seems really welcome fix in my opinion! 👍

Just for clarification, do I understand correctly:

I have item with automatic hash and equality:

struct MyText: Hashable, Equatable {
    var id: Int
    var text: String
}

Change [ {id: 1, text: "a"} ] -> [ {id: 1, text: "b"} ] will result in one deletion and one insert.

but

struct MyText: Hashable, Equatable {
    var id: Int
    var text: String

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

[ {id: 1, text: "a"} ] -> [ {id: 1, text: "b"} ] will result in one update.

@winstondu
Copy link
Author

@ollitapa , that is correct.

var isReloaded: Bool

init(id: ItemID, isReloaded: Bool) {
self.differenceIdentifier = id
self.identifier = id
self.differenceIdentifier = id.hashValue

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hash value is deprecated, so maybe do

Suggested change
self.differenceIdentifier = id.hashValue
var hasher = Hasher()
hasher.combine(id)
self.differenceIdentifier = hasher.finalize()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the docs state that "hashValue is deprecated as a Hashable requirement." However, the variable itself is not deprecated, and is intended as a way to get the exact result produced by the three lines of code you provided.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, was not sure that it was meant like that 😅

@winstondu
Copy link
Author

@ra1028 , awaiting your approval to merge :)

@ollitapa
Copy link

@ra1028 ping. Would be nice to get this merged 😄

@ra1028
Copy link
Owner

ra1028 commented Mar 31, 2020

Thanks @winstondu and reviewers.

Sorry for my late reply.

I think it's difficult to judge this specification change.
I agree with its behavior, but it works differently than the original version UICollectionViewDiffableDataSource.
Because, DiffableDataSources is only a backport, differences between the original version and the specification can adversely affect future replacements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants