Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix Incorrect Supplementary Item Found During WillDisplay and DidEndDisplaying Callbacks #147

Merged
merged 3 commits into from
Aug 16, 2023
Merged

Fix Incorrect Supplementary Item Found During WillDisplay and DidEndDisplaying Callbacks #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21ea71c
Added fix for supplementary view calbacks during updates
brynbodayle Aug 15, 2023
b47778b
Fix cell data selection logic
brynbodayle Aug 15, 2023
dfe4843
Update Sources/EpoxyCollectionView/CollectionView/ReusableViews/Colle…
brynbodayle Aug 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
198 changes: 104 additions & 94 deletions Sources/EpoxyCollectionView/CollectionView/CollectionView.swift
View file
Open in desktop
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 @@ -672,25 +674,46 @@ 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.
/// Helper function which provides the correct models for a given cell 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? {
/// This is used in cases where the collection view might be mid-update and we need to find the underlying item for a cell, 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 itemAndSectionModel(
for cell: CollectionViewCell)
-> (AnyItemModel, SectionModel)?
{
guard let itemPath = cell.itemPath else {
EpoxyLogger.shared.assertionFailure("Cell is missing item path.")
EpoxyLogger.shared.assertionFailure("View is missing item path.")
return nil
}

func itemAndSectionModel(
from data: CollectionViewData?,
for indexPath: IndexPath)
-> (AnyItemModel, SectionModel)?
{
guard
let item = data?.item(at: indexPath),
let section = data?.section(at: indexPath.section) else
{
EpoxyLogger.shared.assertionFailure("Unable to find models in view data.")
return nil
}
return (item, section)
}

switch updateState {
case .notUpdating, .preparingUpdate:
return epoxyDataSource.data
guard let indexPath = epoxyDataSource.data?.indexPathForItem(at: itemPath) else {
EpoxyLogger.shared.assertionFailure("Unable to find models in view data.")
return nil
}
return itemAndSectionModel(from: epoxyDataSource.data, for: indexPath)
case .updating(from: let oldData):
if oldData.indexPathForItem(at: itemPath) == indexPath {
return oldData
} else if epoxyDataSource.data?.indexPathForItem(at: itemPath) == indexPath {
return epoxyDataSource.data
if let indexPath = oldData.indexPathForItem(at: itemPath) {
return itemAndSectionModel(from: oldData, for: indexPath)
} else if let indexPath = epoxyDataSource.data?.indexPathForItem(at: itemPath) {
return itemAndSectionModel(from: epoxyDataSource.data, for: indexPath)
} else {
EpoxyLogger.shared.assertionFailure(
"Cell not found in either old or new data during an update.")
Expand All @@ -699,26 +722,54 @@ open class CollectionView: UICollectionView {
}
}

private func itemForCell(_ cell: CollectionViewCell) -> AnyItemModel? {
guard
let indexPath = indexPath(for: cell),
let model = epoxyDataSource.data?.item(at: indexPath)
else {
EpoxyLogger.shared.assertionFailure("item not found")
/// Helper function which provides the correct models for a given reusable view 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, 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 itemAndSectionModel(
for view: CollectionViewReusableView,
forElementKind elementKind: String)
-> (AnySupplementaryItemModel, SectionModel)?
{
guard let itemPath = view.itemPath else {
EpoxyLogger.shared.assertionFailure("View is missing item path.")
return nil
}
return model
}

private func sectionForCell(_ cell: CollectionViewCell) -> SectionModel? {
guard
let indexPath = indexPath(for: cell),
let section = epoxyDataSource.data?.section(at: indexPath.section)
else {
EpoxyLogger.shared.assertionFailure("item not found")
return nil
func itemAndSectionModel(
from data: CollectionViewData?,
for indexPath: IndexPath)
-> (AnySupplementaryItemModel, SectionModel)?
{
guard
let item = data?.supplementaryItem(ofKind: elementKind, at: indexPath),
let section = data?.section(at: indexPath.section) else
{
EpoxyLogger.shared.assertionFailure("Unable to find models in view data.")
return nil
}
return (item, section)
}

switch updateState {
case .notUpdating, .preparingUpdate:
guard let indexPath = epoxyDataSource.data?.indexPathForSupplementaryItem(at: itemPath) else {
EpoxyLogger.shared.assertionFailure("Unable to find models in view data.")
return nil
}
return itemAndSectionModel(from: epoxyDataSource.data, for: indexPath)
case .updating(from: let oldData):
if let indexPath = oldData.indexPathForSupplementaryItem(at: itemPath) {
return itemAndSectionModel(from: oldData, for: indexPath)
} else if let indexPath = epoxyDataSource.data?.indexPathForSupplementaryItem(at: itemPath) {
return itemAndSectionModel(from: epoxyDataSource.data, for: indexPath)
} else {
EpoxyLogger.shared.assertionFailure(
"View not found in either old or new data during an update.")
return nil
}
}
return section
}
}

Expand Down Expand Up @@ -819,21 +870,14 @@ extension CollectionView: UICollectionViewDelegate {
public func collectionView(
_: UICollectionView,
willDisplay cell: UICollectionViewCell,
forItemAt indexPath: IndexPath)
forItemAt _: IndexPath)
{
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?.item(at: indexPath),
let section = data?.section(at: indexPath.section)
else {
return
}
guard let (item, section) = itemAndSectionModel(for: cell) else { return }

handleSection(section, itemWillDisplay: .item(dataID: item.dataID))

Expand All @@ -849,21 +893,14 @@ extension CollectionView: UICollectionViewDelegate {
public func collectionView(
_: UICollectionView,
didEndDisplaying cell: UICollectionViewCell,
forItemAt indexPath: IndexPath)
forItemAt _: IndexPath)
{
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?.item(at: indexPath),
let section = data?.section(at: indexPath.section)
else {
return
}
guard let (item, section) = itemAndSectionModel(for: cell) else { return }

handleSection(section, itemDidEndDisplaying: .item(dataID: item.dataID))

Expand All @@ -880,27 +917,23 @@ extension CollectionView: UICollectionViewDelegate {
_: UICollectionView,
willDisplaySupplementaryView view: UICollectionReusableView,
forElementKind elementKind: String,
at indexPath: IndexPath)
at _: 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 section = epoxyDataSource.data?.sectionIfPresent(at: indexPath.section),
let item = epoxyDataSource.data?.supplementaryItemIfPresent(ofKind: elementKind, at: indexPath)
else {
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
}

guard
let (item, section) = itemAndSectionModel(
for: view,
forElementKind: elementKind) else { return }

handleSection(
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 @@ -917,36 +950,23 @@ extension CollectionView: UICollectionViewDelegate {
_: UICollectionView,
didEndDisplayingSupplementaryView view: UICollectionReusableView,
forElementOfKind elementKind: String,
at indexPath: IndexPath)
at _: 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof, this sucks. I know we're currently using the cell / reusable view as an easy way to store the item path, but maybe that isn't the right solution (given this limitation). TBH I'm cool with ignoring this for now, but we might need to address this in the future by maintaining a cell / reusable view => item path mapping ourselves.

Copy link
Contributor Author

@brynbodayle brynbodayle Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the limitation you're concerned about? We only need to provide callbacks and track the visibility of our own Epoxy-based views, not ones created internally by compositional layout.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right. I was thinking we'd still want callbacks for those, but makes sense that we wouldn't since we're not making the epoxy models for them.

}

// 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 section = data?.sectionIfPresent(at: indexPath.section),
let item = data?.supplementaryItemIfPresent(ofKind: elementKind, at: indexPath)
else {
return
}
let (item, section) = itemAndSectionModel(
for: view,
forElementKind: elementKind) else { return }

handleSection(
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 Expand Up @@ -1143,33 +1163,23 @@ extension CollectionView: CollectionViewDataSourceReorderingDelegate {

extension CollectionView: CollectionViewCellAccessibilityDelegate {
func collectionViewCellDidBecomeFocused(cell: CollectionViewCell) {
guard
let model = itemForCell(cell),
let section = sectionForCell(cell)
else {
return
}
guard let (item, section) = itemAndSectionModel(for: cell) else { return }

lastFocusedDataID = .init(itemDataID: model.dataID, section: .dataID(section.dataID))
lastFocusedDataID = .init(itemDataID: item.dataID, section: .dataID(section.dataID))

accessibilityDelegate?.collectionView(
self,
itemDidBecomeFocused: model,
itemDidBecomeFocused: item,
with: cell.view,
in: section)
}

func collectionViewCellDidLoseFocus(cell: CollectionViewCell) {
guard
let model = itemForCell(cell),
let section = sectionForCell(cell)
else {
return
}
guard let (item, section) = itemAndSectionModel(for: cell) else { return }

accessibilityDelegate?.collectionView(
self,
itemDidLoseFocus: model,
itemDidLoseFocus: item,
with: cell.view,
in: section)
}
Expand Down
Loading
Loading