Fix Incorrect Item/Cell FoundDuring WillDisplay and DidEndDisplaying …

…Callbacks (#146)

* Fix cell item callbacks during updates

* Fixed tests

CHANGELOG.md

@@ -10,7 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - ...
 
 ### Fixed
-- ...
+- Fixed an issue causing incorrect view callbacks and a corresponding assertion when a view 
+  disappears during a collection view update and is only in the post-update data.
 
 ## [0.10.0](https://github.com/airbnb/epoxy-ios/compare/0.9.0...0.10.0) - 2023-06-29
 

Sources/EpoxyCollectionView/CollectionView/CollectionView.swift

@@ -393,12 +393,17 @@ open class CollectionView: UICollectionView {
   public func reloadItem(at indexPath: IndexPath, animated: Bool) {
     guard
       let cell = cellForItem(at: indexPath as IndexPath) as? CollectionViewCell,
-      let item = epoxyDataSource.data?.item(at: indexPath)
+      let item = epoxyDataSource.data?.item(at: indexPath),
+      let section = epoxyDataSource.data?.section(at: indexPath.section)
     else {
       return
     }
 
-    configure(cell: cell, with: item, animated: animated)
+    configure(
+      cell: cell,
+      with: item,
+      at: .init(itemDataID: item.dataID, section: .dataID(section.dataID)),
+      animated: animated)
   }
 
   /// Invalidates the layout of this collection view's underlying `collectionViewLayout`.
@@ -419,7 +424,12 @@ open class CollectionView: UICollectionView {
       withReuseIdentifier: supplementaryViewReuseID)
   }
 
-  func configure(cell: CollectionViewCell, with item: AnyItemModel, animated: Bool) {
+  func configure(
+    cell: CollectionViewCell,
+    with item: AnyItemModel,
+    at itemPath: ItemPath,
+    animated: Bool)
+  {
     let cellSelectionStyle = item.selectionStyle ?? selectionStyle
     switch cellSelectionStyle {
     case .noBackground:
@@ -429,6 +439,7 @@ open class CollectionView: UICollectionView {
     }
 
     cell.accessibilityDelegate = self
+    cell.itemPath = itemPath
 
     let metadata = ItemCellMetadata(
       traitCollection: traitCollection,
@@ -661,6 +672,33 @@ open class CollectionView: UICollectionView {
     }
   }
 
+  /// Helper function which provides the correct data for a given cell at an index path taking into account the current update state of the
+  /// collection view.
+  ///
+  /// This is used in cases where the collection view might be mid-update and we need to find the underlying item for a cell and index
+  /// path, but there is no guarantee of whether the cell is from the pre-update data or post-update data (so we check both).
+  private func data(for cell: CollectionViewCell, at indexPath: IndexPath) -> CollectionViewData? {
+    guard let itemPath = cell.itemPath else {
+      EpoxyLogger.shared.assertionFailure("Cell is missing item path.")
+      return nil
+    }
+
+    switch updateState {
+    case .notUpdating, .preparingUpdate:
+      return epoxyDataSource.data
+    case .updating(from: let oldData):
+      if oldData.indexPathForItem(at: itemPath) == indexPath {
+        return oldData
+      } else if epoxyDataSource.data?.indexPathForItem(at: itemPath) == indexPath {
+        return epoxyDataSource.data
+      } else {
+        EpoxyLogger.shared.assertionFailure(
+          "Cell not found in either old or new data during an update.")
+        return nil
+      }
+    }
+  }
+
   private func itemForCell(_ cell: CollectionViewCell) -> AnyItemModel? {
     guard
       let indexPath = indexPath(for: cell),
@@ -783,20 +821,22 @@ extension CollectionView: UICollectionViewDelegate {
     willDisplay cell: UICollectionViewCell,
     forItemAt indexPath: IndexPath)
   {
-    guard
-      let item = epoxyDataSource.data?.item(at: indexPath),
-      let section = epoxyDataSource.data?.section(at: indexPath.section)
-    else {
+    guard let cell = cell as? CollectionViewCell else {
+      EpoxyLogger.shared.assertionFailure("Cell does not match expected type CollectionViewCell.")
       return
     }
 
-    handleSection(section, itemWillDisplay: .item(dataID: item.dataID))
+    let data = data(for: cell, at: indexPath)
 
-    guard let cell = cell as? CollectionViewCell else {
-      EpoxyLogger.shared.assertionFailure("Cell does not match expected type CollectionViewCell.")
+    guard
+      let item = data?.item(at: indexPath),
+      let section = data?.section(at: indexPath.section)
+    else {
       return
     }
 
+    handleSection(section, itemWillDisplay: .item(dataID: item.dataID))
+
     item.handleWillDisplay(
       cell,
       with: .init(traitCollection: traitCollection, state: cell.state, animated: false))
@@ -811,29 +851,22 @@ extension CollectionView: UICollectionViewDelegate {
     didEndDisplaying cell: UICollectionViewCell,
     forItemAt indexPath: IndexPath)
   {
-    // When updating, items ending display correspond to items in the old data.
-    let data: CollectionViewData?
-    switch updateState {
-    case .notUpdating, .preparingUpdate:
-      data = epoxyDataSource.data
-    case .updating(from: let oldData):
-      data = oldData
+    guard let cell = cell as? CollectionViewCell else {
+      EpoxyLogger.shared.assertionFailure("Cell does not match expected type CollectionViewCell.")
+      return
     }
 
+    let data = data(for: cell, at: indexPath)
+
     guard
-      let item = data?.itemIfPresent(at: indexPath),
-      let section = data?.sectionIfPresent(at: indexPath.section)
+      let item = data?.item(at: indexPath),
+      let section = data?.section(at: indexPath.section)
     else {
       return
     }
 
     handleSection(section, itemDidEndDisplaying: .item(dataID: item.dataID))
 
-    guard let cell = cell as? CollectionViewCell else {
-      EpoxyLogger.shared.assertionFailure("Cell does not match expected type CollectionViewCell.")
-      return
-    }
-
     item.handleDidEndDisplaying(
       cell,
       with: .init(traitCollection: traitCollection, state: cell.state, animated: false))

Sources/EpoxyCollectionView/CollectionView/Internal/CollectionViewData.swift

@@ -82,16 +82,6 @@ struct CollectionViewData {
     return section.items[indexPath.row].eraseToAnyItemModel()
   }
 
-  /// Returns the erased item model at the given index path, otherwise `nil` if it does not exist.
-  func itemIfPresent(at indexPath: IndexPath) -> AnyItemModel? {
-    guard indexPath.section < sections.count else { return nil }
-
-    let section = sections[indexPath.section]
-    guard indexPath.row < section.items.count else { return nil }
-
-    return section.items[indexPath.row].eraseToAnyItemModel()
-  }
-
   /// Returns the section model at the given index, asserting if it does not exist.
   func section(at index: Int) -> SectionModel? {
     guard index < sections.count else {

Sources/EpoxyCollectionView/CollectionView/Internal/CollectionViewDataSource.swift

@@ -174,6 +174,7 @@ extension CollectionViewDataSource: UICollectionViewDataSource {
   {
     guard
       let item = data?.item(at: indexPath),
+      let section = data?.section(at: indexPath.section),
       let reuseID = reuseIDStore.registeredReuseID(for: item.viewDifferentiator)
     else {
       // The `item(…)` or `registeredReuseID(…)` methods will assert in this scenario.
@@ -183,7 +184,11 @@ extension CollectionViewDataSource: UICollectionViewDataSource {
     let cell = collectionView.dequeueReusableCell(withReuseIdentifier: reuseID, for: indexPath)
 
     if let cell = cell as? CollectionViewCell {
-      self.collectionView?.configure(cell: cell, with: item, animated: false)
+      self.collectionView?.configure(
+        cell: cell,
+        with: item,
+        at: .init(itemDataID: item.dataID, section: .dataID(section.dataID)),
+        animated: false)
     } else {
       EpoxyLogger.shared.assertionFailure(
         "Only CollectionViewCell and subclasses are allowed in a CollectionView.")

Sources/EpoxyCollectionView/CollectionView/ReusableViews/CollectionViewCell.swift

@@ -132,6 +132,11 @@ public final class CollectionViewCell: UICollectionViewCell, ItemCellView {
   weak var accessibilityDelegate: CollectionViewCellAccessibilityDelegate?
   var ephemeralViewCachedStateProvider: ((Any?) -> Void)?
 
+  /// The item path of the cell from its last configuration update. Used to associate the view with the underlying data. When collection
+  /// view provides view display callbacks, if it is mid update, we need this to see if the view came from pre-update data or
+  /// post-update data.
+  var itemPath: ItemPath?
+
   // MARK: Private
 
   private var normalViewBackgroundColor: UIColor?

Tests/EpoxyTests/CollectionViewTests/CollectionViewSpec.swift

@@ -1,12 +1,13 @@
 // Created by eric_horacek on 1/8/21.
 // Copyright © 2021 Airbnb Inc. All rights reserved.
 
-import EpoxyCollectionView
 import EpoxyCore
 import Nimble
 import Quick
 import UIKit
 
+@testable import EpoxyCollectionView
+
 // swiftlint:disable implicitly_unwrapped_optional
 
 // MARK: - CollectionViewSpec
@@ -24,6 +25,14 @@ final class CollectionViewSpec: QuickSpec {
     }
   }
 
+  var mockCell: CollectionViewCell {
+    let cell = CollectionViewCell(frame: .zero)
+    cell.itemPath = .init(
+      itemDataID: DefaultDataID.noneProvided,
+      section: .dataID(DefaultDataID.noneProvided))
+    return cell
+  }
+
   override func spec() {
     let itemModel = ItemModel<TestView>(dataID: DefaultDataID.noneProvided)
     let supplementaryItemModel = SupplementaryItemModel<TestView>(dataID: DefaultDataID.noneProvided)
@@ -85,7 +94,7 @@ final class CollectionViewSpec: QuickSpec {
           beforeEach {
             collectionView.delegate?.collectionView?(
               collectionView,
-              willDisplay: CollectionViewCell(),
+              willDisplay: self.mockCell,
               forItemAt: IndexPath(item: 0, section: 0))
           }
 
@@ -99,7 +108,7 @@ final class CollectionViewSpec: QuickSpec {
           beforeEach {
             collectionView.delegate?.collectionView?(
               collectionView,
-              didEndDisplaying: CollectionViewCell(),
+              didEndDisplaying: self.mockCell,
               forItemAt: IndexPath(item: 0, section: 0))
           }
 
@@ -220,7 +229,7 @@ final class CollectionViewSpec: QuickSpec {
             beforeEach {
               collectionView.delegate?.collectionView?(
                 collectionView,
-                willDisplay: CollectionViewCell(),
+                willDisplay: self.mockCell,
                 forItemAt: IndexPath(item: 0, section: 0))
             }
 
@@ -236,7 +245,7 @@ final class CollectionViewSpec: QuickSpec {
               beforeEach {
                 collectionView.delegate?.collectionView?(
                   collectionView,
-                  didEndDisplaying: CollectionViewCell(),
+                  didEndDisplaying: self.mockCell,
                   forItemAt: IndexPath(item: 0, section: 0))
               }
 
@@ -248,7 +257,7 @@ final class CollectionViewSpec: QuickSpec {
                 beforeEach {
                   collectionView.delegate?.collectionView?(
                     collectionView,
-                    willDisplay: CollectionViewCell(),
+                    willDisplay: self.mockCell,
                     forItemAt: IndexPath(item: 0, section: 0))
                 }
 
@@ -279,7 +288,7 @@ final class CollectionViewSpec: QuickSpec {
                 beforeEach {
                   collectionView.delegate?.collectionView?(
                     collectionView,
-                    didEndDisplaying: CollectionViewCell(),
+                    didEndDisplaying: self.mockCell,
                     forItemAt: IndexPath(item: 0, section: 0))
                   collectionView.delegate?.collectionView?(
                     collectionView,