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

Colspan with column_width breakes the table #407

Closed
devsergiy opened this issue Oct 3, 2012 · 33 comments
Closed

Colspan with column_width breakes the table #407

devsergiy opened this issue Oct 3, 2012 · 33 comments

Comments

@devsergiy
Copy link

Sample code

    data = [ 
      [ { content: "", colspan: 3 } ],
      [ "", "", "" ],
    ]
    table data, column_widths: [100 , 200, 240]

Brakes with such error
Table's width was set too small to contain its contents (min width 620.0, requested 540.0)

@chrisrichard
Copy link

I'm running into the same issue.

@jonsgreen
Copy link
Member

I have looked into this quite a bit and the problem has to do with how min_width is miscalculating in the case of a colspan. The calculated width for the first column first row is the average of the total width (i.e 180). Then #min_width for the column takes the maximum width for all rows. Since 180 is greater than 100 that becomes the min width. However now the sum of the min_widths now totals 620 which throws the error.

The simple fix is to make the following change in cells.rb:

def min_width
    # here is the current implementation:
    #aggregate_cell_values(:column, :avg_spanned_min_width, :max)
    # instead aggregate over :min_width_ignoring_span which seems to defer to the other columns without colspans.
    aggregate_cell_values(:column, :min_width_ignoring_span, :max)
end

Not surprisingly this breaks a couple unit tests and I don't really know enough about tables to be sure that this fix won't create problems in other more complicated scenarios.

Perhaps someone who has been more involved with the Prawn table feature could weigh in on this matter?

BTW, I believe that issue#326 is essentially a duplicate of this issue.

@bradediger
Copy link
Member

@jonsgreen Yeah, I'm the tables guy. Thanks for the research. I'll look into this and update the ticket when I have a fix.

@bjones
Copy link

bjones commented Apr 10, 2013

The change @jonsgreen provides makes it so you can actually use column_widths to set every column's width when using colspan. Without the change, you can only set some of the columns, which can result in tables larger than the containing bounds. I don't see that colspan should actually matter in calculating min width and thus agree with the change above as better than what is currently in master, at least for me.

@PeterBloom
Copy link

Sorry if this is old news... I am using Prawn 0.12.0. I am trying to use colspan on a table as shown in the docs. I get a "undefined method `colspan=' for #<Prawn::Table::Cell::Text:0x000001036cdfc8" error. I thought this was resolved way back when (#335), but maybe not. What to do?

@bradediger
Copy link
Member

Colspan was not present in 0.12.0. Try 1.0.0.rc2 or master from git.

Brad

@PeterBloom
Copy link

1.0.0.rc2 made the colspan work for me. Thanks Brad!

Now I am running into the "Prawn::Errors::CannotFit... Table's width was set larger than its contents' maximum width..." problem.

Does 'column_width' work in 1.0.0.rc2 if I don't call 'colspan'? If so, I can probably refactor my table headers (where I wanted 'colspan') into another horizontal table, then put the non-header part of the doc in tables below that. I really need 'column_width' for the non-header portion of a complex table I am building to make it all fit nicely. Any better ideas?

@bradediger
Copy link
Member

Yes, Prawn's table auto-sizing support can be a bit lacking and can often get into corners like this where it can't find a solution.

You can definitely specify manual column widths, and they work just fine with colspan. A spanned cell just uses the combined width of the two or more columns it lies in, so when you define your column widths make sure you include all of your (non-spanned) columns.

Brad

@ahacking
Copy link

@bradediger, I can confirm that this issue still exists in 1.0.0.rc2 using colspan with column_widths, even when ALL columns have a width specified.

@bradediger
Copy link
Member

@ahacking, thanks for the report. Unfortunately, I don't have the time at present to investigate in detail. I'd be happy to review and merge any pull request that addresses this issue. Sorry and thanks again.

@hbrandl
Copy link
Contributor

hbrandl commented Aug 2, 2013

I think I've tracked the bug down, but haven't yet come up with a fix.

First of all, here is a test case that you can add to table_spec.rb.

describe "You can explicitly set the column widths and use a colspan > 1" do
  it "should a colspan > 1 with given column_widths (issue #407)" do
    data = [ 
      [ { content: "", colspan: 3 } ],
      [ "", "", "" ],
    ]
    pdf = Prawn::Document.new
    table = Prawn::Table.new data, pdf, column_widths: [100 , 200, 240]
  end
end

The problematic code is in the function aggregate_cell_values (lib/prawn/table/cells.rb).

  # Sum up a min/max value over rows or columns in the cells selected.
  # Takes the min/max (per +aggregate+) of the result of sending +meth+ to
  # each cell, grouped by +row_or_column+.
  #
  def aggregate_cell_values(row_or_column, meth, aggregate)
    values = {}
    each do |cell|
      index = cell.send(row_or_column)
      values[index] = [values[index], cell.send(meth)].compact.send(aggregate)
    end
    values.values.inject(0, &:+)
  end

In the given example it will calculate the avg column width (100+200+240)/3=180 and write it to values[0].
values[1] and values[2] will be filled with the "correct" values of 200 and 240.

values => {0=>180.0, 1=>200.0, 2=>240.0}

Thus leading to the error message when you sum it up (180+200+240) = 620.

The expected sum would be (100+200+240)=540 OR (180+180+180)=540.

@hbrandl
Copy link
Contributor

hbrandl commented Aug 2, 2013

I was able to fix the issue and expanded the test case to include more variations of the problem.
pull request #526

@PeterBloom
Copy link

Thanks mate! I need that feature. Can't wait til its deployed!

Peter Bloom
650-291-6707 Mobile
peter@launchcodelabs.com

On Aug 2, 2013, at 11:36 AM, Hartwig Brandl wrote:

I was able to fix the issue and expanded the test case to include more variations of the problem.
pull request #526


Reply to this email directly or view it on GitHub.

@hbrandl
Copy link
Contributor

hbrandl commented Aug 19, 2013

My pull request #526 has been merged into master. I think this issue can thus be marked as closed.

@ahacking
Copy link

@hbrandl Thanks for your effort on tracking this down and fixing. I look forward to giving it a try when I revisit report generation.

@bradediger
Copy link
Member

Marking as closed.

@ksugiarto
Copy link

Excuse me, how can I update my prawn to that this table fixed version? Sorry, rails newbie..

@sigmike
Copy link
Contributor

sigmike commented Nov 15, 2013

There still are problems. For exemple this fails:

data = [['', ''],
        [{:content => '', :colspan => 2}]
        ]
pdf = Prawn::Document.new
table = Prawn::Table.new data, pdf, :column_widths => [50, 200]

I was able to fix it by removing this line in Table::Cell#min_width:

min_widths[column] = [min_widths[column], min_width_ignoring_span].max

But it's probably not a good solution.

@sigmike
Copy link
Contributor

sigmike commented Nov 15, 2013

@ksugiarto use this in your Gemfile:

gem 'prawn', git: 'https://github.com/prawnpdf/prawn.git'

@hbrandl
Copy link
Contributor

hbrandl commented Nov 19, 2013

Issue reported by @sigmike is fixed in pull request #579 .

@ksugiarto if you want to use the fixed version immediately, you may add the following line to your gemfile

gem 'prawn', :git => "https://github.com/hbrandl/prawn.git", :branch => "issue_407_2" 

But do use the original git repository once the pull request is merged, since the branch may not live forever in my forked repository.

@practicingruby
Copy link
Member

Closing this for now. @sigmike: Please try #579 and confirm whether or not it fixes the issues you saw. I will investigate myself soon enough, but knowing that will help!

@ksugiarto
Copy link

That really works now! You guys are really rocks! Thank so much.

@practicingruby
Copy link
Member

@ksugiarto That's good news. I will work on getting the other small patch from @hbrandl merged upstream soon, in time for our 1.0.0 RC3 release.

@ksugiarto
Copy link

Looks like there is still some messy sir for table with span and self.column_widths. Mine using width 540 usually and now it over the right page boundary. So I try to decrease it to 500 and looks perfect. Then i need to span more on some cell, and it began to over the right boundary again. But it looks so great still!

Then I'll prepare my next project using those 1.0.0 RC3 :D

@practicingruby
Copy link
Member

@ksugiarto Can you try this and let me know if it fixes your issue?

gem 'prawn', :git => "https://github.com/hbrandl/prawn.git", :branch => "issue_407_2" 

@ksugiarto
Copy link

@sandal Done great sir. Thanks

@sigmike
Copy link
Contributor

sigmike commented Dec 3, 2013

@sandal: yes it solves my issue

@ksugiarto
Copy link

@sandal @hbrandl Sir, I'm not trying to rushing you, but it seems using https://github.com/hbrandl/prawn/tree/issue_407_2 not working on my production with error something like branch not checked out. Any ideas?

@tfolk
Copy link

tfolk commented Dec 17, 2013

@sandal Fixes a long-standing issue for me, too. Thanks!

@practicingruby
Copy link
Member

@ksugiarto Sorry for not responding I must have missed this. it sounds like you may need to run bundle install.

@practicingruby
Copy link
Member

@tfolk: Nice! I will also try to get the other patch from @hbrandl merged in shortly before or after the holidays and then cut a 0.13.1 release with it.

@tfolk
Copy link

tfolk commented Dec 30, 2013

Did this go into 0.13.1? It still broken for me there, but
gem 'prawn', :git => "https://github.com/hbrandl/prawn.git", :branch => "issue_407_2"
works fine.

@practicingruby
Copy link
Member

@tfolk: This did not make it into 0.13.1, but is now pending review from me and will hopefully be cut it the next release, either in 0.14.0 on January 15 or sooner in a 0.13.2 release.

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

No branches or pull requests