-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Introduce ASCollectionLayout and friends #3130
Introduce ASCollectionLayout and friends #3130
Conversation
Source/ASCollectionView.mm
Outdated
@@ -1469,6 +1485,16 @@ - (void)_beginBatchFetching | |||
|
|||
#pragma mark - ASDataControllerSource | |||
|
|||
- (id<ASCollectionViewLayoutCalculating>)layoutCalculatorInDataController:(ASDataController *)dataController | |||
{ | |||
return _collectionViewLayoutFlags.isASCollectionViewLayout ? ((ASCollectionViewLayout *)self.collectionViewLayout).calculator : 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.
These flag and cast are definitely not ideal and can be improved.
3611eef
to
9697956
Compare
This is very exciting! @master-nevi - check this out. |
a4247de
to
234aa0b
Compare
@nguyenhuy you have an amazing history of seminal contributions to AsyncDisplayKit, including the original ASLayoutSpec and crucial advancements in ASDataController. This change, along with preparatory work around flex-wrap and concurrent layout in stacks, continues this incredible tradition! I've dreamt about achieving something like this for years, but it has never been practical due to the huge amount of work behind it. Along with the equally necessary work from @Adlai-Holler on the collection and @maicki on the layout system, you all have made that dream a reality for thousands of iOS developers! Flat-out inspiring. Thank you, @maicki, @Adlai-Holler, @nguyenhuy, and Pinterest for sharing your awesome talents with the community. |
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.
It seems like there is still work in progress here, but it certainly has my accept for whenever the core team feels ready to merge this landmark! I'll start trying it out as soon as I hear word of that.
Source/ASCollectionView.mm
Outdated
return _collectionViewLayoutFlags.providesLayoutCalculator ? [(id<ASCollectionViewLayoutCalculatorProviding>)self.collectionViewLayout sizeForCalculatingCollectionViewLayout] : CGSizeZero; | ||
} | ||
|
||
#pragma mark - ASCollectionViewLayoutCalculatorResultConsuming |
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.
The specificity of this name is nice, but it remains pretty abstract as to the purpose / function of this protocol.
Should we drop "View" from all of these names, even though it would be less similar to UICollectionView names? Any other ideas on how to either simplify, or clarify the names?
This is mostly a question for the other reviewers and we should definitely not bog down this work in naming choice, but we're much less likely to revisit them after landing, so it's a good time to give it some thought.
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 like the idea of dropping "View" in these names as well. I'm about to experiment letting ASTableView
to provide a calculator but doesn't consume its result. The idea is to entirely remove the layout responsibility from ASDataController and let calculators handle it.
Source/ASCollectionView.mm
Outdated
@@ -1930,6 +1972,11 @@ - (void)layer:(CALayer *)layer didChangeBoundsWithOldValue:(CGRect)oldBounds new | |||
if (self.collectionViewLayout == nil) { | |||
return; | |||
} | |||
if (_collectionViewLayoutFlags.providesLayoutCalculator) { | |||
// TODO Maybe kick off a background calculation here and hope that the layout is ready by the time -prepareLayout |
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.
One risk of this is multiple bounds changes. We should probably schedule it to occur at the end of the runloop if anything, but that will probably mean that layout is called by then anyway. Because the main thread runs at a higher priority, I'm not sure we would gain too much from starting it early -- although there could be other work on the main thread to run before layout is flushed.
The other question is whether there are sometimes offscreen collections that have their bounds set up, but that would be wasteful to actually run the layout for. We could check for that here, but something to consider.
For the sake of simplicity and ensuring reliability before we introduce new layout kick-off points, it probably makes sense to defer this until testing indicates there are some reasonably-common use cases where this would provide an advantage.
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.
Agreed, definitely not gonna work on this in this PR, if ever. It was an idea I thought might worth considering. Gonna remove the TODO for now.
@@ -20,6 +20,7 @@ @interface ASPagerFlowLayout () { | |||
|
|||
@end | |||
|
|||
//TODO make this an ASCollectionViewLayout |
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, especially when combined with vertical flow layouts inside the pages, will be a truly revolutionary impact on performance in pin-open and pin-to-pin swiping performance.
Source/ASTableView.mm
Outdated
@@ -1581,6 +1581,11 @@ - (void)rangeController:(ASRangeController *)rangeController didUpdateWithChange | |||
|
|||
#pragma mark - ASDataControllerDelegate | |||
|
|||
- (BOOL)dataControllerShouldAutomaticallyLayoutNodes:(ASDataController *)dataController |
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.
Does this have a use case yet where NO is used? If not, should we remove it and add it once a use case exists?
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.
Ah no, this is an API I experimented with and forgot to remove :) Removing...
#import <AsyncDisplayKit/ASElementMap.h> | ||
#import <AsyncDisplayKit/ASStackLayoutSpec.h> | ||
|
||
@implementation ASCollectionViewFlowLayoutCalculator |
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 class is so impressively elegant. It's a pure expression of what the coming together of the entire core team's work has produced.
|
||
- (BOOL)shouldInvalidateLayoutForBoundsChange:(CGRect)newBounds | ||
{ | ||
if (!CGSizeEqualToSize(self.collectionView.bounds.size, newBounds.size)) { |
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'm not sure, but should this use sizeForCalculatingCollectionViewLayout in some way? Maybe the API should be sizeFor...LayoutWithBounds: to make it more functional based on the bounds, and then we could compare the current size for layout with the size that would be used once the new bounds is applied.
attrs = [UICollectionViewLayoutAttributes layoutAttributesForSupplementaryViewOfKind:element.supplementaryElementKind withIndexPath:indexPath]; | ||
} | ||
|
||
attrs.frame = CGRectMake(sublayout.position.x, sublayout.position.y, sublayout.size.width, sublayout.size.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.
For both readability and performance, suggest using a CGPoint and CGSize local variable.
Source/Details/ASElementMap.m
Outdated
- (NSArray<ASCollectionElement *> *)filteredElementsUsingBlock:(BOOL (^)(NSIndexPath * _Nonnull, ASCollectionElement * _Nonnull, BOOL * _Nonnull))filteringBlock | ||
{ | ||
NSMutableArray<ASCollectionElement *> *elements = [NSMutableArray array]; | ||
[self enumerateUsingBlock:^(NSIndexPath * _Nonnull indexPath, ASCollectionElement * _Nonnull element, BOOL * _Nonnull stop) { |
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.
Would be great to avoid block-based enumeration. Especially in our highly concurrent system, these retains and releases are the #1 most common impact on performance regressions & wins — this is notable because not only do they show up prominently in time profiles, but especially the system trace shows the runtime lock contention is the biggest cause of incomplete CPU utilization.
cc @Adlai-Holler @maicki to think of ways we can solve this more systematically (if open to the idea, I think a code style with stuff like this specifically for core code could be one step).
This should obviously not block this diff from landing; however I think it is justified to file a followup task now if we'd like to defer addressing it. We can spend the time to profile it, but it is a time investment to do the system tracing necessary to understand the impacts.
|
||
- (nullable id<ASCollectionViewLayoutCalculating>)collectionViewLayoutCalculator; | ||
|
||
- (CGSize)sizeForCalculatingCollectionViewLayout; |
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.
Is there a way to use other terms in the API, like bounds or perhaps viewport? Indeed this is not constrainedSize, but it kinda feels that way since the only places we ask for sizes from developers with layout are for constraints — so some way of disambiguating this would be nice.
boundsForCollectionLayout, viewportSizeForCollectionLayout, etc
* | ||
* @note The map might be weakly retained by the consumer for later re-calculations. | ||
*/ | ||
- (void)setLayout:(ASLayout *)layout calculatedBy:(id<ASCollectionViewLayoutCalculating>)calculator forElementMap:(ASElementMap *)map; |
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 would more naturally follow delegate conventions if lead by the calculator, e.g.
layoutCalculator:didFinishLayout:forElementMap:
7db0eeb
to
00f0fcc
Compare
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 is a great first-pass! Keep 'er going!
|
||
- (ASLayoutSpec *)layoutSpecThatFits:(ASSizeRange)sizeRange forElementMap:(ASElementMap *)map | ||
{ | ||
NSArray<ASCellNode *> *children = ASArrayByFlatMapping(map, ASCollectionElement *element, element.node); |
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.
Should we have an API for them to batch-allocate nodes in parallel, like we currently do in data controller? Maybe:
+ (void)ensureNodesAllocatedForElements:(NSArray<ASCollectionElement *> *elements);
* @discussion This method can be called on multiple theads and thus must be thread-safe (and ideally, stateless). | ||
* This method must block the calling thread but can dispatch to other theads to reduce the blocking time. | ||
*/ | ||
- (ASLayout *)layoutThatFits:(CGSize)viewportSize forElementMap:(ASElementMap *)map; |
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.
Since ASLayout requires the layout to allocate all nodes, should we have the return type here be (ASRectTable<ASCollectionElement *> *)
? In LayoutSpecCalculator
we could easily convert the ASLayout into a rect table.
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.
Also, should we pass some kind of context/helper object?
For example, we could remove the two arguments here, and the sizeRangesShouldBeProvided
flag in the protocol if we gave them something like:
ASCollectionLayoutContext {
CGSize viewportSize;
id<ASCollectionDelegate> collectionDelegate;
ASElementMap *map;
- (void)parallelAllocateNodesForElements:(id<NSFastEnumeration> elements);
// Maybe?
- (void)setFrame:(CGRect)frame forElement:(ASCollectionElement *)element;
- (BOOL)isCancelled;
}
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.
(Sorry to spam) Should this object also synchronously decide what supplementary elements exist? Invalidation is a tough question here, but as a first stab:
- (void)supplementaryElementsForMap:(ASElementMap *)map storage:(id<ASSupplementaryElementStorage>)storage {
// code
[storage addSupplementaryElementOfKind:kind atIndexPath:indexPath];
// code
}
We would call that method synchronously after the user issues each update, so that whatever supplementary elements they specify, we can go get the nodeBlocks for them as needed. Invalidating supplementary elements is a tough question and we should probably shelve it for now.
If we add that, then we can fully-replace the layout inspector.
Source/Details/ASDataController.mm
Outdated
*/ | ||
- (void)_repopulateSupplementaryNodesIntoMap:(ASMutableElementMap *)map | ||
forSectionsContainingIndexPaths:(NSArray<NSIndexPath *> *)indexPaths | ||
changeSet:(_ASHierarchyChangeSet *)changeSet | ||
environment:(id<ASTraitEnvironment>)environment | ||
indexPathsAreNew:(BOOL)indexPathsAreNew | ||
shouldFetchSizeRanges:(BOOL)shouldFetchSizeRanges |
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.
Nit: indentation
* @note Since this calculator doesn't have a layout logic, it doesn't calculate positions of collection elements. | ||
* Thus, the layout returned by the calculator is not meant to be consumed as is. | ||
*/ | ||
@interface ASLegacyCollectionLayoutCalculator : NSObject <ASCollectionLayoutCalculating> |
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.
Nit: Apply AS_SUBCLASSING_RESTRICTED
to new classes where appropriate.
#import <AsyncDisplayKit/ASLayout.h> | ||
#import <AsyncDisplayKit/ASLayoutSpec.h> | ||
|
||
@implementation ASLegacyCollectionLayoutCalculator |
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 adding this, could we say "we have no calculator – we are in 'legacy mode'" and have the data controller go ahead and perform the eager allocation/measurement step as before in a separate codepath? It's a little confusing that this thing doesn't really calculate any layout.
* @discussion This method can be called on multiple theads and thus must be thread-safe (and ideally, stateless). | ||
* This method must block the calling thread but can dispatch to other theads to reduce the blocking time. | ||
*/ | ||
- (ASLayoutSpec *)layoutSpecThatFits:(ASSizeRange)sizeRange forElementMap:(ASElementMap *)map; |
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 think we can eliminate this class and put the burden of doing this on the client, probably. For instance, in the flow layout case, it would amount to:
- (ASLayout *)layoutWithSize:(CGSize)size elementMap:(ASElementMap *)map
{
ASLayoutSpec *spec = [ASStackLayoutSpec …];
// I know I am a vertical layout, so unbounded height:
ASSizeRange sizeRange = ASSizeRangeMake(size.width, 0, size.width, INFINITY);
return [spec layoutThatFits:sizeRange];
}
- There's not much extra code.
- Gives the calculator class control over scrollable directions.
- Eliminates a class.
00f0fcc
to
5d11d82
Compare
@Adlai-Holler a few updates in this PR:
A few things left that I need time to think about:
With your trip coming really soon and the new code is non-intrusive enough that existing clients shouldn't be affected, does it make sense to merge this PR as its current state and improve it over time? |
c3499bc
to
8c0c85d
Compare
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.
@nguyenhuy Great! Thoughts, sorry if they're messy, let me know what you think:
- It's going to be extremely expensive to create attributes on the fly. The range controller is going to call
layoutAttributesForElementsInRect:
every frame.- We could have them return us an array of layout attributes, just like UICollectionViewLayout does.
- We'd add convenience methods to generate those from, say, an element map + a frame table or from a layout spec.
- We could simply generate them on-demand and then store them. I think it would be safe to assume that the consumer won't mutate the attributes (UICollectionView won't, range controller won't.)
ASCollectionElement -> UICollectionViewLayoutAttributes
- Querying these things quickly is important. UICollectionView internally uses a
screenPageMap
which isNSMapTable<(x: NSUInteger, y: NSUInteger) -> NSSet<UICollectionViewLayoutAttributes>>
wherex
andy
are the coordinates of a screen-sized rectangle. By finding all the screen pages that intersect a given rect, uniting the sets of attributes, and then filtering the final result, you can significantly cut down on the amount of time to request chunks of layout.
- We could have them return us an array of layout attributes, just like UICollectionViewLayout does.
- Since supplementaries should probably be determined by this same object as well, the name
LayoutCalculator
may be too restrictive for the future, as we think about things like partial layout querying and invalidation etc.ASCollectionLayoutDelegate
– "the object we talk to about laying out a collection" seems nice. - In the back of our minds, we may want to look to a future where we query rects-worth of layouts at a time, much like
layoutAttributesForElementsInRect:
ordrawRect:
. We shouldn't bend over backwards to accommodate this in the architecture – since YAGNI – but we should also bear it in mind. - Let's aim to land this into a Beta header soon, and we can tweak it as we bring it into production.
|
||
@class ASElementMap, ASLayout; | ||
|
||
@interface ASCollectionViewLayout : UICollectionViewLayout <ASCollectionLayoutCalculatorProviding, ASCollectionLayoutCalculatorResultConsuming> |
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 should be private
and AS_SUBCLASSING_RESTRICTED
#import <AsyncDisplayKit/ASCollectionLayoutCalculating.h> | ||
#import <AsyncDisplayKit/ASScrollDirection.h> | ||
|
||
@interface ASCollectionFlowLayoutCalculator : NSObject <ASCollectionLayoutCalculating> |
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.
Nullability, AS_SUBCLASSING_RESTRICTED
f6e9e9c
to
6b4638a
Compare
26862ae
to
28eb7e4
Compare
@Adlai-Holler @appleguy I've updated most of the code in this PR to implement the second version of this async collection layout API. More details are explained in the description of this PR. Please have another look! |
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 is shaping up well! I think that letting them talk directly to the UICollectionView might be a mistake, at least early on. Maybe instead, they provide a layoutDelegate
which implements calculateLayoutWithContext:
? We can retain layoutDelegate
for their convenience. What do you think?
* | ||
* @param elementToLayoutArrtibutesMap Map between elements to their layout attributes. The map may contain all elements, or a subset of them and to be updated later. Should use weak pointers for elements. | ||
*/ | ||
- (instancetype)initWithElementMap:(ASElementMap *)elementMap contentSize:(CGSize)contentSize elementToLayoutArrtibutesMap:(NSMapTable<ASCollectionElement *, UICollectionViewLayoutAttributes *> *)attrsMap NS_DESIGNATED_INITIALIZER; |
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.
ASLayoutToContentAttributes
may be a little obscure. Should we make a convenience initializer:
- (instancetype)initWithElementMap:(ASElementMap *)elementMap layout:(ASLayout *)layout
#import <AsyncDisplayKit/ASCollectionLayout.h> | ||
#import <AsyncDisplayKit/ASScrollDirection.h> | ||
|
||
@interface ASCollectionFlowLayout : ASCollectionLayout |
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.
Let's restrict subclassing and add nullability in this file
for (NSArray *subarray in twoDimensionalArray) { | ||
ASDisplayNodeCAssert([subarray isKindOfClass:[NSArray class]], @"This function expects NSArray<NSArray *> *"); | ||
for (id element in subarray) { | ||
[result addObject:element]; |
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.
In order to reduce calls to count
, let's track the index manually, like result[i++] = element
.
NS_ASSUME_NONNULL_BEGIN | ||
|
||
AS_SUBCLASSING_RESTRICTED | ||
@interface ASCollectionContentAttributes : NSObject |
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 name is kind of unclear IMO. ASCollectionLayoutState
?
|
||
ASCollectionContentAttributes *ASLayoutToCollectionContentAttributes(ASLayout *layout, ASElementMap *elementMap) | ||
{ | ||
NSMapTable<ASCollectionElement *, UICollectionViewLayoutAttributes *> *attrsMap = [NSMapTable weakToStrongObjectsMapTable]; |
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.
Use this initializer, so that NSMapTable will user pointer for hashing & equality:
[NSMapTable mapTableWithKeyOptions:(NSMapTableObjectPointerPersonality | NSMapTableWeakMemory) valueOptions:NSMapTableStrongMemory];
@interface ASCollectionLayout () <ASDataControllerLayoutDelegate> | ||
|
||
/// The current content, if any. This property must be accessed on main thread. | ||
@property (nonatomic, strong, nullable) ASCollectionContentAttributes *currentContentAttributes; |
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.
Are you sure we should expose this? Users can call -collectionViewContentSize
and -layoutAttributesForItemAtIndexPath:
etc to get this info, if they need it for some reason.
Source/Details/ASCollectionLayout.mm
Outdated
- (CGSize)collectionViewContentSize | ||
{ | ||
ASDisplayNodeAssertMainThread(); | ||
return _currentContentAttributes.contentSize; |
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.
Let's assert that _currentContentAttributes != nil
for good measure here.
Source/Details/ASCollectionLayout.mm
Outdated
if (! ASObjectIsEqual(_currentContentAttributes, newAttrs)) { | ||
_currentContentAttributes = newAttrs; | ||
} | ||
} |
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.
Do we need this setter? Currently isn't called and not public.
Source/Details/ASCollectionLayout.mm
Outdated
|
||
- (instancetype)init | ||
{ | ||
return [super 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.
Remove this empty override
Source/Details/ASCollectionLayout.mm
Outdated
|
||
#pragma mark - Subclass hooks | ||
|
||
- (ASCollectionContentAttributes *)calculateLayoutForLayoutContext:(ASDataControllerLayoutContext *)context |
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.
calculateLayoutWithContext:
?
@Adlai-Holler You meant an I think it's a good idea as it forbids people from accessing the collection view directly. In addition, I'd like to leave an option to subclass |
49d1262
to
4d2f2ee
Compare
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.
@nguyenhuy Left some more nitpicks and ideas, but it's almost landing time. Would you be willing to modify one of the examples to use this layout system?
Source/ASCollectionNode+Beta.h
Outdated
*/ | ||
@property (strong, nonatomic, readonly) ASElementMap *visibleMap; | ||
|
||
- (instancetype)initWithFrame:(CGRect)frame collectionLayoutDelegate:(id<ASCollectionLayoutDelegate>)layoutDelegate layoutFacilitator:(nullable id<ASCollectionViewLayoutFacilitatorProtocol>)layoutFacilitator; |
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.
Remove the frame
argument here. We should also remove it from the other initializer at some time.
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.
Should we call this layoutDelegate
to be simpler?
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.
Removed the frame and changed to layoutDelegate
.
Source/ASCollectionNode+Beta.h
Outdated
- (instancetype)initWithFrame:(CGRect)frame collectionViewLayout:(UICollectionViewLayout *)layout layoutFacilitator:(nullable id<ASCollectionViewLayoutFacilitatorProtocol>)layoutFacilitator; | ||
|
||
- (void)setCollectionLayoutDelegate:(nonnull id<ASCollectionLayoutDelegate>)collectionLayoutDelegate; | ||
|
||
- (nullable id<ASCollectionLayoutDelegate>)collectionLayoutDelegate; |
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.
Let's make this a (nullable, strong, readonly)
property for now, with this rationale:
- The Swift API is cleaner.
- We don't have to implement changing layout delegates on the fly at this early stage.
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.
Good idea! Done.
|
||
- (NSUInteger)hash | ||
{ | ||
return [_elementMap hash] ^ (((NSUInteger)(_viewportSize.width * 255) << 8) + (NSUInteger)(_viewportSize.height * 255)); |
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 believe we have like ASHashCombine
and ASArrayHash
etc that we can use here to simplify.
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.
Oh, I ended up using ASIntegerArrayHash
and implemented a new ASHashFromCGSize
function. We can make use of the new function in a couple of other places. I'll do that in another PR.
@interface ASCollectionLayoutState : NSObject | ||
|
||
/// The element map used to calculate this object | ||
@property (nonatomic, weak, readonly) ASElementMap *elementMap; |
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.
Let's make this strong
so that this object is truly immutable.
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.
My main reason for using weak
here and in ASCollectionLayoutContext
is to make sure only ASDataController
holds strong reference to the map and thus it is released as soon as ASDataController
has a new data or it's deallocated. That being said, having a strong reference here ensures that the state is valid during its lifetime and I updated the code in favor of this. The take away is that now visibleMap
is retained not only by ASDataController
but also by other objects, so we need to be careful to not leak it.
Source/Details/ASDataController.mm
Outdated
_pendingMap = newMap; | ||
|
||
// Step 3: Ask layout delegate for contexts | ||
ASCollectionLayoutContext *layoutContext; |
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.
Initialize this to nil
for good measure.
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.
Done
Source/Private/ASCollectionLayout.mm
Outdated
ASDisplayNodeAssertMainThread(); | ||
ASCollectionLayoutContext *context = [self layoutContextWithElementMap:_collectionNode.visibleMap]; | ||
|
||
ASCollectionLayoutState *state; |
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.
Initialize to 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.
Done
@property (nonatomic, assign, readonly) CGSize viewportSize; | ||
@property (nonatomic, weak, readonly) ASElementMap *elementMap; | ||
|
||
- (instancetype)initWithViewportSize:(CGSize)viewportSize elementMap:(ASElementMap *)map NS_DESIGNATED_INITIALIZER; |
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.
What do you think about an alternative to this API:
LayoutContext
is subclassing-restricted and has no public initializer.- Add an
readonly nullable id userInfo
object to it. - Reach out to the user to get the
userInfo
and attach it to the LayoutContext that we provide them.
That was the class hierarchy is shallower and the separation of control is clearer.
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.
Love this idea. Implemented!
* | ||
* @discussion This method will be called on main thread. | ||
*/ | ||
- (ASCollectionLayoutContext *)layoutContextWithElementMap:(ASElementMap *)map; |
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.
If you agree with https://github.com/facebook/AsyncDisplayKit/pull/3130/files#r110550765 then we'd modify this method to e.g.:
- (nullable id)additionalInfoForLayoutWithElements:(ASElementMap *)elements
And IMO we should make it required, so that all users know it's available. They can easily return nil
if they have nothing else to return.
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.
✅
Source/Details/ASDataController.h
Outdated
- (NSArray<NSString *> *)dataController:(ASDataController *)dataController supplementaryNodeKindsInSections:(NSIndexSet *)sections; | ||
|
||
- (NSUInteger)dataController:(ASDataController *)dataController supplementaryNodesOfKind:(NSString *)kind inSection:(NSUInteger)section; | ||
|
||
- (ASCellNodeBlock)dataController:(ASDataController *)dataController supplementaryNodeBlockOfKind:(NSString *)kind atIndexPath:(NSIndexPath *)indexPath; | ||
|
||
/** | ||
The constrained size range for layout. Called only if a collection layout calculator is not provided. |
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.
calculator
-> delegate
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.
Fixed.
9d1359a
to
0d94529
Compare
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.
Man I'm glad to hit this approve button. You really stuck with this one, @nguyenhuy. Merge it when you feel like it.
- `ASCollectionViewLayout` is an async `UICollectionViewLayout` that encapsulates its layout calculation logic into a separate thread-safe object which can be used ahead of time and/or on multiple threads. - `ASDataController` now can prepare for a new layout resulted from a change set before `ASCollectionView` even knows about it. By the time the change set it ready to be consumed by `ASCollectionView`, its new layout is also ready. - New `ASCollectionViewLayoutCalculating` protocol that is simple and generic enough that many types of calculators can be built on top. `ASCollectionViewLayoutSpecCalculator` conforms to `ASCollectionViewLayoutCalculating` protocol and can be backed by any layout spec (e.g `ASStackLayoutSpec`, `PIMasonryLayoutSpec`, etc). We can even build a `ASCollectionViewLayoutYogaCalculator` that uses Yoga internally. - A built-in `ASCollectionViewFlowLayoutCalculator` that is a subclass of `ASCollectionViewLayoutSpecCalculator` and uses a multi-threaded multi-line `ASStackLayoutSpec` internally. The result is a performant and thread-safe flow layout calculator. - Finally, `ASCollectionViewLayout` can be subclassed to handle a specific type of calculator with optimizations implemented based on the knowledge of such calculator. For example, `ASCollectionViewFlowLayout` can have a highly optimized implementation of `-layoutAttributesForElementsInRect:`. Protocolize layout calculator providing and consuming Add flex wrap documentation Add a `multithreaded` flag to ASStackLayoutSpec that forces it to dispatch even if it's off main - Update ASCollectionViewFlowLayoutSpecCalculator to use that flag. Minor change in ASCollectionViewLayout Implement Mosaic layout calculator Minor change Fix project file Rename and fix project file Skip fetching constrained size only if a layout calculator is available Update examples/ASCollectionView Remove unnecessary change in ASTableView Address comments Rename collection view calculator protocols Minor changes after rebasing with master Add ASLegacyCollectionLayoutCalculator for backward compatibility Remove ASCollectionLayoutSpecCalculator Remove ASLegacyCollectionLayoutCalculator Introduce ASCollectionLayout - A wrapper object that contains content size and an element to rect table. - Collection layout calculators to return this new object instead of an ASLayout. Before adding a content cache Finishing hooking up ASCollectionLayoutDataSource to ASCollectionNode Stash Finish ASCollectionLayout Rough impl of ASCollectionFlowLayout Revert changes in CustomCollectionView example Move ASRectTable back to Private
- Replace `-layoutContextWithElementMap:` in ASCollectionLayoutDelegate with `-additionalInfoForLayoutWithElements:`. The returned object is then stored in ASCollectionLayoutContext for later lookups. - ASCollectionLayoutContext has no public initializer. - ASDataControllerLayoutDelegate no longer requires a context of type ASCollectionLayoutContext but simply an `id`. This helps decouple ASDataController and ASCollectionLayout. - Rename `elementMap` to `elements`. - Rename `visibleMap` to `visibleElements`. - Other minor changes.
32f3cdd
to
3bb9cf9
Compare
Wooho! Thank you, @Adlai-Holler! |
* Introduce ASCollectionViewLayout - `ASCollectionViewLayout` is an async `UICollectionViewLayout` that encapsulates its layout calculation logic into a separate thread-safe object which can be used ahead of time and/or on multiple threads. - `ASDataController` now can prepare for a new layout resulted from a change set before `ASCollectionView` even knows about it. By the time the change set it ready to be consumed by `ASCollectionView`, its new layout is also ready. - New `ASCollectionViewLayoutCalculating` protocol that is simple and generic enough that many types of calculators can be built on top. `ASCollectionViewLayoutSpecCalculator` conforms to `ASCollectionViewLayoutCalculating` protocol and can be backed by any layout spec (e.g `ASStackLayoutSpec`, `PIMasonryLayoutSpec`, etc). We can even build a `ASCollectionViewLayoutYogaCalculator` that uses Yoga internally. - A built-in `ASCollectionViewFlowLayoutCalculator` that is a subclass of `ASCollectionViewLayoutSpecCalculator` and uses a multi-threaded multi-line `ASStackLayoutSpec` internally. The result is a performant and thread-safe flow layout calculator. - Finally, `ASCollectionViewLayout` can be subclassed to handle a specific type of calculator with optimizations implemented based on the knowledge of such calculator. For example, `ASCollectionViewFlowLayout` can have a highly optimized implementation of `-layoutAttributesForElementsInRect:`. Protocolize layout calculator providing and consuming Add flex wrap documentation Add a `multithreaded` flag to ASStackLayoutSpec that forces it to dispatch even if it's off main - Update ASCollectionViewFlowLayoutSpecCalculator to use that flag. Minor change in ASCollectionViewLayout Implement Mosaic layout calculator Minor change Fix project file Rename and fix project file Skip fetching constrained size only if a layout calculator is available Update examples/ASCollectionView Remove unnecessary change in ASTableView Address comments Rename collection view calculator protocols Minor changes after rebasing with master Add ASLegacyCollectionLayoutCalculator for backward compatibility Remove ASCollectionLayoutSpecCalculator Remove ASLegacyCollectionLayoutCalculator Introduce ASCollectionLayout - A wrapper object that contains content size and an element to rect table. - Collection layout calculators to return this new object instead of an ASLayout. Before adding a content cache Finishing hooking up ASCollectionLayoutDataSource to ASCollectionNode Stash Finish ASCollectionLayout Rough impl of ASCollectionFlowLayout Revert changes in CustomCollectionView example Move ASRectTable back to Private * Rename ASCollectionContentAttributes to ASCollectionLayoutState * Address other comments * Introduce ASCollectionLayoutDelegate and make ASCollectionLayout private * Address comments * API tweaks: - Replace `-layoutContextWithElementMap:` in ASCollectionLayoutDelegate with `-additionalInfoForLayoutWithElements:`. The returned object is then stored in ASCollectionLayoutContext for later lookups. - ASCollectionLayoutContext has no public initializer. - ASDataControllerLayoutDelegate no longer requires a context of type ASCollectionLayoutContext but simply an `id`. This helps decouple ASDataController and ASCollectionLayout. - Rename `elementMap` to `elements`. - Rename `visibleMap` to `visibleElements`. - Other minor changes. * Rename ASCGSizeHash to ASHashFromCGSize * Make sure to call super in -[ASCollectionLayout prepareLayout] * Update example/ASCollectionView to use ASCollectionFlowLayoutDelegate * Remove unnecessary change
Author: Thanh Huy Nguyen
v1.0 - Obsolete:
ASCollectionViewLayout
is an asynchronous and thread-safeUICollectionViewLayout
that encapsulates its layout calculation logic into a separate thread-safe object which can be used ahead of time and/or on multiple threads.ASDataController
now can prepare new layouts that are resulted from change sets beforeASCollectionView
even knows about it. By the time a change set is ready to be consumed byASCollectionView
, its new layout is also ready.ASCollectionViewLayoutCalculating
protocol that is simple and generic enough that many types of calculators can be built on top.ASCollectionViewLayoutSpecCalculator
conforms toASCollectionViewLayoutCalculating
protocol and can be backed by any layout spec (e.gASStackLayoutSpec
, etc). We can even build aASCollectionViewLayoutYogaCalculator
that uses Yoga internally.ASCollectionViewFlowLayoutCalculator
that is a subclass ofASCollectionViewLayoutSpecCalculator
and uses a multi-threaded multi-lineASStackLayoutSpec
internally. The result is a performant and thread-safe flow layout calculator.ASCollectionViewLayout
can be subclassed to handle a specific type of calculator with optimizations implemented based on the knowledge of such calculator. For example,ASCollectionViewFlowLayout
can have a highly optimized implementation of-layoutAttributesForElementsInRect:
.v2.0:
ASCollectionLayout
is an asynchronous and thread-safeUICollectionViewLayout
. It can prepare layouts ahead of time and/or on multiple threads.ASDataController
now can prepare new layouts that are resulted from change sets beforeASCollectionView
even knows about it. By the time a change set is ready to be consumed byASCollectionView
, its new layout is also ready.ASDataControllerLayoutDelegate
is a new, simple and generic protocol. If a layout delegate is set toASDataController
, it will delegate all layout operations to the delegate.ASCollectionLayout
conforms to this protocol and handles many threading issues so that its subclasses only need to implement a simple method:-calculateLayoutForLayoutContext:
. For example,ASCollectionFlowLayout
is a subclass ofASCollectionLayout
that uses a multi-line concurrentASStackLayoutSpec
to prepare its layout.ASCollectionLayout
, or even provide an independent object that conforms toASDataControllerLayoutDelegate
, to handle a specific layout with optimizations implemented based on the knowledge of such layout.v2.0.1:
ASCollectionLayout
is private and expects a layout delegate that conforms to a publicASCollectionLayoutDelegate
protocol.v2.0.2:
Issues to be followed up later - not in the scope of this PR:
ASCollectionLayout
,ASCollectionFlowLayoutDelegate
, etc) are incomplete and writing unit tests for them is not critical at this stage.ASCollectionFlowLayoutDelegate
: As mentioned above, the current implementation is a proof of concept and barely works :)ASDataControllerLayoutDelegate
that is responsible for adding supplementary elements to anASMutableElementMap
parameter.Ticket: TextureGroup/Texture#186