-
Notifications
You must be signed in to change notification settings - Fork 222
Bk/add layout state #148
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.
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
Bk/add layout state #148
Conversation
| } | ||
|
|
||
| private init( | ||
| init( |
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.
made internal for unit test support
| /// An internal type for calculating the target content offset for various state of the collection view. Various anchors are possible, each | ||
| /// changing how the collection view prioritizes keeping certain items visible in target content offset calculations. |
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.
All of the static functions below for calculating the targetContentOffsetAnchor and yOffset moved to the LayoutState, making it easier to get this information for pre-update and post-update layout states.
| var contentSize: CGSize { | ||
| // This is a workaround for `layoutAttributesForElementsInRect:` not getting invoked enough | ||
| // times if `collectionViewContentSize.width` is not smaller than the width of the collection | ||
| // view, minus horizontal insets. This results in visual defects when performing batch | ||
| // updates. To work around this, we subtract 0.0001 from our content size width calculation; | ||
| // this small decrease in `collectionViewContentSize.width` is enough to work around the | ||
| // incorrect, internal collection view `CGRect` checks, without introducing any visual | ||
| // differences for elements in the collection view. | ||
| // See https://openradar.appspot.com/radar?id=5025850143539200 for more details. | ||
| let width = bounds.width - contentInset.left - contentInset.right - 0.0001 | ||
|
|
||
| let numberOfSections = modelState.numberOfSections | ||
| let height: CGFloat = | ||
| if numberOfSections <= 0 { | ||
| 0 | ||
| } else { | ||
| modelState.sectionMaxY(forSectionAtIndex: numberOfSections - 1) | ||
| } | ||
|
|
||
| return CGSize(width: width, height: height) |
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.
all of this used to live in MagazineLayout.swift - I moved it here without changing any math or comments
| var targetContentOffsetAnchor: TargetContentOffsetAnchor { | ||
| var visibleItemLocationFramePairs = [ElementLocationFramePair]() | ||
| for itemLocationFramePair in modelState.itemLocationFramePairs(forItemsIn: bounds) { | ||
| visibleItemLocationFramePairs.append(itemLocationFramePair) | ||
| } | ||
| visibleItemLocationFramePairs.sort { $0.elementLocation < $1.elementLocation } | ||
|
|
||
| let firstVisibleItemLocationFramePair = visibleItemLocationFramePairs.first { | ||
| // When scrolling up, only calculate a target content offset based on visible, already-sized | ||
| // cells. Otherwise, scrolling will be jumpy. | ||
| modelState.isItemHeightSettled(indexPath: $0.elementLocation.indexPath) | ||
| } ?? visibleItemLocationFramePairs.first // fallback to the first item if we can't find one with a settled height | ||
|
|
||
| let lastVisibleItemLocationFramePair = visibleItemLocationFramePairs.reversed().first { | ||
| // When scrolling down, only calculate a target content offset based on visible, already-sized | ||
| // cells. Otherwise, scrolling will be jumpy. | ||
| modelState.isItemHeightSettled(indexPath: $0.elementLocation.indexPath) | ||
| } ?? visibleItemLocationFramePairs.last // fallback to the last item if we can't find one with a settled height | ||
|
|
||
| guard | ||
| let firstVisibleItemLocationFramePair, | ||
| let lastVisibleItemLocationFramePair, | ||
| let firstVisibleItemID = modelState.idForItemModel( | ||
| at: firstVisibleItemLocationFramePair.elementLocation.indexPath), | ||
| let lastVisibleItemID = modelState.idForItemModel( | ||
| at: lastVisibleItemLocationFramePair.elementLocation.indexPath) | ||
| else { | ||
| switch verticalLayoutDirection { | ||
| case .topToBottom: return .top | ||
| case .bottomToTop: return .bottom | ||
| } | ||
| } | ||
|
|
||
| let top = minContentOffset.y.alignedToPixel(forScreenWithScale: scale) | ||
| let bottom = maxContentOffset.y.alignedToPixel(forScreenWithScale: scale) | ||
| let isAtTop = bounds.minY <= top | ||
| let isAtBottom = bounds.minY >= bottom | ||
| let position: Position | ||
| if isAtTop, isAtBottom { | ||
| switch verticalLayoutDirection { | ||
| case .topToBottom: | ||
| position = .atTop | ||
| case .bottomToTop: | ||
| position = .atBottom | ||
| } | ||
| } else if isAtTop { | ||
| position = .atTop | ||
| } else if isAtBottom { | ||
| position = .atBottom | ||
| } else { | ||
| position = .inMiddle | ||
| } | ||
|
|
||
| switch verticalLayoutDirection { | ||
| case .topToBottom: | ||
| switch position { | ||
| case .atTop: | ||
| return .top | ||
| case .inMiddle, .atBottom: | ||
| let top = bounds.minY + contentInset.top | ||
| let distanceFromTop = firstVisibleItemLocationFramePair.frame.minY - top | ||
| return .topItem( | ||
| id: firstVisibleItemID, | ||
| distanceFromTop: distanceFromTop.alignedToPixel(forScreenWithScale: scale)) | ||
| } | ||
| case .bottomToTop: | ||
| switch position { | ||
| case .atTop, .inMiddle: | ||
| let bottom = bounds.maxY - contentInset.bottom | ||
| let distanceFromBottom = lastVisibleItemLocationFramePair.frame.maxY - bottom | ||
| return .bottomItem( | ||
| id: lastVisibleItemID, | ||
| distanceFromBottom: distanceFromBottom.alignedToPixel(forScreenWithScale: scale)) | ||
| case .atBottom: | ||
| return .bottom | ||
| } | ||
| } | ||
| } |
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.
logic pulled out of a function that used to live in MagazineLayout.swift
| if !disableSectionMaxYsCache { | ||
| cacheMaxY(maxY, forSectionAtIndex: targetSectionIndex) | ||
| } |
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.
I meant to delete this in my last PR - this extra property / check isn't necessary, and was never actually used
|
|
||
| @testable import MagazineLayout | ||
|
|
||
| final class LayoutStateTargetContentOffsetTests: XCTestCase { |
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.
Deleted the old target-content-offset-related tests, and replaced them with these new ones. The test cases are the same, but the implementations are different due to the tested logic moving to the LayoutState
| let numberOfSections = modelState.numberOfSections | ||
|
|
||
| let width: CGFloat | ||
| if let collectionView = collectionView { | ||
| // This is a workaround for `layoutAttributesForElementsInRect:` not getting invoked enough | ||
| // times if `collectionViewContentSize.width` is not smaller than the width of the collection | ||
| // view, minus horizontal insets. This results in visual defects when performing batch | ||
| // updates. To work around this, we subtract 0.0001 from our content size width calculation; | ||
| // this small decrease in `collectionViewContentSize.width` is enough to work around the | ||
| // incorrect, internal collection view `CGRect` checks, without introducing any visual | ||
| // differences for elements in the collection view. | ||
| // See https://openradar.appspot.com/radar?id=5025850143539200 for more details. | ||
| width = collectionView.bounds.width - contentInset.left - contentInset.right - 0.0001 | ||
| } else { | ||
| width = 0 | ||
| } | ||
|
|
||
| let height: CGFloat | ||
| if numberOfSections <= 0 { | ||
| height = 0 | ||
| } else { | ||
| height = modelState.sectionMaxY(forSectionAtIndex: numberOfSections - 1) | ||
| } | ||
|
|
||
| return CGSize(width: width, height: height) |
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.
moved to the LayoutState
| // Calculate the target offset before applying updates, since the target offset should be based | ||
| // on the pre-update state. | ||
| targetContentOffsetAnchor = currentTargetContentOffsetAnchor | ||
| contentHeightBeforeUpdates = collectionViewContentSize.height | ||
|
|
||
| modelState.applyUpdates(updates, modelStateBeforeBatchUpdates: modelStateBeforeBatchUpdates) |
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.
able to simplify here thanks to the new approach of using LayoutState to help us figure out target content offsets
| if let layoutStateBeforeCollectionViewUpdates{ | ||
| let targetContentOffsetAnchor = layoutStateBeforeCollectionViewUpdates.targetContentOffsetAnchor | ||
| let targetYOffset = layoutState.yOffset(for: targetContentOffsetAnchor) | ||
| let context = MagazineLayoutInvalidationContext() | ||
| context.invalidateLayoutMetrics = false | ||
| context.contentOffsetAdjustment.y = targetYOffset - currentCollectionView.contentOffset.y | ||
| context.contentOffsetAdjustment.y = targetYOffset - layoutState.bounds.minY |
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.
same logic as before, but now using the layout state instead of a targetContentOffsetAnchor property stored in the layout
| targetContentOffsetAnchor = nil | ||
| contentHeightBeforeUpdates = nil |
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.
slightly less bookkeeping - more cleanup to come next!
| let attributes = super.initialLayoutAttributesForAppearingItem(at: itemIndexPath) | ||
| attributes?.frame = modelState.frameForItem(at: ElementLocation(indexPath: itemIndexPath)) | ||
|
|
||
| if | ||
| modelState.itemIndexPathsToInsert.contains(itemIndexPath) || | ||
| modelState.sectionIndicesToInsert.contains(itemIndexPath.section) | ||
| { | ||
| let attributes = layoutAttributesForItem(at: itemIndexPath)?.copy() as? UICollectionViewLayoutAttributes | ||
| attributes.map { | ||
| delegateMagazineLayout?.collectionView( | ||
| currentCollectionView, | ||
| layout: self, | ||
| initialLayoutAttributesForInsertedItemAt: itemIndexPath, | ||
| byModifying: $0) | ||
| } | ||
|
|
||
| attributes?.frame.origin.y += targetContentOffsetCompensatingYOffsetForAppearingItem ?? 0 | ||
| attributes?.transform = CGAffineTransform( | ||
| translationX: 0, | ||
| y: targetContentOffsetCompensatingYOffsetForAppearingItem ?? 0, | ||
| ) |
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.
Instead of manually updating the frame of these initial layout attributes (for an appearing item), we can use a non-destructive transform.
| switch targetContentOffsetAnchor { | ||
| case .top: | ||
| context.contentOffsetAdjustment.y = layoutState.minContentOffset.y - layoutState.bounds.minY | ||
|
|
||
| case .bottom: | ||
| context.contentOffsetAdjustment.y = layoutState.maxContentOffset.y - layoutState.bounds.minY | ||
|
|
||
| case .topItem, .bottomItem: | ||
| let targetYOffsetAfter = layoutState.yOffset(for: targetContentOffsetAnchor) | ||
| context.contentOffsetAdjustment.y = targetYOffsetAfter - targetYOffsetBefore |
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 revised logic enables us go start scrolled all the way to the bottom when using the .bottomToTop layout direction, without feature code needing to manually set the content offset to be at the bottom.
| switch verticalLayoutDirection { | ||
| case .topToBottom: | ||
| attributes.frame = modelState.frameForItem(at: ElementLocation(indexPath: preferredAttributes.indexPath)) | ||
|
|
||
| case .bottomToTop: | ||
| if case .bottom = targetContentOffsetAnchor { | ||
| attributes.transform = .identity | ||
| attributes.frame = modelState.frameForItem(at: ElementLocation(indexPath: preferredAttributes.indexPath)) | ||
| } else { | ||
| let previousHeight = attributes.frame.height | ||
| attributes.frame = modelState.frameForItem(at: ElementLocation(indexPath: preferredAttributes.indexPath)) | ||
|
|
||
| var targetContentOffsetCompensatingYOffsetForAppearingItem = targetContentOffsetCompensatingYOffsetForAppearingItem ?? 0 | ||
| targetContentOffsetCompensatingYOffsetForAppearingItem -= (attributes.frame.height - previousHeight) | ||
| self.targetContentOffsetCompensatingYOffsetForAppearingItem = targetContentOffsetCompensatingYOffsetForAppearingItem | ||
| attributes.transform = CGAffineTransform(translationX: 0, y: targetContentOffsetCompensatingYOffsetForAppearingItem) | ||
| } | ||
| } |
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.
kind of annoying code that only serves to improve the cell insertion animation when inserting multiple items at the top. I'm still exploring ways to simplify this, but for now, it works well.
| let context = super.invalidationContext( | ||
| forPreferredLayoutAttributes: preferredAttributes, | ||
| withOriginalAttributes: originalAttributes) as! MagazineLayoutInvalidationContext | ||
|
|
||
| if let contentOffsetAdjustment, !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`. | ||
| context.contentOffsetAdjustment = contentOffsetAdjustment | ||
| } | ||
|
|
||
| context.invalidateLayoutMetrics = false | ||
|
|
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.
I haven't found a need for this special handling after this refactor - another simplification!
| private func targetContentOffsetAnchor( | ||
| bounds: CGRect, | ||
| contentHeight: CGFloat, | ||
| topInset: CGFloat, | ||
| bottomInset: CGFloat) | ||
| -> TargetContentOffsetAnchor? | ||
| { | ||
| var visibleItemLocationFramePairs = [ElementLocationFramePair]() | ||
| for itemLocationFramePair in modelState.itemLocationFramePairs(forItemsIn: bounds) { | ||
| visibleItemLocationFramePairs.append(itemLocationFramePair) | ||
| } | ||
| visibleItemLocationFramePairs.sort { $0.elementLocation < $1.elementLocation } | ||
|
|
||
| let firstVisibleItemLocationFramePair = visibleItemLocationFramePairs.first { | ||
| // When scrolling up, only calculate a target content offset based on visible, already-sized | ||
| // cells. Otherwise, scrolling will be jumpy. | ||
| modelState.isItemHeightSettled(indexPath: $0.elementLocation.indexPath) | ||
| } ?? visibleItemLocationFramePairs.first // fallback to the first item if we can't find one with a settled height | ||
|
|
||
| let lastVisibleItemLocationFramePair = visibleItemLocationFramePairs.last | ||
|
|
||
| guard | ||
| let firstVisibleItemLocationFramePair, | ||
| let lastVisibleItemLocationFramePair, | ||
| let firstVisibleItemID = modelState.idForItemModel( | ||
| at: firstVisibleItemLocationFramePair.elementLocation.indexPath), | ||
| let lastVisibleItemID = modelState.idForItemModel( | ||
| at: lastVisibleItemLocationFramePair.elementLocation.indexPath) | ||
| else { | ||
| return nil | ||
| } | ||
|
|
||
| return TargetContentOffsetAnchor.targetContentOffsetAnchor( | ||
| verticalLayoutDirection: verticalLayoutDirection, | ||
| topInset: topInset, | ||
| bottomInset: bottomInset, | ||
| bounds: bounds, | ||
| contentHeight: contentHeight, | ||
| scale: scale, | ||
| firstVisibleItemID: firstVisibleItemID, | ||
| lastVisibleItemID: lastVisibleItemID, | ||
| firstVisibleItemFrame: firstVisibleItemLocationFramePair.frame, | ||
| lastVisibleItemFrame: lastVisibleItemLocationFramePair.frame) | ||
| } | ||
|
|
||
| private func yOffset(for targetContentOffsetAnchor: TargetContentOffsetAnchor) -> CGFloat { | ||
| targetContentOffsetAnchor.yOffset( | ||
| topInset: contentInset.top, | ||
| bottomInset: contentInset.bottom, | ||
| bounds: currentCollectionView.bounds, | ||
| contentHeight: collectionViewContentSize.height, | ||
| indexPathForItemID: { modelState.indexPathForItemModel(withID: $0) }, | ||
| frameForItemAtIndexPath: { | ||
| modelState.frameForItem(at: ElementLocation(indexPath: $0)) | ||
| }) | ||
| } | ||
|
|
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.
Moved to the LayoutState
brynbodayle
left a comment
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.
nice improvement!
Details
This PR builds on the last few, introducing a new abstraction called the
LayoutState. TheLayoutStatecontains a snapshot of theModelState(which contains section and item models), as well as collection view metrics like the current bounds and content insets.When updates occur (due to
prepareLayout,prepareForCollectionViewUpdates, andprepareForAnimatedBoundsChangebeing called), we save a snapshot of the layout by creating a LayoutState instance before making any changes. This is very useful, as it allows us to later on reference the exact state the collection view was in when doing things like:contentOffsetAdjustmentas items are self-sized when scrollingOnce we land this, I plan to further simplify
MagazineLayout.swift, which currently does a ton of layout metric caching / tracking across updates, each one being done in a bespoke way. With the introduction of theLayoutState, we should be able to clean up most of this one-off metric tracking.How Has This Been Tested
Tested new example app, Airbnb app, unit tests
Types of changes
Checklist