Skip to content

Add Text Justification Options - #285

Merged
JimBobSquarePants merged 5 commits into
mainfrom
js/justified-text
Jul 2, 2022
Merged

Add Text Justification Options#285
JimBobSquarePants merged 5 commits into
mainfrom
js/justified-text

Conversation

@JimBobSquarePants

@JimBobSquarePants JimBobSquarePants commented Jun 30, 2022

Copy link
Copy Markdown
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Prompted by this Stack Overflow question This PR adds a new TextJustification enum to TextOptions. The options and implementation supporting the
CSS Text Module Level 3 text-justify property.

Horizontal

TextJustification.None
none

TextJustification.InterWord
inter-word

TextJustification.InterCharacter
inter-char

Vertical

TextJustification.None
v-none

TextJustification.InterWord
v-inter-word

TextJustification.InterCharacter
v-inter-char

@codecov

codecov Bot commented Jun 30, 2022

Copy link
Copy Markdown

Codecov Report

Merging #285 (dd419f4) into main (cab42e5) will increase coverage by 0%.
The diff coverage is 82%.

@@          Coverage Diff          @@
##            main    #285   +/-   ##
=====================================
  Coverage     83%     83%           
=====================================
  Files        222     222           
  Lines      12211   12266   +55     
  Branches    1761    1779   +18     
=====================================
+ Hits       10191   10248   +57     
+ Misses      1591    1586    -5     
- Partials     429     432    +3     
Flag Coverage Δ
unittests 83% <82%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/SixLabors.Fonts/TextMeasurer.cs 83% <58%> (-8%) ⬇️
src/SixLabors.Fonts/TextLayout.cs 87% <90%> (+1%) ⬆️
src/SixLabors.Fonts/TextOptions.cs 100% <100%> (ø)
src/SixLabors.Fonts/Unicode/UnicodeTrie.cs 88% <100%> (ø)
src/SixLabors.Fonts/GlyphLayout.cs 80% <0%> (+3%) ⬆️
src/SixLabors.Fonts/Unicode/BidiData.cs 85% <0%> (+9%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cab42e5...dd419f4. Read the comment docs.

@JimBobSquarePants
JimBobSquarePants requested a review from a team June 30, 2022 11:50
@tocsoft

tocsoft commented Jun 30, 2022

Copy link
Copy Markdown
Member

You seem to have zero test coverage for validating the justification.

2 test cases I can think of,

  1. For both justification types you want to run TextMeasurer.TryMeasureCharacterBounds and validate the first and last visible character of each line approximately aligns with the target bounding box.

  2. for all justification types track the location of a few well distributed characters (i.e. the position of the e in elit and the m in maximus on the 2nd line of your samples.) that have different locations in all the layouts. and validate those.

Additionally how does this effect rtl and ttb etc, they should also be accounted for in the tests.

@JimBobSquarePants

Copy link
Copy Markdown
Member Author

@tocsoft Yeah... Trying to figure out some way of testing things. It's a meaty gap.

I think I can also calculate for the InterCharacter value that the sum of each glyph advance is equal to the wrapping length.

Regarding RTL and TTB layouts. They all use the same ScaledAdvance to operate so this will automatically work for all layout versions. I'll try and add tests though.

@JimBobSquarePants

Copy link
Copy Markdown
Member Author

@tocsoft I've added some tests. In order to do so I was required to add a new method to TextMeasurer which allows getting the size of each glyph. This end up making the class more symmetrical.

@JimBobSquarePants
JimBobSquarePants merged commit 691f770 into main Jul 2, 2022
@JimBobSquarePants
JimBobSquarePants deleted the js/justified-text branch July 2, 2022 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants