Fix Pager Node Issues #3028

Adlai-Holler · 2017-02-14T05:37:17Z

Resolves #2974 and other major pager node rotation/content inset issues.

Deprecates ASCollectionView.zeroContentInsets.
- This flag was added as a hack during development of ASPagerNode to "undo" the effects of UIViewController.automaticallyAdjustsScrollViewInsets.
- The flag adds contentInsets = .zero into layoutSubviews, after [super layoutSubviews]
- This means our contentInsets is thrashing around constantly.
- The view controller also sets contentOffset during its insets adjustment, which we didn't account for.
Adds a flag ASPagerNode.allowsAutomaticInsetsAdjustment.
- Defaults to NO since you virtually NEVER want UIVC to adjust pager node insets.
- If NO, finds the owning view controller of the pager and sets automaticallyAdjustsScrollViewInsets=NO, and logs a message.
- In the future, we may remove the automatic setting of this property and just log the message, so users retain control of their view controllers' flags.
- If they for some reason want automatic insets adjustment from their VC, they can set allowsAutomaticInsetsAdjustment=YES on their pager node.
Adds a public category method -[UIResponder asdk_associatedViewController] that safely travels up the responder chain and finds the nearest view controller. This is used above, and it may very well be handy for users especially when debugging. I also put the view controller in the node's -debugDescription for fun. I could have left it out to keep this diff smaller.
Adds a private class ASResponderChainEnumerator that is used in implementing above, and may come in handy in other places later.
Replaces ASPagerFlowLayout.currentIndexPath with ASPagerFlowLayout.currentCellNode so that we can stay pointing at the same node across inserts/deletes.
Makes ASPagerFlowLayout use -shouldInvalidateLayoutForBoundsChange: – which is like the UICollectionViewLayout version of scrollViewDidScroll: – to update its currentIndexPath, rather than using targetContentOffsetForProposedContentOffset:withScrollingVelocity:, which is not called during programmatic scrolls.
When a collection view's bounds change, removes the performBatchUpdates: that we placed around _relayoutAllNodes. The stated reason for the batch updates was to get the relayout into the data controller's transaction pipeline, but it already goes in that pipeline internally, and we don't need to submit an empty update to trigger a layout invalidation, since we're already manually invalidating the layout on the next line. This avoids triggering UICollectionViewLayout.targetContentOffsetForProposedContentOffset: needlessly, which happens on every collection view update, even empty ones.
Fixes indentation throughout ASPagerFlowLayout.m.

In short:
Before:

After:

Note the flashing may look ugly here, but it's intentionally added by flow layout because in a real app, the layout transition is usually going to be bad.

Adlai-Holler · 2017-02-14T17:15:11Z

I know this diff is huge, and I attempted to decompose it into smaller updates but to get the correct pager behavior, all of these changes were necessary (excluding e.g. adding the view controller into node debugDescription which was just for fun and seems very low-risk).

garrettmoon · 2017-02-14T21:13:39Z

AsyncDisplayKit/ASCollectionView.mm

-      [self performBatchAnimated:YES updates:^{
-        [_dataController relayoutAllNodes];
-      } completion:nil];
+      [_dataController relayoutAllNodes];


So much clearer.

garrettmoon · 2017-02-14T21:43:01Z

AsyncDisplayKit/ASPagerFlowLayout.m

+  }
+
+  CGRect bounds = self.collectionView.bounds;
+  CGRect rect = CGRectMake(CGRectGetMidX(bounds), CGRectGetMidY(bounds), 1, 1);


Probably doesn't matter, but should this possibly be CGRectMake(CGRectGetMidX(bounds) - 0.5, CGRectGetMidY(bounds) - 0.5, 1, 1);

I assume it doesn't because the pager should have cells that fill the view?

Right – "government work" level precision is enough here 🏗

garrettmoon · 2017-02-14T21:45:35Z

AsyncDisplayKit/ASPagerNode.h

+ * have automaticallyAdjustsScrollViewInsets=NO.
+ *
+ * If this property is NO, but your view controller has automaticallyAdjustsScrollViewInsets=YES,
+ * the pager node will set the property to NO and log a warning message. In the future,


This isn't 100% clear to me. It will set the property on the view controller to NO? Will it leave the pageNode property NO? Or, will it actually set the pagerNode property to YES?

It will leave the pager property (as NO) but it'll automatically set NO on the view controller (for backwards compatibility) and log a message that it did so. In the future, we want users to be responsible for configuring their own view controllers, so we'll just log the message.

Maybe just a slight adjustment to your comment then: If this property is NO, but your view controller has automaticallyAdjustsScrollViewInsets=YES, the pager node will set the property **on the view controller** to NO and log a warning message.

But without the stars :)

garrettmoon · 2017-02-14T21:49:32Z

AsyncDisplayKit/ASPagerNode.m

+	[super didEnterVisibleState];
+
+	// Check that our view controller does not automatically set our content insets
+	// It would be better to have a -didEnterHierarchy hook to put this in, but


Will willEnterHierarchy not suffice?

No unfortunately, because we're not yet in the hierarchy =(

Interesting, the isInHierarchy flag is set before willEnterHierarchy is called…

garrettmoon · 2017-02-14T21:53:04Z

AsyncDisplayKit/UIResponder+AsyncDisplayKit.m

+  ASDisplayNodeAssertMainThread();
+
+  for (UIResponder *responder in [self asdk_responderChainEnumerator]) {
+    UIViewController *vc = ASDynamicCast(responder, UIViewController);


Is this documented behavior? A responder will have it's viewController in the responder chain prior to any other possible view controllers at all times? If not, maybe a comment indicating it's been tested to be true in practice? If so a link would be awesome :)

Hmm I'll leave the link out because the best I could do is a general link to an overview of the responder chain. But responder chain always goes aView -> aView.superview -> aView.superview.viewController -> aView.superview.viewController.view.superview etc, which is documented.

garrettmoon · 2017-02-14T21:59:29Z

examples/PagerNode/Sample/PageNode.m

@@ -21,9 +21,9 @@

 @implementation PageNode

- (ASLayout *)calculateLayoutThatFits:(ASSizeRange)constrainedSize
+- (CGSize)calculateSizeThatFits:(CGSize)constrainedSize


@note Subclasses that override are committed to manual layout. Therefore, -layout: must be overriden to layout all subnodes or subviews.

Not needed in this case?

Right because we have no subnodes.

Adlai Holler and others added 3 commits February 13, 2017 19:17

Fix pager node and deprecate zeroContentInsets flag

1c7b895

Do it with the visible state callback

28e829e

There we are

ab2b6c8

facebook-github-bot added the CLA Signed label Feb 14, 2017

Put viewController in node debug description

cdba92f

Adlai-Holler requested review from maicki and nguyenhuy February 14, 2017 17:15

Adlai-Holler added Bug Collection Table Pager labels Feb 14, 2017

Adlai-Holler mentioned this pull request Feb 14, 2017

ASCellNode resizing not working in 2.1 #3006

Closed

garrettmoon added this to the 2.2 milestone Feb 14, 2017

Adlai-Holler requested a review from garrettmoon February 14, 2017 20:39

garrettmoon approved these changes Feb 14, 2017

View reviewed changes

Adlai-Holler merged commit fab98b3 into master Feb 14, 2017

Adlai-Holler deleted the AHFixPager branch February 14, 2017 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Pager Node Issues #3028

Fix Pager Node Issues #3028

Adlai-Holler commented Feb 14, 2017 •

edited

Loading

Adlai-Holler commented Feb 14, 2017

garrettmoon Feb 14, 2017

garrettmoon Feb 14, 2017

garrettmoon Feb 14, 2017

Adlai-Holler Feb 14, 2017

garrettmoon Feb 14, 2017

Adlai-Holler Feb 14, 2017

garrettmoon Feb 14, 2017

garrettmoon Feb 14, 2017

garrettmoon Feb 14, 2017

Adlai-Holler Feb 14, 2017

garrettmoon Feb 14, 2017

garrettmoon Feb 14, 2017

Adlai-Holler Feb 14, 2017

garrettmoon Feb 14, 2017

garrettmoon Feb 14, 2017

Adlai-Holler Feb 14, 2017

Fix Pager Node Issues #3028

Fix Pager Node Issues #3028

Conversation

Adlai-Holler commented Feb 14, 2017 • edited Loading

Adlai-Holler commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler commented Feb 14, 2017 •

edited

Loading