Add Document#font_style and Document#font_family #1225
Conversation
c960657
changed the title
c960657
changed the title
lib/prawn/font.rb
Outdated
| def font_style(style = nil, &block) | ||
| return @font ? @font.style : :normal if style.nil? | ||
|
|
||
| font(font_family, style: style, &block) |
There was a problem hiding this comment.
So, thinking a bit more about this, why not just pass nil here? Then it would automatically use what's defined in #font and we would have only one place where it would be defined.
There was a problem hiding this comment.
Another thing that came to mind: What happens if we pass the path to a e.g. TrueType font into #font and then call font_style(:bold)?
Since nothing is defined and we cannot know which file contains the bold version, I guess the only thing to do is to error out here. What do you think?
There was a problem hiding this comment.
Maybe we should check here for font_families.key?(font_family) and error out if nothing is found. This would solve both problems, the one when using a TrueType font path and the one where the used key is different from the family name. The error message should probably include the reason for the failure, i.e. that the family name was not registered.
And updating the documentation for #font_families in this regard would also make sense.
There was a problem hiding this comment.
I agree that calling font_style with a key not present in font_families for the current font should raise an error. Likewise if style is other than normal for a specific font file.
Currently font does not raise in the latter situation, and changing this would be a BC break, so I suggest we avoid raising in that specific situation (font called with style other than normal for a specific font file). I imagine that it is not uncommon that people do e.g. font 'ArialBlack.ttf', style: :bold in some (mis-guided) attempt to signal that this is a bold font.
There was a problem hiding this comment.
Agreed on both accounts.
@pointlessone What do you think?
There was a problem hiding this comment.
It's a little confusing because we have two very similar things:
font_familieswhich afford for stylesfont_registrywhich does not quite do that.
font_families eventually gets mapped to entries in font_registry. Style is only meaningful on the stage of this mapping. Whenever a font file is used directly style has no meaning. It still will be present in the font but there's no way to look up a font for any other style. We never add these fonts to font_families, we never look up fonts by their internal names so we're unable to correlate fonts from the same family. I don't think this should change now. Maybe in the next major release if we ever get there.
I suggest, we make font_style(:random_style) { ... } a noop but issue a warning. Possibly, when $DEBUG is set but it rarely is so it's not very useful.
spec/prawn/font_spec.rb
Outdated
|
|
||
| it 'allows setting font style for a TTF font' do | ||
| pdf.font_families.update( | ||
| 'DejaVu Sans' => { |
There was a problem hiding this comment.
Thanks for the test case. However, I think to really test this out we need another entry where the key is different from the font family name, e.g. using 'deja' as key.
I don't think that the current implementation would work since the look-up key would be the family name from the TTF.
Just like you can use
font_sizeto adjust the size without passing the current font name, addfont_styleto allow changing the style while preserving the font size and family.