[glyphs] Handle RTL kerning#1570
Conversation
| } | ||
| } | ||
|
|
||
| #[allow(clippy::type_complexity)] // it's not _that_ bad |
There was a problem hiding this comment.
We have generally chosen to fix these. A type alias for the rather ugly map type seems like it would do it.
I would expect get_kerning to merely fetch, it's a bit weird for it to take a Cow and modify things. It's also deviating from the intended behavior that kerning is done, we're just fetching it at this point. Is there a reason we can't do this earlier such that get_kerning remains just a lookup?
There was a problem hiding this comment.
Added some docs explaining what's going on.
| assert!(bad_kerns.is_empty(), "{bad_kerns:#?}"); | ||
| } | ||
|
|
||
| // helpers: |
There was a problem hiding this comment.
This comment isn't helpful.
Why isn't parse_side impl From<&str> for KernSide (since it's infallible)?
There was a problem hiding this comment.
this is just a helper used in tests, not sure there's any clear advantage to having a From impl since it wouldn't ever be used anywhere else.
| } | ||
|
|
||
| font_info.font.kerning_ltr.get(our_id) | ||
| fn flip_class_side(s: &SmolStr) -> SmolStr { |
There was a problem hiding this comment.
I wonder if introducing a type (enum? there seem to be distinct cases) for kerning identifier and hanging some of this stuff off it would work out nicely?
There was a problem hiding this comment.
We turn this into an enum later, when we're actually generating the IR; this is about making the pre-IR RTL kerns look like the pre-IR RTL kerns.
rsheeter
left a comment
There was a problem hiding this comment.
Broadly LGTM, though I hope we can avoid the get_kerning makes changes aspect by altering where the work is done.
This matches glyphsLib, which just converts these into LTR kerning rules.
This matches glyphsLib, which just converts these into LTR kerning rules.