search

robotools / fontMath

A collection of objects that implement fast font, glyph, etc. math.
MIT License
42 stars 17 forks source link

Test groups for equality before doing fontMath kerning math #245

Open benkiel opened 3 years ago

benkiel commented 3 years ago

This fixes #22, but will be breaking to a lot of code (though it's more correct). Perhaps better to add a strict option?

benkiel commented 3 years ago

@behdad @typesupply

codecov[bot] commented 3 years ago

Codecov Report

Merging #245 (4852acf) into master (2da8061) will increase coverage by 0.10%. The diff coverage is 97.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   88.46%   88.57%   +0.10%     
==========================================
  Files          13       13              
  Lines        2359     2390      +31     
  Branches      305      307       +2     
==========================================
+ Hits         2087     2117      +30     
  Misses        195      195              
- Partials       77       78       +1
Flag Coverage Δ
unittests 88.57% <97.22%> (+0.10%) :arrow_up:

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

Impacted Files Coverage Δ
Lib/fontMath/mathKerning.py 90.69% <85.71%> (-0.30%) :arrow_down:
Lib/fontMath/test/test_mathKerning.py 99.06% <100.00%> (+0.13%) :arrow_up:

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 2da8061...4852acf. Read the comment docs.

typesupply commented 3 years ago

I'm on a deadline now, so this will take me at least a few days to review. The non-equal groups was a feature, not a bug, but I'll have to try to remember why I did it the way that I did. I wrote this particular bit of code around 2003 and all that I remember off the top of my head is that it was very difficult. 😟

benkiel commented 3 years ago

I think that this may be better with a strict option, so one can decide what they want, the default set to False, will re-work. No rush on this at all, the issue has been sitting a long time.

behdad commented 3 years ago

Thanks Ben. I see Tal's comment about this being intentional.

If, say, group1 has a particular glyph A in a group, but other side doesn't have A in any group, one might want to assume that it was an omission in group2 and hence add it anyway? I find that dubious.

The alternative is to split the groups when mismatches occur. We have code to do that in fontTools.misc.classifyTools.