googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
151 stars 43 forks source link

Raise error for overloaded codepoints, and drop codepoints in colour-layer glyphs #739

Closed Hoolean closed 1 year ago

Hoolean commented 1 year ago

Hello!

util.makeUnicodeToGlyphNameMapping() intends to raise an error when multiple glyphs claim the same code point, but unfortunately a typo leads this error to be swallowed. Instead, with the current behaviour, we silently give the first glyph that claims an overloaded code point precedence.

This PR adds a raise to fix the typo that caused the error to be swallowed, and adds a test for the intended behaviour.

Next Steps

  1. [x] Fix failing COLR test:
    • This test UFO specifies a code point for the 'a' glyph in its colour layers; when these are exploded into the default layer, they clash with the default layer 'a', which specifies the same code point.
    • Should colour glyphs be allowed to specify code points? If not, should we warn and strip such code points or raise an InvalidFontData error?
  2. [x] Check whether this issue was reachable through the top-level compilation functions:
    • The COLR test failing suggests that this issue may have been reachable through our top-level compileTTF() functions, as opposed to solely through calling the util function directly.
    • If we do not have one already, we could add a general test for compileTTF() to check that ufo2ft raises an appropriate error when supplied with a UFO that overloads code points.
    • To be safe, we could try compiling a small corpus of test fonts to see if any fonts are accidentally relying on this implicit behaviour (cc @madig).
anthrotype commented 1 year ago

good catch, thanks!

Should colour glyphs be allowed to specify code points?

no they should not. the filter that creates them should probably unset any codepointds they may have. No need to warn IMO.

add a general test for compileTTF() to check that ufo2ft raises an appropriate error when supplied with a UFO that overloads code points

you can add one in integration_test.py, where compileTTF is tested I believe; I suggest you make the test UFO from code using ufoLib2/defcon APIs right before you compile and assert; better than checking in a lot of boilerplate test files.

we could try compiling a small corpus of test fonts to see if any fonts are accidentally relying on this implicit behaviour]

I don't think these double encoded glyphs happen frequently; and it is a font source bug that is worth raising an error for, as compiler can't really encode the ambiguous mappings. Not too worried about sudden breakages.

Hoolean commented 1 year ago

The commits I have just pushed modify the filter to unset codepoints when exploding colour glyphs, and add some dedicated tests for this behaviour. When I have added integration tests for the handling of overloaded codepoints, I will mark the PR as Ready for Review :) Thanks for the feedback Cosimo!

Hoolean commented 1 year ago

I have now completed the PR with an integration test; as discussed, the UFO is constructed in-memory to avoid unnecessary boilerplate 🚀

Running the integration test on main shows that previously no error was raised at the top-level when compiling sources with duplicate codepoints. Because of this, the next release of ufo2ft may reveal this source issue to users for the first time, where it will need to be corrected for compilation to succeed.

anthrotype commented 1 year ago

the next release of ufo2ft may reveal this source issue to users for the first time, where it will need to be corrected for compilation to succeed.

it'd say that's as an improvement 👍