Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix fvar axis/instance name parsing and writing (fix #687) #694

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

Connum
Copy link
Contributor

@Connum Connum commented Apr 3, 2024

Description

Fixes parsing of name IDs in the fvar table (fix #687) and adds parsing/optional writing of the postScriptNameID field.
I also refactored name handling a bit. Having the fvar make function modify the name table by creating new entries (without checking if a name entry already existed) was a bad design choice and shouldn't be done during writing anyway, so I removed the addName() function completely. New names should be added to the name tables and then be referenced by ID in the fvar table. When we add "comfort" functions to the variable features later, these should handle this accordingly, but for now this has to be done manually.

Motivation and Context

Parsing the names for axes/instances in the fvar table did not work correctly in all cases. Name ID values 2 and 17 for the instance name, as well as 6 for the postScriptNameID were not parsed correctly (and postScriptNameID was so far not parsed at all). These are allowed according to the spec:

How Has This Been Tested?

Test font provided in #687 and additional tests created.

Types of changes

  • 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:

  • I did npm run test and 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 CONTRIBUTING document.

@eweracs
Copy link

eweracs commented Apr 8, 2024

Thanks a lot, Constantin! This is a rather crucial fix for a serious bug, is there some way this could be reviewed in the near future?

@Connum
Copy link
Contributor Author

Connum commented Apr 8, 2024

is there some way this could be reviewed in the near future?

I hope so! Pinging @yne @ILOVEPIE

Copy link
Contributor

@yne yne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Connum 💪

@Connum Connum requested a review from yne April 8, 2024 19:32
@Connum
Copy link
Contributor Author

Connum commented Apr 8, 2024

Rebased onto current master, needs another review to be able to merge

@Connum Connum assigned Connum and yne Apr 9, 2024
@Connum
Copy link
Contributor Author

Connum commented Apr 10, 2024

Oh boy, the rebase broke something... I'll have a look at it later.

@Connum
Copy link
Contributor Author

Connum commented Apr 10, 2024

@yne ready for final review

@Connum Connum merged commit 86b289f into opentypejs:master Apr 11, 2024
1 check passed
@Connum Connum deleted the fix/687 branch April 11, 2024 06:57
@Connum Connum added this to the Release 2.0.0 milestone Apr 11, 2024
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.

fvar.instances returns empty name entry if nameID is 17 (preferredSubfamily)
3 participants