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

[RTL/Batching] Make ASDisplayShouldFetchBatchForScrollView aware of flipped CV layouts #1985

Merged
merged 3 commits into from May 3, 2021
Merged

[RTL/Batching] Make ASDisplayShouldFetchBatchForScrollView aware of flipped CV layouts #1985

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
4ee877d
[RTL/Batching] Make ASDisplayShouldFetchBatchForScrollView aware of f…
rcancro Apr 30, 2021
168f71c
fix indentation
rcancro May 3, 2021
4edc28d
assert that we are on main when accessing CV layout
rcancro May 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion Source/ASCollectionView.mm
View file
Open in desktop
Expand Up @@ -1857,7 +1857,10 @@ - (void)_checkForBatchFetching

- (void)_beginBatchFetchingIfNeededWithContentOffset:(CGPoint)contentOffset velocity:(CGPoint)velocity
{
if (ASDisplayShouldFetchBatchForScrollView(self, self.scrollDirection, self.scrollableDirections, contentOffset, velocity)) {
// Since we are accessing self.collectionViewLayout, we should make sure we are on main
ASDisplayNodeAssertMainThread();

if (ASDisplayShouldFetchBatchForScrollView(self, self.scrollDirection, self.scrollableDirections, contentOffset, velocity, self.collectionViewLayout.flipsHorizontallyInOppositeLayoutDirection)) {
Copy link
Member

@nguyenhuy nguyenhuy May 3, 2021 •
edited

Choose a reason for hiding this comment

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

As @wsdwsd0829 pointed out to me, UICollectionView actually asks for this property in its initializer and caches it. Perhaps we should do the same for ASCollectionView? Not a blocker for this diff though.

image(1)

Screen Shot 2021-05-03 at 10 25 00 AM

Copy link
Member

Choose a reason for hiding this comment

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

Nvm, it's actually cached by UICollectionViewLayout in its initializer and so asking for flipsHorizontallyInOppositeLayoutDirection on the layout object here should be fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to worry about calling self.collectionViewLayout. flipsHorizontallyInOppositeLayoutDirection on a potentially off-main thread? Since it is cached and presumably not called again, do I not need to worry about it?

Copy link
Member

@nguyenhuy nguyenhuy May 3, 2021 •
edited

Choose a reason for hiding this comment

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

I think the risk is very small. I checked and _beginBatchFetchingIfNeededWithContentOffset is called by multiple methods but all of them are called on the main thread. You can add a main thread assertion there so we will be notified in the slim chance that's not the case or when a new method starts calling it off main.

Copy link
Contributor

@foxware00 foxware00 Jun 1, 2021 •
edited

Choose a reason for hiding this comment

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

Worth noting @nguyenhuy and @rcancro self.collectionViewLayout.flipsHorizontallyInOppositeLayoutDirection is not availiable on iOS 10 so this PR causes crashing on those versions.

[self _beginBatchFetching];
}
}
Expand Down
2 changes: 1 addition & 1 deletion Source/ASTableView.mm
View file
Open in desktop
Expand Up @@ -1486,7 +1486,7 @@ - (void)_checkForBatchFetching

- (void)_beginBatchFetchingIfNeededWithContentOffset:(CGPoint)contentOffset velocity:(CGPoint)velocity
{
if (ASDisplayShouldFetchBatchForScrollView(self, self.scrollDirection, ASScrollDirectionVerticalDirections, contentOffset, velocity)) {
if (ASDisplayShouldFetchBatchForScrollView(self, self.scrollDirection, ASScrollDirectionVerticalDirections, contentOffset, velocity, NO)) {
[self _beginBatchFetching];
}
}
Expand Down
8 changes: 7 additions & 1 deletion Source/Private/ASBatchFetching.h
View file
Open in desktop
Expand Up @@ -34,13 +34,16 @@ NS_ASSUME_NONNULL_BEGIN
@param scrollableDirections The possible scrolling directions of the scroll view.
@param contentOffset The offset that the scrollview will scroll to.
@param velocity The velocity of the scroll view (in points) at the moment the touch was released.
@param flipsHorizontallyInOppositeLayoutDirection Whether or not this scroll view flips its layout automatically in RTL.
See flipsHorizontallyInOppositeLayoutDirection in UICollectionViewLayout
@return Whether or not the current state should proceed with batch fetching.
*/
ASDK_EXTERN BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetchingScrollView> *scrollView,
ASScrollDirection scrollDirection,
ASScrollDirection scrollableDirections,
CGPoint contentOffset,
CGPoint velocity);
CGPoint velocity,
BOOL flipsHorizontallyInOppositeLayoutDirection);


/**
Expand All @@ -55,6 +58,8 @@ ASDK_EXTERN BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetc
@param visible Whether the view is visible or not.
@param velocity The velocity of the scroll view (in points) at the moment the touch was released.
@param delegate The delegate to be consulted if needed.
@param flipsHorizontallyInOppositeLayoutDirection Whether or not this scroll view flips its layout automatically in RTL.
See flipsHorizontallyInOppositeLayoutDirection in UICollectionViewLayout
@return Whether or not the current state should proceed with batch fetching.
@discussion This method is broken into a category for unit testing purposes and should be used with the ASTableView and
* ASCollectionView batch fetching API.
Expand All @@ -69,6 +74,7 @@ ASDK_EXTERN BOOL ASDisplayShouldFetchBatchForContext(ASBatchContext *context,
BOOL visible,
BOOL shouldRenderRTLLayout,
CGPoint velocity,
BOOL flipsHorizontallyInOppositeLayoutDirection,
_Nullable id<ASBatchFetchingDelegate> delegate);

NS_ASSUME_NONNULL_END
14 changes: 8 additions & 6 deletions Source/Private/ASBatchFetching.mm
View file
Open in desktop
Expand Up @@ -15,7 +15,8 @@ BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetchingScrollVi
ASScrollDirection scrollDirection,
ASScrollDirection scrollableDirections,
CGPoint contentOffset,
CGPoint velocity)
CGPoint velocity,
BOOL flipsHorizontallyInOppositeLayoutDirection)
{
// Don't fetch if the scroll view does not allow
if (![scrollView canBatchFetch]) {
Expand All @@ -30,7 +31,7 @@ BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetchingScrollVi
id<ASBatchFetchingDelegate> delegate = scrollView.batchFetchingDelegate;
BOOL visible = (scrollView.window != nil);
BOOL shouldRenderRTLLayout = [UIView userInterfaceLayoutDirectionForSemanticContentAttribute:scrollView.semanticContentAttribute] == UIUserInterfaceLayoutDirectionRightToLeft;
return ASDisplayShouldFetchBatchForContext(context, scrollDirection, scrollableDirections, bounds, contentSize, contentOffset, leadingScreens, visible, shouldRenderRTLLayout, velocity, delegate);
return ASDisplayShouldFetchBatchForContext(context, scrollDirection, scrollableDirections, bounds, contentSize, contentOffset, leadingScreens, visible, shouldRenderRTLLayout, velocity, flipsHorizontallyInOppositeLayoutDirection, delegate);
}

BOOL ASDisplayShouldFetchBatchForContext(ASBatchContext *context,
Expand All @@ -43,6 +44,7 @@ BOOL ASDisplayShouldFetchBatchForContext(ASBatchContext *context,
BOOL visible,
BOOL shouldRenderRTLLayout,
CGPoint velocity,
BOOL flipsHorizontallyInOppositeLayoutDirection,
id<ASBatchFetchingDelegate> delegate)
{
// Do not allow fetching if a batch is already in-flight and hasn't been completed or cancelled
Expand Down Expand Up @@ -87,11 +89,11 @@ BOOL ASDisplayShouldFetchBatchForContext(ASBatchContext *context,
}

CGFloat triggerDistance = viewLength * leadingScreens;
CGFloat remainingDistance = contentLength - viewLength - offset;
if (shouldRenderRTLLayout && ASScrollDirectionContainsHorizontalDirection(scrollableDirections)) {
remainingDistance = offset;
CGFloat remainingDistance = 0;
if (!flipsHorizontallyInOppositeLayoutDirection && shouldRenderRTLLayout && ASScrollDirectionContainsHorizontalDirection(scrollableDirections)) {
remainingDistance = offset;
} else {
remainingDistance = contentLength - viewLength - offset;
remainingDistance = contentLength - viewLength - offset;
}
BOOL result = remainingDistance <= triggerDistance;

Expand Down