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

Improve handling of fea-rs output #306

Merged
merged 1 commit into from
May 26, 2023
Merged

Improve handling of fea-rs output #306

merged 1 commit into from
May 26, 2023

Conversation

rsheeter
Copy link
Contributor

@rsheeter rsheeter commented May 26, 2023

Split the single feature font from fea-rs into pieces and include those pieces in the final font instead of running fea-rs and then just abandoning the results if it succeeded.

This reveals that for Oswald GSUB doesn't match and we aren't ending up with a GPOS at all; filed #308.

@rsheeter rsheeter force-pushed the os2_3 branch 3 times, most recently from f62b28e to 82bfc69 Compare May 26, 2023 20:50
@rsheeter rsheeter changed the title [WIP] Compute usMaxContext Improve handling of fea-rs output May 26, 2023
@rsheeter rsheeter marked this pull request as ready for review May 26, 2023 20:59
@rsheeter rsheeter mentioned this pull request May 26, 2023
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.

one note inline.

Comment on lines +132 to +143
let font = FontRef::new(&buf).unwrap();
debug!(
"Built features, gpos? {} gsub? {}",
font.gpos().is_ok(),
font.gsub().is_ok()
);
if let Ok(gpos) = font.gpos() {
context.set_gpos(gpos.to_owned_table());
}
if let Ok(gsub) = font.gsub() {
context.set_gsub(gsub.to_owned_table());
}
Copy link
Member

Choose a reason for hiding this comment

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

this is a very convoluted way of trying to do what we want, I think? Wouldn't it be nicer if we just had some way of retrieving the bytes of a specific table from the FontBuilder? (we'd need to add a method there, but nothing crazy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way it's typed which is nice perhaps? - I agree it feels a bit clunky but ... it does express the intent in fairly straight forward terms

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is a sign that fea-rs should be returning something other than a FontBuilder?

It would also be nice if fea-rs did not return a type from write-fonts directly, since this is a reason why every breaking change in write-fonts is currently a breaking change in fea-rs...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is a sign that fea-rs should be returning something other than a FontBuilder?

It would also be nice if fea-rs did not return a type from write-fonts directly, since this is a reason why every breaking change in write-fonts is currently a breaking change in fea-rs...

although maybe there's no good work around from this? it certainly doesn't change if I'm returning a set of write-fonts tables instead of a write-fonts TableBuilder. But it still might be a better API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this is a sign that fea-rs should be returning something other than a FontBuilder?

It would also be nice if fea-rs did not return a type from write-fonts directly, since this is a reason why every breaking change in write-fonts is currently a breaking change in fea-rs...

Returning any type from write-fonts has the same effect. Raw bytes, or something close to raw bytes (map of tag to raw bytes?) would work. I don't want to have multiple write-fonts in my dependency tree but perhaps it would bumping through versions simpler. Or we could just live with it; I would expect the rate of write-fonts breaks to decline over time as it stabilizes.

@rsheeter rsheeter merged commit 044c7be into main May 26, 2023
@rsheeter rsheeter deleted the os2_3 branch May 26, 2023 21:31
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