-
Notifications
You must be signed in to change notification settings - Fork 686
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
Add support for TTC collections #1007
Conversation
(to be clear -- the CI check is failing because of the dependency on my branch of ttfunk) |
The answer to this program from ruby
|
@juanfal -- it looks like you're using the prawn gem (see the backtrace, it's loading prawn 2.1.0 in the gems directory). This pull request is not yet released (and in fact has not yet been merged or--to my knowledge--reviewed). If you want to test this PR, you'll need to check out the ttc-support branch of my fork of Prawn, as well as my ttc-support branch of my fork of TTFunk. |
|
||
private | ||
|
||
def read_ttf_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not quite an appropriate name for the method. If you want to follow inheritance path I'd suggest renaming the method to something less specific. For instance, read_file
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may not be appropriate...but it's an inherited method from the superclass that is being overridden in the subclass. You can see that the dfont.rb implementation overrides it, too. You're right that the name is misleading, but as it was already named this before, it seems that refactoring it might be out of scope for this pull request.
lib/prawn/font/ttc.rb
Outdated
# Copyright November 2008, Jamis Buck. All Rights Reserved. | ||
# | ||
# This is free software. Please see the LICENSE and COPYING files for details. | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prawn uses git to track code changes and thus authorship information. Given the nature of a highly collaborative project such a Prawn, this information can get outdated rather quickly. Because of this I'd suggest leaving this comment out.
We acknowledge your contribution and you definitely keep copyright for the code.
If you feel strongly about inclusion of this comment please update it to be factually correct as of now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, sorry... as you can see from the date, that was actually not recent. :) I copied the font/dfont.rb file (which I also contributed, more than eight years ago, apparently!) and adapted it to TTC, and forgot to clean up the header comment. I'll clean it up there, and on dfont.rb.
lib/prawn/font/ttc.rb
Outdated
private | ||
|
||
def read_ttf_file | ||
TTFunk::File.from_ttc(@name, @options[:font] || 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fonts are usually referred to by name, not index. :font
doesn't seem to be appropriate. Maybe rename the argument to :index
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, :font
is not the best name when the value is an index, but that's how dfont.rb originally defined it. The difference is that dfont.rb supports specifying fonts by both name and index. I'll rework this so that it supports name look ups as well, to mimic the dfont implementation.
lib/prawn/font/ttc.rb
Outdated
TTFunk::Collection.open(file) do |f| | ||
list = [] | ||
|
||
f.each do |font| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get a proper map
by including Enumerable
into Collection
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent point. I've updated both PR's.
@jamis I rebased, squashed and merged this PR. I will close it now. Thank you for your contribution. |
This PR depends on prawnpdf/ttfunk#33 (which adds the underlying TTC support to TTFunk).
The TTC support in Prawn mirrors the approach taken with the DFont collections, requiring users to specify which font they want from the collection via the
:font
key. It does not currently allow you to specify fonts by name, but requires you to specify them by numeric index within the file.A simple program demonstrating usage: