[ASDisplayNode] Provide safeAreaInsets and layoutMargins bridge by ypogribnyi · Pull Request #685 · TextureGroup/Texture

ypogribnyi · 2017-11-23T18:38:45Z

My attempt to implement #658.

Safe area insets are quite important when designing for iPhone X or when dealing with view controller containers with overlapping bars (e.g. UINavigationController with a translucent navigation bar).

This PR includes:

safeAreaInsets, insetsLayoutMarginsFromSafeArea, layoutMargins, preservesSuperviewLayoutMargins properties and -safeAreaInsetsDidChange, -layoutMarginsDidChange callbacks on ASDisplayNode all bridged to the underlying view.
automaticallyRelayoutOnSafeAreaChanges and automaticallyRelayoutOnLayoutMarginsChanges flags on ASDisplayNode so we can automatically send -setNeedsLayout when the insets change.
A compatibility safeAreaInsets calculation algorithm for old iOS versions (for iOS < 11 we have to observe view controller's topLayoutGuide and bottomLayoutGuide lengths, which is very inconvenient).

…ies to ASDisplayNode * Add layoutMargins bridged to the underlying view * Add safeAreaInsets bridged to the underlying view * Add fallback calculation of safeAreaInsets for old iOS versions * Add automaticallyRelayoutOnSafeAreaChanges and automaticallyRelayoutOnLayoutMarginsChanges properties * Add additionalSafeAreaInsets property to ASViewController for compatibility with old iOS versions

ghost · 2017-11-24T13:15:17Z

🚫 CI failed with log

ghost · 2017-11-28T12:46:26Z

🚫 CI failed with log

nguyenhuy

@ypogribnyi Thanks for submitting the PR. Please take a look at failing tests whenever you have time.

This also fixes tests.

ghost · 2017-11-30T12:26:55Z

🚫 CI failed with log

ghost · 2017-11-30T13:00:47Z

🚫 CI failed with log

ypogribnyi · 2017-12-04T12:18:10Z

@nguyenhuy, I fixed the tests that were failing because of my code. Now the log shows ticks ✓ for all tests and a strange error 'Early unexpected exit, operation never finished bootstrapping - no restart will be attempted' from xcodebuild. Is it because some tests silently fail, or is it some problem with Xcode?

ghost · 2017-12-04T13:52:01Z

🚫 CI failed with log

appleguy · 2018-01-14T03:14:58Z

@ypogribnyi thanks for developing this! I think we could probably merge it soon if we can get it passing CI (apologies for the long delay over the holiday).

Perhaps you could sync to latest and make any final tweaks you had in mind and see if the build server is cooperating now?

# Conflicts: # CHANGELOG.md

ghost · 2018-01-15T10:23:48Z

🚫 CI failed with log

…nd thread

ypogribnyi · 2018-01-15T17:36:33Z

@appleguy I have finally fixed failing tests. Have nothing else to add here.

… safeAreaInsets # Conflicts: # Source/Private/_ASPendingState.mm

nguyenhuy

Thanks for working on this, @ypogribnyi. Overall, great work! I saw you took extra steps to make sure the diff follows existing practices and conventions, which is awesome!

nguyenhuy · 2018-01-16T11:19:58Z

Source/ASDisplayNode.h

 @property (nonatomic, assign)           UIViewAutoresizing autoresizingMask;   // default==UIViewAutoresizingNone (undefined for layer-backed nodes)

+// Content margins
+@property (nonatomic, assign)           UIEdgeInsets layoutMargins;


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.

nguyenhuy · 2018-01-16T12:27:55Z

Source/ASDisplayNode.mm

+    _fallbackSafeAreaInsets = insets;
+  }
+
+  BOOL needsManualUpdate = !AS_AT_LEAST_IOS11 || _flags.layerBacked;


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

nguyenhuy · 2018-01-16T12:33:22Z

Source/ASDisplayNode.mm

+  }
+
+  BOOL needsManualUpdate = !AS_AT_LEAST_IOS11 || _flags.layerBacked;
+  BOOL updatesLayoutMargins = needsManualUpdate && self.insetsLayoutMarginsFromSafeArea;


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.

nguyenhuy · 2018-01-16T12:34:41Z

Source/Private/ASDisplayNode+UIViewBridge.mm

+- (void)safeAreaInsetsDidChange
+{
+  ASDisplayNodeAssertMainThread();
+  BOOL automaticallyRelayout;


Let's use the automaticallyRelayoutOnSafeAreaChanges getter here.

nguyenhuy · 2018-01-16T12:35:39Z

Source/Private/ASDisplayNode+UIViewBridge.mm

+- (void)layoutMarginsDidChange
+{
+  ASDisplayNodeAssertMainThread();
+  BOOL automaticallyRelayout;


Let's call self.automaticallyRelayoutOnLayoutMarginsChanges here.

nguyenhuy · 2018-01-16T13:17:50Z

Source/ASViewController.mm

+
+  safeArea = ASConcatInsets(safeArea, additionalInsets);
+
+  _node.fallbackSafeAreaInsets = safeArea;


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.

nguyenhuy · 2018-01-16T13:20:18Z

Source/ASViewController.mm

+    [super setAdditionalSafeAreaInsets:additionalSafeAreaInsets];
+  }
+
+  _fallbackAdditionalSafeAreaInsets = additionalSafeAreaInsets;


Should we recalculate the root node's insets here?

nguyenhuy · 2018-01-16T13:21:51Z

Source/Private/ASDisplayNode+FrameworkPrivate.h

+ * 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;


Good call on making this property private :)

nguyenhuy · 2018-01-16T13:41:16Z

Source/Private/ASDisplayNodeInternal.h

 - (void)nodeViewDidAddGestureRecognizer;

+// Recalculates fallbackSafeAreaInsets for the subnodes
+- (void)_fallbackUpdateSafeAreaOnChildren;


Nit: May not needed because _fallbackUpdateSafeAreaOnChildren is implemented and called in ASDisplayNode.mm only.

This is also used by -[ASDisplayNode safeAreaDidChange], which is implemented in ASDisplayNode+UIViewBridge.mm.

nguyenhuy · 2018-01-16T13:54:12Z

Source/Private/ASDisplayNode+UIViewBridge.mm

+    }
+  }
+
+  if (__loaded(self) && (!AS_AT_LEAST_IOS11 || _flags.layerBacked)) {


Warning: accessing _flags without the lock.

* Update documentation for layoutMargins and safeAreaInsets properties. Suggest that users set the automaticallyRelayout* flags to ensure that their layout is synchronized to the margin's values. * Fix accessing ASDisplayNode internal structures without a lock. * Add shortcut in -[ASDisplayNode _fallbackUpdateSafeAreaOnChildren] to skip a child when possible. * Add shortcut in ASViewController to avoid fallback safe area insets recalculation in iOS 11. Fix fallback safe area insets recalculation when the additionalSafeAreaInsets are set. * Add debug check that a view controller's node is never reused without its view controller, so the viewControllerRoot flag value is always consistent. * Use getters instead of reading ivars directly in -layoutMarginsDidChange and -safeAreaInsetsDidChange.

… safeAreaInsets # Conflicts: # CHANGELOG.md

ypogribnyi · 2018-01-17T17:17:13Z

@nguyenhuy, thank you for the detailed review!

ay8s · 2018-01-23T21:14:01Z

This is awesome @ypogribnyi. I wonder if similar could be done to expose the readableContentGuide within cell nodes etc? Think it could even just be the layoutFrame from the readable content guide that is accessible so you can then constrain the layout to that within layoutSpecThatFits:

appleguy · 2018-02-19T23:56:03Z

@ypogribnyi do you have any cycles to wrap up remaining requests from Huy? Let us know either way - if you need someone to take this over, I'm not sure who might be able to, but then we'd be progressing towards landing it.

This is a great contribution overall, so thanks for taking the time to work with us!

philtre · 2018-03-16T17:37:06Z

It would be super helpful to have a ASSafeAreaLayoutSpec that abstracts this functionality. Looking forward to the merge!

nguyenhuy

@ypogribnyi Sorry for the long delay here. Awesome contribution. Thank you!

nguyenhuy · 2018-03-21T13:15:27Z

Source/ASDisplayNode.mm

+  }
+}
+
+- (void)_fallbackUpdateSafeAreaOnChildren


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

nguyenhuy · 2018-03-21T13:19:54Z

Source/ASViewController.mm

    return;
  }
+
+  _node.viewControllerRoot = YES;


…ge (TextureGroup#685)" This reverts commit 7f01b89.

…ureGroup#685) * [ASDisplayNode] Add safeAreaInsets, layoutMargins and related properties to ASDisplayNode * Add layoutMargins bridged to the underlying view * Add safeAreaInsets bridged to the underlying view * Add fallback calculation of safeAreaInsets for old iOS versions * Add automaticallyRelayoutOnSafeAreaChanges and automaticallyRelayoutOnLayoutMarginsChanges properties * Add additionalSafeAreaInsets property to ASViewController for compatibility with old iOS versions * Provide safeAreaInsets for layer-backed nodes. This also fixes tests. * Fix crash when insetsLayoutMarginsFromSafeArea is set from a background thread * Changes requested at code review: * Update documentation for layoutMargins and safeAreaInsets properties. Suggest that users set the automaticallyRelayout* flags to ensure that their layout is synchronized to the margin's values. * Fix accessing ASDisplayNode internal structures without a lock. * Add shortcut in -[ASDisplayNode _fallbackUpdateSafeAreaOnChildren] to skip a child when possible. * Add shortcut in ASViewController to avoid fallback safe area insets recalculation in iOS 11. Fix fallback safe area insets recalculation when the additionalSafeAreaInsets are set. * Add debug check that a view controller's node is never reused without its view controller, so the viewControllerRoot flag value is always consistent. * Use getters instead of reading ivars directly in -layoutMarginsDidChange and -safeAreaInsetsDidChange. * Minor change in CHANGELOG * Minor change in ASDisplayNodeTests.mm

nguyenhuy requested changes Nov 28, 2017

View reviewed changes

Provide safeAreaInsets for layer-backed nodes.

f881370

This also fixes tests.

Merge branch 'master' into safeAreaInsets

5b47e31

Merge branch 'master' into safeAreaInsets

7e9ff4e

# Conflicts: # CHANGELOG.md

Fix crash when insetsLayoutMarginsFromSafeArea is set from a backgrou…

727b4ae

…nd thread

Merge branch 'master' of https://github.com/TextureGroup/Texture into…

fd27ff7

… safeAreaInsets # Conflicts: # Source/Private/_ASPendingState.mm

nguyenhuy requested changes Jan 16, 2018

View reviewed changes

ypogribnyi added 2 commits January 17, 2018 16:10

Merge branch 'master' of https://github.com/TextureGroup/Texture into…

5268f14

… safeAreaInsets # Conflicts: # CHANGELOG.md

nguyenhuy self-assigned this Jan 31, 2018

Merge branch 'master' into safeAreaInsets

1abbebc

nguyenhuy approved these changes Mar 21, 2018

View reviewed changes

nguyenhuy added 4 commits March 21, 2018 13:39

Merge branch 'master' into safeAreaInsets

701a61d

Minor change in CHANGELOG

a97c3bc

Minor change in ASDisplayNodeTests.mm

40de117

Merge branch 'master' into safeAreaInsets

43cf7c2

nguyenhuy merged commit 7f01b89 into TextureGroup:master Mar 27, 2018

alexhillc added a commit to franklyinc/Texture that referenced this pull request Apr 19, 2018

Revert "[ASDisplayNode] Provide safeAreaInsets and layoutMargins brid…

8f71e90

…ge (TextureGroup#685)" This reverts commit 7f01b89.


		safeArea = ASConcatInsets(safeArea, additionalInsets);

		_node.fallbackSafeAreaInsets = safeArea;

Conversation

ypogribnyi commented Nov 23, 2017

Uh oh!

ghost commented Nov 24, 2017

Uh oh!

ghost commented Nov 28, 2017

Uh oh!

nguyenhuy left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 30, 2017

Uh oh!

ghost commented Nov 30, 2017

Uh oh!

ypogribnyi commented Dec 4, 2017

Uh oh!

ghost commented Dec 4, 2017

Uh oh!

appleguy commented Jan 14, 2018

Uh oh!

ghost commented Jan 15, 2018

Uh oh!

ypogribnyi commented Jan 15, 2018

Uh oh!

nguyenhuy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguyenhuy Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ypogribnyi commented Jan 17, 2018

Uh oh!

ay8s commented Jan 23, 2018

Uh oh!

appleguy commented Feb 19, 2018

Uh oh!

philtre commented Mar 16, 2018

Uh oh!

nguyenhuy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

nguyenhuy Jan 16, 2018 •

edited

Loading