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

Support vertical metrics tables #104

Merged
merged 3 commits into from
Nov 17, 2022
Merged

Support vertical metrics tables #104

merged 3 commits into from
Nov 17, 2022

Conversation

anthrotype
Copy link
Member

Will eventually fix #103

@anthrotype anthrotype marked this pull request as draft November 2, 2022 15:54
@anthrotype
Copy link
Member Author

I went for the name "HVhea" instead of "Hea" because the latter might be confused with "Head".

I kept the read_fonts::tables::{v,h}hea modules but they now only contain the const TAG and nothing more. I thought about adding an alias like pub type Hhea = HVhea; in both, but I wasn't sure whether that would be useful or not.

As for write_fonts crate, the original hhea.rs simply included the generated_hhea.rs and didn't export any additonal symbols (no TAG is defined in the write_fonts table modules, unlike the read counterpart), so I just renamed write_fonts::tables::{h,hv}hea and make that include the generated_hvhea.rs.

If this is deemed to go in the right direction, I can continue and similarly adapt hmtx to use a common struct with vmtx.

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.

Looks good! I have a minor suggestion for how we can structure the modules and exports, but overall I think this is the right approach.

resources/codegen_inputs/hvmtx.rs Outdated Show resolved Hide resolved
use font_types::Tag;

/// 'vmtx'
pub const TAG: Tag = Tag::new(b"vmtx");
Copy link
Member

Choose a reason for hiding this comment

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

okay, I have an idea for how we might want to structure this:

Here's what I think I would do:

  • make the hvmtx and hvhea modules marked pub(crate) in tables.rs.
  • manually reexport any shared structs (LongMetric in hvmtx, e.g.) from here, into each of the hmtx and vmtx modules
  • add pub type aliases for Hhea and Vhea in the appropriate modules, with the appropriate doc strings

So in this file we would add something like,

pub use super::hvmtx::LongMetric;

/// The [vmtx (Vertical Metrics)](https://docs.microsoft.com/en-us/typography/opentype/spec/vmtx) table
pub type Vmtx = super::hvmtx::HVmtx;

and do this same trick in the others.

Copy link
Member Author

@anthrotype anthrotype Nov 3, 2022

Choose a reason for hiding this comment

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

I can do that and add those type aliases, but what should methods like TableProvider::hhea() return, the shared HVhea struct or the type-aliased Hhea one?

- fn hhea(&self) -> Result<tables::hvhea::HVhea<'a>, ReadError>
+ fn hhea(&self) -> Result<tables::hhea::Hhea<'a>, ReadError>
- fn vhea(&self) -> Result<tables::hvhea::HVhea<'a>, ReadError>
+ fn vhea(&self) -> Result<tables::vhea::Vhea<'a>, ReadError>

Wouldn't we then basically hide from the API the fact that those tables are actually binary-encoded the same way and one can expect to use them interchangeably in user code?

Copy link
Member

Choose a reason for hiding this comment

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

that's a nice point. The final answer will depend on what we resolve for #109. let's leave this as is (no type aliases) for now, and we can revisit once we know our plan for the tag trait.

@anthrotype anthrotype marked this pull request as ready for review November 3, 2022 18:49
@anthrotype anthrotype changed the title WIP: Support vertical metrics tables Support vertical metrics tables Nov 3, 2022
@anthrotype anthrotype force-pushed the vertical-metrics branch 2 times, most recently from 1d7160f to 825b182 Compare November 7, 2022 16:07
@anthrotype
Copy link
Member Author

I just rebased this on main, I hope I didn't screw things up. At least the bots are happy 🤖

@anthrotype anthrotype merged commit 689cbe7 into main Nov 17, 2022
@anthrotype anthrotype deleted the vertical-metrics branch November 17, 2022 16:14
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.

support vertical metrics tables vhea/vmtx
2 participants