[ASTextNode] Fix text node truncation #1863
Conversation
ghost
added
the
|
@appleguy @Adlai-Holler Can I get some look over it please |
|
@nickvelloff is also an interested reviewer! Thanks @maicki :) |
| @property (nonatomic, assign, readwrite) CGSize constrainedSize; | ||
|
|
||
| /** | ||
| Resets the text storage to the original value in case it was truncated before. This method is called within |
There was a problem hiding this comment.
Could you clarify in the comment what "the original value" means? Is that the original, un-truncated string? It seems like there might be a few possible things that could be reset (e.g. like clearing the calculated size).
|
@maicki Please ping me if there is anything I can do to help. Happy to verify fix against my use case when you are ready. |
|
@nickvelloff Would be awesome if you could point your ASDK entry in your pod file to my fork and the branch with the fix and try it out on your side:
Please let me know if this would resolve your issue. |
|
@maicki Unfortunately this did not resolve my issue. The example i'm using is https://github.com/the-grid/TruncationBug-AsyncDisplayKitPlayground It's a playground so updates or changes can be very quickly tested. It's updated to point to your branch. Thanks a lot. Let me know if there is anything else you would like me to try. |
|
@nickvelloff Just tried your example and it worked as expected. No truncation anymore if you scroll down after zooming. Worth checking out a fresh copy of your example and pod update as well as build ASDK from fresh before trying out the playground. Furthermore I moved your playground example to a real project as it's not possible to debug within playgrounds so I had to create a project for it. I attached it to this comment. Just extract and run it. |
|
@maicki I cleared derived data, and it worked perfectly. Can no longer reproduce. So for me this completely resolves the issue. Thanks for resolving! |
…rage to original value
We should pass in the constrained size in both cases and the sizes should be the same. We adjust the calculated size in ASTextNode to be a bit narrower in the second case if we truncate again with the calculated size as constrained size it will truncate more and the resulting size will shrink.
This reverts commit 6238e5e. We will re-apply this change, but there are some early signs of performance impacts that need to be investigated.
|
@maicki @nickvelloff I had to revert this change on master; details are in an email. There is a performance impact of this change and a possible thread safety issue (unit tests trigger the thread safety crash on my machine, but it is very timing sensitive because some debug options make it not happen, so it may not occur on your machine). @nickvelloff feel free to continue pointing at the master commit that has the change as it is probably the better option for your app, until we get it re-landed next week after some analysis. |
|
@appleguy Thanks for the heads up. Do let me know if there is any way I can assist. |
* master_up: (43 commits) Do not expose tgmath.h to all clients of Texture (facebookarchive#1900) Call will / did display node for ASTextNode. Fixes facebookarchive#1680 (facebookarchive#1893) Remove background deallocation helper code (facebookarchive#1890) [Accessibility] Ship ASExperimentalDoNotCacheAccessibilityElements (facebookarchive#1888) 🎉 3.0.0 (facebookarchive#1883) Upgrade to Xcode 11.5 (facebookarchive#1877) Renames AS_EXTERN and ASViewController (facebookarchive#1876) Improve ThreeMigrationGuide.md (facebookarchive#1878) Add a 3.0 migration guide (facebookarchive#1875) I forgot this in the last PR and I'm pushing to master, I'm a bad person. Update for 3.0.0-rc.2 (facebookarchive#1874) Update RELEASE.md (facebookarchive#1873) Fix all the warnings and re-enable on CI (facebookarchive#1872) Prepare for 3.0.0-rc.1 release (facebookarchive#1870) -[ASNetworkImageNode setURL:resetToDefault:] forget to reset animatedImage (facebookarchive#1861) [ASDisplayNode] Implement accessibilityElementsHidden (facebookarchive#1859) Fix documentation for ASCornerRoundingTypeClipping (facebookarchive#1863) Add iOS13 UIContextMenu api to ASCommonCollectionDelegate (facebookarchive#1860) [ASDisplayNode] Implement accessibilityViewIsModal (facebookarchive#1858) Update FBSnapshotTestCase to iOSSnapshotTestCase (=6.2) (facebookarchive#1855) ...
Before truncation happens no invalidation of the text storage happened. That had the effect, if the constrained size of a text node increases a new size for the text node will be calculated. While this calculation is happening a truncation on the text will happen. Unfortunately the truncation will happen on the already truncated string.
This PR resets the text storage to the original value before the truncation is happening so it always will truncate on the same string no matter what constrained size is given.
Should fix #1841