onmyway133 / DeepDiff

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

Data source update should be inside performBatchUpdates #27

Closed dsanghan closed 5 years ago

dsanghan commented 6 years ago

Both UICollectionView+Extensions and UITableView+Extensions need to add closures for updating the data model after performBatchUpdates is called but before updates are applied (Example fix below):

public func reload<T: Hashable>(
    changes: [Change<T>],
    section: Int = 0,
    updateModel: (() -> Void)? = nil, // <===== This should fix #8 
    completion: ((Bool) -> Void)? = nil) {

    let changesWithIndexPath = IndexPathConverter().convert(changes: changes, section: section)

    // reloadRows needs to be called outside the batch

    performBatchUpdates({
      updateModel?()                  // <===== This should fix #8 
      internalBatchUpdates(changesWithIndexPath: changesWithIndexPath)
    }, completion: { finished in
      completion?(finished)
    })

    changesWithIndexPath.replaces.executeIfPresent {
      self.reloadItems(at: $0)
    }
  }

Otherwise, you'll see NSInternalInconsistencyExceptions since the model will already have the same number of items before and after update.

starlight173 commented 5 years ago

I also face this issue. A workaround is to check data source for number of objects before perform changes of diffing. Should you apply the performBatchUpdates for the latest API in iOS 11 and further @onmyway133 ?

MankiAnki commented 5 years ago

I have the same issue:

Assertion failure in -[UITableView _endCellAnimationsWithContext:], /BuildRoot/Library/Caches/com.apple.xbs/Sources/UIKitCore/UIKit-3698.94.10/UITableView.m:2062

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Invalid update: invalid number of rows in section 0. The number of rows contained in an existing section after the update (93) must be equal to the number of rows contained in that section before the update (93), plus or minus the number of rows inserted or deleted from that section (93 inserted, 0 deleted) and plus or minus the number of rows moved into or out of that section (0 moved in, 0 moved out).'

dsanghan commented 5 years ago

@StefanSourcico I've already included the solution in the code pasted above. In the updateModel closure, update your data model and it should work.

onmyway133 commented 5 years ago

@dsanghan Hi, thanks for raising the issue. I applied your suggestion here https://github.com/onmyway133/DeepDiff/releases/tag/1.4.0. Hope it helps

snoozemoose commented 5 years ago

From what I have experienced this is not enough. When doing deletions followed by adds I have had inconsistency crashes if I update the model inside performBatchUpdates before the deletion calls. The only that seems to work is to update the model in between the deletes and the adds. If someone can prove me wrong I would appreciate it greatly to learn what I have been doing wrong. Anyone care to comment on this?

onmyway133 commented 5 years ago

@snoozemoose does it happen consistently ?

snoozemoose commented 5 years ago

@onmyway133 Hmm, well I looked back into the code just now and tried to reproduce the errors but I wasn't able to. I then realised that the code I wrote back then wasn't top notch in regards to model update so I think that the new feature you've added is working as intended and I was incorrect :-). Sorry for any confusion I've caused and I'm now looking forward to trying out the latest version of DeepDiff!

onmyway133 commented 5 years ago

@snoozemoose no problem ❤️ glad you find DeepDiff helpful

dsanghan commented 5 years ago

Yup, should fix the issue. Closing.