Improve PairPos validation #589
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This ensures that the arrays are all the correct shape, and that all of the value records report the expected format. I believe we are getting these things wrong in fontc/fea-rs, in some cases.
Another check to ensure that all value records report the correct format.
|
I suggest filing issues for additional logic as we identify specific cases +1 to enhance write-fonts w/builders |
rsheeter
reviewed
Aug 31, 2023
| }) | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
I don't love the nesting, how might we structure this more like an iter chain where it tends to extend down more than right?
There was a problem hiding this comment.
Agreed. currently the vast majority of validation code is auto-generated, which makes this less painful. I think that improving the API is out of scope for this PR, but I'll open an issue to look into improving this somewhat. I'm not sure that an iter-like API is possible since it doesn't really support scoping, but we might be able to at least reduce the amount of nesting required.
rsheeter
approved these changes
Aug 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In write-fonts we have a 'validation' pass before compilation, which is an opportunity to go and inspect the tables we're writing and make sure that the data is sane. A bunch of these checks are generated automatically, but a lot of the validation logic we would want is case-specific; in this case we have a mechanism for naming additional methods to be called in particular places during validation.
We haven't made a ton of use of this custom validation logic, but we probably should, since it helps us be much more confident in the data that we're writing.
This adds some custom validation logic to the two PairPos subtable formats; there was something fishy happening here that was messing up my table packing.
Another thing I'm reminding of while doing this work is how tricky it is to construct these tables correctly without the assistance of a builder; there's a bunch of builders in
fea-rs, and they should probably eventually migrate towrite-fonts.