[TextKit] Fix text layout issue for CJK laungages #3026
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
|
Just signed the CLA. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
There was a problem hiding this comment.
@yxztj Thanks for the diff! As a potentially smaller change, is it sufficient to instead call one of these?
ensureAttributesAreFixedInRange:fixAttributesInRange:invalidateAttributesInRange:
If not, it would be great to see profiling results from this change if possible. Text nodes are some of the most common nodes that are used, and changing the attributed string of NSTextStorage could be a quite expensive operation.
…ntil calling addLayoutManager.
|
@Adlai-Holler Thanks for the suggestions. I tried the 3 methods you suggested but none of them worked.. After some profiling(copy and run the related code, from _textStorage init to setAttributedString 30000 times), setting attributedString both in init and later is doubling the execution time than before. So I changed the code: always init _textStorage with no parameter, and set attributedString later. Running the profiling code this time I can see the execution time is very close to before. |
|
@yxztj awesome, thanks for investigating this issue and then also guiding your work via profiling! An allstar contributor :), really appreciate how you've helped out over time. Both @Adlai-Holler and @maicki are the de-facto experts on ASTextNode these days, so I'll leave it to them to make the call on safety of taking this change. However, we're all interested in seeing improvements to a few nagging correctness issues that still affect the component and so will seriously consider any PRs in this area. One last suggestion: consider adding this to examples_extra/TextStressTest? Not a blocker for this PR, just a suggested followup. I've been trying to collect our known ASTextNode bugs there and then the fixes so we can run the app to check them. Of course it would be better to add snapshot tests for all of these, but either one would be a great step. |
|
Great work @yxztj, thanks! |
A quick fix for CJK language layout issue, which is supposed to bring no side effect. Has been tested in our production for one month.
See #2894