-
Notifications
You must be signed in to change notification settings - Fork 33
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 horizontal text scaling. #22
Conversation
@@ -62,6 +64,11 @@ def set_word_spacing(*params) | |||
@word_spacing << params[0] | |||
end | |||
|
|||
def set_horizontal_text_scaling(*params) |
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.
Why do you use catch all here instead of single parameter?
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.
To be honest, for this, given that the PDF operators Tc (character spacing) and Tz (horizontal text scaling) seem to work very similarly, I've copied the method from set_character_spacing.
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.
I see. OK then. We'll refactor it later probably.
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.
I've been looking into writing specs for this, and I now understand that the reason why this method, and all other similar methods take the catch all is because it can be a variable argument list, as different PDF operators take different numbers of params. E.g., the set_text_font_and_size
method (Tm
PDF operator) takes two arguments.
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.
That crossed my mind, actually. It may be the case for other operators. But as far as I can tell from PDF 1.7 spec Tm
takes exactly 6 arguments. As well as other text-related operators take fixed number of arguments.
In this case we only use the first argument anyway (which, I suppose is an array of number). So why do we use more generic code and fiddle with argument extraction is we can have just one argument (or six)?
@msbit Thank you for all the contributions you've made recently. Specs? ;) |
Can write some if you want, was taking my cue from the fact that there were none there already 😃 |
I know the coverage is not very good, to say the least. We should strive for better. ;) |
So, as you may have guessed from my other PR on pdf-core, my motivation is to get horizontal text scaling going in prawn itself. From my testing it's working, and in order to get the specs running for that change, pdf-inspector needed to be able to keep track of horizontal scaling changes coming in from pdf-reader. I'm not, however, especially au fait with how pdf-inspector and pdf-reader work together, so I'm probably not the right person to start getting the testing in place for this gem. I can definitely add to the specs once something is in place. |
Bit the bullet and wrote some specs for easily settable text attributes (character spacing and horizontal text scaling). |
@@ -0,0 +1,3 @@ | |||
source "https://rubygems.org" |
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.
I understand the motivation but this seem to be unrelated to this PR.
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.
I've removed that particular line as part of a follow up commit.
I was thinking though, did you mean just that line or adding Gemfile + bundler in general?
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.
Gemfile and bundler.
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.
👍
@msbit This gem is mostly used in specs elsewhere. Could you please reference relevant PRs on other gems if there are any? Just so that we knew we need to merge this one before the PRs that depend on it. |
Add character_spacing and horizontal_text_scaling specs for PDF::Inspector::Text.analyze_file.
For set_character_spacing and set_horizontal_text_scaling methods, move away from catch all argument as we know these only take one argument.
No outstanding PRs in other projects rely on this. Once this and my two other PRs for pdf-core (prawnpdf/pdf-core#26 and prawnpdf/pdf-core#27) are tagged and versioned, I've got a branch on my fork of prawn that I'll put forward as a PR. |
👍 |
Add methods and attrs for storing and inspecting horizontal text scaling references.