GlyphLayout: justify text option(s)#7609
Conversation
tommyettinger
left a comment
tommyettinger
left a comment
There was a problem hiding this comment.
Mostly looks good, but there's no documentation here and the new behavior will definitely need some kind of help provided to users if they are going to make use of this at all. I think Wrapping should just be 3 protected ints defined in GlyphLayout if it isn't meant to be used by the public, or a Wrapping enum if it is meant to be used by the public. Justify, its constants, and any public code that takes a Justify constant all really need documentation, even a little.
This seems like a really useful PR, and I'm willing to fill out or polish up documentation if you want, but I don't adequately understand what all the Justify constants do at this point.
tommyettinger
left a comment
tommyettinger
left a comment
There was a problem hiding this comment.
This looks much better! I have no qualms here with the PR, great work!
|
It looks well done! I don't like The impact on code paths that don't use justify seems minimal. @StartsMercury Why are GlyphRun |
My thought was that it's currently internal to justify, though it is open to change to public. Users might find wrapState useful, thinking back now, so changing it (back) to public access may be a good idea. I do agree that the constants be an enum. Should a WrapState enum be contained in GlyphLayout, along with GlyphRun? |
|
Yeah, I think it's good to put the enum in GlyphLayout. |
|
I just noticed, should |
|
Sure, it could. The toString mainly for debugging. |
|
Except from maybe a line in CHANGES, can this be merged @NathanSweet ? |
|
Yes, I think so. I hate making GlyphLayout even more complex and hope to never have to debug this, but it looks well done and doesn't add much overhead when not using justification. |
5 participants
Initial implementation of adding layout support on GlyphLayout for justified text.
Which lines to justify:
How to expand:
Additional parameter on
GlyphLayout.setText. Its previous signature remains, defaulting to useJustify.None.Note: some constant names were changed, but the ordinals should still match.