Bk/target content offset improvements #116
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,13 +99,6 @@ public final class MagazineLayout: UICollectionViewLayout { | |
| override public func prepare() { | ||
| super.prepare() | ||
|
|
||
| // Save the previous collection view width if necessary | ||
| if prepareActions.contains(.cachePreviousWidth) { | ||
| cachedCollectionViewWidth = currentCollectionView.bounds.width | ||
|
|
@@ -255,6 +248,9 @@ public final class MagazineLayout: UICollectionViewLayout { | |
| modelState.applyUpdates(updates) | ||
| hasDataSourceCountInvalidationBeforeReceivingUpdateItems = false | ||
|
|
||
| lastSizedElementMinY = nil | ||
| lastSizedElementPreferredHeight = nil | ||
|
|
||
| super.prepare(forCollectionViewUpdates: updateItems) | ||
| } | ||
|
|
||
|
|
@@ -265,32 +261,37 @@ public final class MagazineLayout: UICollectionViewLayout { | |
| supplementaryViewLayoutAttributesForPendingAnimations.removeAll() | ||
|
|
||
| targetContentOffsetAnchor = nil | ||
| preInvalidationContentSize = nil | ||
|
|
||
| if let stagedContentOffsetAdjustment { | ||
| let context = MagazineLayoutInvalidationContext() | ||
| context.invalidateLayoutMetrics = false | ||
| context.contentOffsetAdjustment = stagedContentOffsetAdjustment | ||
| invalidateLayout(with: context) | ||
| } | ||
| stagedContentOffsetAdjustment = nil | ||
|
Comment on lines
+266
to
+272
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. |
||
|
|
||
| super.finalizeCollectionViewUpdates() | ||
| } | ||
|
|
||
| override public func prepare(forAnimatedBoundsChange oldBounds: CGRect) { | ||
| super.prepare(forAnimatedBoundsChange: oldBounds) | ||
|
|
||
| targetContentOffsetAnchor = targetContentOffsetAnchor( | ||
| bounds: oldBounds, | ||
| 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. | ||
|
|
||
| // The downside is that if your insets change on rotation, you won't always land in the exact | ||
| // correct spot if you're in the middle of the content. Being at the top or bottom works fine. | ||
| topInset: 0, | ||
| bottomInset: 0) | ||
| } | ||
|
|
||
| override public func finalizeAnimatedBoundsChange() { | ||
| targetContentOffsetAnchor = nil | ||
| preInvalidationContentSize = nil | ||
|
|
||
| super.finalizeAnimatedBoundsChange() | ||
| } | ||
|
|
@@ -671,6 +672,8 @@ public final class MagazineLayout: UICollectionViewLayout { | |
| withOriginalAttributes originalAttributes: UICollectionViewLayoutAttributes) | ||
| -> UICollectionViewLayoutInvalidationContext | ||
| { | ||
| let originalAttributes = originalAttributes.copy() as! UICollectionViewLayoutAttributes | ||
|
|
||
| switch preferredAttributes.representedElementCategory { | ||
| case .cell: | ||
| modelState.updateItemHeight( | ||
|
|
@@ -721,29 +724,60 @@ public final class MagazineLayout: UICollectionViewLayout { | |
| forPreferredLayoutAttributes: preferredAttributes, | ||
| withOriginalAttributes: originalAttributes) as! MagazineLayoutInvalidationContext | ||
|
|
||
| let isScrolling = currentCollectionView.isDragging || currentCollectionView.isDecelerating | ||
| let top = currentCollectionView.bounds.minY + contentInset.top | ||
| let bottom = currentCollectionView.bounds.maxY | ||
| let isSizingElementAboveTopEdge = originalAttributes.frame.minY < top | ||
| let isSizingElementBelowBottomEdge = originalAttributes.frame.minY > bottom | ||
|
|
||
| let contentOffsetAdjustment: CGPoint | ||
| if verticalLayoutDirection == .topToBottom, isSizingElementAboveTopEdge { | ||
| // If layout information is discarded above our current scroll position (on rotation, for | ||
| // example), we need to compensate for preferred size changes to items as we're scrolling up, | ||
| // otherwise, the collection view will appear to jump each time an element is sized. | ||
| // Since size adjustments can occur for multiple items in the same soon-to-be-visible row, we | ||
| // need to account for this by considering the preferred height for previously sized elements | ||
| // in the same row so that we only adjust the content offset by the exact amount needed to | ||
| // create smooth scrolling. | ||
| let isSameRowAsLastSizedElement = lastSizedElementMinY?.isEqual( | ||
| to: currentElementY, | ||
| threshold: 1 / scale) | ||
| ?? false | ||
|
|
||
| if isSameRowAsLastSizedElement { | ||
| let lastSizedElementPreferredHeight = self.lastSizedElementPreferredHeight ?? 0 | ||
| if preferredAttributes.size.height > lastSizedElementPreferredHeight { | ||
| contentOffsetAdjustment = CGPoint( | ||
| x: 0, | ||
| y: preferredAttributes.size.height - lastSizedElementPreferredHeight) | ||
| } else { | ||
| contentOffsetAdjustment = .zero | ||
| } | ||
| } else { | ||
| contentOffsetAdjustment = CGPoint( | ||
| x: 0, | ||
| y: preferredAttributes.size.height - originalAttributes.size.height) | ||
| } | ||
| } 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 |
||
| { | ||
| 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 |
||
| 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 | ||
|
Comment on lines
+761
to
+780
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 |
||
| } | ||
|
|
||
| lastSizedElementMinY = currentElementY | ||
|
|
@@ -760,30 +794,6 @@ public final class MagazineLayout: UICollectionViewLayout { | |
| return | ||
| } | ||
|
|
||
|
Comment on lines
-767
to
-785
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 |
||
| let shouldInvalidateLayoutMetrics = !context.invalidateEverything && | ||
| !context.invalidateDataSourceCounts | ||
|
|
||
|
|
@@ -854,16 +864,8 @@ public final class MagazineLayout: UICollectionViewLayout { | |
| private let _flipsHorizontallyInOppositeLayoutDirection: Bool | ||
| private let verticalLayoutDirection: MagazineLayoutVerticalLayoutDirection | ||
|
|
||
| private var cachedCollectionViewWidth: CGFloat? | ||
|
|
||
| // These properties are used to prevent scroll jumpiness due to self-sizing after rotation; see | ||
| // comment in `invalidationContext(forPreferredLayoutAttributes:withOriginalAttributes:)` for more | ||
| // details. | ||
|
|
@@ -900,6 +902,8 @@ public final class MagazineLayout: UICollectionViewLayout { | |
|
|
||
| private var isPerformingAnimatedBoundsChange = false | ||
| private var targetContentOffsetAnchor: TargetContentOffsetAnchor? | ||
| private var preInvalidationContentSize: CGSize? | ||
| private var stagedContentOffsetAdjustment: CGPoint? | ||
|
|
||
| // Used to provide the model state with the current visible bounds for the sole purpose of | ||
| // supporting pinned headers and footers. | ||
|
|
@@ -1271,17 +1275,15 @@ public final class MagazineLayout: UICollectionViewLayout { | |
| bottomInset: CGFloat) | ||
| -> TargetContentOffsetAnchor? | ||
| { | ||
| let insetBounds = bounds.inset(by: .init(top: topInset, left: 0, bottom: bottomInset, right: 0)) | ||
| var visibleItemLocationFramePairs = [ElementLocationFramePair]() | ||
| for itemLocationFramePair in modelState.itemLocationFramePairs(forItemsIn: insetBounds) { | ||
| 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! |
||
|
|
||
| let firstVisibleItemLocationFramePair = visibleItemLocationFramePairs.first | ||
| let lastVisibleItemLocationFramePair = visibleItemLocationFramePairs.last | ||
|
|
||
| guard | ||
| let firstVisibleItemLocationFramePair, | ||
|
|
@@ -1302,6 +1304,7 @@ public final class MagazineLayout: UICollectionViewLayout { | |
| bottomInset: bottomInset, | ||
| bounds: bounds, | ||
| contentHeight: contentHeight, | ||
| scale: scale, | ||
| firstVisibleItemID: firstVisibleItemID, | ||
| lastVisibleItemID: lastVisibleItemID, | ||
| firstVisibleItemFrame: firstVisibleItemLocationFramePair.frame, | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)