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

Initial support for parsing the PANOSE number #172

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

Conversation

inferiorhumanorgans
Copy link
Contributor

This includes a ton of boilerplate (apologies), a few tests, and adds checks for the PANOSE number in is_bold / is_italic / is_monospaced.

The PANOSE number also encodes a font weight that's distinct from the weight class in the OS/2 table so I've included a function to attempt to map the PANOSE weight to a weight class.

@@ -1408,16 +1408,38 @@ impl<'a> Face<'a> {
/// Checks that face is marked as *Italic*.
#[inline]
pub fn is_italic(&self) -> bool {
let panose_italic = self
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make it less functional... We can move a panose getter into a private function, similar to Face::style().

const SIZE: usize = 1;

fn parse(data: &[u8]) -> Option<Self> {
if data.is_empty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to check for that. FromData caller already did it.

@RazrFalcon
Copy link
Collaborator

It's definitely verbose... Do we really need all those enums? Maybe we can simply expose:

struct Panose {
    pub data: [u8; 10]
}

plus is_bold/etc. getters. And if someone wants to get some super specific/niche values they can do this themselves.

1250 LOC for a deprecated TrueType feature is a lot... Stuff like aspect_211 is a dead weight, imho.

@inferiorhumanorgans
Copy link
Contributor Author

It's definitely verbose... Do we really need all those enums? Maybe we can simply expose:

struct Panose {
    pub data: [u8; 10]
}

plus is_bold/etc. getters. And if someone wants to get some super specific/niche values they can do this themselves.

1250 LOC for a deprecated TrueType feature is a lot... Stuff like aspect_211 is a dead weight, imho.

Yeah it's a lot…

In terms of exposing a 10 byte slice, since it's a fixed length/offset field I think that's more suitable for RawFaceTables.

Can the enums be eliminated? Sure. At its most basic this code is currently there to support three questions (is the font face italic/bold/monospaced). The checks could be collapsed into simply checking the bytes manually, but I think there's a legibility tradeoff.

e.g.

fn is_italic() -> bool {
  (panose[0] == 2 && (panose[7] >= 9 && panose[7] <= 15)) ||
  (panose[0] == 3 && (panose[7] >= 6 && panose[7] <= 13))
}

Unsure what a happy medium would look like. I doubt that folks are clamoring to build an rtf parser/renderer, but as other use cases get added on (e.g. mapping weight to a weight class, or distinguishing between text / handwriting / symbol/wingdings font) I think having some sort of abstraction makes more sense.

@RazrFalcon
Copy link
Collaborator

We can keep only enums used by the public API. Aka is_italic and friends.
To my understanding, no one uses panose anymore. It's an outdated feature. So having a 1000 LOC for something no one will use is an overkill for me.

@RazrFalcon
Copy link
Collaborator

Any updates on this?

@inferiorhumanorgans
Copy link
Contributor Author

Apologies — I'll have time next week to dig into this.

@RazrFalcon
Copy link
Collaborator

No problem, just checking in.

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