Disable interface coalescing #862
Conversation
Source/ASDisplayNode.mm
Outdated
| if (!isStillInHierarchy && isVisible) { | ||
| if (![self supportsRangeManagedInterfaceState]) { | ||
| newState = ASInterfaceStateNone; | ||
| if (![self supportsRangeManagedInterfaceState]) { |
There was a problem hiding this comment.
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.
Source/ASDisplayNode.mm
Outdated
| newState = ASInterfaceStateNone; | ||
| } | ||
| self.interfaceState = newState; | ||
| if (!ASCATransactionQueue.sharedQueue.enabled) { |
There was a problem hiding this comment.
This is to add back reverted code before interface coalescing.
An put current code in "else" for ease of reading & future debugging.
Source/ASDisplayNode.mm
Outdated
| }; | ||
| } | ||
| } else { | ||
| // TODO(wsdwsd0829): rework on this piece code that alter interface behavior of non-interfaceCoalescing. |
There was a problem hiding this comment.
// Rework this code to ensure that interface state behavior is not altered when ASCATransactionQueue is disabled.
Source/ASDisplayNode.mm
Outdated
| @@ -58,6 +58,8 @@ | |||
| #define TIME_SCOPED(outVar) | |||
| #endif | |||
|
|
|||
| #define ENABLE_NEW_EXIT_HIERARCHY_BEHAVIOR 0 | |||
There was a problem hiding this comment.
Would you be willing to add a comment describing this macro?
There was a problem hiding this comment.
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.
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.
Thanks @wsdwsd0829 for the minimal patch and the quick coding.
Before we compare this to the revert diff, could you describe the new macro and also rebase/merge master for a clean merge?
There was a problem hiding this comment.
This is a very nice complete restoration of the previous behavior. Here's the comparison with the straight-revert, for reference (ignore changes to irrelevant files): AHRevertInterfaceStateQueue...wsdwsd0829:disable-interface-coalescing
So that you guys can continue to iterate easily on the range-managed-change, I'm cool with the new default-0 compile flag, and I think we should land this diff pending CI passing.
|
Thanks for the fast fIx here @appleguy and @wsdwsd0829 |
|
Thanks for the quick reply.
…On Wed, Mar 28, 2018 at 6:29 PM, Adlai Holler ***@***.***> wrote:
Thanks for the fast fIx here @appleguy <https://github.com/appleguy> and
@wsdwsd0829 <https://github.com/wsdwsd0829>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#862 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFFq5Gikha2mj7MBJ_nXhODkGgdE9OoJks5tjDkFgaJpZM4S_NnH>
.
|
* fix SIMULATE_WEB_RESPONSE not imported TextureGroup#449 * Fix to make rangeMode update in right time * disable interface coalescing and fix tests * Revert to before coalescing for ease of reading * refactor, make min change * refactor * add comments * Add change log
[Fix #853] This trying to disable coalescing feature to avoid coalescing.
It the DisplayNode change is to match behavior exactly before coalescing. Hopefully we can avoid reverting for ease of future work.
Sorry about the troubles it causes. I will try to fix the reported issues in #788 and give it another check.
One potential bug fixed by #847 is one example we found recently (there maybe other issues). It's ok to submit that also since accessing pendingInterfaceState is same as self.interfaceState when the feature is disabled.