Bk/target content offset improvements #116
Conversation
| case topItem(id: String, itemEdge: ItemEdge, distanceFromTop: CGFloat) | ||
| case bottomItem(id: String, itemEdge: ItemEdge, distanceFromBottom: CGFloat) |
There was a problem hiding this comment.
this change allows us to preserve the scroll position more accurately when cells change size on rotation (their height might grow / shrink depending on the width after rotation)
| if let stagedContentOffsetAdjustment { | ||
| let context = MagazineLayoutInvalidationContext() | ||
| context.invalidateLayoutMetrics = false | ||
| context.contentOffsetAdjustment = stagedContentOffsetAdjustment | ||
| invalidateLayout(with: context) | ||
| } | ||
| stagedContentOffsetAdjustment = nil |
There was a problem hiding this comment.
This was a key insight from https://itnext.io/my-covid-19-lockdown-project-or-how-i-started-to-dig-into-a-custom-uicollectionviewlayout-to-get-a-d053e1ad3aa0 (there's a comment describing it below)
There was a problem hiding this comment.
I'm not sure I'm seeing it, maybe we should add this in a comment? Curious why we invalidate the layout here.
| } else if | ||
| verticalLayoutDirection == .bottomToTop, | ||
| !isSizingElementBelowBottomEdge, | ||
| modelState.isPerformingBatchUpdates | ||
| { | ||
| contentOffsetAdjustment = CGPoint( | ||
| x: 0, | ||
| y: preferredAttributes.size.height - originalAttributes.size.height) | ||
| } else { | ||
| contentOffsetAdjustment = .zero | ||
| } | ||
|
|
||
| if modelState.isPerformingBatchUpdates { | ||
| // If we're in the middle of a batch update, we need to adjust our content offset. Doing it | ||
| // here in the middle of a batch update gets ignored for some reason. Instead, we delay | ||
| // slightly and do it in `finalizeCollectionViewUpdates`. | ||
| // This needs to be set in `finalizeCollectionViewUpdates`, otherwise it doesn't get applied. | ||
| stagedContentOffsetAdjustment = contentOffsetAdjustment | ||
| } else if isScrolling { | ||
| context.contentOffsetAdjustment = contentOffsetAdjustment |
There was a problem hiding this comment.
this code was already barely intelligible trash, and this certainly didn't help. It works though, and handles the cases where self-sizing items are inserted at the top / bottom
| let metricsSnapshot = MetricsSnapshot( | ||
| bounds: currentCollectionView.bounds, | ||
| contentSize: collectionViewContentSize, | ||
| contentInset: lastContentInset ?? contentInset) | ||
|
|
||
| // Replace existing snapshot if the bounds is the same | ||
| let indexOfExistingMetricsSnapshot = metricsSnapshots.firstIndex { | ||
| $0.bounds == metricsSnapshot.bounds | ||
| } | ||
| if let indexOfExistingMetricsSnapshot { | ||
| metricsSnapshots[indexOfExistingMetricsSnapshot] = metricsSnapshot | ||
| } else { | ||
| metricsSnapshots.append(metricsSnapshot) | ||
| } | ||
|
|
||
| // Limit to 3 snapshots | ||
| if metricsSnapshots.count > 3 { | ||
| metricsSnapshots.removeFirst() | ||
| } |
There was a problem hiding this comment.
we don't need any of this historical tracking of layout metrics, since we simply no longer take into account content insets on rotation
| visibleItemLocationFramePairs.append(itemLocationFramePair) | ||
| } | ||
| visibleItemLocationFramePairs.sort { $0.elementLocation < $1.elementLocation } |
There was a problem hiding this comment.
there were some bugs because this array wasn't guaranteed to be sorted before - whoops!
| contentHeight: preInvalidationContentSize?.height ?? currentCollectionView.contentSize.height, | ||
| // There doesn't seem to be a reliable way to get the correct content insets here. We can try | ||
| // track them in invalidateLayout or prepare, but then there are edge cases where we need to | ||
| // track multiple past inset values. It's a huge mess, and I don't think it's worth solving. |
bryankeller
force-pushed
the
bk/target-content-offset-improvements
branch
from
bcef59d to
fe53e8c
Compare
| !isSizingElementBelowBottomEdge, | ||
| modelState.isPerformingBatchUpdates | ||
| { | ||
| contentOffsetAdjustment = CGPoint( |
There was a problem hiding this comment.
it seems like we don't need to do this content offset adjustment for an onscreen cell, otherwise we get content offset jumps
There was a problem hiding this comment.
Simulator.Screen.Recording.-.iPhone.15.-.2024-02-14.at.14.28.57.mp4
| } else if | ||
| verticalLayoutDirection == .bottomToTop, | ||
| !isSizingElementBelowBottomEdge, | ||
| modelState.isPerformingBatchUpdates |
There was a problem hiding this comment.
I'm seeing in testing that we need this content offset even when batch updates aren't being performed. Here's a video of the below scenario where removing this check prevented a content offset jump. It's when you insert a cell at the top and it's offscreen.
Simulator.Screen.Recording.-.iPhone.15.-.2024-02-14.at.14.47.36.mp4
Details
This further improves our scroll position preservation logic. I've gone down a funny rabbit hole trying to solve this, discovering https://github.com/ekazaev/ChatLayout and the related blog post https://itnext.io/my-covid-19-lockdown-project-or-how-i-started-to-dig-into-a-custom-uicollectionviewlayout-to-get-a-d053e1ad3aa0 - this project referenced a ton of MagazineLayout code, but also took things a step further for scroll position preservation since it's a key requirement of a chat interface. This blog post has some solid information about the complications of using self-sizing cells with
targetContentOffset.The main benefit to this PR is that we now handle some self-sizing-cell edge cases that previously caused some jumps.
Related Issue
N/A
Motivation and Context
Improve scroll position preservation logic
How Has This Been Tested
Types of changes
Checklist