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

Use opentype.js to test font data #102

Closed
wants to merge 16 commits into from

Conversation

yisibl
Copy link
Contributor

@yisibl yisibl commented Mar 10, 2021

Through this PR, we can better test fonts and no longer use binary-based comparison methods.

@yisibl
Copy link
Contributor Author

yisibl commented Mar 10, 2021

@puzrin PTAL.

@yisibl
Copy link
Contributor Author

yisibl commented May 9, 2021

@puzrin We really need to test based on the font table, I recently upgraded from v2.0.0 to the latest version and there were a lot of problems. Without these tests, it is difficult to troubleshoot and find problems.

Only when the 7th bit of os2.fsSelection is turned off, the usWinAscent/usWinDescent will be used to calculate the row height.

The spec says:

> Early versions of this specification suggested that the usWinAscent value be computed as the yMax for all characters in the Windows “ANSI” character set. For new fonts, the value should be determined based on the primary languages the font is designed to support, and should take into consideration additional height that may be required to accommodate tall glyphs or mark positioning.
@@ -28,10 +28,10 @@ function svg2ttf(svgString, options) {
font.copyright = options.copyright || svgFont.metadata;
font.description = options.description || 'Generated by svg2ttf from Fontello project.';
font.url = options.url || 'http://fontello.com';
font.sfntNames.push({ id: 2, value: options.subfamilyname || svgFont.subfamilyName || 'Regular' }); // subfamily name
font.sfntNames.push({ id: 4, value: options.fullname || svgFont.id }); // full name
font.sfntNames.push({ id: 2, value: (options.subfamilyname || svgFont.subfamilyName || 'Regular').trim() }); // subfamily name
Copy link
Member

Choose a reason for hiding this comment

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

I don't like idea of auto-trim. IMO that's a garbage for artificial case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer svg2ttf to filter out these extraneous whitespace characters in order to produce a cleaner font file. Similar processing is also done in ufo2ft, of course we can only consider familyName and styleName.

https://github.com/googlefonts/ufo2ft/blob/a5267d135d5a8dba7e6a5d3ac44139afa6159429/Lib/ufo2ft/fontInfoData.py#L74

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, i do not agree, IMO tool should write data exactly as been passed. IMO if you need trimming, it should be filtered on upper app level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also changed my mind, and now I agree with you.

@yisibl
Copy link
Contributor Author

yisibl commented May 20, 2021

Sorry, please let me test in the fork's svg2ttf-new first. I need to solve the problem of line-height in Windows first(#105). Later, I will deploy to our product to test, if there is no problem, I will clean up this PR.


// it('bin compare', function () {
// assert.deepStrictEqual(new Uint8Array(svg2ttf(src, { ts: 1457357570703 }).buffer), dst);
// });
Copy link
Contributor Author

@yisibl yisibl May 20, 2021

Choose a reason for hiding this comment

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

We no longer need binary-based testing.

@puzrin
Copy link
Member

puzrin commented May 21, 2021

@yisibl see master updates. IMO this PR has no more worth to merge.

@yisibl
Copy link
Contributor Author

yisibl commented May 24, 2021

@yisibl see master updates. IMO this PR has no more worth to merge.

I can resubmit a new PR.

@puzrin
Copy link
Member

puzrin commented May 24, 2021

@yisibl if you think something important was missed in master commits - please do PR, i will review.

@puzrin
Copy link
Member

puzrin commented May 24, 2021

IMO master is ready for release, but i'll be waiting for your comments for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants