Introduce ASCollectionGalleryLayoutDelegate #76

nguyenhuy · 2017-04-27T18:57:23Z

ASCollectionGalleryLayoutDelegate arranges items of the same size into a multi-line stack (say photo gallery or pager). It takes advantage of the fact that its items always have a fixed size to measure as few items as possible while still being able to track their positions at all time. This helps reduce startup/reloadData time, as well as memory footprint.
It then uses a measure range, which also works as a allocate range, to figure out which items to measure ahead of time. And it guarantees that each item is scheduled to measure only once.
Lastly, ASCollectionLayoutDelegate has some new methods that allow delegates to hook up and stay ahead of layout attributes requests from the backing view. ASCollectionGalleryLayoutDelegate for example uses these methods to ensure elements that have their layout attributes requested are always ready for consumption, and to measure more elements in the background.

Follow up tasks:

Unit tests (for real!).
~~Layout delegate to take over node allocation responsibility from ASDataController.~~
Let layout delegate report scrollable directions directly to ASRangeManager, thus by pass layout inspector completely.
~~Revisit ASCollectionViewLayoutController and its friends.~~

Ticket: #186

nguyenhuy · 2017-05-04T12:35:16Z

@Adlai-Holler @maicki @garrettmoon @appleguy: This PR is ready for review!

Adlai-Holler

Rather than add API to the CollectionLayoutDelegate, it seems that this logic isn't specific to the gallery layout. Any collection should probably have this same behavior – ensuring that nodes are measured asynchronously in advance. In the case that the node was already measured during the calculation, we'll hit the cache and it'll go fast.

So why not move this logic up into ASCollectionLayout or ASDataController and remove the new ensure methods from the API?

- It arranges items of the same size into a multi-line stack (say photo gallery or pager). It takes advantage of the fact that its items always have a fixed size to measure as few items as possible while still being able to track their positions at all time. This helps reduce startup/reloadData time, as well as memory footprint. - It then uses a measure range, which also works as a allocate range, to figure out which items to measure ahead of time. And it guarantees that each item is scheduled to measure only once. - Lastly, ASCollectionLayoutDelegate has some new methods that allow delegates to hook up and stay ahead of layout attributes requests from the backing view. ASCollectionGalleryLayoutDelegate for example uses these methods to ensure elements that have their layout attributes requested are always ready for consumption, and to measure more elements in the background.

…ry delegate

- Collection layout delegates must have a crollable directions property. - Simplify gallery delegate by not storing unmeasured attributes since calling measure on already measured elements should be cache hits and super fast. - Abstact some code in gallery delegate to ASCollectionLayoutState+Private and _ASCollectionGalleryLayoutItem. - Other improvements in gallery delegate

nguyenhuy · 2017-07-10T18:43:18Z

@Adlai-Holler To answer your question here:

The answer is that, not all layout delegates want to calculate layout attributes for all elements at the beginning, and I don't want to enforce that. The only requirement for them is that by the time -calculateLayoutWithContext returns, there must be a content size calculated. How does a delegate come up with that number is totally up to it. It may measure and position all elements at the beginning (masonry layout), it may position all elements and measure some of them (like the gallery layout here), or it may position and measure only a very few of them (such as a pager layout).

Because of that, the elements-to-layout-attributes map after -calculateLayoutWithContext may not contain enough information to determine which elements fall into a given rect. Calling the "ensure" methods at appropriate time makes sure that the map is updated by the layout delegate. In addition, the delegate may, or may not, take advantage of that moment to measure some more elements. That's entirely up to its implementation because it's the one that knows best about its layout.

I see your point for putting the measure range logic into a place where clients can take advantage of. Later, we may consider putting it into an abstract layout delegate that gallery delegate then subclasses. I just don't want to put it in ASCollectionLayout or ASDataController because then the framework needs to make certain assumptions/restrictions on the API which in turn makes it less extensible.

nguyenhuy · 2017-07-12T16:33:49Z

@Adlai-Holler Thinking again, I agree with you that it's better to limit mutations and require clients to always return final rects for all items via -calculateLayoutWithContext:. I've updated this PR to move the measure logic to ASCollectionLayout.

I'm gonna work on preventing ASDataController from allocating nodes since either ASCollectionLayout or its delegate can handle that quite effortlessly. This work shouldn't block this PR, so please go ahead with another round of review. Thank you!

Adlai-Holler · 2017-07-13T03:29:58Z

examples/ASCollectionView/Sample/ViewController.m

@@ -44,7 +44,10 @@ - (void)viewDidLoad
 {
  [super viewDidLoad];

-  self.collectionNode = [[ASCollectionNode alloc] initWithLayoutDelegate:[[ASCollectionFlowLayoutDelegate alloc] init] layoutFacilitator:nil];
+  id<ASCollectionLayoutDelegate> layoutDelegate = [[ASCollectionGalleryLayoutDelegate alloc] initWithScrollableDirections:ASScrollDirectionVerticalDirections itemSize:CGSizeMake(180, 90)];
+//  id<ASCollectionLayoutDelegate> layoutDelegate = [[ASCollectionFlowLayoutDelegate alloc] init];


Adlai-Holler · 2017-07-13T03:31:07Z

Source/Private/ASCollectionLayout.mm

+/**
+ * Measures all elements in the specified rect and blocks the calling thread while measuring those in the blocking rect.
+ */
+ASDISPLAYNODE_INLINE void ASCollectionLayoutMeasureElementsInRects(CGRect rect, CGRect blockingRect, ASCollectionLayoutState *layout)


There's too much code in this function IMO for it to be inlined. I'd make it a plain old C function or a class method.

Yeap, changed to a class method.

Adlai-Holler · 2017-07-13T03:34:24Z

Source/Details/ASPageTable.m

+
+  for (id key in self) {
+    [result setObject:[[self objectForKey:key] copy] forKey:key];
+  }


Two performance questions here that deserve some profiling or thought:

initWithKeyOptions:valueOptions: is optimized compared to initWithKeyPointerFunctions:valuePointerFunctions:. I don't know whether the resulting storage also performs differently.

self.keyPointerFunctions and self.valuePointerFunctions both create new NSPointerFunctions objects.

Now that measurement and layoutAttributes are totally disconnected, not sure if we even need this. Right?

Valid questions!

I ended up removing this method after implementing a method that walks through the table and gathers unmeasured layout attributes, which is what I should have done in the first place.

Adlai-Holler · 2017-07-13T03:35:19Z

Source/Details/ASDataController.h

 */
- (void)prepareLayoutWithContext:(id)context;
+- (ASCollectionLayoutState *)calculateLayoutWithContext:(ASCollectionLayoutContext *)context;


What do you think about making this a class method to encourage immutable state?

I played around with this idea and in the end I actually like it. Thanks for the suggestion!

- Make -calculateLayoutWithContext: to be class methods in ASDataControllerLayoutDelegate and ASCollectionLayoutDelegate. - Add layout delegate class and layout cache to ASCollectionLayoutContext+Private, to be use by ASCollectionLayout only. - ASDataController no longer allocates all nodes but lets ASCollectionLayout determine. - Add scrollableDirections to the layout context since it's often needed by the layout pass. Otherwise users have to wrap it in an info object. - Update built-in layout delegates and CustomCollectionView example. - Publish ASHashing. It might be helpful for clients that implement custom collection info objects.

Adlai-Holler

Wonderful! I love to see the reduction in state and in information passing! Let's get this deployed and battle-tested!

ghost · 2017-07-14T18:22:39Z

	1 Warning
⚠️	This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

nguyenhuy · 2017-07-14T18:23:43Z

Awesome, thanks so much for the reviews! Pushed a 1 line change, gonna merge after BuildKite passes.

* Implement ASCollectionGalleryLayoutDelegate - It arranges items of the same size into a multi-line stack (say photo gallery or pager). It takes advantage of the fact that its items always have a fixed size to measure as few items as possible while still being able to track their positions at all time. This helps reduce startup/reloadData time, as well as memory footprint. - It then uses a measure range, which also works as a allocate range, to figure out which items to measure ahead of time. And it guarantees that each item is scheduled to measure only once. - Lastly, ASCollectionLayoutDelegate has some new methods that allow delegates to hook up and stay ahead of layout attributes requests from the backing view. ASCollectionGalleryLayoutDelegate for example uses these methods to ensure elements that have their layout attributes requested are always ready for consumption, and to measure more elements in the background. * Handle items that span multiple pages and other improvements in gallery delegate * Minor fixes * Fix failing tests * Fix custom collection example * Implement missing method in gallery layout delegate * Fix warnings * Some improvements - Collection layout delegates must have a crollable directions property. - Simplify gallery delegate by not storing unmeasured attributes since calling measure on already measured elements should be cache hits and super fast. - Abstact some code in gallery delegate to ASCollectionLayoutState+Private and _ASCollectionGalleryLayoutItem. - Other improvements in gallery delegate * Fix file licenses * Move measure range logic to ASCollectionLayout * Track unmeasured elements * Remove pending layout in ASCollectionLayout * Get back pending layout because the timing to latch new data is not ideal * Add ASCollectionLayoutCache * Fix file licenses * Fix xcodeproj * Add async collection layout to examples/ASCollectionView * Measure method in ASCollectionLayout to be a class method * Encourage more immutable states - Make -calculateLayoutWithContext: to be class methods in ASDataControllerLayoutDelegate and ASCollectionLayoutDelegate. - Add layout delegate class and layout cache to ASCollectionLayoutContext+Private, to be use by ASCollectionLayout only. - ASDataController no longer allocates all nodes but lets ASCollectionLayout determine. - Add scrollableDirections to the layout context since it's often needed by the layout pass. Otherwise users have to wrap it in an info object. - Update built-in layout delegates and CustomCollectionView example. - Publish ASHashing. It might be helpful for clients that implement custom collection info objects. * Remove additionalInfo property in ASCollectionLayoutState * ASCollectionLayoutState to correctly filter unmeasured elements * Add ASHashing to umbrella header * Fix file licenses * Add ASDispatchAsync and use it in ASCollectionLayout * Improve code comment in ASCollectionLayoutState

nguyenhuy mentioned this pull request Apr 27, 2017

Introduce ASCollectionLayout and friends facebookarchive/AsyncDisplayKit#3130

Merged

4 tasks

nguyenhuy force-pushed the HNGalleryLayoutDelegate branch 4 times, most recently from 237dd9f to 941fb2a Compare April 28, 2017 10:32

nguyenhuy mentioned this pull request Apr 28, 2017

Implement ASPageTable #81

Merged

nguyenhuy changed the title ~~Introduce ASCollectionGalleryLayoutDelegate~~ [HOLD] Introduce ASCollectionGalleryLayoutDelegate Apr 28, 2017

nguyenhuy force-pushed the HNGalleryLayoutDelegate branch 3 times, most recently from 0f39b76 to 4af0eb0 Compare May 4, 2017 12:29

nguyenhuy changed the title ~~[HOLD] Introduce ASCollectionGalleryLayoutDelegate~~ Introduce ASCollectionGalleryLayoutDelegate May 4, 2017

nguyenhuy requested review from maicki, garrettmoon, appleguy and Adlai-Holler May 4, 2017 12:35

garrettmoon mentioned this pull request May 4, 2017

Ditch the Changelog #228

Closed

Adlai-Holler reviewed May 15, 2017

View reviewed changes

nguyenhuy force-pushed the HNGalleryLayoutDelegate branch from 97732ca to 89a4d2b Compare July 6, 2017 14:54

nguyenhuy added 4 commits July 10, 2017 13:58

Handle items that span multiple pages and other improvements in galle…

ff06ad5

…ry delegate

Minor fixes

7b0c9f8

Fix failing tests

a82a0be

nguyenhuy force-pushed the HNGalleryLayoutDelegate branch from 89a4d2b to a82a0be Compare July 10, 2017 13:22

nguyenhuy added 4 commits July 10, 2017 15:16

Fix custom collection example

abec0f4

Implement missing method in gallery layout delegate

600f145

Fix warnings

a2d789e

Fix file licenses

ab98d55

TextureGroup deleted a comment Jul 12, 2017

Adlai-Holler reviewed Jul 13, 2017

View reviewed changes

nguyenhuy force-pushed the HNGalleryLayoutDelegate branch from 1115f9d to 9835fe6 Compare July 13, 2017 16:03

TextureGroup deleted a comment Jul 13, 2017

nguyenhuy force-pushed the HNGalleryLayoutDelegate branch 3 times, most recently from 068fa68 to e4ffbee Compare July 13, 2017 18:27

TextureGroup deleted a comment Jul 13, 2017

nguyenhuy added 8 commits July 14, 2017 17:55

Add async collection layout to examples/ASCollectionView

6b92d9a

Measure method in ASCollectionLayout to be a class method

25afc72

Remove additionalInfo property in ASCollectionLayoutState

7461c13

ASCollectionLayoutState to correctly filter unmeasured elements

b4df3c8

Add ASHashing to umbrella header

e7b1482

Fix file licenses

1a643cb

Add ASDispatchAsync and use it in ASCollectionLayout

5950a9f

nguyenhuy force-pushed the HNGalleryLayoutDelegate branch from e4ffbee to 5950a9f Compare July 14, 2017 16:57

Adlai-Holler approved these changes Jul 14, 2017

View reviewed changes

Improve code comment in ASCollectionLayoutState

bbca3f2

nguyenhuy merged commit 3ccc2f0 into TextureGroup:master Jul 14, 2017

nguyenhuy mentioned this pull request Jul 17, 2017

[ASCollectionLayout] Add ASCollectionGalleryLayoutSizeProviding #451

Merged

CoderXL mentioned this pull request Sep 28, 2019

ios13 -[ASTextKitContext initWithAttributedString:lineBreakMode:maximumNumberOfLines:exclusionPaths:constrainedSize:] deadlock appear in rare cases #1690

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ASCollectionGalleryLayoutDelegate #76

Introduce ASCollectionGalleryLayoutDelegate #76

nguyenhuy commented Apr 27, 2017 •

edited

Loading

nguyenhuy commented May 4, 2017

Adlai-Holler left a comment

nguyenhuy commented Jul 10, 2017

nguyenhuy commented Jul 12, 2017

Adlai-Holler Jul 13, 2017

Adlai-Holler Jul 13, 2017

nguyenhuy Jul 13, 2017

Adlai-Holler Jul 13, 2017

nguyenhuy Jul 13, 2017

Adlai-Holler Jul 13, 2017

nguyenhuy Jul 13, 2017

Adlai-Holler left a comment

ghost commented Jul 14, 2017

nguyenhuy commented Jul 14, 2017

Introduce ASCollectionGalleryLayoutDelegate #76

Introduce ASCollectionGalleryLayoutDelegate #76

Conversation

nguyenhuy commented Apr 27, 2017 • edited Loading

nguyenhuy commented May 4, 2017

Adlai-Holler left a comment

Choose a reason for hiding this comment

nguyenhuy commented Jul 10, 2017

nguyenhuy commented Jul 12, 2017

Adlai-Holler Jul 13, 2017

Choose a reason for hiding this comment

Adlai-Holler Jul 13, 2017

Choose a reason for hiding this comment

nguyenhuy Jul 13, 2017

Choose a reason for hiding this comment

Adlai-Holler Jul 13, 2017

Choose a reason for hiding this comment

nguyenhuy Jul 13, 2017

Choose a reason for hiding this comment

Adlai-Holler Jul 13, 2017

Choose a reason for hiding this comment

nguyenhuy Jul 13, 2017

Choose a reason for hiding this comment

Adlai-Holler left a comment

Choose a reason for hiding this comment

ghost commented Jul 14, 2017

nguyenhuy commented Jul 14, 2017

nguyenhuy commented Apr 27, 2017 •

edited

Loading