Table sizing should respect manually-set column widths

[bradediger]

Sizing algorithm currently only takes hints from provided column widths (by widths= or column_widths). We should indicate which, if any, widths have been set by the user, and respect those over all.

[rubyredrick]

I think this is the same bug.

setting column_widths doesn't prevent getting CannotFit errors, although the documentation indicates that it should.

The problem is that the Prawn::Table#column_width method checks to see if @column_widths is set but the #column_widths = method never sets that instance variable.

[bradediger]

Hi Rick, You might be right that this is the same bug. The fundamental problem, as I see it, is that we don't make a clear distinction between user-requested and auto-sized widths.

If you have a minimized example of a situation where you're getting CannotFit errors with manually-set widths, that would help me dig into this.

Thanks!

[rubyredrick]

It's not 'minimized' since I'm still struggling to understand table layouts, but this fails (Note I've manually changed a few things to protect the client identity, and to isolate the code from the rails model where it's actually getting some of the data, so this might not be exactly right.:

def header_table left_header_width = bounds.width / 2 right_header_width = bounds.width / 2 form_id_width = 30 umc_width = left_header_width - form_id_width form_cell = make_cell("FORM\nM0902CP\nREV\n11/02", :size => 7, :width => form_id_width) umc_cell = make_cell( [ "UNIVERSITY MEDICAL CENTER", "DIVISION OF NEUROLOGY", "ELECTROMYOGRAPHY LABORATORY" ].join("\n"), :size => 10, :width => umc_width) date_cell = make_cell("DATE OF STUDY: #{Date.today}", :width => left_header_width) label = make_table([[form_cell, umc_cell]], :column_widths => [form_id_width, umc_width]) do |table| table.rows(0).width = form_cell.width + umc_cell.width end left_header = make_table( [ [label], [date_cell] ], :column_widths => [left_header_width]) do |table| table.rows(0..1).width = left_header_width end right_header = make_table( [ ["John Doe], ["XYZZY123)], ["AGE: #{29} HT: #{71} in."] ], :column_widths => right_header_width ) data = [ [left_header, right_header] ] table(data, :column_widths => [left_header.width, right_header.width], :cell_style => {:borders => []}) do |table| table.rows(0).width = left_header.width + right_header.width + 10 end end

I tried everything I could think of to both manual sett the column widths, AND try to make sure things were set up as I understand things, so I still don't understand why I'm getting the CannotFit error even without setting the column widths.

The doc says that using the :column_widths option (which by convention calls column_widths= should work:

+column_widths+::

Sets widths for individual columns. Manually setting widths can give

better results than letting Prawn guess at them, as Prawn's algorithm

for defaulting widths is currently pretty boneheaded. If you experience

problems like weird column widths or CannotFit errors, try manually

setting widths on more columns.

But it also seems obvious that setting column_widths doesn't prevent the table from trying to recompute the widths and blowing up, and why that is so.

Here's Prawn::Table#column_widths starting at line 296

Calculate and return the constrained column widths, taking into account

each cell's min_width, max_width, and any user-specified constraints on

the table or column size.

#

Because the natural widths can be silly, this does not always work so well

at guessing a good size for columns that have vastly different content. If

you see weird problems like CannotFit errors or shockingly bad column

sizes, you should specify more column widths manually.

# def column_widths @column_widths ||= begin if width < cells.min_width raise Errors::CannotFit, "Table's width was set too small to contain its contents" end

   if width > cells.max_width
     raise Errors::CannotFit,
       "Table's width was set larger than its contents' maximum width"
   end

   if width < natural_width
     # Shrink the table to fit the requested width.
     f = (width - cells.min_width).to_f / (natural_width - cells.min_width)

     (0...column_length).map do |c|
       min, nat = column(c).min_width, column(c).width
       (f * (nat - min)) + min
     end
   elsif width > natural_width
     # Expand the table to fit the requested width.
     f = (width - cells.width).to_f / (cells.max_width - cells.width)

     (0...column_length).map do |c|
       nat, max = column(c).width, column(c).max_width
       (f * (max - nat)) + nat
     end
   else
     natural_column_widths
   end
 end

end

And here's #column_widths= starting on line 164

def column_widths=(widths) case widths when Array widths.each_with_index { |w, i| column(i).width = w } when Hash widths.each { |i, w| column(i).width = w } when Numeric columns.width = widths else raise ArgumentError, "cannot interpret column widths" end end

Note that the @column_widths instance variable is not set by this method so the column_widths method will ALWAYS try to calculate the widths even if you try to set them with the :column_widths option when instantiating the table.

In fact the only reference to @column_widths is that one line.

Now the first idea would be to have the column_widths= method just set @column_widths to the argument, but this probably isn't exactly right. It probably needs to do something like:

def column_widths=(widths) case widths when Array widths.each_with_index { |w, i| column(i).width = w } @column_widths = widths when Hash @column_widths = [] widths.each { |i, w| @column_widths[i] = w column(i).width = w } when Numeric columns.width = widths @column_widths = [widths] * column_length else raise ArgumentError, "cannot interpret column widths" end end

But this leaves some holes:

if the argument is an array, what if it's a different size than the number of columns in the table? If it's too short the some columns at the left side won't have 'manually' set widths. If it's too long then some of the settings will be extraneous.

if the argument is a hash, what if it doesn't have a key for each column?

So the column_widths method would need to be able to compute widths for columns not 'manually' set

Some more observations.

1) Raising CannotFit rather than just having the cells either be clipped or overflow makes it very hard to debug this if it's an app error. If I could see an incorrect result I'd be much better equipped to understand what's wrong.

2) There seems to be a bit of confusion in the code WRT methods vs. instance variables. For example, the table has a make_cells method which is used to initialize the @cells instance variable, the second statement in Prawn::Table#initialize is

@cells = make_cells(data)

and the make_cells method returns a Ruby Array of Cells. But in cells.rb Prawn::Table gets monkey patched with a cells method

   def cells
     @cell_proxy ||= Cells.new(@cells)
   end

This is really contorted code, why not just have make_cells return an instance of Cells, and do away with @cell_proxy

[bradediger]

Hi Rick, I think your problem is in this part of the code:

table.rows(0).width = form_cell.width + umc_cell.width

Admittedly, this is a bit of a confusing part of the table API. Any cell proxy (such as that returned by table.rows(0)) proxies calls on itself to calls on its constituent cells. So that statement is setting the width of each cell in the first row to the value provided. Thus, the outer table containing this one (left_header) gets a CannotFit error because you are trying to put 540pt of width in a table you have constrained to 270pt.

Brad

[rubyredrick]

Well, I'll try taking that out, but it was an attempt to solve the problem which existed before I added that.

[bradediger]

I've tried, and failed, to reproduce this issue on master. If we come across this problem again, we can re-open the ticket to investigate.