[ASTextNode] Possible layout fix (fixes a case with Interface Builder initialization ordering) #877
Conversation
| @@ -10,4 +10,4 @@ SPEC CHECKSUMS: | |||
| FBSnapshotTestCase: 3dc3899168747a0319c5278f5b3445c13a6532dd | |||
| OCMock: a6a7dc0e3997fb9f35d99f72528698ebf60d64f2 | |||
|
|
|||
| COCOAPODS: 0.38.2 | |||
| COCOAPODS: 0.39.0 | |||
There was a problem hiding this comment.
We should actually delete this file and the Pods directory, as it should be installed locally by each user with "pod install". 39.0 is actually not compatible with the Pinterest setup right now, so even if we leave the file, please remove this change.
|
@soniccat thanks for investigating this, and especially for providing a test project. AsyncDisplayKit is not considered compatible with Interface Builder. If this fix is carefully tested to be safe for all use cases that don't rely on Interface Builder, and also doesn't have any negative impact on performance, I'm happy to take it even if it only benefits IB. However, the text code paths are tremendously critical to the framework so we need to move very carefully here. If you could tweak the brace style to match the nearby methods and remove the Podfile.lock method, we'll be closer to landing. @levi or @rcancro if you happen to have time to look into the test project to understand the issue and the fix, that would be great. It will take me at least a week to get back to this because of some key deliverables before Thanksgiving, and then visiting family for the rest of the week, but we will keep this PR open until a decision is made. Good to have you engaging with the project @soniccat - let us know if we can help with your implementation! |
appleguy
changed the title
| @@ -410,6 +418,8 @@ + (void)drawRect:(CGRect)bounds withParameters:(ASTextNodeDrawParameters *)param | |||
|
|
|||
| - (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer | |||
| { | |||
| [self _invalidateRendererIfNeeded]; | |||
There was a problem hiding this comment.
Quick question to help my understanding of this path. Is there ever a point when the backing view has not loaded here and doesn't need to be in order for the params to be generated? /cc @appleguy
There was a problem hiding this comment.
@levi Now I see that drawParametersForAsyncLayer: call takes place from _ASDisplayLayer::display method. And this call chain happens only when we have a layer or a view.
There was a problem hiding this comment.
@soniccat is correct. Though it is something I would like to change in the future, for a variety of reasons, it will be a complex change, so I'm not worried about relying on this for now. My main concern is that this could introduce some other inefficiency or behavior change from what clients have relied on to date, but it may be safe.
…erIfNeeded if there are no view and no layer
|
@soniccat I'm still looking at this one actively - thanks for your patience, and providing detailed information. Is it correct to understand this diff as automatically re-measuring the text /synchronously on the drawing thread/ in the event there is a size mismatch? @ocrickard, @levi I would be interested in your thoughts on this behavior. I really don't think this is necessarily the right behavior to fix the XIB case, but it may be a better behavior for the framework overall. In the XIB case, we might as well skip the first measurement entirely if the text node's size is not essential to calculating the layout (e.g., if the layout defines the text node's constrained size, but the calculated size is not actually needed). @nguyenhuy, @soniccat my concern with this is that the overall layout may be incorrect in this scenario, and this behavior could potentially hide the incorrectness in some cases (e.g. making it behave correctly when the actual inputs / code flows are not normal, such as requiring double-sizing of text). The alternative might be to rely on -invalidateLayout or some other call, and ensure that if the display pass detects there is a mismatch, it triggers as complete of a layout pass is necessary to guarantee that any components that are affected by the newly calculated size of the text have a chance to be adjusted. This could even mean abandoning the current display pass with an early return and calling -setNeedsDisplay, awaiting a new display call after the relayout is complete? |
|
@soniccat ok, I finally had more time to inspect the surrounding code. I apologize for how long it took me to review this. It doesn't meet my standard of service for the community - but this diff is also a bit complex to understand without looking at more of the code. I am ready to merge the diff, but sadly waited long enough for a conflict to occur. If you can fix the merge conflicts, I will land this as soon as I see it. Thanks a lot for debugging this case! |
|
Looks fine to me as well. We should really get asdk under automated benchmark testing so we can understand the perf implications of changes like this before landing. |
|
@ocrickard great suggestion. I think the biggest challenge to this is that the Travis machines have very inconsistent performance - but we could make a test harness that can be run manually on certain devices with known baselines. I will file a task and try to drive this in the next couple months. Does ComponentKit have anything comparable, either internally or open? |
|
@appleguy thanks for your and the project's team attention to my small improvement. I'm really happy that you found it useful. Now I've fixed the conflicts. |
|
@appleguy We do have a jerry-rigged perf testing harness for ComponentKit, but it wouldn't run on any of these open source systems, and honestly it breaks all the time. I hope that someone invests in something along these lines in the near future -- we'd love to use it. |
|
@soniccat no, thank you! @ocrickard good to know. It will probably take until end of Q1 at the earliest, plausibly into Q2, but my team will endeavor to add some automated perf testing capable of being run manually on device by a developer interested in assessing such changes. |
[ASTextNode] Possible layout fix (fixes a case with Interface Builder initialization ordering)
* fix SIMULATE_WEB_RESPONSE not imported facebookarchive#449 * Fix to make rangeMode update in right time * Fix pager node for interface coalescing * Fix typo * change log
Playing with ASTextNode I found that after launching I could have a wrong text layout. It happens if I call measure in viewDidLoad and at this moment I have frame from the xib.Then layer's layout method is called when the frame is different. And in drawParametersForAsyncLayer _constrainedSize and view.bounds.size are different.
I have example reproducing the bug: https://github.com/soniccat/AsyncDisplayKitTest. To see fixed version you should find "To fix layout bug uncomment this line" in ASTextNode.mm and uncomment the below line.