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

Properly copy graphics states #635

Merged
merged 2 commits into from
Jan 15, 2014

Conversation

dmarkow
Copy link
Contributor

@dmarkow dmarkow commented Jan 14, 2014

GraphicState's initializer was properly using dup to copy color_space from previous_state, but it was not using dup on the dash attribute, keeping dash tied to the previous state's dash.

Also added an initialize_copy method to make sure those two attributes are properly copied whenever we dup/clone a graphic state.

Added specs for both updates.


This somewhat helps with #473 because it makes sure the graphics state is truly immune to any changes after setting a repeater. However, for the time being, a repeater still has to be defined before anything else to ensure a clean slate.

@practicingruby practicingruby merged commit 77ffca0 into prawnpdf:master Jan 15, 2014
@practicingruby
Copy link
Member

Thanks! I made the copying a little more thorough ( 255cc70 ), but they should still do what you want them to.

You also now have commit access to Prawn! You can read more about our contribution guidelines here:
https://github.com/prawnpdf/prawn/wiki/Contributor-welcome-notes

Thanks again for the patch!

@dmarkow
Copy link
Contributor Author

dmarkow commented Jan 15, 2014

Whoops, I completely spaced on the fact that strings are mutable. Duh.

The immutable objects shouldn't need to be specifically copied within the initialize_copy method, since calling something like @graphic_state.dup already copies all the attributes. The initialize_copy method is only to do something different than the default (in this case, to dup the mutable attributes). Or maybe you added them since you're now calling initialize_copy directly from the main initialize method?

Just want to make sure I'm not missing something. Thanks for merging!

@practicingruby
Copy link
Member

You're right, it's probably not necessary to reset those if we were just using this for affecting dup. But since we also call it directly, I suppose it's better to be explicit and set all attributes. I think this code is a candidate for refactoring in general, but I'm not sure we need to address that issue right now.

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