fixes bug reported under issue #407 comment 28556698

[hbrandl]

The original fix solved the problem for cells.rb.

The additional bug reported in comment 28556698 was similar in nature, but located in tables.rb.

This pull request fixes the new bug, including test cases.

[practicingruby]

Hi @hbrandl,

Thanks for the patch. I've added it to my review queue (which is sadly really long). I'm going to need to compare this to what was attempted by @sigmike in #576, and also read up a bit more on #407 before I can give a proper review.

[hbrandl]

@sandal thanks for the quick reply. No worries, I can use my own branch for my production system.

I've described the first bug in detail in this comment
#407 (comment)

Here we ran into the same faulty logic by the original code.

I'll try to give you some quick background:
Assuming we have a table with a width of 50 for column 1 and 200 for column 2. (Testcase provided by @sigmike.)
And assuming we have a table data as follows

data = [['', ''],
        [{:content => '', :colspan => 2}]
       ]

The original code calculated the width as follows
Row 1: [50,200]
Row 2:
(50+200)/2=125; 125 is bigger than 50 but smaller than 200
=> [125,200]

Resulting in an obvious error. (Expected result would be [50,200] OR [125,125]. Either result would result in an end result of 250.)

Hope this helps.

[practicingruby]

@hbrandl:

Thanks again for working on this. I think I'll push this branch upstream and then we can continue to revise it from there... I want to attempt extracting this code into a class, since it seems to all operate on the cells collection.

[hbrandl]

@sandal I'd be happy to help out, but I'm not entirely sure what you're planning to do. Could you elaborate? Or should we get together on IRC?

[hbrandl]

I found a problem with this pull request (being using it on a project of mine).

Ammending the test that "illustrates issue #407 - comment 28556698" to check the final column widths:

    it "illustrates issue #407 - comment 28556698" do
      data = [['', ''],
              [{:content => '', :colspan => 2}]
              ]
      pdf = Prawn::Document.new
      table = Prawn::Table.new data, pdf, :column_widths => [50, 200]
      table.column_widths.should == [50.0, 200.0]
    end

yields an error:

  1) Prawn::Table You can explicitly set the column widths and use a colspan > 1 illustrates issue #407 - comment 28556698
     Failure/Error: table.column_widths.should == [50.0, 200.0]
       expected: [50.0, 200.0]
            got: [125.0, 125.0] (using ==)
     # ./spec/table_spec.rb:111

I'll push a fix to this problem, but this does break one test relating to issue #502 .

I'm pretty sure that the cause is somewhere else. Probably natural_content_height.
I'll look into this, fix it and then update this pull request again.

[practicingruby]

The fixes I want to make are structural only, so go ahead and get the tests passing again and I'll make my revisions when I merge, it's trivial stuff.

[practicingruby]

Just a heads up... this won't make the 0.13.0 release but if I find time in the next couple weeks to work on this I can certainly ship it out in a 0.13.x maintenance release.

[hbrandl]

I'll try to get to this before christmas.

[practicingruby]

Great, Thanks @hbrandl!

[hbrandl]

I've looked into the issue with the test case for (resolved) bug report #502.

It seems that - as I expected - the issue is not with the code touched by this commit. However since the code ensures that subtable cell/column widths are now as expected the specified width (15) is simply too small to hold the letter "B" (plus default space left & right). Thus the test case fails.

In the current master branch the subtables cells/columns will be larger than requested, thus the test case works.

I've adapted the testcase (width to 20) see commit c914cf1 in pull request.

I've also documented the bug I found in bug report #612 for further investigation by myself of whoever comes first.

Over the course of the next few weeks I'll continue testing the code for bugs related to merged cells. Since that is what I currently need most.
I don't know when I'll find time to work on issue #612.

P.S.: I noticed that I left some unnecessary code in the mentioned commit, I've corrected this with a new commit. Sorry about that.

[practicingruby]

@hbrandl Thanks for working on this. We won't ship this in the 0.13.1 release that I'm cutting today, but it sounds like this pull request is ready for me to review now, right? If so, I'll try to get it merged before our next release.

[hbrandl]

@sandal Yes, it's ready for review.

[practicingruby]

@hbrandl: There is no assertion in this test... what is it meant to be testing?

[hbrandl]

Line 111 fails with the current master branch, reporting the following error:

  1) Prawn::Table You can explicitly set the column widths and use a colspan > 1 illustrates issue #407 - comment 28556698
     Failure/Error: table = Prawn::Table.new data, pdf, :column_widths => [50, 200]
     Prawn::Errors::CannotFit:
       Table's width was set larger than its contents' maximum width (max width 250, requested 325.0)
     # ./lib/prawn/table.rb:403:in `column_widths'
     # ./lib/prawn/table.rb:584:in `set_column_widths'
     # ./lib/prawn/table.rb:137:in `initialize'
     # ./spec/table_spec.rb:111:in `new'
     # ./spec/table_spec.rb:111:in `block (3 levels) in <top (required)>'

So while technically there is no assert in that line, it does assert, that prawn doesn't raise an error. Is there an assert for that?

Also line 112 asserts (.should) that the calculated column widths are the same as the ones that where requested in line 111.

[pointlessone]

@hbrandl There's an assertion for that.

expect {
  # your code here
}.to_not raise_error

[bradediger]

@cheba I really don't like to_not raise_error and the like, because they don't add anything semantically and they can obscure the cause of the error. See this blog post for a better explanation than I can provide here.

I don't personally have a problem with assertionless tests in this context, catching regressions that raise errors.

[hbrandl]

@cheba Thanks! I didn't know that one.
I've just comited a fixed version to the new colspan-fixes branch.

@bradediger Sorry, I didn't see your comment in time. I'm fine with any solution.

[pointlessone]

@bradediger Well, in this case I don't see nearly enough context in the code. There's no mention of any exceptions at all here. I don't think you either can recall what it's about without finding that very specific comment. BTW, description includes comment id but not a link to it so you'd have to figure out how to find it, too.

Another concern is that assertion-less spec mixes up setup and the code that actually could raise an exception.

[practicingruby]

My email was really slow in notifying me on this, so I was working on addressing this before I saw these comments...

For edge cases like this one, I think a comment is sufficient. But we definitely need a comment at least, and not a just reference to a comment on a github issue. I updated the code accordingly on #620.

[practicingruby]

@hbrandl I opened a new pull request so that we can do revisions together on the same branch (you have commit access). Please see #620. Only pending feedback I need from you at this point is about that test without assertions, but I will also work on some refactorings of this code on that branch.