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

Cmap table 14 fix #20

Merged
merged 2 commits into from Aug 29, 2014

Conversation

Projects
None yet
2 participants
@practicingruby
Member

practicingruby commented Aug 29, 2014

A slightly modified version of #19.

mojavelinux and others added some commits Aug 29, 2014

ignore cmap tables with a format number > 14
- ignore cmap tables with a format number > 14 (CJK fonts)
- migrate rspec expectations to expect API
@practicingruby

This comment has been minimized.

Member

practicingruby commented Aug 29, 2014

Cmap subtables already have a method to determine whether they're supported, so we can use that rather than making a numeric comparison. This covers the format 2 edge case, and feels more maintainable to me.

@mojavelinux Please confirm that this still fixes the problem for you. If so, I'll merge.

@mojavelinux

This comment has been minimized.

Contributor

mojavelinux commented Aug 29, 2014

Super! You captured the essence of my pull request and came up with a proper API to fix it. A total win. 🍻

@practicingruby

This comment has been minimized.

Member

practicingruby commented Aug 29, 2014

And for our friends in the Linux distros that will inevitably ask the question, it appears that the M+ font is freely licensed: http://en.wikipedia.org/wiki/M%2B_Fonts#Licensing

@mojavelinux

This comment has been minimized.

Contributor

mojavelinux commented Aug 29, 2014

👍

Fix confirmed!

@mojavelinux

This comment has been minimized.

Contributor

mojavelinux commented Aug 29, 2014

Thanks again for taking time out of your day to address this issue. I really appreciate your efforts.

@practicingruby practicingruby merged commit 23af5c8 into master Aug 29, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@mojavelinux

This comment has been minimized.

Contributor

mojavelinux commented Aug 29, 2014

Yes, I forgot to mention the licensing for M+. Here's the clause I managed to track down when I included it in another open source project:

M+ FONTS Copyright (C) 2002-2014 M+ FONTS PROJECT

LICENSE_E

These fonts are free software.
Unlimited permission is granted to use, copy, and distribute them, with
or without modification, either commercially or noncommercially.
THESE FONTS ARE PROVIDED "AS IS" WITHOUT WARRANTY.

http://mplus-fonts.sourceforge.jp/mplus-outline-fonts/
@practicingruby

This comment has been minimized.

Member

practicingruby commented Aug 29, 2014

I've cut a TTFunk release and updated the stable / master branches of Prawn to use it. You can try those out if you want to confirm the fix in production.

@mojavelinux

This comment has been minimized.

Contributor

mojavelinux commented Aug 29, 2014

Fantastic. I'll update my libraries asap. I confirmed it in a local installation of a toolchain that uses it. I can guarantee I'll know whether this works in production because the client will be quick to inform if it doesn't (I have very high confidence they are going to be sailing smoothly now, though).

@practicingruby

This comment has been minimized.

Member

practicingruby commented Aug 29, 2014

@mojavelinux:

Thanks for the patch! Because it was accepted, you now have commit access to all of Prawn's repositories. Please read our contributor guidelines here, and happy hacking!

@mojavelinux

This comment has been minimized.

Contributor

mojavelinux commented Aug 29, 2014

Awesome! Thanks for the invite! I look forward to help maintaining and improving Prawn in whatever way I can. It's essential for Asciidoctor PDF and hopefully it will bring much deserved attention to this awesome family of libraries. Thanks again and I look forward to working with the group!

@mojavelinux mojavelinux deleted the cmap-table-14-fix branch Aug 29, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment