-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[ASDisplayNode] Add automatic measure before layout #1673
Changes from all 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 |
|---|---|---|
|
|
@@ -48,8 +48,11 @@ - (void)_staticInitialize; | |
|
|
||
| @end | ||
|
|
||
| //#define LOG(...) NSLog(__VA_ARGS__) | ||
| #define LOG(...) | ||
| #if ASDisplayNodeLoggingEnabled | ||
| #define LOG(...) NSLog(__VA_ARGS__) | ||
| #else | ||
| #define LOG(...) | ||
| #endif | ||
|
|
||
| // Conditionally time these scopes to our debug ivars (only exist in debug/profile builds) | ||
| #if TIME_DISPLAYNODE_OPS | ||
|
|
@@ -1074,19 +1077,54 @@ - (void)__layout | |
| ASDisplayNodeAssertMainThread(); | ||
| ASDN::MutexLocker l(_propertyLock); | ||
| CGRect bounds = self.bounds; | ||
| if (CGRectEqualToRect(bounds, CGRectZero)) { | ||
| // Performing layout on a zero-bounds view often results in frame calculations | ||
| // with negative sizes after applying margins, which will cause | ||
| // measureWithSizeRange: on subnodes to assert. | ||
|
|
||
| [self measureNodeWithBoundsIfNecessary:bounds]; | ||
|
|
||
| // Performing layout on a zero-bounds view often results in frame calculations | ||
| // with negative sizes after applying margins, which will cause | ||
| // measureWithSizeRange: on subnodes to assert. | ||
| if (!CGRectEqualToRect(bounds, CGRectZero)) { | ||
| _placeholderLayer.frame = bounds; | ||
| [self layout]; | ||
| [self layoutDidFinish]; | ||
| } | ||
| } | ||
|
|
||
| - (void)measureNodeWithBoundsIfNecessary:(CGRect)bounds | ||
| { | ||
| // Normally measure will be called before layout occurs. If this doesn't happen, nothing is going to call it at all. | ||
| // We simply call measureWithSizeRange: using a size range equal to whatever bounds were provided to that element or | ||
| // try to measure the node with the largest size as possible | ||
| if (self.supernode == nil && !self.supportsRangeManagedInterfaceState && !_flags.isMeasured) { | ||
|
Contributor
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. I don't think we should check that there is no parent. It is very possible in practice for an element to be included in manual layout, but not be present in any layout specification for its parent. @maicki it is possible I am overlooking something that you have considered, feel free to let me know if you think we should keep that part of the condition. I actually have exactly the same question about the range managed check. Assuming an element is excluded from the layout specification, perhaps accidentally. It would be better for something to ultimately trigger a measure pass even if it appears in the upper left of the view by default. This could help developers understand that they forgot to position the view with the layout spec |
||
| if (CGRectEqualToRect(bounds, CGRectZero)) { | ||
| LOG(@"Warning: No size given for node before node was trying to layout itself: %@. Please provide a frame for the node.", self); | ||
| } else { | ||
| [self measureWithSizeRange:ASSizeRangeMake(CGSizeZero, bounds.size)]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| - (void)layout | ||
| { | ||
| ASDisplayNodeAssertMainThread(); | ||
|
|
||
| if (!_flags.isMeasured) { | ||
| return; | ||
| } | ||
| _placeholderLayer.frame = bounds; | ||
| [self layout]; | ||
| [self layoutDidFinish]; | ||
|
|
||
| [self __layoutSublayouts]; | ||
| } | ||
|
|
||
| - (void)__layoutSublayouts | ||
| { | ||
| for (ASLayout *subnodeLayout in _layout.immediateSublayouts) { | ||
| ((ASDisplayNode *)subnodeLayout.layoutableObject).frame = [subnodeLayout frame]; | ||
| } | ||
| } | ||
|
|
||
| - (void)layoutDidFinish | ||
| { | ||
| // Hook for subclasses | ||
| } | ||
|
|
||
| - (CATransform3D)_transformToAncestor:(ASDisplayNode *)ancestor | ||
|
|
@@ -2369,24 +2407,6 @@ - (void)applyLayout:(ASLayout *)layout | |
| } | ||
| } | ||
|
|
||
| - (void)layout | ||
| { | ||
| ASDisplayNodeAssertMainThread(); | ||
|
|
||
| if (!_flags.isMeasured) { | ||
| return; | ||
| } | ||
|
|
||
| [self __layoutSublayouts]; | ||
| } | ||
|
|
||
| - (void)__layoutSublayouts | ||
| { | ||
| for (ASLayout *subnodeLayout in _layout.immediateSublayouts) { | ||
| ((ASDisplayNode *)subnodeLayout.layoutableObject).frame = [subnodeLayout frame]; | ||
| } | ||
| } | ||
|
|
||
| - (void)displayWillStart | ||
| { | ||
| ASDisplayNodeAssertMainThread(); | ||
|
|
||
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 probably the right trade-off, but unfortunately it can result in some side effects. Specifically, an element that does not enable clipping would normally be able to show contents it holds even if it is zero sized, but this depends on the layout pass occurring.
This condition should improve efficiency (a bit, in some cases) and is probably best to keep, but we should be aware of this trade-off and consider if it is necessary to document somewhere
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 refer to the
CGRectEqualToRectline ... it was already in there, just change the logic around it a bit. What would you recommend to add or where should we document it?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 see, indeed I had missed that it was there before. The issue is pretty subtle and so it is unclear how to effectively documented, so let's allow this one to stay in, until we see at least one example of a real app use case that exposes it.