Skip to content
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

Merged
merged 4 commits into from
Jan 2, 2017
Merged

Add support for horizontal text scaling. #22

merged 4 commits into from
Jan 2, 2017

Conversation

msbit
Copy link
Contributor

@msbit msbit commented Feb 21, 2016

Add methods and attrs for storing and inspecting horizontal text scaling references.

@@ -62,6 +64,11 @@ def set_word_spacing(*params)
@word_spacing << params[0]
end

def set_horizontal_text_scaling(*params)
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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)?

@pointlessone
Copy link
Member

@msbit Thank you for all the contributions you've made recently.

Specs? ;)

@msbit
Copy link
Contributor Author

msbit commented Feb 21, 2016

Can write some if you want, was taking my cue from the fact that there were none there already 😃

@pointlessone
Copy link
Member

I know the coverage is not very good, to say the least. We should strive for better. ;)

@msbit
Copy link
Contributor Author

msbit commented Feb 21, 2016

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.

@msbit
Copy link
Contributor Author

msbit commented Mar 1, 2016

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"
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gemfile and bundler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@pointlessone
Copy link
Member

@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.

msbit added 2 commits March 2, 2016 01:36
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.
@msbit
Copy link
Contributor Author

msbit commented Mar 1, 2016

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.

@pointlessone pointlessone merged commit bc90686 into prawnpdf:master Jan 2, 2017
@msbit msbit deleted the horizontal-text-scaling branch January 2, 2017 13:47
@msbit
Copy link
Contributor Author

msbit commented Jan 2, 2017

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants