Fix Incorrect Item/Cell FoundDuring WillDisplay and DidEndDisplaying Callbacks #146
Conversation
brynbodayle
force-pushed
the
bb-item-display-callbacks-during-updates-fix
branch
from
d3d65a0
to
763bf10
Compare
|
Will be following-up with the same fix for supplementary items, wanted to split these PRs up. |
| /// | ||
| /// 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? { |
There was a problem hiding this comment.
but there is no guarantee of whether the cell is from the pre-update data or post-update data (so we check both).
collection view bein' collection view
| guard | ||
| let item = data?.itemIfPresent(at: indexPath), | ||
| let section = data?.sectionIfPresent(at: indexPath.section) | ||
| let item = data?.item(at: indexPath), |
There was a problem hiding this comment.
Notice how the willDisplay callback didn't call the ifPresent variant of this method. I believe this is because someone ran into this issue I just fixed in the past, but decided to fix it by calling the non-asserting function (the ifNeeded variant).
With this more robust fix, we can bring this in parity with willDisplay and add a helpful assert back.
| 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) |
There was a problem hiding this comment.
Can we delete func itemIfPresent(at indexPath: IndexPath) -> AnyItemModel? in CollectionViewData now?
There was a problem hiding this comment.
ah yes, good catch!
| /// 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? |
There was a problem hiding this comment.
We should probably make the same fix in CollectionViewReusableView for supplementary views
There was a problem hiding this comment.
fast follow, see above PR comment ;)
There was a problem hiding this comment.
love a fast follow - SGTM!
There was a problem hiding this comment.
Glad this works around the issue! I've seen this kind of "I have no way of knowing if I should reference old or new data" issue before at the layout level. A little bird told me Apple has private API for resolving some of these ambiguities. classic.
Left a few comments, but otherwise LGTM!
brynbodayle
force-pushed
the
bb-item-display-callbacks-during-updates-fix
branch
from
763bf10
to
47aca71
Compare
brynbodayle
force-pushed
the
bb-item-display-callbacks-during-updates-fix
branch
2 times, most recently
from
3259834
to
972494e
Compare
brynbodayle
force-pushed
the
bb-item-display-callbacks-during-updates-fix
branch
from
972494e
to
ae2df9d
Compare
Change summary
How was it tested?
How did you verify that this change accomplished what you expected? Add more detail as needed.
Pull request checklist
All items in this checklist must be completed before a pull request will be reviewed.
CollectionViewConfigurationCHANGELOG.mdentry in the "Unreleased" section for any library changes