-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're deprecating it, shouldn't we leave it as is? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the very least the note should be an There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>)" | ||
|
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 the msg also mention
supernodesIncludingSelf
?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.
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.