[ASTextNode2] Add improved support for all line-break modes in experimental text node. #1150
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ASTextNode2 was only setting truncationMode (lineBreakMode) on existing paragraph styles in attributedString (thus having no effect for the two non-truncating modes if there were not any existing paragraph style runs). ASTextLayout (essentially YYTextLayout) was not rendering the two non-tail truncation lineBreakModes correctly. There's not much history on github but it appears to me that it was set up correctly at one time and then some additional code was added for unclear reasons that assumed any truncation was at the end of the string. This commit corrects both issues.
wiseoldduck
changed the title
Adlai-Holler
approved these changes
Oct 17, 2018
There was a problem hiding this comment.
Pretty complicated stuff! I went line-by-line and if I squint it all makes sense. I thought our text node tests were all being run against text node 2 thanks to @wsdwsd0829 's change a while back. Can we easily cover these truncation modes?
|
Yes I intend/need to add snapshot tests! Thanks for squinting through it so far, let me see if I can get those pushed today.
… On Oct 17, 2018, at 8:19 AM, Adlai Holler ***@***.***> wrote:
@Adlai-Holler approved this pull request.
Pretty complicated stuff! I went line-by-line and if I squint it all makes sense. I thought our text node tests were all being run against text node 2 thanks to @wsdwsd0829 's change a while back. Can we easily cover these truncation modes?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Also my merge with master yesterday was kind of back-of-the-envelope, I mostly pushed it in case I wanted to work on it from a different machine. So I would discourage merging this until I get to audit that more carefully... which adding those tests should give me a chance to do!
… On Oct 17, 2018, at 8:34 AM, Kevin Smith ***@***.***> wrote:
Yes I intend/need to add snapshot tests! Thanks for squinting through it so far, let me see if I can get those pushed today.
> On Oct 17, 2018, at 8:19 AM, Adlai Holler ***@***.***> wrote:
>
> @Adlai-Holler approved this pull request.
>
> Pretty complicated stuff! I went line-by-line and if I squint it all makes sense. I thought our text node tests were all being run against text node 2 thanks to @wsdwsd0829 's change a while back. Can we easily cover these truncation modes?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or mute the thread.
|
Before:
After:
|
Before:
After:
|
Before:
After:
|
Before:
After:
|
Before:
After:
|
|
etc. 😄 |
|
oops I might have buried the lead with all my screenshots
But umm I'm sure it's fine! I'll put the tests into another PR 😁 |
mikezucc
pushed a commit
to mikezucc/Texture
that referenced
this pull request
Nov 7, 2018
…mental text node. (TextureGroup#1150) * ASTextNode2 rendering corrections. ASTextNode2 was only setting truncationMode (lineBreakMode) on existing paragraph styles in attributedString (thus having no effect for the two non-truncating modes if there were not any existing paragraph style runs). ASTextLayout (essentially YYTextLayout) was not rendering the two non-tail truncation lineBreakModes correctly. There's not much history on github but it appears to me that it was set up correctly at one time and then some additional code was added for unclear reasons that assumed any truncation was at the end of the string. This commit corrects both issues. * Update CHANGELOG.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ASTextNode2 was only setting truncationMode (lineBreakMode) on existing paragraph styles in attributedString (thus having no effect for the two non-truncating modes if there were not any existing paragraph style runs).
ASTextLayout (essentially YYTextLayout) was not rendering the two non-tail truncation lineBreakModes correctly. There's not much history on github but it appears to me that it was set up correctly at one time and then some additional code was added for unclear reasons that assumed any truncation was at the end of the string.
This commit corrects both issues.