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

Simple use case triggers unnecessary page break #22

Closed
practicingruby opened this issue Sep 15, 2014 · 9 comments
Closed

Simple use case triggers unnecessary page break #22

practicingruby opened this issue Sep 15, 2014 · 9 comments
Labels

Comments

@practicingruby
Copy link
Member

The following code will result in a two page PDF on prawn-table 0.1.1:

gem "prawn-table", "= 0.1.1"

require "prawn"
require "prawn/table"

Prawn::Document.generate("x.pdf") do
  text "HELLO"

  table [["A", "B", "C"], ["D", "E", "F"]], :position => :center
end

On 0.1.0, it works as expected, rendering the following results;

@practicingruby
Copy link
Member Author

@hbrandl: This is a fairly severe bug, it seems. Because I'm not sure if there's a workaround, I feel like this is something we should address sooner rather than later.

If you know the source of the problem and can easily resolve it in the next day or two (w. a test to prevent regressions), let's try to cut a new release w. a fix. Otherwise, I think we should consider releasing a 0.1.2 that reverts the work done in 0.1.1, effectively bringing us back to the same behavior of 0.1.0 until the issue is resolved.

(I'd probably cut a reverted release off a branch based on the 0.1.0 tag, to prevent sending master far back in time).

Let me know what you'd like to do.

@ruanwz
Copy link

ruanwz commented Sep 15, 2014

I have met this issue yesterday. Because I haven't time to dig into it. My workaround is don't use :position => :center in the table method, and make it full width as the bounding box. Hope this issue can be fixed.

@practicingruby
Copy link
Member Author

@ruanwz: The code we're working on together is where I extracted this case from. 😁

The problem is that using the full width of the bounding box isn't a suitable workaround for all but the most trivial cases. Once you set the width to the full width of the box, it eliminates the point of trying to center the table, because it's filling the entire space.

There's also a deeper problem, in that you can't even use a nested bounding box to work around this problem. So if you do something like this, you'd still get the unnecessary page break:

bounding_box [bounds.width/2 - 200, bounds.top], :width => 400 do
  text "HELLO"
  table [["A", "B", "C"], ["D", "E", "F"]], :width => bounds.width
end

That's why I'm worried about the potential impact of this bug. It's going to hit a lot of ordinary use cases.

@hbrandl
Copy link
Contributor

hbrandl commented Sep 15, 2014

@sandal I aggree, that this is a severe bug. I'll try to fix it within 48 hours. If I can't make it by then, please go forward with the proposed plan of reverting to 0.1.0.

This bug is probably related to pull request #23.

Seems like I'll have to think of a strategy on how to address refactoring for the future. I was hoping that the test suite covered at least nearly all ordinary cases.

@practicingruby
Copy link
Member Author

@hbrandl: Thanks. Don't feel pressured to rush a fix, I don't view reverting as a bad choice unless this is an easy fix. We will get some things wrong as we try to fix up prawn-table, and so having to press an "undo" button once in a while is just fine!

We could think about building an acceptance test suite of some sort... just a bunch of common use cases and then make some basic assertions about what we expect to happen. This would be easily caught by that sort of test, even if its only assertion was "Expect to fit on one page".

hbrandl added a commit that referenced this issue Sep 16, 2014
fixes #22 - unnecessary page break with centered tables
@practicingruby
Copy link
Member Author

@hbrandl Do you want to attempt to follow the release checklist and cut 0.1.2 yourself? https://github.com/prawnpdf/prawn-table/wiki/Release-checklist -- You can always email me if there are questions or if we snag on permissions issues anywhere.

@practicingruby
Copy link
Member Author

@hbrandl Please let me know about whether you plan to cut the 0.1.2 release in the next day or so. If not, I'll take care of it over the weekend.

@hbrandl
Copy link
Contributor

hbrandl commented Sep 18, 2014

@sandal I was about to do it today, when I was interrupted. The changelog should be fine, version is 0.1.2 and the tests pass (but do check again just to be sure). Please go ahead and release it. I'll cut 0.1.3.

@practicingruby
Copy link
Member Author

@hbrandl OK, I'll take care of this when I get up tomorrow morning. Thanks for the quick turnaround on a fix!

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

No branches or pull requests

3 participants