[read-fonts] layout: selecting scripts for shaping #1154
Conversation
Adds HB equivalent functionality for selecting an OpenType script for a given Unicode script code.
| /// Index of the script in the [`ScriptList`]. | ||
| pub index: u16, | ||
| /// True if a script was chosen that wasn't in the requested list. | ||
| pub is_fallback: bool, |
There was a problem hiding this comment.
Speculation: I wonder if a Rusty SelectedScript should be an enum instead?
There was a problem hiding this comment.
That's an equivalent representation but an enum with all variants carrying the same payload where the variant itself is secondary information is often awkward to use. In other words, we'll always want the tag/index but might occasionally care if we're using a fallback so it doesn't make sense to primarily categorize on that aspect.
| /// Generated by [`script_tags_from_unicode`]. | ||
| #[derive(Copy, Clone, PartialEq, Eq, Default)] | ||
| pub struct ScriptTags { | ||
| tags: [Tag; 3], |
There was a problem hiding this comment.
this merits a comment explaining what it is
There was a problem hiding this comment.
The comment above says "A prioritized list of OpenType script tags mapped from a Unicode script tag." but I added some additional context.
| len: usize, | ||
| } | ||
|
|
||
| impl Deref for ScriptTags { |
There was a problem hiding this comment.
Optional, would an explicit call like as_tags() be overly horrid? - not a huge fan of Deref.
There was a problem hiding this comment.
as_slice() would be the idiomatic conversion fn here, but since this type is essentially an array deref to slice is totally reasonable (having both would be the most idiomatic approach)
There was a problem hiding this comment.
Agreed with Colin on both points. Added as_slice() for those that prefer explicit calls.
| } | ||
|
|
||
| // See <https://github.com/harfbuzz/harfbuzz/blob/63d09dbefcf7ad9f794ca96445d37b6d8c3c9124/src/hb-ot-tag.cc#L84> | ||
| fn new_tag_from_unicode(unicode_script: Tag) -> Option<Tag> { |
There was a problem hiding this comment.
I've definitely impl'd this somewhere before... looks like it's in fontbe? Let's open an issue to deduplicate.
There was a problem hiding this comment.
can we just find it and fix it now? - that smells like an issue that won't actually get fixed
There was a problem hiding this comment.
it's in a different repo, at the very least we need to do a release of read-fonts before we can change anything there.
There was a problem hiding this comment.
Oh sorry, I skimmed pastit being in fontc not here. Then +1 to issue or fast-follow PR to update fontc.
There was a problem hiding this comment.
@cmyr I exposed a UNICODE_TO_NEW_OPENTYPE_SCRIPT_TAGS const that is basically the same as NEW_SCRIPT_TAGS in fontbe except the key is &[u8; 4] instead of &str. Does that work for you?
There was a problem hiding this comment.
i'm not going to have time to dig into this before I take off, so let's merge as-is and we can figure that out when I get back.
| len: usize, | ||
| } | ||
|
|
||
| impl Deref for ScriptTags { |
There was a problem hiding this comment.
as_slice() would be the idiomatic conversion fn here, but since this type is essentially an array deref to slice is totally reasonable (having both would be the most idiomatic approach)
Adds HB equivalent functionality for selecting an OpenType script for a given Unicode script code.