Skip to content
This repository was archived by the owner on Feb 2, 2023. It is now read-only.
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: 2 additions & 0 deletions AsyncDisplayKit/ASDisplayNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#import <AsyncDisplayKit/ASAsciiArtBoxCreator.h>
#import <AsyncDisplayKit/ASLayoutable.h>

#define ASDisplayNodeLoggingEnabled 0

@class ASDisplayNode;

/**
Expand Down
74 changes: 47 additions & 27 deletions AsyncDisplayKit/ASDisplayNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

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

Copy link
Contributor Author

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 CGRectEqualToRect line ... 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?

Copy link
Contributor

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.

_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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions examples/Videos/Sample/ViewController.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
* ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
* CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
#import <AsyncDisplayKit/AsyncDisplayKit.h>
#import <AsyncDisplayKit/ASVideoNode.h>

#include <UIKit/UIKit.h>

@interface ViewController : UIViewController

Expand Down
28 changes: 14 additions & 14 deletions examples/Videos/Sample/ViewController.m
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
*/

#import "ViewController.h"
#import "ASLayoutSpec.h"
#import "ASStaticLayoutSpec.h"
#import <AsyncDisplayKit/AsyncDisplayKit.h>

@interface ViewController()<ASVideoNodeDelegate>
@property (nonatomic, strong) ASDisplayNode *rootNode;
Expand All @@ -22,12 +21,23 @@ @implementation ViewController

#pragma mark - UIViewController

- (void)viewWillAppear:(BOOL)animated
- (instancetype)initWithNibName:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil
{
[super viewWillAppear:animated];
self = [super initWithNibName:nil bundle:nil];
if (self) {


}
return self;
}

- (void)viewDidLoad
{
[super viewDidLoad];

// Root node for the view controller
_rootNode = [ASDisplayNode new];
_rootNode.frame = self.view.bounds;
_rootNode.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight;

ASVideoNode *guitarVideoNode = self.guitarVideoNode;
Expand All @@ -54,16 +64,6 @@ - (void)viewWillAppear:(BOOL)animated
[self.view addSubnode:_rootNode];
}

- (void)viewDidLayoutSubviews
{
[super viewDidLayoutSubviews];

// After all subviews are layed out we have to measure it and move the root node to the right place
CGSize viewSize = self.view.bounds.size;
[self.rootNode measureWithSizeRange:ASSizeRangeMake(viewSize, viewSize)];
[self.rootNode setNeedsLayout];
}

#pragma mark - Getter / Setter

- (ASVideoNode *)guitarVideoNode;
Expand Down