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

Rename variable arr to array, fixes a call to an undefined variable below. #15

Merged
merged 1 commit into from Mar 12, 2015

Conversation

Projects
None yet
3 participants
@arachnist
Contributor

arachnist commented Mar 6, 2015

No description provided.

@packetmonkey

This comment has been minimized.

Contributor

packetmonkey commented Mar 6, 2015

If we had a call to a nil value we are missing a test somewhere that exercises this code branch.

@practicingruby is it possible we missed a test when we forked over pdf-core? Or is a new test needed.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Mar 6, 2015

@packetmonkey: We're probably missing a test here, and it looks like I may also have missed a spot (in the else case for decimal rounding here too.

I was trying to think through why this wouldn't show up in Prawn's test or manual build, because I know the manual has rotated text, and we have specs that cover that behavior too.

The problem here is basically this: Higher level text methods in Prawn actually use generic matrix transformations to rotate text, whereas this low-level code uses the Tm operator, which combines text rendering and matrix transforms.

To get a reproduction in Prawn, we need to call the draw_text method with a :rotate parameter, because that brings us as close to the PDF::Core level as Prawn gets. I wouldn't be surprised if we lacked a test for this method in Prawn, and as far as testing for PDF::Core itself goes, we're not even close to being comprehensive (most features are indirectly covered through Prawn's tests, but there's huge room for improvement here).

So in summary, we do need a test, either here or in Prawn, ideally in both, and I need to look to see if I missed a spot on the decimal rounding, because that'd be another small bug. We should get this all fixed up and cut a release, but it's less severe than I initially thought because it should only affect these very low-level draw_text calls.

@arachnist: If you encountered this error using one of Prawn's higher level APIs, let me know. Otherwise we'll try to get this patched up soonish and cut a release once we do.

@arachnist

This comment has been minimized.

Contributor

arachnist commented Mar 6, 2015

I encountered it with the draw_text call and only because i have just started playing around with prawn and didn't read through the docs to notice higher-level APIs for rotated text.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Mar 6, 2015

That's bad luck, because any other method (text, text_box, etc.) would not have caused an error for you, but at least you helped us catch a bug!

practicingruby added a commit that referenced this pull request Mar 12, 2015

Merge pull request #15 from arachnist/fix-add_text_content
Rename variable arr to array, fixes a call to an undefined variable below.

@practicingruby practicingruby merged commit 7ea5e76 into prawnpdf:master Mar 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@practicingruby

This comment has been minimized.

Member

practicingruby commented Mar 12, 2015

Going to add a test in Prawn for this, because all the draw_text tests are still over there. We should move them into PDF::Core, but this is a known issue with the test suite... a lot of it is still in Prawn.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Mar 12, 2015

Here's the crappy test I wrote:
prawnpdf/prawn@eebd0f5

Anyone is welcome to improve it and/or extract the inspector I built into pdf-inspector. But it at least gives us a smoke test for now.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Mar 12, 2015

@arachnist: Because your pull request was accepted, you now have commit access to all of Prawn's repositories. Please see our contribution guidelines, and thanks for the help!

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