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

support for type 6/10/12 CMAPs (and thus Unicode astral-plane characters) #10

Merged
merged 1 commit into from
Jan 21, 2014

Conversation

pobrelkey
Copy link

Hello there,

I needed to create a document in Prawn using some astral-plane Unicode characters (i.e. ones with a codepoint larger than 16 bits), and noticed that this didn't work because TTFunk couldn't grok the relevant CMAP table in the font file. So I added support for a few more modern CMAP format types, and this now works fine for me.

Cheers,
Rob

@yob
Copy link
Member

yob commented Jan 1, 2013

It's embarrassing that ttfunk is so untested. This feels like behavior that we want, but I'm also nervous of regressions.

@bradediger any thoughts on the best way forward here?

@bradediger
Copy link
Member

@yob Agreed on both counts. This is a lot of code to add without tests, but it's also a good feature to have.

ttfunk does have some "smoke tests" via Prawn... if ttfunk is broken in any nontrivial way, Prawn's specs will probably fail or the manual build will be wonky. But I'd definitely appreciate having a full unit test suite in ttfunk itself, especially for a change like this that doesn't have a corresponding example in Prawn.

And since I don't have a way to verify this change other than manual poke-testing, I think I'd like to see unit tests (even if they only cover this new functionality) before merging. I can't commit myself to being able to add a test suite to ttfunk anytime soon, but I think that's probably what we need before adding any more significant functionality.

@yob
Copy link
Member

yob commented Jan 1, 2013

@rlpelkey If I add the foundations for an rspec suite to ttfunk, can you add some basic specs for this new functionality?

It would also be good to add a text integration test to prawn that uses an astral plane character.

I'll also try to add some basic integration specs for core functionality to ttfunk.

@bradediger
Copy link
Member

Thanks for the offer @yob! Much appreciated.

@pobrelkey
Copy link
Author

@yob I'll add specs in ttfunk to test the affected code once you set up the suite, and see what I can do about an integration test in prawn in the meantime.

@yob
Copy link
Member

yob commented Jan 1, 2013

I don't really grok the finer detail of how ttfunk works, so I've just started at a high level and added the infrastructure for rspec and and some basic integration specs for TTFunk::File.

@rlpelkey - is that enough for you to add some specs for this PR? I'd suggest some integration specs as a starting point, but feel free to add unit tests too.

@eugene1g
Copy link

@rlpelkey - your PR really helped with processing the most recent TTFs from Apple (format12). Thank you for taking the time to submit it!

+1 "works for me"

@practicingruby
Copy link
Member

I don't see anything here that appears to be a risk for regressions, so I'd be okay with cutting a new release in time for the next major Prawn release (0.14.0 on January 15). Anyone have major concerns about that?

@emk
Copy link

emk commented Jan 21, 2014

Ah, excellent. I'll have a use for this very soon—I need character metric data on a bunch of astral plane characters. Thank you to everybody who's worked on getting it integrated.

practicingruby added a commit that referenced this pull request Jan 21, 2014
support for type 6/10/12 CMAPs (and thus Unicode astral-plane characters)
@practicingruby practicingruby merged commit 640b88e into prawnpdf:master Jan 21, 2014
@practicingruby
Copy link
Member

Merged! I will cut a 1.1.0 release of ttfunk soon and then update Prawn master to use it. If everything goes smoothly the Prawn 0.15.0 release will depend on the new ttfunk release with this code in it.

@emk
Copy link

emk commented Jan 21, 2014

Looks excellent, and I can confirm it's working here. Before my astral plane characters were all mapping to glyph ID 0 and giving me identical bounding boxes (actual characters deleted because github does not love the astral plane):

$ be ruby dump_metrics.rb 
Char: 78283 0
(330,-267)-(1306,1431)
Char: 77825 0
(330,-267)-(1306,1431)
Char: 78358 0
(330,-267)-(1306,1431)

And now they're showing actual glyphs IDs and useful metrics:

$ be ruby dump_metrics.rb 
Char: 78283 756
(90,-360)-(531,1531)
Char: 77825 298
(90,-362)-(1488,1534)
Char: 78358 831
(90,-13)-(1979,340)

Many thanks for the quick response! This will be a sanity-saver.

@practicingruby
Copy link
Member

A new release of ttfunk (1.1.0) has been cut, and Prawn's master branch has been updated to use it. Keep in mind that ttfunk still is pretty much untested and we're not currently even running code that walks this path via Prawn's tests or examples.

But since this is a bolt-on feature that doesn't affect existing use cases, I figure it's worth shipping to get some folks using it! We will certainly put more energy into ttfunk when Prawn has fewer fires to put out itself.

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

Successfully merging this pull request may close these issues.

6 participants