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

[ttLib/pyftmerge] Handle cmap merge better #635

Merged
merged 1 commit into from
Jul 13, 2016

Conversation

yincrash
Copy link
Contributor

Changed the merge algorithm to properly handle cmap subtables
of type 4 and 12 with platform id and encoding ids of 3/1 and 3/10
respectively. All other subtables are not merged and ignored.
The resulting merged cmap table includes a subtable of format 4/3/1
and a format 12 subtable iff there are mappings outside of the BMP.

If one font has two codepoints that point to the same glyph,
and another font has the same code points pointing to two other glyphs,
keep the behavior where the first replacement glyph is stored in 'locl'
and output the third glpyh (or more) to let the user know that they were
dropped, instead of failing to merge the font.

Fixes #444
Fixes #322

@yincrash
Copy link
Contributor Author

I was trying to merge Emoji fonts so I could get some color emoji on my Windows 7 machine occasionally when I discovered that pyftmerge doesn't output valid Windows font files if there are any code points outside the BMP, so I originally wrote a script to generate a cmap4 table, then wrote this patch to fix it in FontTools. Then got a little carried away to try and fix other cmap merging related errors.

@coveralls
Copy link

coveralls commented Jul 12, 2016

Coverage Status

Coverage increased (+0.2%) to 71.036% when pulling fcad0c2 on yincrash:cmap_merge into 3b8387f on behdad:master.

@coveralls
Copy link

coveralls commented Jul 12, 2016

Coverage Status

Coverage increased (+0.2%) to 71.071% when pulling fcad0c2 on yincrash:cmap_merge into 3b8387f on behdad:master.

class CmapMergeUnitTest(unittest.TestCase):
def setUp(self):
self.merger = Merger()
self.table1 = ttLib.getTableClass('cmap')('cmap')
Copy link
Member

Choose a reason for hiding this comment

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

Use ttLib.newTable()

@behdad
Copy link
Member

behdad commented Jul 12, 2016

Thanks for this! I left a few minor comments. Please address those and I'll merge.

Changed the merge algorithm to properly handle cmap subtables
of type 4 and 12 with platform id and encoding ids of 3/1 and 3/10
respectively. All other subtables are not merged and ignored.
The resulting merged cmap table includes a subtable of format 4/3/1
and a format 12 subtable iff there are mappings outside of the BMP.

If one font has two codepoints that point to the same glyph,
and another font has the same code points pointing to two other glyphs,
keep the behavior where the first replacement glyph is stored in 'locl'
and output the third glpyh (or more) to let the user know that they were
dropped, instead of failing to merge the font.

Fixes fonttools#444
Fixes fonttools#322
@yincrash
Copy link
Contributor Author

Amended my commit with your recommendations :)

@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage increased (+0.1%) to 71.051% when pulling 9ed62af on yincrash:cmap_merge into 5cd1fba on behdad:master.

@behdad behdad merged commit 18b905b into fonttools:master Jul 13, 2016
@behdad
Copy link
Member

behdad commented Jul 13, 2016

Thanks!

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