From 2675204d5031fc776700bdfee8b3a05f133b2c8c Mon Sep 17 00:00:00 2001 From: ricky Date: Tue, 7 Jun 2016 10:07:59 -0700 Subject: [PATCH 1/3] [ASLayoutSpec.mm] Remove use of dictionary to hold children Converted the backing store of children to a std::vector. Subclass now have defined indexes instead of string keys to add children. --- .../Layout/ASBackgroundLayoutSpec.mm | 9 +-- AsyncDisplayKit/Layout/ASLayoutSpec.h | 16 ++--- AsyncDisplayKit/Layout/ASLayoutSpec.mm | 63 +++++++------------ AsyncDisplayKit/Layout/ASOverlayLayoutSpec.mm | 9 +-- 4 files changed, 41 insertions(+), 56 deletions(-) diff --git a/AsyncDisplayKit/Layout/ASBackgroundLayoutSpec.mm b/AsyncDisplayKit/Layout/ASBackgroundLayoutSpec.mm index e060aab0c8..00201a79b9 100644 --- a/AsyncDisplayKit/Layout/ASBackgroundLayoutSpec.mm +++ b/AsyncDisplayKit/Layout/ASBackgroundLayoutSpec.mm @@ -14,7 +14,8 @@ #import "ASBaseDefines.h" #import "ASLayout.h" -static NSString * const kBackgroundChildKey = @"kBackgroundChildKey"; +static NSUInteger const kForegroundChildIndex = 0; +static NSUInteger const kBackgroundChildIndex = 1; @interface ASBackgroundLayoutSpec () @end @@ -28,7 +29,7 @@ static NSString * const kBackgroundChildKey = @"kBackgroundChildKey"; } ASDisplayNodeAssertNotNil(child, @"Child cannot be nil"); - [self setChild:child]; + [self setChild:child forIndex:kForegroundChildIndex]; self.background = background; return self; } @@ -63,12 +64,12 @@ static NSString * const kBackgroundChildKey = @"kBackgroundChildKey"; - (void)setBackground:(id)background { - [super setChild:background forIdentifier:kBackgroundChildKey]; + [super setChild:background forIndex:kBackgroundChildIndex]; } - (id)background { - return [super childForIdentifier:kBackgroundChildKey]; + return [super childForIndex:kBackgroundChildIndex]; } @end diff --git a/AsyncDisplayKit/Layout/ASLayoutSpec.h b/AsyncDisplayKit/Layout/ASLayoutSpec.h index 32e2d43779..7d7e4bc5d4 100644 --- a/AsyncDisplayKit/Layout/ASLayoutSpec.h +++ b/AsyncDisplayKit/Layout/ASLayoutSpec.h @@ -52,19 +52,19 @@ NS_ASSUME_NONNULL_BEGIN * * @param child A child to be added. * - * @param identifier An identifier associated with the child. + * @param index An index associated with the child. * * @discussion Every ASLayoutSpec must act on at least one child. The ASLayoutSpec base class takes the * responsibility of holding on to the spec children. Some layout specs, like ASInsetLayoutSpec, * only require a single child. * * For layout specs that require a known number of children (ASBackgroundLayoutSpec, for example) - * a subclass should use the setChild method to set the "primary" child. It can then use this method + * a subclass can use the setChild method to set the "primary" child. It should then use this method * to set any other required children. Ideally a subclass would hide this from the user, and use the - * setChild:forIdentifier: internally. For example, ASBackgroundLayoutSpec exposes a backgroundChild - * property that behind the scenes is calling setChild:forIdentifier:. + * setChild:forIndex: internally. For example, ASBackgroundLayoutSpec exposes a backgroundChild + * property that behind the scenes is calling setChild:forIndex:. */ -- (void)setChild:(id)child forIdentifier:(NSString *)identifier; +- (void)setChild:(id)child forIndex:(NSUInteger)index; /** * Adds childen to this layout spec. @@ -94,11 +94,11 @@ NS_ASSUME_NONNULL_BEGIN - (nullable id)child; /** - * Returns the child added to this layout spec using the given identifier. + * Returns the child added to this layout spec using the given index. * - * @param identifier An identifier associated withe the child. + * @param index An identifier associated withe the child. */ -- (nullable id)childForIdentifier:(NSString *)identifier; +- (nullable id)childForIndex:(NSUInteger)index; /** * Returns all children added to this layout spec. diff --git a/AsyncDisplayKit/Layout/ASLayoutSpec.mm b/AsyncDisplayKit/Layout/ASLayoutSpec.mm index e7eb57bae0..db522daa4f 100644 --- a/AsyncDisplayKit/Layout/ASLayoutSpec.mm +++ b/AsyncDisplayKit/Layout/ASLayoutSpec.mm @@ -25,9 +25,7 @@ @interface ASLayoutSpec() { ASEnvironmentState _environmentState; ASDN::RecursiveMutex _propertyLock; - - NSArray *_children; - NSMutableDictionary *_childrenWithIdentifier; + std::vector> _children; } @end @@ -45,7 +43,6 @@ } _isMutable = YES; _environmentState = ASEnvironmentStateMakeDefault(); - _children = [NSArray array]; return self; } @@ -102,14 +99,6 @@ return child; } -- (NSMutableDictionary *)childrenWithIdentifier -{ - if (!_childrenWithIdentifier) { - _childrenWithIdentifier = [NSMutableDictionary dictionary]; - } - return _childrenWithIdentifier; -} - - (void)setParent:(id)parent { // FIXME: Locking should be evaluated here. _parent is not widely used yet, though. @@ -126,34 +115,29 @@ if (child) { id finalLayoutable = [self layoutableToAddFromLayoutable:child]; if (finalLayoutable) { - _children = @[finalLayoutable]; + _children.insert(_children.begin(), finalLayoutable); [self propagateUpLayoutable:finalLayoutable]; } - } else { - // remove the only child - _children = [NSArray array]; + } else if (_children.size() > 0) { + _children.erase(_children.begin()); } } -- (void)setChild:(id)child forIdentifier:(NSString *)identifier +- (void)setChild:(id)child forIndex:(NSUInteger)index { ASDisplayNodeAssert(self.isMutable, @"Cannot set properties when layout spec is not mutable"); if (child) { id finalLayoutable = [self layoutableToAddFromLayoutable:child]; - self.childrenWithIdentifier[identifier] = finalLayoutable; - if (finalLayoutable) { - _children = [_children arrayByAddingObject:finalLayoutable]; + if (index > _children.size()) { + _children.insert(_children.end(), finalLayoutable); + } else { + _children.insert(_children.begin() + index, finalLayoutable); } } else { - id oldChild = self.childrenWithIdentifier[identifier]; - if (oldChild) { - self.childrenWithIdentifier[identifier] = nil; - NSMutableArray *mutableChildren = [_children mutableCopy]; - [mutableChildren removeObject:oldChild]; - _children = [mutableChildren copy]; + if (_children.begin() + index < _children.end()) { + _children.erase(_children.begin() + index); } } - // TODO: Should we propagate up the layoutable at it could happen that multiple children will propagated up their // layout options and one child will overwrite values from another child // [self propagateUpLayoutable:finalLayoutable]; @@ -163,32 +147,31 @@ { ASDisplayNodeAssert(self.isMutable, @"Cannot set properties when layout spec is not mutable"); - std::vector> finalChildren; + _children.clear(); for (id child in children) { - finalChildren.push_back([self layoutableToAddFromLayoutable:child]); - } - - _children = nil; - if (finalChildren.size() > 0) { - _children = [NSArray arrayWithObjects:&finalChildren[0] count:finalChildren.size()]; - } else { - _children = [NSArray array]; + _children.push_back([self layoutableToAddFromLayoutable:child]); } } -- (id)childForIdentifier:(NSString *)identifier +- (id)childForIndex:(NSUInteger)index { - return self.childrenWithIdentifier[identifier]; + if (index < _children.size()) { + return _children[index]; + } + return nil; } - (id)child { - return [_children firstObject]; + if (_children.size() > 0) { + return _children[0]; + } + return nil; } - (NSArray *)children { - return _children; + return [NSArray arrayWithObjects:&_children[0] count:_children.size()]; } #pragma mark - ASEnvironment diff --git a/AsyncDisplayKit/Layout/ASOverlayLayoutSpec.mm b/AsyncDisplayKit/Layout/ASOverlayLayoutSpec.mm index 35a686db3b..669a5ad9eb 100644 --- a/AsyncDisplayKit/Layout/ASOverlayLayoutSpec.mm +++ b/AsyncDisplayKit/Layout/ASOverlayLayoutSpec.mm @@ -14,7 +14,8 @@ #import "ASBaseDefines.h" #import "ASLayout.h" -static NSString * const kOverlayChildKey = @"kOverlayChildKey"; +static NSUInteger const kUnderlayChildIndex = 0; +static NSUInteger const kOverlayChildIndex = 1; @implementation ASOverlayLayoutSpec @@ -25,7 +26,7 @@ static NSString * const kOverlayChildKey = @"kOverlayChildKey"; } ASDisplayNodeAssertNotNil(child, @"Child that will be overlayed on shouldn't be nil"); self.overlay = overlay; - [self setChild:child]; + [self setChild:child forIndex:kUnderlayChildIndex]; return self; } @@ -36,12 +37,12 @@ static NSString * const kOverlayChildKey = @"kOverlayChildKey"; - (void)setOverlay:(id)overlay { - [super setChild:overlay forIdentifier:kOverlayChildKey]; + [super setChild:overlay forIndex:kOverlayChildIndex]; } - (id)overlay { - return [super childForIdentifier:kOverlayChildKey]; + return [super childForIndex:kOverlayChildIndex]; } /** From eb715e5836d673a5e139838bc7055406a870e528 Mon Sep 17 00:00:00 2001 From: ricky Date: Tue, 21 Jun 2016 14:04:48 -0700 Subject: [PATCH 2/3] hold children in a map instead of vector --- AsyncDisplayKit/Layout/ASLayoutSpec.mm | 37 +++++++++++++------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/AsyncDisplayKit/Layout/ASLayoutSpec.mm b/AsyncDisplayKit/Layout/ASLayoutSpec.mm index db522daa4f..a366cc5848 100644 --- a/AsyncDisplayKit/Layout/ASLayoutSpec.mm +++ b/AsyncDisplayKit/Layout/ASLayoutSpec.mm @@ -20,12 +20,15 @@ #import "ASTraitCollection.h" #import +#import #import +typedef std::map, std::less> ASChildMap; + @interface ASLayoutSpec() { ASEnvironmentState _environmentState; ASDN::RecursiveMutex _propertyLock; - std::vector> _children; + ASChildMap _children; } @end @@ -115,11 +118,11 @@ if (child) { id finalLayoutable = [self layoutableToAddFromLayoutable:child]; if (finalLayoutable) { - _children.insert(_children.begin(), finalLayoutable); + _children[0] = finalLayoutable; [self propagateUpLayoutable:finalLayoutable]; } } else if (_children.size() > 0) { - _children.erase(_children.begin()); + _children.erase(0); } } @@ -128,15 +131,9 @@ ASDisplayNodeAssert(self.isMutable, @"Cannot set properties when layout spec is not mutable"); if (child) { id finalLayoutable = [self layoutableToAddFromLayoutable:child]; - if (index > _children.size()) { - _children.insert(_children.end(), finalLayoutable); - } else { - _children.insert(_children.begin() + index, finalLayoutable); - } + _children[index] = finalLayoutable; } else { - if (_children.begin() + index < _children.end()) { - _children.erase(_children.begin() + index); - } + _children.erase(index); } // TODO: Should we propagate up the layoutable at it could happen that multiple children will propagated up their // layout options and one child will overwrite values from another child @@ -148,9 +145,9 @@ ASDisplayNodeAssert(self.isMutable, @"Cannot set properties when layout spec is not mutable"); _children.clear(); - for (id child in children) { - _children.push_back([self layoutableToAddFromLayoutable:child]); - } + [children enumerateObjectsUsingBlock:^(id _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) { + _children[idx] = obj; + }]; } - (id)childForIndex:(NSUInteger)index @@ -163,15 +160,17 @@ - (id)child { - if (_children.size() > 0) { - return _children[0]; - } - return nil; + return _children[0]; } - (NSArray *)children { - return [NSArray arrayWithObjects:&_children[0] count:_children.size()]; + std::vector children; + for (ASChildMap::iterator it = _children.begin(); it != _children.end(); ++it ) { + children.push_back(it->second); + } + + return [NSArray arrayWithObjects:&children[0] count:children.size()]; } #pragma mark - ASEnvironment From af98d23bf01fbb919492cb79a4e3d277749159b6 Mon Sep 17 00:00:00 2001 From: ricky Date: Tue, 21 Jun 2016 14:48:43 -0700 Subject: [PATCH 3/3] michael's comments --- AsyncDisplayKit/Layout/ASLayoutSpec.h | 7 +++---- AsyncDisplayKit/Layout/ASLayoutSpec.mm | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/AsyncDisplayKit/Layout/ASLayoutSpec.h b/AsyncDisplayKit/Layout/ASLayoutSpec.h index 7d7e4bc5d4..97f9fe1ca8 100644 --- a/AsyncDisplayKit/Layout/ASLayoutSpec.h +++ b/AsyncDisplayKit/Layout/ASLayoutSpec.h @@ -40,10 +40,9 @@ NS_ASSUME_NONNULL_BEGIN * only require a single child. * * For layout specs that require a known number of children (ASBackgroundLayoutSpec, for example) - * a subclass should use this method to set the "primary" child. It can then use setChild:forIdentifier: - * to set any other required children. Ideally a subclass would hide this from the user, and use the - * setChild:forIdentifier: internally. For example, ASBackgroundLayoutSpec exposes a backgroundChild - * property that behind the scenes is calling setChild:forIdentifier:. + * a subclass should use this method to set the "primary" child. This is actually the same as calling + * setChild:forIdentifier:0. All other children should be set by defining convenience methods + * that call setChild:forIdentifier behind the scenes. */ - (void)setChild:(id)child; diff --git a/AsyncDisplayKit/Layout/ASLayoutSpec.mm b/AsyncDisplayKit/Layout/ASLayoutSpec.mm index a366cc5848..b0bd8ee8ed 100644 --- a/AsyncDisplayKit/Layout/ASLayoutSpec.mm +++ b/AsyncDisplayKit/Layout/ASLayoutSpec.mm @@ -121,7 +121,7 @@ typedef std::map, std::less> ASCh _children[0] = finalLayoutable; [self propagateUpLayoutable:finalLayoutable]; } - } else if (_children.size() > 0) { + } else { _children.erase(0); } }