Allow a user to pass an IO object when adding a font #730

Merged
merged 8 commits into from Jun 23, 2014

2 participants

@packetmonkey

This PR lets

require 'bundler'
Bundler.setup
require 'prawn'
require 'open-uri'
pdf = Prawn::Document.new
pdf.font_families.update(
  "MyFont" => {
    format: 'ttf',
    bold: open("https://mybucket.s3.amazonaws.com/myfont.ttf")
  }
)
pdf.font("MyFont", :style => :bold)
pdf.text "Some bold text"
pdf.render_file 'output.pdf'

Work as expected. The big thing to notice is when passing in an IO object we need to also pass in a format: key so Prawn knows what kind of format the font is. Normally prawn uses the file extension at the end of a path to determine what format the file is in, TTF, DFONT or AFM. Really I don't think this is ideal either, where I would prefer to see the TTF class be able to tell you if a given binary stream is a TTF file or not (really asking the class if it can work with said binary stream). I glanced at the TTF spec and it didn't look incredibly easy (no simple magic number at the head of the file).

The other thing I notice is the name variable isn't really the font name so much as either the path the user supplied or the IO object for the font, I didn't refactor all the methods to use a more correct name but I did leave a comment in the method. If we think it's worth doing now I can try to make that update but I didn't want to risk mucking up the font code as this is the first time I have really gotten into it.

I also changed the gemfile to require a more recent commit of TTFunk as mentioned in TTFunk Pull #17, once we are ready to merge the code we can remove that and instead cut a new release of TTFunk and make prawn require that release.

Any thoughts? It works, but it's not quite ideal.

@practicingruby practicingruby and 1 other commented on an outdated diff Jun 6, 2014
lib/prawn/font.rb
@@ -280,12 +280,23 @@ class Font
# *.ttf will call Font::TTF.new, *.dfont Font::DFont.new, and anything else
# will be passed through to Font::AFM.new()
#
- def self.load(document,name,options={})
+ def self.load(document, name, options={})
+ # Name is really either a string containing a file path or an IO object
@practicingruby
prawnpdf member

Rather than adding a comment here, can we rename the variable to src or something like that?

We totally can, the reason I didn't is I wasn't sure how to handle updating #find_font which is where that name variable comes from, I would think we want the variable name to be the same all the way down the stack, but I'm not 100% sure I'm yet comfortable messing with code that interacts with the font_registry. It looks like we are building up our registry key using the font 'name' but really the location should be in there (to cache multiple hits to the same URL or file path I'm sure). I'll give it a shot and go from there.

@practicingruby
prawnpdf member

Consistency doesn't matter for the names of local variables across different methods, particularly when one of them is private. We can fix that later, if it bugs us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@practicingruby practicingruby and 1 other commented on an outdated diff Jun 6, 2014
spec/font_spec.rb
@@ -159,6 +159,22 @@
name = name.sub(/\w+\+/, "subset+")
name.should == "subset+DejaVuSans"
end
+
+ it "should accept IO objects for font files" do
+ io = File.open "#{Prawn::DATADIR}/fonts/DejaVuSans.ttf"
+ @pdf.font_families["DejaVu Sans"] = {
+ :format => 'ttf',
@practicingruby
prawnpdf member

I'm not sure this is the right place to introduce an extension point for specifying the font format. Currently all of the keys in font families are meant to be styles, and so adding this metadata makes the dictionary non-homogeneous.

I understand the reason for doing this... we want to not have to specify the font type for each and every font loaded by way of an I/O call. But we need to find a better way.

All of this is mostly a sign of how our existing font_families mechanism is underspecified and kind of limited in its behavior. We should look at a way to directly pass TTF, DFont, and AFM objects to font_families, e.g.

font_family["DejaVu Sans"] = {
  :normal => Font.load(open("http://example.com/dejavu.ttf"), :format => :ttf)
}

Making :ttf the default would reduce the above down to:

font_family["DejaVu Sans"] = {
  :normal => Font.load(open("http://example.com/dejavu.ttf"))
}

This is still a little weird because the overall font system is wonky, but I think it's a safer bet than changing the meaning of the font families dictionary.

It's possible we are suffering from type envy in that font_families is a real Hash instance that we update. The core of the problem is there a bit of metadata about the font we want to add (the format) that we where deriving from the path. Maybe a better solution is to create a Prawn::FontStore class, that has a similar API to Hash so it can act as a drop in replacement, and be backwards compatible, but can then expose Prawn::Document.new.add_ttf_font(:name, :normal_path_or_io, :bold_path_or_io, :italic_path_or_io)

That is a more fundamental reworking but it can then let us encapsulate font information, name, style paths, format, and any future metadata we care about.

Another possibility is if we can have Prawn::Font::TTF, Prawn::Font::DFont and Prawn::Font:AFM act similar to the image handlers and detect if a given binary stream is supported by that class.

@practicingruby
prawnpdf member

I was thinking about having a real object for FontFamily, but I still don't know what I'd want the interface to look like for that and I'm hesitant to rush into defining it.

I like the idea of smart detection via some sort of FontHandler protocol, so that may be worth experimenting with.

But I'm also okay with the idea of just having the hash stay as it is. It isn't highly complex to say "either convert the string into a font object, or if you've already been provided a font object, use it directly without attempting any processing".

Even if we make Font.load() smart so it can return the right font object based on the binary stream, we're still going to need to make those fonts usable by families. So let's start w. making this tiny change to families (values can be strings or pre-loaded Font objects), and a small change to Font.load (either intelligently detect format or let it be explicitly specified, i.e. Font.load(src, :format => :ttf), with :ttf being default).

From there, we can evaluate how well used the feature is and what might be a way to formalize the APIs further.

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

I tweaked the API so this now works

    io = File.open "#{Prawn::DATADIR}/fonts/DejaVuSans.ttf"
    @pdf.font_families["DejaVu Sans"] = {
      normal: Prawn::Font.load(@pdf, io, format: 'ttf')
    }

I had to leave the default font AFM because changing it to TTF caused a lot of tests to break when it Prawn couldn't find the Helvetica font, but I may have made that change in error.

Regardless this removes the format symbol from the font_families hash and will just try to use whatever Prawn::Font instance got passed, which in turns takes an IO object, which in turn uses TTFunk's IO support.

I feel like we are getting fairly close here, any more thoughts about the TTF default breakage or anything else?

@practicingruby
prawnpdf member

I had assumed that what we'd do is this:

  • If src is a string, check to see if options[:format] is set. If so, use it. Otherwise, use the old way of font detection (look at the extension, if present, default to AFM otherwise)

  • If src is anything else, assume it's an I/O like thing, and do options.fetch(:format, :ttf)

I think this will prevent the breakage, because it is a proper superset of existing behavior. Give it a try and we'll see what happens.

@packetmonkey

I also suspect the broken tests is due to an un-locked dependency on rspec, they released version 3 recently and I think it's backwards incompatible.

@packetmonkey

This commit will default any src with a #read method to the 'ttf' format. I decided to look for a #read method as opposed to a String class as according to the spec files we can also pass in a Pathname so as a last resort we cast the src to a string and check for a file extension.

@packetmonkey

@sandal are there any other changes you would like to see on this PR? I would love to get this merged before the next release tentatively scheduled for next sunday.

@practicingruby
prawnpdf member

I think it's probably OK to merge, but would like to be the one that merges it, because I need to cut a TTFunk release before this can go into master. That will also give me a chance to kick the tires. I'll do that in time for the 1.1 release.

@packetmonkey

Sounds good, I just want to make sure you aren't waiting on anything from me to get it done.

Thanks!

@practicingruby
prawnpdf member

@packetmonkey: I'll work on getting this merged and released this week. To help me out, can you add some notes to the MasterCHANGELOG about this change? Just add a note that it hasn't been merged yet but will be very soon.

@practicingruby
prawnpdf member

Great, thanks. I've not come up with time to cut the release yet, and probably won't until at least next week some time, but there's no blockers for including this in the next release. I'll merge as soon as I get ttfunk out.

@practicingruby practicingruby merged commit f969d61 into master Jun 23, 2014

1 check was pending

Details continuous-integration/travis-ci The Travis CI build is in progress
@practicingruby practicingruby deleted the io-fonts branch Jul 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment