Update ASTextNode renderer to latest ComponentKit implementation #825
Conversation
|
Fantastic, thanks @levi! This is something that @ocrickard and I have talked about doing for basically a year. Oliver, if you have a few minutes to skim this or have any notable comments (e.g., known issues or planned work in the CK version) — please do :) |
There was a problem hiding this comment.
lol, did I write this on the original diff? Must have leaned on my keyboard or somehting :P
There was a problem hiding this comment.
|
Hey guys! This looks great. I'm going to have to review this more thoroughly later, but on a first read through looks reasonable to me. Are you still seeing float math errors here? That's a pretty common thing to see with Text code... inevitably +1's creep in, and the sizes change a little bit. If you point me towards the failing unit test I may be able to help debug. |
|
Thanks for checking this out, @ocrickard. The current ASTextNode unit tests do an initial measurement pass and remeasure, checking the equality of the results. You're right in that they're running into floating point specifics. Here's a link to the first failing one: https://github.com/facebook/AsyncDisplayKit/blob/master/AsyncDisplayKitTests/ASTextNodeTests.m#L100 The other failing test is the one below it. |
|
@ocrickard any clue why re-measuring the ASTextNode would produce different results with the new renderer? That seems pretty darn odd to me, although I haven't looked at the implementation for which objects / state are shared. No worries if you're busy right now, we'll eventually figure it out :) |
|
Interesting. My guess is that you're hitting a cached value where you On Sunday, November 8, 2015, appleguy notifications@github.com wrote:
Oliver Clark Rickard (707) 536-1352 |
AsyncDisplayKit/AsyncDisplayKit.h
Outdated
There was a problem hiding this comment.
Hmm, the renderer is an implementation detail. I wouldn't put it in the public header...
There was a problem hiding this comment.
Wanted to include this in just to keep header consistency, but removing it is probably for the best to clean up the 2.0 umbrella file.
|
cc @mboyd1017 can you help review? Ahh, yeah now I remember. There's a hack in ASTextNode that allows that test to work. It was added by @mboyd1017. Not sure exactly how to fix that in the cached renderer world... perhaps store a value with the calculated size as its key in the renderer cache? Both CoreText and TextKit will return different values when laid out with one constraint, then recompute the layout with the calculated bounds. Due to a combination of float math, complex line breaking algorithms, and magic... it's inconsistent. The best we can do is workaround it. BTW, I didn't notice before, but this doesn't pull in the renderer cache. You're gonna miss out on some of the perf and memory benefits without that... but this should be a cleaner implementation to support in the future. |
There was a problem hiding this comment.
nit: indentation is off here
|
@ocrickard good catch on the LRU cache. I decided not to bring it in this patch just to keep the scope of changes easier to review. I would like to further investigate the best approach of including it in ASDK 2.0 and currently planning to put up a second patch once this lands. |
|
I agree we don't need the LRU cache right now. The ASDK architecture makes it significantly less necessary, although conceivably beneficial - particularly to accelerate things like full table / collection data reloads. @levi will you be able to investigate the failing test soon? It looks like some rebasing is required as well, so I'd like to get this landed ASAP so we don't have the surface area exposed to merge conflicts. |
|
@appleguy I'll look into this tonight and get it to a landable stage. Thanks for the ping! |
|
Hey guys, yeah that sounds reasonable to me! I'm not going to have time to help debug the test this weekend or probably this week. Good luck @levi! |
levi
force-pushed
the
levi/text-renderer
branch
from
9f28b02 to
fb2333d
Compare
|
All tests passing but the framework smoke test. I forgot where I need to add the new files to make it green. To make the floating point inconsistencies pass, I added a simple cgsize comparison method to take into account a small delta of change. CK has a large body of snapshot tests that test the layout behavior of their text and label nodes. I would like to spend sometime porting those over to ASDK once this PR lands. I think using snapshots would be a much more accurate way of testing the final result of the ASTextNode. |
|
Tests fixed, thanks for you help @appleguy! |
|
Awesome. What was the issue? Sent from my iPhone
|
|
@ocrickard linking the newly added files to the iOS framework. Unrelated to the actual implementation of this diff. |
There was a problem hiding this comment.
We should rename these functions, or otherwise retain the CK filename and place such files in a directory that designates them as unmodified CK ports.
|
@levi - some brief notes I wrote on the plane home. I'm going to merge this as-is, and the notes can be for a followup (we can make a task referencing this PR if you're busy!) @ocrickard - this is really some serious software. Thanks for all your great work on it — both the original ASDK version, and the robust + unique refinements you made with CK! It's really cool to see such workhorse code be valuable in both projects. |
|
@appleguy great to hear it! Probably won't be able to get to these until next weekend. A task pointer would definitely be helpful, thanks. |
levi
force-pushed
the
levi/text-renderer
branch
from
6c2cfff to
04ad021
Compare
|
No problem. Looks like some good ol' pbxproj issues. This branch should be good to merge now. I updated a few of the renaming requests above, however, still need to find a block of time where I can implement better tests and update this with the latest fixes from CK. |
|
@levi I'm ready to land this - but for some reason it is tripping up on my Visible range stuff? "Undefined symbols for architecture x86_64: |
|
Oh man, my failed builds 😓. I'll have some time this afternoon to take a look. |
|
Alright, this one should be a winner 😸 |
|
WOO-HOO!!! |
Update ASTextNode renderer with the latest derivative of it developed for ComponentKit
|
Awesome! I'm happy to see this get in—finally! 💮 |
…e#825) * A few small fixes plus a failing test for ASTextNode * Change the approach * Update changelog * Eh screw null_resettable * No need for changelog now
4 participants
I had some free time on a plane yesterday and saw this piece of low hanging fruit that didn't require much focus. I would like to contribute an ASLabelNode in the next couple of weeks and updating the backing renderer is a much needed addition before implementing methods like
minimumFontSizeandadjustsFontSizeToFitWidth.Some unit tests are failing right now, as it seems that the floating point math doesn't produce consistent results. @appleguy, you might have deeper insight into why this is. Going to dig a bit deeper into the TextKit sizing behavior to see what changed from the original implementation of ASTextNodeRenderer.
There were some API differences. I ended up adding an exclusionPaths attribute to the CKTextKitRenderer, which probably would make sense to contribute back to ComponentKit.
isTruncatedis reimplemented, as the truncation range is no longer exposed. And I ran into a build error when trying to includeASTextKitRenderer.hin the umbrella AsyncDisplayKit.h, as it seems to be trying to build the header as a C header and is confused where to find the#import <vector>c++ std lib header. I haven't worked much with Xcode's C++ integration, so help would be appreciated.I removed the legacy tests that were specific to the original renderer API. Naturally, I would like to bring up the tests to speed and need to decide a good testing strategy for this implementation. CK has a great suite of unit tests and there might be some value in copying them over to increase the coverage stats in the project. On the integration side, a snapshot test or two could be valuable, but need to identify possible areas of regression before spending too much time on creating test cases.
Resolves #804