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

Always reset boxes to allow for redefinition of grid #663

Closed
wants to merge 2 commits into from

Conversation

traels
Copy link

@traels traels commented Feb 8, 2014

A small change with a test to prove that it works for issue #492

This MIGHT have a small impact on speed as the thing screwing things up was a instance variable caching the boxes that a grid creates.

@practicingruby
Copy link
Member

Thanks for the patch. I will take a quick look at this before the 0.15.0 release and give some feedback when I do.

@practicingruby
Copy link
Member

The way I understand this is that you want to clear @boxes when a grid is redefined, to avoid references to the old grid object.

If that's the case, why are we clearing the @boxes variable each time a grid's data is accessed, rather than each time it gets redefined?

@traels
Copy link
Author

traels commented Feb 14, 2014

You are right, that would make more sense

@practicingruby
Copy link
Member

Ok, so what this patch needs before it is merged is:

  • move the @boxes assignment up into define_grid
  • Make sure the tests still pass
  • Add documentation indicating that calling define_grid multiple times will create a completely new grid object, affecting all grid calls that follow it. In other words, grids do not nest like bounding boxes.

@practicingruby
Copy link
Member

@traels: If you want to work on updating the patch that may help get it merged sooner, otherwise I'll work on this when I get a chance.

@traels
Copy link
Author

traels commented Feb 14, 2014

Should i just add manual update to this pull request?

kh
Simon Træls Ravn
Sent from my iPhone

On 14/02/2014, at 17.24, Gregory Brown notifications@github.com wrote:

@traels: If you want to work on updating the patch that may help get it merged sooner, otherwise I'll work on this when I get a chance.


Reply to this email directly or view it on GitHub.

@practicingruby
Copy link
Member

All of the above you can do directly on this pull request, yeah. Just add some additional commits.

@practicingruby
Copy link
Member

I see, you already moved the box clearing code. Yep, all we need is a manual and/or API doc update now!

@practicingruby
Copy link
Member

I'll go ahead and try this patch out today and document it if it's working as I'd expect. I'll be cutting the 0.15.0 release this afternoon and I'd like to include this patch if possible.

practicingruby added a commit that referenced this pull request Feb 16, 2014
Squashed commit of the following:

commit 48e95b8
Author: Gregory Brown <gregory.t.brown@gmail.com>
Date:   Sun Feb 16 09:24:22 2014 -0500

    Add some documentation about new Grid behavior

commit 420bee9
Author: Simon T. Ravn <cs2@cs2.dk>
Date:   Fri Feb 14 16:07:37 2014 +0000

    More logical way of resetting boxes

commit 9ff499a
Author: Simon T. Ravn <cs2@cs2.dk>
Date:   Sat Feb 8 16:18:16 2014 +0000

    Always reset boxes to allow for redefinition of grid
@practicingruby
Copy link
Member

Merged, thank you!

@practicingruby
Copy link
Member

@traels: 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants