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

Disable interface coalescing #862

Merged
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
39 changes: 20 additions & 19 deletions Source/ASDisplayNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -2898,27 +2898,28 @@ - (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 (ASInterfaceStateIncludesVisible(_pendingInterfaceState)) {
void(^exitVisibleInterfaceState)(void) = ^{
// This block intentionally retains self.
__instanceLock__.lock();
unsigned isStillInHierarchy = _flags.isInHierarchy;
BOOL isVisible = ASInterfaceStateIncludesVisible(_pendingInterfaceState);
ASInterfaceState newState = (_pendingInterfaceState & ~ASInterfaceStateVisible);
__instanceLock__.unlock();

if (!isStillInHierarchy && isVisible) {
if (![self supportsRangeManagedInterfaceState]) {
newState = ASInterfaceStateNone;
if (![self supportsRangeManagedInterfaceState]) {
Copy link
Member

Choose a reason for hiding this comment

The 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;
} else {
if (ASInterfaceStateIncludesVisible(_pendingInterfaceState)) {
void(^exitVisibleInterfaceState)(void) = ^{
// This block intentionally retains self.
__instanceLock__.lock();
unsigned isStillInHierarchy = _flags.isInHierarchy;
BOOL isVisible = ASInterfaceStateIncludesVisible(_pendingInterfaceState);
ASInterfaceState newState = (_pendingInterfaceState & ~ASInterfaceStateVisible);
__instanceLock__.unlock();

if (!isStillInHierarchy && isVisible) {
self.interfaceState = newState;
}
self.interfaceState = newState;
}
};
};

if (!ASCATransactionQueue.sharedQueue.enabled) {
dispatch_async(dispatch_get_main_queue(), exitVisibleInterfaceState);
} else {
exitVisibleInterfaceState();
if (!ASCATransactionQueue.sharedQueue.enabled) {
dispatch_async(dispatch_get_main_queue(), exitVisibleInterfaceState);
} else {
exitVisibleInterfaceState();
}
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions Source/Private/ASConfigurationInternal.m
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ + (CFTypeRef)sharedInstance
+ (ASConfiguration *)defaultConfiguration NS_RETURNS_RETAINED
{
ASConfiguration *config = [[ASConfiguration alloc] init];
// On by default for now, pending fix for https://github.com/TextureGroup/Texture/issues/853
config.experimentalFeatures = ASExperimentalInterfaceStateCoalescing;
// TODO(wsdwsd0829): Fix #788 before enabling it.
// config.experimentalFeatures = ASExperimentalInterfaceStateCoalescing;
return config;
}

Expand Down
4 changes: 4 additions & 0 deletions Tests/ASRunLoopQueueTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ - (void)testASCATransactionQueueDisable

- (void)testASCATransactionQueueProcess
{
ASConfiguration *config = [[ASConfiguration alloc] initWithDictionary:nil];
config.experimentalFeatures = ASExperimentalInterfaceStateCoalescing;
[ASConfigurationManager test_resetWithConfiguration:config];

ASCATransactionQueue *queue = [[ASCATransactionQueue alloc] init];
QueueObject *object = [[QueueObject alloc] init];
[queue enqueue:object];
Expand Down