search

prawnpdf / ttfunk

Font Metrics Parser for Prawn
http://prawnpdf.org
Other
125 stars 75 forks source link

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

Closed pobrelkey closed 10 years ago

pobrelkey commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

@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 commented 11 years ago

Thanks for the offer @yob! Much appreciated.

pobrelkey commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

@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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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.