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

Add initial implementation of meta #1237

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Hoolean
Copy link

@Hoolean Hoolean commented Nov 13, 2024

Hello!

This PR adds adds read and write for the meta table, including a codegen definition, some manual parts to handle the offset slices, and plumbing to surface the table within read-fonts and write-fonts.

This is my first contribution, so where possible I've borrowed liberally from the implementation of other tables, especially name (for its handling of offset prescribed-length data).

This is an early implementation, and has not had the manual and automated testing it will require yet. Additionally, there is an implementation of the FontWrite trait for &[u8] that may be redundant.

As such, I'm making the PR as a draft for now, until I can gather feedback and dig a bit deeper :)

Note: this is a personal contribution independent of my employer, and so I've submitted from a fork under my personal profile and email to make this distinction

This commit adds the code necessary to read and write the `meta` table,
including its codegen definition, manual extras to handle the variable
length data fields, and plumbing to surface it within `read-fonts` and
`write-fonts`.

Where possible, this commit borrows liberally from the implementation of
other tables, especially `name` (for its handling of variable length
data).

This is an early implementation, and as such requires manual and
automated testing. Additionally, there is an implementation of the
FontWrite trait for &[u8] that may be redundant.
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Nice, I think this is the right general direction, but after staring at it for a while I think it makes sense for us to use more types to describe the actual metadata contents; and since figuring out how best to do that requires more or less just writing the implementation I took the liberty of doing that in #1244.

Overall though I think this was a reasonable first pass. If you're happy with #1244 we can merge that, and then as future work we can actually implement some types that enforce the spec for the LangScriptTag types.

Oh: we also need some test data, that's missing from #1244 and it's complicated enough to need it.

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