Improve Ancestry Handling, Avoid Assertion Failure #246
Conversation
| 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.
If we're deprecating it, shouldn't we leave it as is?
There was a problem hiding this comment.
Ah, I think you're keeping the behavior but the comment is incorrect? Same with below?
There was a problem hiding this comment.
Yep we're keeping it the same.
| @@ -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."); | |||
There was a problem hiding this comment.
should the msg also mention supernodesIncludingSelf?
There was a problem hiding this comment.
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.
| * } | ||
| * } | ||
| * | ||
| * Note: If this property is read on the main thread, the enumeration will attempt to go up |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
At the very least the note should be an @warning
There was a problem hiding this comment.
Frankly this behavior actually seems pretty crazy, are we sure we want to expose this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is a great improvement, because it allows us to inspect as much of the hierarchy is safe to explore at any given time.
There is still one small issue for #240 remaining, but it is orthogonal to this one -- accessing self.alpha of a loaded node when printing any description on a background thread. Look at the -description method in ASDisplayNode.
|
Hey great a CHANGELOG conflict I can't wait to not be able to merge this until the CI runs again. 😞 |
This is a more complete fix for #240 and supersedes #241 .
supernodessupernodesIncludingSelfandsupernodeOfClass:includingSelf:.ancestryis more precise but I think these names are more user-friendly.ASDisplayNodeFindFirstSupernode()andASDisplayNodeFindFirstSupernodeOfClass()in favor of the new category methods.ASDisplayNode.layerthat checksself.view.layer. No! We either have_layeror we need to load it, end of story.-_locked_loadViewOrLayerIsLayerBacked:to simply-_locked_loadViewOrLayer. We can just check the flag ourselves inside the method.