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

[ASDisplayNode] Provide safeAreaInsets and layoutMargins bridge #685

Merged
merged 13 commits into from
Mar 27, 2018
Merged
Show file tree
Hide file tree
Changes from 5 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## master
* Add your own contributions to the next release on the line below this with your name.
- [ASDisplayNode] Add safeAreaInsets, layoutMargins and related properties to ASDisplayNode [Yevgen Pogribnyi](https://github.com/ypogribnyi)
- [ASRectMap] Replace implementation of ASRectTable with a simpler one based on unordered_map.[Scott Goodson](https://github.com/appleguy) [#719](https://github.com/TextureGroup/Texture/pull/719)
- [ASCollectionView] Add missing flags for ASCollectionDelegate [Ilya Zheleznikov](https://github.com/ilyailya) [#718](https://github.com/TextureGroup/Texture/pull/718)
- [ASNetworkImageNode] Deprecates .URLs in favor of .URL [Garrett Moon](https://github.com/garrettmoon) [#699](https://github.com/TextureGroup/Texture/pull/699)
Expand Down
25 changes: 25 additions & 0 deletions Source/ASDisplayNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,20 @@ extern NSInteger const ASDefaultDrawingPriority;
*/
@property (nonatomic, readonly) BOOL supportsLayerBacking;

/**
* Whether or not the node layout should be automatically updated when it receives safeAreaInsetsDidChange.
*
* Defaults to NO.
*/
@property (nonatomic, assign) BOOL automaticallyRelayoutOnSafeAreaChanges;

/**
* Whether or not the node layout should be automatically updated when it receives layoutMarginsDidChange.
*
* Defaults to NO.
*/
@property (nonatomic, assign) BOOL automaticallyRelayoutOnLayoutMarginsChanges;

@end

/**
Expand Down Expand Up @@ -719,6 +733,17 @@ extern NSInteger const ASDefaultDrawingPriority;
@property (nonatomic, assign) BOOL autoresizesSubviews; // default==YES (undefined for layer-backed nodes)
@property (nonatomic, assign) UIViewAutoresizing autoresizingMask; // default==UIViewAutoresizingNone (undefined for layer-backed nodes)

// Content margins
@property (nonatomic, assign) UIEdgeInsets layoutMargins;
Copy link
Member

Choose a reason for hiding this comment

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

Regarding documentation, I think it's good to remind users that if they access this property, as well as safeAreaInsets, during a layout pass, they should enable automaticallyRelayoutOnSafeAreaChanges and automaticallyRelayoutOnLayoutMarginsChanges. That is because their layout relies on these properties.

@property (nonatomic, assign) BOOL preservesSuperviewLayoutMargins; // default is NO - set to enable pass-through or cascading behavior of margins from this view’s parent to its children
- (void)layoutMarginsDidChange;

// Safe area
@property (nonatomic, readonly) UIEdgeInsets safeAreaInsets;
@property (nonatomic, assign) BOOL insetsLayoutMarginsFromSafeArea; // Default: YES
- (void)safeAreaInsetsDidChange;


// UIResponder methods
// By default these fall through to the underlying view, but can be overridden.
- (BOOL)canBecomeFirstResponder; // default==NO
Expand Down
100 changes: 100 additions & 0 deletions Source/ASDisplayNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,14 @@ - (void)_initializeInstance

_flags.canClearContentsOfLayer = YES;
_flags.canCallSetNeedsDisplayOfLayer = YES;

_fallbackSafeAreaInsets = UIEdgeInsetsZero;
_fallbackInsetsLayoutMarginsFromSafeArea = YES;
_isViewControllerRoot = NO;

_automaticallyRelayoutOnSafeAreaChanges = NO;
_automaticallyRelayoutOnLayoutMarginsChanges = NO;

ASDisplayNodeLogEvent(self, @"init");
}

Expand Down Expand Up @@ -829,6 +837,96 @@ - (void)nodeViewDidAddGestureRecognizer
_flags.viewEverHadAGestureRecognizerAttached = YES;
}

- (UIEdgeInsets)fallbackSafeAreaInsets
{
ASDN::MutexLocker l(__instanceLock__);
return _fallbackSafeAreaInsets;
}

- (void)setFallbackSafeAreaInsets:(UIEdgeInsets)insets
{
{
ASDN::MutexLocker l(__instanceLock__);
ASDisplayNodeAssertThreadAffinity(self);

if (UIEdgeInsetsEqualToEdgeInsets(insets, _fallbackSafeAreaInsets)) {
return;
}

_fallbackSafeAreaInsets = insets;
}

BOOL needsManualUpdate = !AS_AT_LEAST_IOS11 || _flags.layerBacked;
Copy link
Member

Choose a reason for hiding this comment

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

Warning: accessing _flags without instance lock. Let's move this line into the scope above.

BOOL updatesLayoutMargins = needsManualUpdate && self.insetsLayoutMarginsFromSafeArea;
Copy link
Member

Choose a reason for hiding this comment

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

Calling self.insetsLayoutMarginsFromSafeArea is fine here as the getter acquires the lock itself. However, given the fact that we did acquire the lock just a moment ago (in the scope above), let's try to grab it during that time. One common pattern we use frequently is to add an internal _locked_ version of the getter. See isNodeLoaded and _locked_isNodeLoaded as an example.


if (needsManualUpdate) {
[self safeAreaInsetsDidChange];
}

if (updatesLayoutMargins) {
[self layoutMarginsDidChange];
}
}

- (void)_fallbackUpdateSafeAreaOnChildren
Copy link
Member

Choose a reason for hiding this comment

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

This method is called by __layout which means it runs on every frame and must be treated as a hot path. I haven't profiled to see how fast/slow it is, but I think it's worthy to try to add some straightforward optimizations right from the beginning.

One optimization I can think of is that, since fallback safe area insets are only used for backward compatibility, we can bail early/skip a child whenever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a shortcut that skips a child if it is a view-backed node and we run on iOS 11. And -[ASDisplayNode setFallbackSafeAreaInsets:] returns early when the insets don't change.

Although we could try to skip calculations for the children that use fallback safe area insets but don't really need to have them updated (e.g. neither the child's frame, the parent's frame nor the parent's safe area wasn't updated), I doubt that it would save more CPU time than would spend running the checks.

The other thing I'm thinking about is that on iOS 11 we can disable fallback logic completely if we avoid supporting the safe area for the layer-backed nodes. But I'm not sure if we should do that.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the note. I think this implementation and your trade-offs are reasonable. Let's roll with this.

{
ASDisplayNodeAssertThreadAffinity(self);

UIEdgeInsets insets = self.safeAreaInsets;
CGRect bounds = self.bounds;

for (ASDisplayNode *child in self.subnodes) {
if (child.viewControllerRoot) {
// Its safe area is controlled by a view controller. Don't override it.
continue;
}

CGRect childFrame = child.frame;
UIEdgeInsets childInsets = UIEdgeInsetsMake(MAX(insets.top - (CGRectGetMinY(childFrame) - CGRectGetMinY(bounds)), 0),
MAX(insets.left - (CGRectGetMinX(childFrame) - CGRectGetMinX(bounds)), 0),
MAX(insets.bottom - (CGRectGetMaxY(bounds) - CGRectGetMaxY(childFrame)), 0),
MAX(insets.right - (CGRectGetMaxX(bounds) - CGRectGetMaxX(childFrame)), 0));

child.fallbackSafeAreaInsets = childInsets;
}
}

- (BOOL)isViewControllerRoot
{
ASDN::MutexLocker l(__instanceLock__);
return _isViewControllerRoot;
}

- (void)setViewControllerRoot:(BOOL)flag
{
ASDN::MutexLocker l(__instanceLock__);
_isViewControllerRoot = flag;
}

- (BOOL)automaticallyRelayoutOnSafeAreaChanges
{
ASDN::MutexLocker l(__instanceLock__);
return _automaticallyRelayoutOnSafeAreaChanges;
}

- (void)setAutomaticallyRelayoutOnSafeAreaChanges:(BOOL)flag
{
ASDN::MutexLocker l(__instanceLock__);
_automaticallyRelayoutOnSafeAreaChanges = flag;
}

- (BOOL)automaticallyRelayoutOnLayoutMarginsChanges
{
ASDN::MutexLocker l(__instanceLock__);
return _automaticallyRelayoutOnLayoutMarginsChanges;
}

- (void)setAutomaticallyRelayoutOnLayoutMarginsChanges:(BOOL)flag
{
ASDN::MutexLocker l(__instanceLock__);
_automaticallyRelayoutOnLayoutMarginsChanges = flag;
}

#pragma mark <ASDebugNameProvider>

- (NSString *)debugName
Expand Down Expand Up @@ -929,6 +1027,8 @@ - (void)__layout
[self layoutDidFinish];
});
}

[self _fallbackUpdateSafeAreaOnChildren];
}

- (void)layoutDidFinish
Expand Down
5 changes: 5 additions & 0 deletions Source/ASViewController.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ NS_ASSUME_NONNULL_BEGIN
// Refer to examples/SynchronousConcurrency, AsyncViewController.m
@property (nonatomic, assign) BOOL neverShowPlaceholders;

/* Custom container UIViewController subclasses can use this property to add to the overlay
that UIViewController calculates for the safeAreaInsets for contained view controllers.
*/
@property(nonatomic) UIEdgeInsets additionalSafeAreaInsets;

@end

@interface ASViewController (ASRangeControllerUpdateRangeProtocol)
Expand Down
30 changes: 30 additions & 0 deletions Source/ASViewController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ @implementation ASViewController
NSInteger _visibilityDepth;
BOOL _selfConformsToRangeModeProtocol;
BOOL _nodeConformsToRangeModeProtocol;
UIEdgeInsets _fallbackAdditionalSafeAreaInsets;
}

- (instancetype)initWithNibName:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil
Expand Down Expand Up @@ -73,10 +74,14 @@ - (void)_initializeInstance
if (_node == nil) {
return;
}

_node.viewControllerRoot = YES;
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now, especially because it's thread-safe and faster than -[ASDisplayNode(Convenience) closestViewController].

One edge case I can think of is that a root-of-view-controller node is reused and added to a different node hierarchy. It is rare, but we should document is somehow, and maybe assert in debug mode (e.g if viewControllerRoot is true, assert that its close view controller indeed points to itself).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an assertion to -[_ASDisplayView didMoveToSuperview] to check for that case. Would that be enough?

Copy link
Member

Choose a reason for hiding this comment

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

👍


_selfConformsToRangeModeProtocol = [self conformsToProtocol:@protocol(ASRangeControllerUpdateRangeProtocol)];
_nodeConformsToRangeModeProtocol = [_node conformsToProtocol:@protocol(ASRangeControllerUpdateRangeProtocol)];
_automaticallyAdjustRangeModeBasedOnViewEvents = _selfConformsToRangeModeProtocol || _nodeConformsToRangeModeProtocol;

_fallbackAdditionalSafeAreaInsets = UIEdgeInsetsZero;

// In case the node will get loaded
if (_node.nodeLoaded) {
Expand Down Expand Up @@ -159,6 +164,13 @@ - (void)viewDidLayoutSubviews
[_node recursivelyEnsureDisplaySynchronously:YES];
}
[super viewDidLayoutSubviews];

UIEdgeInsets safeArea = UIEdgeInsetsMake(self.topLayoutGuide.length, 0, self.bottomLayoutGuide.length, 0);
UIEdgeInsets additionalInsets = self.additionalSafeAreaInsets;

safeArea = ASConcatInsets(safeArea, additionalInsets);

_node.fallbackSafeAreaInsets = safeArea;
Copy link
Member

@nguyenhuy nguyenhuy Jan 16, 2018

Choose a reason for hiding this comment

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

  • This node is guaranteed to be view-backed and loaded. The only reason the fallback insets is needed here is that the runtime is iOS 10 or earlier. Let's take advantage of that and avoid doing unnecessary work if possible.

}

ASVisibilityDidMoveToParentViewController;
Expand Down Expand Up @@ -264,6 +276,24 @@ - (ASInterfaceState)interfaceState
return _node.interfaceState;
}

- (UIEdgeInsets)additionalSafeAreaInsets
{
if (AS_AVAILABLE_IOS(11.0)) {
return super.additionalSafeAreaInsets;
}

return _fallbackAdditionalSafeAreaInsets;
}

- (void)setAdditionalSafeAreaInsets:(UIEdgeInsets)additionalSafeAreaInsets
{
if (AS_AVAILABLE_IOS(11.0)) {
[super setAdditionalSafeAreaInsets:additionalSafeAreaInsets];
}

_fallbackAdditionalSafeAreaInsets = additionalSafeAreaInsets;
Copy link
Member

Choose a reason for hiding this comment

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

Should we recalculate the root node's insets here?

}

#pragma mark - ASTraitEnvironment

- (ASPrimitiveTraitCollection)primitiveTraitCollectionForUITraitCollection:(UITraitCollection *)traitCollection
Expand Down
3 changes: 3 additions & 0 deletions Source/Details/UIView+ASConvenience.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ NS_ASSUME_NONNULL_BEGIN
@property (nonatomic, assign, getter=isUserInteractionEnabled) BOOL userInteractionEnabled;
@property (nonatomic, assign, getter=isExclusiveTouch) BOOL exclusiveTouch;
@property (nonatomic, assign, getter=asyncdisplaykit_isAsyncTransactionContainer, setter = asyncdisplaykit_setAsyncTransactionContainer:) BOOL asyncdisplaykit_asyncTransactionContainer;
@property (nonatomic, assign) UIEdgeInsets layoutMargins;
@property (nonatomic, assign) BOOL preservesSuperviewLayoutMargins;
@property (nonatomic, assign) BOOL insetsLayoutMarginsFromSafeArea;

/**
Following properties of the UIAccessibility informal protocol are supported as well.
Expand Down
16 changes: 16 additions & 0 deletions Source/Details/_ASDisplayView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,22 @@ - (BOOL)canPerformAction:(SEL)action withSender:(id)sender
return ([super canPerformAction:action withSender:sender] || [node respondsToSelector:action]);
}

- (void)layoutMarginsDidChange
{
ASDisplayNode *node = _asyncdisplaykit_node; // Create strong reference to weak ivar.
[super layoutMarginsDidChange];

[node layoutMarginsDidChange];
}

- (void)safeAreaInsetsDidChange
{
ASDisplayNode *node = _asyncdisplaykit_node; // Create strong reference to weak ivar.
[super safeAreaInsetsDidChange];

[node safeAreaInsetsDidChange];
}

- (id)forwardingTargetForSelector:(SEL)aSelector
{
// Ideally, we would implement -targetForAction:withSender: and simply return the node where we don't respond personally.
Expand Down
19 changes: 19 additions & 0 deletions Source/Private/ASDisplayNode+FrameworkPrivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,25 @@ __unused static NSString * _Nonnull NSStringFromASHierarchyStateChange(ASHierarc
*/
- (BOOL)shouldScheduleDisplayWithNewInterfaceState:(ASInterfaceState)newInterfaceState;

/**
* @abstract safeAreaInsets will fallback to this value if the corresponding UIKit property is not available
* (due to an old iOS version).
*
* @discussion This should be set by the owning view controller based on it's layout guides.
* If this is not a view controllet's node the value will be calculated automatically by the parent node.
*/
@property (nonatomic, assign) UIEdgeInsets fallbackSafeAreaInsets;

/**
* @abstract Indicates if this node is a view controller's root node. Defaults to NO.
*
* @discussion Set to YES in -[ASViewController initWithNode:].
*
* YES here only means that this node is used as an ASViewController node. It doesn't mean that this node is a root of
* ASDisplayNode hierarchy, e.g. when its view controller is parented by another ASViewController.
*/
@property (nonatomic, assign, getter=isViewControllerRoot) BOOL viewControllerRoot;
Copy link
Member

Choose a reason for hiding this comment

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

Good call on making this property private :)


@end


Expand Down
Loading