onmyway133 / DeepDiff

🦀Amazingly incredible extraordinary lightning fast diffing in Swift
https://onmyway133.com/apps/
Other
2.05k stars 145 forks source link

Heckel algorithm does not want to do replacements. #23

Closed archagon closed 5 years ago

archagon commented 5 years ago

I'm having trouble getting the default algorithm to do replacements instead of deletes followed by inserts. Even this basic code...

let old = [
    "New York",
    "Imagine City",
    "London"
]

let new = [
    "New York",
    "Oslo",
    "London",
    ]

let changes = diff(old: old, new: new)

...gives me a delete followed by an insert. Whereas using let changes = diff(old: old, new: new, algorithm: WagnerFischer()) makes everything work correctly.

Is this an intentional attribute of the Heckel algorithm?

onmyway133 commented 5 years ago

@archagon Hi, it is because currently I use a hashValue as a key to manage uniqueness of objects, so a replacement occurs only if the key remains but the content changes, see https://github.com/onmyway133/DeepDiff/issues/16

It works a bit differently from Wagner Fischer. I will see if I have time to make this more intuitive

ValCanBuild commented 5 years ago

@onmyway133 isn't the way you're using hashvalue technically incorrect? You're using it as a unique identifier for an object and the two aren't necessarily the same thing.

A hash of a struct should be computed based on all its properties, not just one. This makes it hard to handle replacement operations. I like what the android DiffUtil library does in this case where it gives the library user the callback to determine whether two items are the same and if they are, then it compares their contents to figure out if they are the same.

This leads to a very straightforward implementation:

  1. Have a unique id for each object (up to developer to choose)
  2. Use that id to determine if two items are the same in the context of DeepDiff
  3. If they aren't, then this is either an insertion or deletion. Stop here
  4. If they are, then check if their contents are the same (this is where just using Equatable would work)
  5. If they are, then nothing has changed
  6. If they aren't then a change has occured.

Any chance something like this could be implemented?

onmyway133 commented 5 years ago

@ValCanBuild Hi, I was thinking so too. I 'd like to expose comparison callback instead of implicitly relying on Hashable.

onmyway133 commented 5 years ago

@archagon Hi, I introduced DiffAware protocol to make diff changes more explicit https://github.com/onmyway133/DeepDiff/releases/tag/2.0.0. Can you check if that works for you ?

ValCanBuild commented 5 years ago

@onmyway133 shouldn't DiffAware be constrained to a Hashable?

snoozemoose commented 5 years ago

@ValCanBuild I can't see any reason for that, would you care to enlighten me?

ValCanBuild commented 5 years ago

@snoozemoose well in order to use the reload extension on TableViews/CollectionViews the item must be Hashable. So making your data only conform to DiffAware makes it usable in the diff part of the algorithm but not in the applying changes part.

So either DiffAware needs to be made Hashable or reload function Hashable constraint must ve removed.

snoozemoose commented 5 years ago

@ValCanBuild I haven't yet submitted a pull request for this but I've changed the UICollectionView extension to be reload<T: DiffAware> and it is working fine. I guess I should submit that PR along with the same change for UITableView...

snoozemoose commented 5 years ago

PR is submitted now: #31

onmyway133 commented 5 years ago

❤️