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

Use fontations for shape planning #8

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Use fontations for shape planning #8

merged 2 commits into from
Sep 11, 2024

Conversation

dfrg
Copy link
Collaborator

@dfrg dfrg commented Jul 27, 2024

Replaces the ttf-parser code for script, langsys and feature lookup.

GSUB/GPOS is now fully fontations.

dfrg added 2 commits July 27, 2024 14:56
Replaces the ttf-parser code for script, langsys and feature lookup.

GSUB/GPOS is now fully fontations.
@dfrg dfrg mentioned this pull request Jul 28, 2024
.flatten()
}

fn script_index(&self, tag: hb_tag_t) -> Option<u16> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The non-trivial methods (e.g. the ones that do more than things like feature_list or feature_variations) warrant comment, particularly wrt what they are meant to be used for. Imagine the reader is relatively new to both Rust and shaping.

}

impl crate::hb::ot_layout::LayoutTableExt for LayoutTable<'_> {
// hb_ot_layout_table_select_script
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually link to the hb fns here and below

None
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

We've added lots of code that seems plausible to unit test and 0 unit tests. Seems sus.

Copy link
Collaborator

@rsheeter rsheeter left a comment

Choose a reason for hiding this comment

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

LGTM. Would like to see unit tests or an issue to add the in future.

@dfrg
Copy link
Collaborator Author

dfrg commented Sep 11, 2024

Filed issue #13 to describe the process so we can get these merged before they go stale.

@dfrg dfrg merged commit 2fb01f7 into main Sep 11, 2024
4 checks passed
@dfrg dfrg deleted the layout-table branch September 11, 2024 14:21
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