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

Implement plugins to support (subset) fonts in standalone CFF1 and Type1 format #704

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Connum
Copy link
Contributor

@Connum Connum commented Apr 24, 2024

Description

This PR implements a simple yet flexible plugin API and demonstrates its use with two plugins for parsing additional font formats, as well as some fixes.

The plugin API has not been documented in the README yet. It may still change drastically before we finally release 2.0.0, as we should figure out in which other places plugin hooks make sense and what data to supply to each entry point.

  • build and dist separate plugin files
  • gracefully parse partial font data from extracted subset fonts without throwing too early
  • parse CFF1 standalone font files
  • parse PostScript/PS1/T1/Adobe Type 1 format (.ps, .psa, .psb)
  • fix some issues in the Glyph Inspector regarding contours of non-variable fonts
  • keep comment blocks containing license information (required for meeting legal requirements and also stating our license in the minified dist files)
  • fix nGlyphs calculation: count was treated differently for CFF1 and CFF2, but is actually the same (one more offset value than glyphs)

Motivation and Context

My PR #648 tried to achieve the graceful parsing by implementing a custom logger functionality, but that idea didn't seem to get any traction. This PR now achieves the same by logging warnings and errors to the console instead of throwing excepions in several places.

How Has This Been Tested?

Added new test fonts and tests for the plugins.

Screenshots (if appropriate):

image

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.

@Connum Connum added this to the Release 2.0.0 milestone Apr 24, 2024
@Connum Connum requested a review from ILOVEPIE April 24, 2024 13:13
@Connum
Copy link
Contributor Author

Connum commented May 8, 2024

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.

Love this new plugin approach !

We can now start considering offering a woff2 plugin !

@@ -317,7 +317,7 @@ function addGlyphNamesAll(font) {
const c = charCodes[i];
const glyphIndex = glyphIndexMap[c];
glyph = font.glyphs.get(glyphIndex);
glyph.addUnicode(parseInt(c));
glyph && glyph.addUnicode(parseInt(c));
Copy link
Contributor

@yne yne May 8, 2024

Choose a reason for hiding this comment

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

you already used the optional chaining .? syntax early on,
so keep it unified (I have no preference, so it's up to you to chose one :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, did I use optional chaining? I thought that would throw an error with the current reify setup... I'll take a look at it!

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,33 @@
export const plugins = [];
Copy link
Contributor

@yne yne May 8, 2024

Choose a reason for hiding this comment

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

would it be cleaner to abstract/hide this plugins array with a registerPlugins
so we could do all the registration check (typeof == function etc...) at registration time

(just a suggestion)

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 thought about that as well, but then we'd also need methods to unregister a plugin and change the execution order in case two plugins modify the same entry point data. I think everyone should be familiar with modifying an array, so we should be fine leaving this without abstraction.

let isResponsible = new WeakMap();

const plugin_cff1file = {
parseBuffer_signature: function(returnData, params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are two ways of declaring a method:

parseBuffer_signature: function(returnData, params) {
parseBuffer_signature(returnData, params) {

one of them bind the this at creation , the other at runtime
(maybe this would help avoid using those .call() usage ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll take a look at it

src/[email protected] Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants