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

fix: force StringIO encoding to ASCII_8BIT #661

Merged

Conversation

pparidans
Copy link
Contributor

render method in Prawn::Document uses a StringIO instance as default output.
But on JRuby for Windows this class uses a default encoding of UTF-8 instead of ASCII_8BIT, than breaking inrender_header method by pushing non-UTF characters in the StringIO instance.

It was also breaking a lot of tests (I'm not listing all of them. Should be at least every in-memory render calls).

Environment:

  • Windows 7 x64 SP1
  • Java 1.7rev51
  • JRuby 1.7.10

render method in Prawn::Document uses a StringIO instance as default output.
But on JRuby for Windows this class uses a default encoding of UTF-8 instead of ASCII_8BIT, than breaking in render_header by pushing non-UTF characters in the StringIO instance.
@practicingruby
Copy link
Member

Hi, I think we're seeing a few bugs related to encodings in JRuby, so this may help. I'll take a closer look at it in a few days. Thanks!

@pparidans
Copy link
Contributor Author

Actually, I also test it on OS X: all tests are passing (without this modification) for both MRI 2.1.0 and JRuby 1.7.10. Didn't checked yet on Linux.

If you need more tests from a Win machine, let me know!

@practicingruby
Copy link
Member

If you don't mind testing something unrelated and have Adobe Reader XI handy, you could try simply opening the PDF generated by the code in the description of this ticket and see if you get an error: #631

We've been unable to reproduce on OS X.

@pparidans
Copy link
Contributor Author

Done!

BTW, I'm not sure that it would be related to the current issue because generate and render_file use a File object and not a StringIO. And I never had any problem when directly generating pdf to file.

@pparidans
Copy link
Contributor Author

I still need to run tests for this modification with a MRI implementation on Windows to check that tests are still passing on this configuration.

Anyway, Travis-CI seems to be happy! 😎

@practicingruby
Copy link
Member

No, I doubt #631 has anything to do with this issue. We were just lacking windows testers 😁

@pparidans
Copy link
Contributor Author

Go ahead! If you need more Windows' tests, I would happily help!

@practicingruby
Copy link
Member

I'm going to go ahead and merge this, but I don't have a way to reproduce it myself. If it causes any problems I'll need to revert first and ask questions later.

practicingruby added a commit that referenced this pull request Feb 3, 2014
fix: force StringIO encoding to ASCII_8BIT
@practicingruby practicingruby merged commit 7ea6dad into prawnpdf:master Feb 3, 2014
@pparidans
Copy link
Contributor Author

Thank you @sandal ! :)

@practicingruby
Copy link
Member

@pparidans: Because your pull request was accepted, you now have commit access to all of prawnpdf's repositories. Please see the link below for contribution guidelines, and thanks again!

https://github.com/prawnpdf/prawn/wiki/Contributor-welcome-notes

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