Skip to content

Commit

Permalink
Added fix for supplementary view calbacks during updates
Browse files Browse the repository at this point in the history
  • Loading branch information
@brynbodayle
brynbodayle committed Aug 15, 2023
1 parent 4be15df commit 21ea71c
Show file tree
Hide file tree
Showing 8 changed files with 223 additions and 131 deletions.
79 changes: 52 additions & 27 deletions Sources/EpoxyCollectionView/CollectionView/CollectionView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,8 +460,10 @@ open class CollectionView: UICollectionView {
func configure(
supplementaryView: CollectionViewReusableView,
with model: AnySupplementaryItemModel,
at itemPath: SupplementaryItemPath,
animated: Bool)
{
supplementaryView.itemPath = itemPath
model.configure(
reusableView: supplementaryView,
traitCollection: traitCollection,
Expand Down Expand Up @@ -699,6 +701,38 @@ open class CollectionView: UICollectionView {
}
}

/// Helper function which provides the correct data for a given reusable view 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 supplementary item for a
/// view and index path, but there is no guarantee of whether the view is from the pre-update data or post-update
/// data (so we check both).
private func data(
for view: CollectionViewReusableView,
at indexPath: IndexPath)
-> CollectionViewData?
{
guard let itemPath = view.itemPath else {
EpoxyLogger.shared.assertionFailure("View is missing item path.")
return nil
}

switch updateState {
case .notUpdating, .preparingUpdate:
return epoxyDataSource.data
case .updating(from: let oldData):
if oldData.indexPathForSupplementaryItem(at: itemPath) == indexPath {
return oldData
} else if epoxyDataSource.data?.indexPathForSupplementaryItem(at: itemPath) == indexPath {
return epoxyDataSource.data
} else {
EpoxyLogger.shared.assertionFailure(
"View 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),
Expand Down Expand Up @@ -882,11 +916,17 @@ extension CollectionView: UICollectionViewDelegate {
forElementKind elementKind: String,
at indexPath: IndexPath)
{
// We don't assert since `UICollectionViewCompositionalLayout` can create and configure its own
// supplementary views e.g. with a `.list(using: .init(appearance: .plain))` config.
guard let view = view as? CollectionViewReusableView else {
// We don't assert since `UICollectionViewCompositionalLayout` can create and configure its
// own supplementary views e.g. with a `.list(using: .init(appearance: .plain))` config.
return
}

let data = data(for: view, at: indexPath)

guard
let section = epoxyDataSource.data?.sectionIfPresent(at: indexPath.section),
let item = epoxyDataSource.data?.supplementaryItemIfPresent(ofKind: elementKind, at: indexPath)
let section = data?.section(at: indexPath.section),
let item = data?.supplementaryItem(ofKind: elementKind, at: indexPath)
else {
return
}
Expand All @@ -895,12 +935,6 @@ extension CollectionView: UICollectionViewDelegate {
section,
itemWillDisplay: .supplementaryItem(elementKind: elementKind, dataID: item.dataID))

guard let view = view as? CollectionViewReusableView else {
EpoxyLogger.shared.assertionFailure(
"Supplementary view does not match expected type CollectionViewReusableView.")
return
}

item.handleWillDisplay(view, traitCollection: traitCollection, animated: false)

(view.view as? DisplayRespondingView)?.didDisplay(true)
Expand All @@ -919,20 +953,17 @@ extension CollectionView: UICollectionViewDelegate {
forElementOfKind elementKind: String,
at 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 view = view as? CollectionViewReusableView else {
// We don't assert since `UICollectionViewCompositionalLayout` can create and configure its
// own supplementary views e.g. with a `.list(using: .init(appearance: .plain))` config.
return
}

// We don't assert since `UICollectionViewCompositionalLayout` can create and configure its own
// supplementary views e.g. with a `.list(using: .init(appearance: .plain))` config.
let data = data(for: view, at: indexPath)

guard
let section = data?.sectionIfPresent(at: indexPath.section),
let item = data?.supplementaryItemIfPresent(ofKind: elementKind, at: indexPath)
let section = data?.section(at: indexPath.section),
let item = data?.supplementaryItem(ofKind: elementKind, at: indexPath)
else {
return
}
Expand All @@ -941,12 +972,6 @@ extension CollectionView: UICollectionViewDelegate {
section,
itemDidEndDisplaying: .supplementaryItem(elementKind: elementKind, dataID: item.dataID))

guard let view = view as? CollectionViewReusableView else {
EpoxyLogger.shared.assertionFailure(
"Supplementary view does not match expected type CollectionViewReusableView.")
return
}

item.handleDidEndDisplaying(view, traitCollection: traitCollection, animated: false)

(view.view as? DisplayRespondingView)?.didDisplay(false)
Expand Down
189 changes: 103 additions & 86 deletions Sources/EpoxyCollectionView/CollectionView/Internal/CollectionViewData.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@ struct CollectionViewData {
private init(
sections: [SectionModel],
sectionIndexMap: SectionIndexMap,
itemIndexMap: ItemIndexMap)
itemIndexMap: ItemIndexMap,
supplementaryItemIndexMap: SupplementaryItemIndexMap)
{
self.sections = sections
self.sectionIndexMap = sectionIndexMap
self.itemIndexMap = itemIndexMap
self.supplementaryItemIndexMap = supplementaryItemIndexMap
}

// MARK: Internal
Expand All @@ -28,6 +30,7 @@ struct CollectionViewData {
static func make(sections: [SectionModel]) -> Self {
var sectionIndexMap = SectionIndexMap()
var itemIndexMap = ItemIndexMap()
var supplementaryItemIndexMap = SupplementaryItemIndexMap()

for sectionIndex in sections.indices {
let section = sections[sectionIndex]
Expand All @@ -40,9 +43,25 @@ struct CollectionViewData {
let indexPath = IndexPath(item: itemIndex, section: sectionIndex)
itemIndexMap[item.dataID, default: [:]][sectionDataID, default: []].append(indexPath)
}

let sectionSupplementaryItems = section.supplementaryItems
for (elementKind, supplementaryItems) in sectionSupplementaryItems {
var itemIndexMap = ItemIndexMap()
for itemIndex in supplementaryItems.indices {
let item = supplementaryItems[itemIndex]
let indexPath = IndexPath(item: itemIndex, section: sectionIndex)
itemIndexMap[item.internalItemModel.dataID, default: [:]][sectionDataID, default: []]
.append(indexPath)
}
supplementaryItemIndexMap[elementKind] = itemIndexMap
}
}

return .init(sections: sections, sectionIndexMap: sectionIndexMap, itemIndexMap: itemIndexMap)
return .init(
sections: sections,
sectionIndexMap: sectionIndexMap,
itemIndexMap: itemIndexMap,
supplementaryItemIndexMap: supplementaryItemIndexMap)
}

func makeChangeset(from otherData: Self) -> CollectionViewChangeset {
Expand Down Expand Up @@ -96,30 +115,6 @@ struct CollectionViewData {
return sections[index]
}

/// Returns the section model at the given index, otherwise `nil` if it does not exist.
func sectionIfPresent(at index: Int) -> SectionModel? {
guard index < sections.count else { return nil }

return sections[index]
}

/// Returns the supplementary item model of the provided kind at the given index, otherwise `nil`
/// if it does not exist.
func supplementaryItemIfPresent(
ofKind elementKind: String,
at indexPath: IndexPath)
-> AnySupplementaryItemModel?
{
guard indexPath.section < sections.count else { return nil }

let section = sections[indexPath.section]

guard let models = section.supplementaryItems[elementKind] else { return nil }
guard indexPath.item < models.count else { return nil }

return models[indexPath.item].eraseToAnySupplementaryItemModel()
}

/// Returns the supplementary item model of the provided kind at the given index, asserting if it
/// does not exist.
func supplementaryItem(
Expand All @@ -146,70 +141,20 @@ struct CollectionViewData {
return model.eraseToAnySupplementaryItemModel()
}

/// Returns the `IndexPath` corresponding to the given `ItemPath`, logging a warning if the
/// `ItemPath` corresponds to multiple items due to duplicate data IDs.
func indexPathForItem(at path: ItemPath) -> IndexPath? {
guard let itemIndexMapBySectionID = itemIndexMap[path.itemDataID] else {
/// Returns the `IndexPath` corresponding to the given `SupplementaryItemPath`, logging a warning if the
/// `SupplementaryItemPath` corresponds to multiple supplementary items due to duplicate data IDs.
func indexPathForSupplementaryItem(at path: SupplementaryItemPath) -> IndexPath? {
guard let itemIndexMap = supplementaryItemIndexMap[path.elementKind] else {
return nil
}

func lastIndexPath(in indexPaths: [IndexPath], sectionID: AnyHashable) -> IndexPath? {
if indexPaths.count > 1 {
EpoxyLogger.shared.warn({
// `sectionIndexMap` is constructed from the same data as `itemIndexMap` so we can force
// unwrap.
// swiftlint:disable:next force_unwrapping
let sectionIndex = sectionIndexMap[sectionID]!

return """
Warning! Attempted to locate item \(path.itemDataID) in section \(sectionID) at indexes \
\(sectionIndex.map { $0 }) when there are multiple items with that ID at the indexes \
\(indexPaths.map { $0.item }). Choosing the last index. Items should have unique data \
IDs within a section as duplicate data IDs cause undefined behavior.
"""
}())
}

return indexPaths.last
}

switch path.section {
case .dataID(let sectionID):
if let indexPaths = itemIndexMapBySectionID[sectionID] {
// If the section ID is specified, just look up the indexes for that section.
return lastIndexPath(in: indexPaths, sectionID: sectionID)
}
return nil

case .lastWithItemDataID:
// If the section ID is unspecified but there's only one section with this data ID:
if itemIndexMapBySectionID.count == 1, let idAndIndexes = itemIndexMapBySectionID.first {
return lastIndexPath(in: idAndIndexes.value, sectionID: idAndIndexes.key)
}

// Otherwise there's multiple sections with the same data ID so we pick the last section so
// that it's stable.
let lastSectionID = itemIndexMapBySectionID.max(by: { first, second in
// `sectionIndexMap` is constructed from the same data as `itemIndexMap` so we can safely
// force unwrap.
// swiftlint:disable:next force_unwrapping
sectionIndexMap[first.key]!.last! < sectionIndexMap[second.key]!.last!
})

if let sectionID = lastSectionID {
EpoxyLogger.shared.warn("""
Warning! Attempted to locate item \(path.itemDataID) when there are multiple sections that \
contain it each with IDs \(itemIndexMapBySectionID.keys) at the indexes \
\(itemIndexMapBySectionID.keys.map { sectionIndexMap[$0] }). Choosing the last section \
\(sectionID.key). To fix this warning specify the desired section data ID when \
constructing your `ItemPath`.
""")

return lastIndexPath(in: sectionID.value, sectionID: sectionID.key)
}
return indexPath(from: itemIndexMap, for: path.itemDataID, in: path.section)
}

return nil
}
/// Returns the `IndexPath` corresponding to the given `ItemPath`, logging a warning if the
/// `ItemPath` corresponds to multiple items due to duplicate data IDs.
func indexPathForItem(at path: ItemPath) -> IndexPath? {
indexPath(from: itemIndexMap, for: path.itemDataID, in: path.section)
}

/// Returns the `Int` index corresponding to the given section `dataID`, logging a warning if the
Expand Down Expand Up @@ -245,8 +190,12 @@ struct CollectionViewData {
/// `IndexPath`s with both the item and section `dataID`.
private typealias ItemIndexMap = [AnyHashable: [AnyHashable: [IndexPath]]]

/// The item index map for supplementary views keyed by their element kind.
private typealias SupplementaryItemIndexMap = [String: ItemIndexMap]

private let sectionIndexMap: SectionIndexMap
private let itemIndexMap: ItemIndexMap
private let supplementaryItemIndexMap: SupplementaryItemIndexMap

private func supplementaryItemChangeset(
from otherData: Self,
Expand Down Expand Up @@ -349,4 +298,72 @@ struct CollectionViewData {
}())
}

private func indexPath(
from itemIndexMapBySectionID: ItemIndexMap,
for itemDataID: AnyHashable,
in section: ItemSectionPath)
-> IndexPath?
{
guard let itemIndexMapBySectionID = itemIndexMapBySectionID[itemDataID] else {
return nil
}
func lastIndexPath(in indexPaths: [IndexPath], sectionID: AnyHashable) -> IndexPath? {
if indexPaths.count > 1 {
EpoxyLogger.shared.warn({
// `sectionIndexMap` is constructed from the same data as `itemIndexMap` so we can force
// unwrap.
// swiftlint:disable:next force_unwrapping
let sectionIndex = sectionIndexMap[sectionID]!

return """
Warning! Attempted to locate item \(itemDataID) in section \(sectionID) at indexes \
\(sectionIndex.map { $0 }) when there are multiple items with that ID at the indexes \
\(indexPaths.map { $0.item }). Choosing the last index. Items should have unique data \
IDs within a section as duplicate data IDs cause undefined behavior.
"""
}())
}

return indexPaths.last
}

switch section {
case .dataID(let sectionID):
if let indexPaths = itemIndexMapBySectionID[sectionID] {
// If the section ID is specified, just look up the indexes for that section.
return lastIndexPath(in: indexPaths, sectionID: sectionID)
}
return nil

case .lastWithItemDataID:
// If the section ID is unspecified but there's only one section with this data ID:
if itemIndexMapBySectionID.count == 1, let idAndIndexes = itemIndexMapBySectionID.first {
return lastIndexPath(in: idAndIndexes.value, sectionID: idAndIndexes.key)
}

// Otherwise there's multiple sections with the same data ID so we pick the last section so
// that it's stable.
let lastSectionID = itemIndexMapBySectionID.max(by: { first, second in
// `sectionIndexMap` is constructed from the same data as `itemIndexMap` so we can safely
// force unwrap.
// swiftlint:disable:next force_unwrapping
sectionIndexMap[first.key]!.last! < sectionIndexMap[second.key]!.last!
})

if let sectionID = lastSectionID {
EpoxyLogger.shared.warn("""
Warning! Attempted to locate item \(itemDataID) when there are multiple sections that \
contain it each with IDs \(itemIndexMapBySectionID.keys) at the indexes \
\(itemIndexMapBySectionID.keys.map { sectionIndexMap[$0] }). Choosing the last section \
\(sectionID.key). To fix this warning specify the desired section data ID when \
constructing your `ItemPath`.
""")

return lastIndexPath(in: sectionID.value, sectionID: sectionID.key)
}

return nil
}
}

}
2 changes: 2 additions & 0 deletions Sources/EpoxyCollectionView/CollectionView/Internal/CollectionViewDataSource.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ extension CollectionViewDataSource: UICollectionViewDataSource {
{
guard
let item = data?.supplementaryItem(ofKind: kind, at: indexPath),
let section = data?.section(at: indexPath.section),
let reuseID = reuseIDStore.registeredReuseID(for: item.viewDifferentiator)
else {
// The `supplementaryItem(…)` or `registeredReuseID(…)` methods will assert in this scenario.
Expand All @@ -219,6 +220,7 @@ extension CollectionViewDataSource: UICollectionViewDataSource {
self.collectionView?.configure(
supplementaryView: supplementaryView,
with: item,
at: .init(elementKind: kind, itemDataID: item.dataID, section: .dataID(section.dataID)),
animated: false)
} else {
EpoxyLogger.shared.assertionFailure(
Expand Down
Loading

0 comments on commit 21ea71c

Please sign in to comment.