-
Notifications
You must be signed in to change notification settings - Fork 69
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
base: master
Are you sure you want to change the base?
Conversation
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 but struct MyText: Hashable, Equatable {
var id: Int
var text: String
func hash(into hasher: inout Hasher) {
hasher.combine(id)
}
}
|
@ollitapa , that is correct. |
var isReloaded: Bool | ||
|
||
init(id: ItemID, isReloaded: Bool) { | ||
self.differenceIdentifier = id | ||
self.identifier = id | ||
self.differenceIdentifier = id.hashValue |
There was a problem hiding this comment.
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
self.differenceIdentifier = id.hashValue | |
var hasher = Hasher() | |
hasher.combine(id) | |
self.differenceIdentifier = hasher.finalize() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
@ra1028 , awaiting your approval to merge :) |
@ra1028 ping. Would be nice to get this merged 😄 |
Thanks @winstondu and reviewers. Sorry for my late reply. I think it's difficult to judge this specification change. |
Checklist
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 theItem
(as determined by the implemention ofItem
.)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: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:
After: