-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Disable interface coalescing #862
Changes from 18 commits
929d118
dd24d8f
b8eaffa
2918ea0
9c42266
329f35f
5f8b7ec
d87bb11
269c2ab
24c1ce8
233169e
67f908b
9890825
819ca1c
e96d270
b19f90d
a9b4045
833e32a
029a4c5
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 |
---|---|---|
|
@@ -57,6 +57,10 @@ | |
#else | ||
#define TIME_SCOPED(outVar) | ||
#endif | ||
// This is trying to merge non-rangeManaged with rangeManaged, so both range-managed and standalone nodes wait before firing their exit-visibility handlers, as UIViewController transitions now do rehosting at both start & end of animation. | ||
// Enable this will mitigate interface updating state when coalescing disabled. | ||
// TODO(wsdwsd0829): Rework enabling code to ensure that interface state behavior is not altered when ASCATransactionQueue is disabled. | ||
#define ENABLE_NEW_EXIT_HIERARCHY_BEHAVIOR 0 | ||
|
||
static ASDisplayNodeNonFatalErrorBlock _nonFatalErrorBlock = nil; | ||
NSInteger const ASDefaultDrawingPriority = ASDefaultTransactionPriority; | ||
|
@@ -3005,6 +3009,13 @@ - (void)didExitHierarchy | |
// same runloop. Strategy: strong reference (might be the last!), wait one runloop, and confirm we are still outside the hierarchy (both layer-backed and view-backed). | ||
// TODO: This approach could be optimized by only performing the dispatch for root elements + recursively apply the interface state change. This would require a closer | ||
// integration with _ASDisplayLayer to ensure that the superlayer pointer has been cleared by this stage (to check if we are root or not), or a different delegate call. | ||
|
||
#if !ENABLE_NEW_EXIT_HIERARCHY_BEHAVIOR | ||
if (![self supportsRangeManagedInterfaceState]) { | ||
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. This change is equivalent to the change in the revert, when looking at specifically the disabled codepath flow - by doing this, we ensure that there are no changes to the codepath when in the disabled state. https://github.com/TextureGroup/Texture/pull/861/files#diff-fb6dadff65468c53595c164828199c83 The root of the issue is that we made some changes to sanitize the handling of non-range managed nodes (e.g. top level nodes / outside of cells) so they behaved more like inside cells. We now have Texture tests running on our internal build system and so we would have caught this at the time, but alas we didn’t notice this broke the tests in the disabled case. Definitely an oversight on our part, which I think won’t happen again considering the build system and ASConfiguration systems in place. Fortunately I think we are on the path to fixing some long-standing issues where visible state will thrash during UINavigationController transitions, which will be a big help for logging accuracy. |
||
self.interfaceState = ASInterfaceStateNone; | ||
return; | ||
} | ||
#endif | ||
if (ASInterfaceStateIncludesVisible(_pendingInterfaceState)) { | ||
void(^exitVisibleInterfaceState)(void) = ^{ | ||
// This block intentionally retains self. | ||
|
@@ -3013,11 +3024,12 @@ - (void)didExitHierarchy | |
BOOL isVisible = ASInterfaceStateIncludesVisible(_pendingInterfaceState); | ||
ASInterfaceState newState = (_pendingInterfaceState & ~ASInterfaceStateVisible); | ||
__instanceLock__.unlock(); | ||
|
||
if (!isStillInHierarchy && isVisible) { | ||
#if ENABLE_NEW_EXIT_HIERARCHY_BEHAVIOR | ||
if (![self supportsRangeManagedInterfaceState]) { | ||
newState = ASInterfaceStateNone; | ||
} | ||
#endif | ||
self.interfaceState = newState; | ||
} | ||
}; | ||
|
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.
Would you be willing to add a comment describing this macro?
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.
Hi, you mean I added in code or here?
The new NEW_EXIT_HIERARCHY_BEHAVIOR is trying to merge non-rangeManaged with rangeManaged, so both range-managed and standalone nodes wait before firing their exit-visibility handlers, as UIViewController transitions now do rehosting at both start & end of animation. (that was to try to mitigate interface updating state when coalescing disabled)
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.
OK that makes sense. Could you add that on line 60 so that future developers will know the intention?
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.
Done