opentype.js
opentype.js copied to clipboard
os2: Fill Panose when writing font
Parsing a font from a buffer and writing it back to a buffer changes all Panose values to zero. This PR fixes this.
Description
[why] When an existing font file is read from a buffer and then written back into a buffer all Panose values will be zero.
The problem has been most likly been introduced with PR #630 (but I did not check that).
There are two disjunct data structures in the font.tables.os2 object that describe the panose values:
- An array with the Panose values
.panose = [ 1, 2, ...] - Dedicated properties for each Panose value e.g.
.bFamilyType
The aforementioned PR seems to address only fonts created from scratch and not parsed from a buffer.
Writing out the font into a buffer will always use the dedicated Panose properties and ignore the .panose array.
Parsing a font does set the array but not the dedicated properties. They are not even existing then.
[how]
When an existing font is parsed the .panose array is filled (as before). But now the dedicated properties are also created and filled with the individual values.
[note] The written font is always using the dedicated values. If a user changes the panose array that will have no effect. There are no checks if the data is consistent. Having the same data in two disjunct structures is not so nice to handle.
Motivation and Context
I want to read a font file and write it (modified) back to a file; as unchanged as possible.
How Has This Been Tested?
Manual tests with ttx ;)
Screenshots (if appropriate):
Types of changes
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
Checklist:
- [x] I did
npm run testand all tests passed green (including code styling checks). - [ ] I have added tests to cover my changes.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the README accordingly.
- [ ] I have read the Contribute README section.
Well, now (with this PR) the user has both, the array and the individual properties (see image below). For writing only the individual properties are used (as before this PR).
To avoid confusion I would drop one of the two. Dropping the individual properties feels most natural for me as I always think about the Panose as indexed values. But dropping the array would be simpler and would not break any existing code (because changing the values never had any effect - only reading would be impossible if the array is dropped).
Anyhow, changing API with v2.0.0 is a good time to clean this up and drop one of the two. Well, thats just a suggestion.
I believe how Panose is represented in other applications can be important, to see what users might expect.
showttf
ttfdump
fontforge
Access via Python, via List (Array)
Glyphs3
(Glyphs3, the only commercial software here, does not believe in Panose and has hidden it away in a very raw form. See for example https://forum.glyphsapp.com/t/when-to-set-panose-numbers-typoascender-winascent-etc/10596)
Having the same data in two disjunct structures is not so nice to handle.
Yeah, that's a good point... We have this issue in other places as well I think. Proxy objects would be a solution, or we should drop the raw values alltogether as you suggested.
Added some test for os2 table parsing writing parsing (full round trip).
I struggled a bit with where to put the tests, as test/tables/ in principle maybe should only test stuff inside that modules; but we need other modules to check this completely, so maybe test in font, but then the tests are to specific to one table.
Hope you get what I mean.
Anyhow, this is at least some starting point, feel free to move/remove the tests ;-D