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

Fixed gasp support bugs per #738 #739

Merged
merged 3 commits into from
Sep 9, 2024
Merged

Conversation

Balearica
Copy link
Contributor

Context

See #738. In short, write support for the gasp table was implemented recently in #595, however is broken. Any font written by opentype.js that includes a gasp table cannot be re-imported by opentype.js.

Changes

This PR makes two changes to resolve. First, it fixes the typo in the gasp write function, which was trying to iterate over the integer gasp.numRanges rather than the array gasp.gaspRanges. Second, it wraps the entire gasp read function in a try block. This should prevent any future issues where failing to parse this optional table prevents a font from being read.

How Has This Been Tested?

I confirmed that the Changa-Regular.ttf font could be saved/read using toArrayBuffer/opentype.parse, which (as described in #738) previously resulted in an error.

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 Contribute README section.

@Connum
Copy link
Contributor

Connum commented Jul 9, 2024

Please add a test that will prevent this issue from being reintroduced in the future.

@Balearica
Copy link
Contributor Author

I added a test that confirms fonts written with a gasp table can be read, and that the gasp table is identical to the original.

@Connum
Copy link
Contributor

Connum commented Jul 9, 2024

I added a test that confirms fonts written with a gasp table can be read, and that the gasp table is identical to the original.

Very nice, thanks! We shall have a review on this fix soon.

@Connum Connum requested review from ILOVEPIE and yne July 9, 2024 08:27
@Connum Connum added the bug label Jul 9, 2024
@Connum Connum added this to the Release 2.0.0 milestone Jul 9, 2024
@Connum Connum linked an issue Jul 9, 2024 that may be closed by this pull request
test/tables/gasp.spec.mjs Outdated Show resolved Hide resolved
src/opentype.mjs Show resolved Hide resolved
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 for the fix :)
just 2 simple questions/suggestion but no issue overall

@Connum
Copy link
Contributor

Connum commented Sep 9, 2024

Hi, (why) are we waiting for another review? It looks to me as if this is ready to be merged.

@yne
Copy link
Contributor

yne commented Sep 9, 2024

Hi, (why) are we waiting for another review? It looks to me as if this is ready to be merged.

I've approved but the repo owner seems to want 2 approval, and @ILOVEPIE don't seems available to approve.

can't you also approve ? so there is no need to bypass the process and force the merge

@Connum Connum merged commit aa8ad76 into opentypejs:master Sep 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gasp write support broken, opentype.js cannot read the fonts it creates
3 participants