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

[makeotfexe] generates incorrect secondary lookups with useMarkFilteringSet flag #538

Merged
merged 2 commits into from
Aug 10, 2018

Conversation

readroberts
Copy link
Contributor

[makeotfexe] generates incorrect secondary lookups with useMarkFilteringSet flag · Issue #196

Fix bug by adding check that lookup flag and mark filter sets match, before adding a new substitution rule to the current secondary lookup table.

@@ -2229,7 +2229,7 @@ static Label addAnonRule(hotCtx g, GSUBCtx h, GNode *pMarked, unsigned nMarked,

si = dnaINDEX(h->anonSubtable, i - 1);
/* Don't need to match lkpFlag */
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment about not needing to match lkpFlag outdated now?

@miguelsousa
Copy link
Member

  1. Should the test case include a ligature substitution lookup? Asking because of Norbert's comment

  2. Should the hotconv library get a version bump?

@readroberts
Copy link
Contributor Author

@miguelsousa @cjchapman Norbert's comment is misleading - his test case did have a ligature substitution, but that lookup did not have a problem. The bug showed up in the contextual single substitutions. In any case, the bug fix will apply to all the lookup types, so I think the current test is sufficient.

I do think the hotconv library should get bumped. However, I am not clear on how often we need to do this - for every change, or for every release, which may span several changes to makeotfexe/hotconv library. There is some overhead; as long as we not skip the name table Version string, all the ttx files in an expected_output directory need to be updated whenever either hotconv or makeotfexe versions are bumped.

The comment is indeed out of date- I will fix that immediately.

@miguelsousa
Copy link
Member

miguelsousa commented Aug 9, 2018

@readroberts I think it's good to bump the version every time that library is changed, even though we may not issue afdko releases after each change. I'm aware of the overhead this creates, and I've been meaning to add an option to the runner differ to address it. I'll take this opportunity to do it, and finish the PR.

…ingSet flag · Issue #196

When makeotf is processing contextual substitution rules, it accumulates the substitution rules in a single secondary lookup, even when the lookup flags and mark filtering sets differ.

Create test case in makeotf_test.py, with data files that produce the current incorrect output.
Validate that makeotf produces the expected font.
…ingSet flag · Issue #196

Fix bug by adding check that lookup flag and mark filter sets match, before adding a new substitution rule to the current secondary lookup table.

This is already done in for the equivalent case in GPOS.

Update expected_output file to be the correct output.

Validate that makeotf produces the expected font.
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.

3 participants