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.

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

Improve Ancestry Handling, Avoid Assertion Failure #246

Merged
merged 3 commits into from
May 10, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AsyncDisplayKit.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@
CC57EAF71E3939350034C595 /* ASCollectionView+Undeprecated.h in Headers */ = {isa = PBXBuildFile; fileRef = CC2E317F1DAC353700EEE891 /* ASCollectionView+Undeprecated.h */; settings = {ATTRIBUTES = (Private, ); }; };
CC57EAF81E3939450034C595 /* ASTableView+Undeprecated.h in Headers */ = {isa = PBXBuildFile; fileRef = CC512B841DAC45C60054848E /* ASTableView+Undeprecated.h */; settings = {ATTRIBUTES = (Private, ); }; };
CC58AA4B1E398E1D002C8CB4 /* ASBlockTypes.h in Headers */ = {isa = PBXBuildFile; fileRef = CC58AA4A1E398E1D002C8CB4 /* ASBlockTypes.h */; settings = {ATTRIBUTES = (Public, ); }; };
CC6AA2DA1E9F03B900978E87 /* ASDisplayNode+Ancestry.h in Headers */ = {isa = PBXBuildFile; fileRef = CC6AA2D81E9F03B900978E87 /* ASDisplayNode+Ancestry.h */; };
CC6AA2DA1E9F03B900978E87 /* ASDisplayNode+Ancestry.h in Headers */ = {isa = PBXBuildFile; fileRef = CC6AA2D81E9F03B900978E87 /* ASDisplayNode+Ancestry.h */; settings = {ATTRIBUTES = (Public, ); }; };
CC6AA2DB1E9F03B900978E87 /* ASDisplayNode+Ancestry.m in Sources */ = {isa = PBXBuildFile; fileRef = CC6AA2D91E9F03B900978E87 /* ASDisplayNode+Ancestry.m */; };
CC7FD9E11BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CC7FD9E01BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m */; };
CC7FD9E21BB603FF005CCB2B /* ASPhotosFrameworkImageRequest.h in Headers */ = {isa = PBXBuildFile; fileRef = CC7FD9DC1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.h */; settings = {ATTRIBUTES = (Public, ); }; };
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
- Move more properties from ASTableView, ASCollectionView to their respective node classes. [Adlai Holler](https://github.com/Adlai-Holler)
- Remove finalLayoutElement [Michael Schneider] (https://github.com/maicki)[#96](https://github.com/TextureGroup/Texture/pull/96)
- Add ASPageTable - A map table for fast retrieval of objects within a certain page [Huy Nguyen](https://github.com/nguyenhuy)
- Add new public `-supernodes`, `-supernodesIncludingSelf`, and `-supernodeOfClass:includingSelf:` methods. [Adlai Holler](https://github.com/Adlai-Holler)[#246](https://github.com/TextureGroup/Texture/pull/246)
- Improve our handling supernode traversal to avoid loading layers and fix assertion failures you might hit in debug. [Adlai Holler](https://github.com/Adlai-Holler)[#246](https://github.com/TextureGroup/Texture/pull/246)
- [ASDisplayNode] Pass drawParameter in rendering context callbacks [Michael Schneider](https://github.com/maicki)[#248](https://github.com/TextureGroup/Texture/pull/248)
- [ASTextNode] Move to class method of drawRect:withParameters:isCancelled:isRasterizing: for drawing [Michael Schneider] (https://github.com/maicki)[#232](https://github.com/TextureGroup/Texture/pull/232)
- [ASDisplayNode] Remove instance:-drawRect:withParameters:isCancelled:isRasterizing: (https://github.com/maicki)[#232](https://github.com/TextureGroup/Texture/pull/232)
20 changes: 7 additions & 13 deletions Source/ASDisplayNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#import <AsyncDisplayKit/ASDisplayNodeInternal.h>

#import <AsyncDisplayKit/ASDisplayNode+Ancestry.h>
#import <AsyncDisplayKit/ASDisplayNode+FrameworkSubclasses.h>
#import <AsyncDisplayKit/ASDisplayNode+Beta.h>
#import <AsyncDisplayKit/ASDisplayNode+Deprecated.h>
Expand Down Expand Up @@ -600,9 +601,9 @@ - (CALayer *)_locked_layerToLoad
return layer;
}

- (void)_locked_loadViewOrLayerIsLayerBacked:(BOOL)isLayerBacked
- (void)_locked_loadViewOrLayer
{
if (isLayerBacked) {
if (_flags.layerBacked) {
TIME_SCOPED(_debugTimeToCreateView);
_layer = [self _locked_layerToLoad];
static int ASLayerDelegateAssociationKey;
Expand Down Expand Up @@ -692,7 +693,7 @@ - (UIView *)view

// Loading a view needs to happen on the main thread
ASDisplayNodeAssertMainThread();
[self _locked_loadViewOrLayerIsLayerBacked:isLayerBacked];
[self _locked_loadViewOrLayer];

// FIXME: Ideally we'd call this as soon as the node receives -setNeedsLayout
// but automatic subnode management would require us to modify the node tree
Expand Down Expand Up @@ -727,20 +728,13 @@ - (CALayer *)layer
return _layer;
}

BOOL isLayerBacked = _flags.layerBacked;
if (!isLayerBacked) {
// No need for the lock and call the view explicitly in case it needs to be loaded first
ASDN::MutexUnlocker u(__instanceLock__);
return self.view.layer;
}

if (![self _locked_shouldLoadViewOrLayer]) {
return nil;
}

// Loading a layer needs to happen on the main thread
ASDisplayNodeAssertMainThread();
[self _locked_loadViewOrLayerIsLayerBacked:isLayerBacked];
[self _locked_loadViewOrLayer];

// FIXME: Ideally we'd call this as soon as the node receives -setNeedsLayout
// but automatic subnode management would require us to modify the node tree
Expand Down Expand Up @@ -3909,8 +3903,8 @@ - (ASEventLog *)eventLog
[result addObject:@{ @"frame" : [NSValue valueWithCGRect:_pendingViewState.frame] }];
}

// Check supernode so that if we are cell node we don't find self.
ASCellNode *cellNode = ASDisplayNodeFindFirstSupernodeOfClass(self.supernode, [ASCellNode class]);
// Check supernode so that if we are a cell node we don't find self.
ASCellNode *cellNode = [self supernodeOfClass:[ASCellNode class] includingSelf:NO];
if (cellNode != nil) {
[result addObject:@{ @"cellNode" : ASObjectDescriptionMakeTiny(cellNode) }];
}
Expand Down
4 changes: 2 additions & 2 deletions Source/ASDisplayNodeExtras.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,12 @@ extern void ASDisplayNodePerformBlockOnEverySubnode(ASDisplayNode *node, BOOL tr
/**
Given a display node, traverses up the layer tree hierarchy, returning the first display node that passes block.
*/
extern ASDisplayNode * _Nullable ASDisplayNodeFindFirstSupernode(ASDisplayNode * _Nullable node, BOOL (^block)(ASDisplayNode *node)) AS_WARN_UNUSED_RESULT;
extern ASDisplayNode * _Nullable ASDisplayNodeFindFirstSupernode(ASDisplayNode * _Nullable node, BOOL (^block)(ASDisplayNode *node)) AS_WARN_UNUSED_RESULT ASDISPLAYNODE_DEPRECATED_MSG("Use the `supernodes` property instead.");
Copy link
Member

Choose a reason for hiding this comment

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

should the msg also mention supernodesIncludingSelf?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh it's a matter of taste but I don't think so – they'll see it when they search around, and these messages are better when they're shorter.


/**
Given a display node, traverses up the layer tree hierarchy, returning the first display node of kind class.
*/
extern __kindof ASDisplayNode * _Nullable ASDisplayNodeFindFirstSupernodeOfClass(ASDisplayNode *start, Class c) AS_WARN_UNUSED_RESULT;
extern __kindof ASDisplayNode * _Nullable ASDisplayNodeFindFirstSupernodeOfClass(ASDisplayNode *start, Class c) AS_WARN_UNUSED_RESULT ASDISPLAYNODE_DEPRECATED_MSG("Use the `supernodeOfClass:includingSelf:` method instead.");

/**
* Given a layer, find the window it lives in, if any.
Expand Down
20 changes: 9 additions & 11 deletions Source/ASDisplayNodeExtras.mm
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#import <AsyncDisplayKit/ASDisplayNodeExtras.h>
#import <AsyncDisplayKit/ASDisplayNodeInternal.h>
#import <AsyncDisplayKit/ASDisplayNode+FrameworkPrivate.h>
#import <AsyncDisplayKit/ASDisplayNode+Ancestry.h>

#import <queue>
#import <AsyncDisplayKit/ASRunLoopQueue.h>
Expand Down Expand Up @@ -138,24 +139,21 @@ extern void ASDisplayNodePerformBlockOnEverySubnode(ASDisplayNode *node, BOOL tr

ASDisplayNode *ASDisplayNodeFindFirstSupernode(ASDisplayNode *node, BOOL (^block)(ASDisplayNode *node))
{
CALayer *layer = node.layer;

while (layer) {
node = ASLayerToDisplayNode(layer);
if (block(node)) {
return node;
// This function has historically started with `self` but the name suggests
// that it wouldn't. Perhaps we should change the behavior.
Copy link
Member

Choose a reason for hiding this comment

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

If we're deprecating it, shouldn't we leave it as is?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think you're keeping the behavior but the comment is incorrect? Same with below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep we're keeping it the same.

for (ASDisplayNode *ancestor in node.supernodesIncludingSelf) {
if (block(ancestor)) {
return ancestor;
}
layer = layer.superlayer;
}

return nil;
}

__kindof ASDisplayNode *ASDisplayNodeFindFirstSupernodeOfClass(ASDisplayNode *start, Class c)
{
return ASDisplayNodeFindFirstSupernode(start, ^(ASDisplayNode *n) {
return [n isKindOfClass:c];
});
// This function has historically started with `self` but the name suggests
// that it wouldn't. Perhaps we should change the behavior.
return [start supernodeOfClass:c includingSelf:YES];
}

static void _ASCollectDisplayNodes(NSMutableArray *array, CALayer *layer)
Expand Down
1 change: 1 addition & 0 deletions Source/AsyncDisplayKit.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#import <AsyncDisplayKit/ASAvailability.h>
#import <AsyncDisplayKit/ASDisplayNode.h>
#import <AsyncDisplayKit/ASDisplayNode+Ancestry.h>
#import <AsyncDisplayKit/ASDisplayNode+Convenience.h>
#import <AsyncDisplayKit/ASDisplayNodeExtras.h>

Expand Down
31 changes: 30 additions & 1 deletion Source/Base/ASDisplayNode+Ancestry.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,36 @@ NS_ASSUME_NONNULL_BEGIN

@interface ASDisplayNode (Ancestry)

- (NSEnumerator *)ancestorEnumeratorWithSelf:(BOOL)includeSelf;
/**
* Returns an object to enumerate the supernode ancestry of this node, starting with its supernode.
*
* For instance, you could write:
* for (ASDisplayNode *node in self.supernodes) {
* if ([node.backgroundColor isEqual:[UIColor blueColor]]) {
* node.hidden = YES;
* }
* }
*
* Note: If this property is read on the main thread, the enumeration will attempt to go up
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the above example code dangerous then? It's casting to an ASDisplayNode when it could be either a node or a layer? I don't think the above would crash but it's a little misleading.

Copy link
Member

Choose a reason for hiding this comment

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

At the very least the note should be an @warning

Copy link
Member

Choose a reason for hiding this comment

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

Frankly this behavior actually seems pretty crazy, are we sure we want to expose this?

Copy link
Member Author

Choose a reason for hiding this comment

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

No these are always going to be nodes. We are just traversing the layer hierarchy to find more nodes when there's a break. Same behavior that we use when traversing subnodes in ASPerformBlockOnEveryNode.

* the layer hierarchy if it finds a break in the display node hierarchy.
*/
@property (atomic, readonly) id<NSFastEnumeration> supernodes;

/**
* Same as `supernodes` but begins the enumeration with self.
*/
@property (atomic, readonly) id<NSFastEnumeration> supernodesIncludingSelf;

/**
* Searches the supernodes of this node for one matching the given class.
*
* @param supernodeClass The class of node you're looking for.
* @param includeSelf Whether to include self in the search.
* @return A node of the given class that is an ancestor of this node, or nil.
*
* @note See the documentation on `supernodes` for details about the upward traversal.
*/
- (nullable __kindof ASDisplayNode *)supernodeOfClass:(Class)supernodeClass includingSelf:(BOOL)includeSelf;

/**
* e.g. "(<MYTextNode: 0xFFFF>, <MYTextContainingNode: 0xFFFF>, <MYCellNode: 0xFFFF>)"
Expand Down
56 changes: 45 additions & 11 deletions Source/Base/ASDisplayNode+Ancestry.m
Original file line number Diff line number Diff line change
Expand Up @@ -16,46 +16,80 @@
//

#import "ASDisplayNode+Ancestry.h"
#import <AsyncDisplayKit/ASThread.h>
#import <AsyncDisplayKit/ASDisplayNodeExtras.h>

AS_SUBCLASSING_RESTRICTED
@interface ASNodeAncestryEnumerator : NSEnumerator
@end

@implementation ASNodeAncestryEnumerator {
/// Would be nice to use __unsafe_unretained but nodes can be
/// deallocated on arbitrary threads so nope.
__weak ASDisplayNode * _nextNode;
ASDisplayNode *_lastNode; // This needs to be strong because enumeration will not retain the current batch of objects
BOOL _initialState;
}

- (instancetype)initWithNode:(ASDisplayNode *)node
{
if (self = [super init]) {
_nextNode = node;
_initialState = YES;
_lastNode = node;
}
return self;
}

- (id)nextObject
{
ASDisplayNode *node = _nextNode;
_nextNode = [node supernode];
return node;
if (_initialState) {
_initialState = NO;
return _lastNode;
}

ASDisplayNode *nextNode = _lastNode.supernode;
if (nextNode == nil && ASDisplayNodeThreadIsMain()) {
CALayer *layer = _lastNode.nodeLoaded ? _lastNode.layer.superlayer : nil;
while (layer != nil) {
nextNode = ASLayerToDisplayNode(layer);
if (nextNode != nil) {
break;
}
layer = layer.superlayer;
}
}
_lastNode = nextNode;
return nextNode;
}

@end

@implementation ASDisplayNode (Ancestry)

- (NSEnumerator *)ancestorEnumeratorWithSelf:(BOOL)includeSelf
- (id<NSFastEnumeration>)supernodes
{
NSEnumerator *result = [[ASNodeAncestryEnumerator alloc] initWithNode:self];
[result nextObject]; // discard first object (self)
return result;
}

- (id<NSFastEnumeration>)supernodesIncludingSelf
{
ASDisplayNode *node = includeSelf ? self : self.supernode;
return [[ASNodeAncestryEnumerator alloc] initWithNode:node];
return [[ASNodeAncestryEnumerator alloc] initWithNode:self];
}

- (nullable __kindof ASDisplayNode *)supernodeOfClass:(Class)supernodeClass includingSelf:(BOOL)includeSelf
{
id<NSFastEnumeration> chain = includeSelf ? self.supernodesIncludingSelf : self.supernodes;
for (ASDisplayNode *ancestor in chain) {
if ([ancestor isKindOfClass:supernodeClass]) {
return ancestor;
}
}
return nil;
}

- (NSString *)ancestryDescription
{
NSMutableArray *strings = [NSMutableArray array];
for (ASDisplayNode *node in [self ancestorEnumeratorWithSelf:YES]) {
for (ASDisplayNode *node in self.supernodes) {
[strings addObject:ASObjectDescriptionMakeTiny(node)];
}
return strings.description;
Expand Down