From bb296b5fba9b941432744ec501ea573f01afc721 Mon Sep 17 00:00:00 2001 From: Hartwig Brandl Date: Mon, 21 Apr 2014 13:16:16 +0200 Subject: [PATCH 1/2] refactoring and fixes #710 refactoring code from cells.rb to column_width_calculator, from now on all complex width related code is in this file. fixed #710 be eliminating an edge case as reported under issue #710 --- lib/prawn/table/cells.rb | 51 +-------- lib/prawn/table/column_width_calculator.rb | 125 ++++++++++++++++++++- spec/table_spec.rb | 28 +++++ 3 files changed, 150 insertions(+), 54 deletions(-) diff --git a/lib/prawn/table/cells.rb b/lib/prawn/table/cells.rb index 2dfd4343f..739e02795 100644 --- a/lib/prawn/table/cells.rb +++ b/lib/prawn/table/cells.rb @@ -228,56 +228,7 @@ def index_cells # each cell, grouped by +row_or_column+. # def aggregate_cell_values(row_or_column, meth, aggregate) - values = {} - - #calculate values for all cells that do not span accross multiple cells - #this ensures that we don't have a problem if the first line includes - #a cell that spans across multiple cells - each do |cell| - #don't take spanned cells - if cell.colspan == 1 and cell.class != Prawn::Table::Cell::SpanDummy - index = cell.send(row_or_column) - values[index] = [values[index], cell.send(meth)].compact.send(aggregate) - end - end - - #if there are only colspanned or rowspanned cells in a table - spanned_width_needs_fixing = true - - each do |cell| - index = cell.send(row_or_column) - if cell.colspan > 1 - #calculate current (old) return value before we do anything - old_sum = 0 - cell.colspan.times { |i| - old_sum += values[index+i] unless values[index+i].nil? - } - - #calculate future return value - new_sum = cell.send(meth) * cell.colspan - - #due to float rounding errors we need to ignore a small difference in the new - #and the old sum the same had to be done in - #the column_width_calculator#natural_width - spanned_width_needs_fixing = ((new_sum - old_sum) > Prawn::FLOAT_PRECISION) - - if spanned_width_needs_fixing - #not entirely sure why we need this line, but with it the tests pass - values[index] = [values[index], cell.send(meth)].compact.send(aggregate) - #overwrite the old values with the new ones, but only if all entries existed - entries_exist = true - cell.colspan.times { |i| entries_exist = false if values[index+i].nil? } - cell.colspan.times { |i| - values[index+i] = cell.send(meth) if entries_exist - } - end - else - if spanned_width_needs_fixing && cell.class == Prawn::Table::Cell::SpanDummy - values[index] = [values[index], cell.send(meth)].compact.send(aggregate) - end - end - end - values.values.inject(0, &:+) + ColumnWidthCalculator.new(self).aggregate_cell_values(row_or_column, meth, aggregate) end # Transforms +spec+, a column / row specification, into an object that diff --git a/lib/prawn/table/column_width_calculator.rb b/lib/prawn/table/column_width_calculator.rb index 39f9fbf93..60cda1fec 100644 --- a/lib/prawn/table/column_width_calculator.rb +++ b/lib/prawn/table/column_width_calculator.rb @@ -9,16 +9,67 @@ def initialize(cells) @widths_by_column = Hash.new(0) @rows_with_a_span_dummy = Hash.new(false) - end - def natural_widths + #calculate for each row if it includes a Cell:SpanDummy @cells.each do |cell| @rows_with_a_span_dummy[cell.row] = true if cell.is_a?(Cell::SpanDummy) end + end + + # does this row include a Cell:SpanDummy? + # + # @param row - the row that should be checked for Cell:SpanDummy elements + # + def has_a_span_dummy?(row) + @rows_with_a_span_dummy[row] + end + + # helper method + # column widths are stored in the values array + # a cell may span cells whose value is only partly given + # this function handles this special case + # + # @param values - The columns widths calculated up until now + # @param cell - The current cell + # @param index - The current column + # @param meth - Meth (min/max); used to calculate values to be filled + # + def fill_values_if_needed(values, cell, index, meth) + #have all spanned indices been filled with a value? + #e.g. values[0], values[1] and values[2] don't return nil given a index of 0 and a colspan of 3 + number_of_nil_values = 0 + cell.colspan.times do |i| + number_of_nil_values += 1 if values[index+i].nil? + end + + #nothing to do? because + #a) all values are filled + return values if number_of_nil_values == 0 + #b) no values are filled + return values if number_of_nil_values == cell.colspan + return values unless has_a_span_dummy?(cell.row) + #fill up the values array + + #calculate the new sum + new_sum = cell.send(meth) * cell.colspan + #substract any calculated values + cell.colspan.times do |i| + new_sum -= values[index+i] unless values[index+i].nil? + end + + #calculate value for the remaining - not yet filled - cells. + new_value = new_sum.to_f / number_of_nil_values + #fill the not yet filled cells + cell.colspan.times do |i| + values[index+i] = new_value if values[index+i].nil? + end + return values + end + def natural_widths #calculate natural column width for all rows that do not include a span dummy @cells.each do |cell| - unless @rows_with_a_span_dummy[cell.row] + unless has_a_span_dummy?(cell.row) @widths_by_column[cell.column] = [@widths_by_column[cell.column], cell.width.to_f].max end @@ -26,7 +77,7 @@ def natural_widths #integrate natural column widths for all rows that do include a span dummy @cells.each do |cell| - next unless @rows_with_a_span_dummy[cell.row] + next unless has_a_span_dummy?(cell.row) #the width of a SpanDummy cell will be calculated by the "mother" cell next if cell.is_a?(Cell::SpanDummy) @@ -58,6 +109,72 @@ def natural_widths @widths_by_column.sort_by { |col, _| col }.map { |_, w| w } end + + # get column widths (either min or max depending on meth) + # used in cells.rb + # + # @param row_or_column - you may call this on either rows or columns + # @param meth - min/max + # @param aggregate - functions from cell.rb to be used to aggregate e.g. avg_spanned_min_width + # + def aggregate_cell_values(row_or_column, meth, aggregate) + values = {} + + #calculate values for all cells that do not span accross multiple cells + #this ensures that we don't have a problem if the first line includes + #a cell that spans across multiple cells + @cells.each do |cell| + #don't take spanned cells + if cell.colspan == 1 and cell.class != Prawn::Table::Cell::SpanDummy + index = cell.send(row_or_column) + values[index] = [values[index], cell.send(meth)].compact.send(aggregate) + end + end + + # if there are only colspanned or rowspanned cells in a table + spanned_width_needs_fixing = true + + @cells.each do |cell| + index = cell.send(row_or_column) + #puts "index=#{index}" + if cell.colspan > 1 + #special treatment if some but not all spanned indices in the values array have been calculated + #only applies to rows + values = fill_values_if_needed(values, cell, index, meth) if row_or_column == :column + #calculate current (old) return value before we do anything + old_sum = 0 + cell.colspan.times { |i| + old_sum += values[index+i] unless values[index+i].nil? + } + + #calculate future return value + new_sum = cell.send(meth) * cell.colspan + + #due to float rounding errors we need to ignore a small difference in the new + #and the old sum the same had to be done in + #the column_width_calculator#natural_width + spanned_width_needs_fixing = ((new_sum - old_sum) > Prawn::FLOAT_PRECISION) + + if spanned_width_needs_fixing + #not entirely sure why we need this line, but with it the tests pass + values[index] = [values[index], cell.send(meth)].compact.send(aggregate) + #overwrite the old values with the new ones, but only if all entries existed + entries_exist = true + cell.colspan.times { |i| entries_exist = false if values[index+i].nil? } + cell.colspan.times { |i| + values[index+i] = cell.send(meth) if entries_exist + } + end + else + if spanned_width_needs_fixing && cell.class == Prawn::Table::Cell::SpanDummy + values[index] = [values[index], cell.send(meth)].compact.send(aggregate) + end + end + end + puts values.values.inject(0, &:+) + return values.values.inject(0, &:+) + end end + end end diff --git a/spec/table_spec.rb b/spec/table_spec.rb index 74cfb8a99..3167bbb37 100644 --- a/spec/table_spec.rb +++ b/spec/table_spec.rb @@ -151,6 +151,34 @@ table.column_widths.should == [50.0, 70.0] end + it "illustrates issue #710", :issue => 710 do + partial_width = 40 + pdf = Prawn::Document.new({page_size: "LETTER", page_layout: :portrait}) + col_widths = [ + 50, + partial_width, partial_width, partial_width, partial_width + ] + + day_header = [{ + content: "Monday, August 5th, A.S. XLIX", + colspan: 5, + }] + + times = [{ + content: "Loc", + colspan: 1, + }, { + content: "8:00", + colspan: 4, + }] + + data = [ day_header ] + [ times ] + + #raised a Prawn::Errors::CannotFit: + #Table's width was set larger than its contents' maximum width (max width 210, requested 218.0) + table = Prawn::Table.new data, pdf, :column_widths => col_widths + end + it "illustrate issue #533" do data = [['', '', '', '', '',''], ['',{:content => '', :colspan => 5}]] From f10762ab086138e988a6ae17b6cac8374046f315 Mon Sep 17 00:00:00 2001 From: Hartwig Brandl Date: Mon, 21 Apr 2014 13:29:20 +0200 Subject: [PATCH 2/2] improved documentation of fix for issue #710 --- lib/prawn/table/column_width_calculator.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/prawn/table/column_width_calculator.rb b/lib/prawn/table/column_width_calculator.rb index 60cda1fec..e328c95e7 100644 --- a/lib/prawn/table/column_width_calculator.rb +++ b/lib/prawn/table/column_width_calculator.rb @@ -47,6 +47,9 @@ def fill_values_if_needed(values, cell, index, meth) return values if number_of_nil_values == 0 #b) no values are filled return values if number_of_nil_values == cell.colspan + #c) I am not sure why this line is needed FIXXME + #some test cases manage to this line even though there is no dummy cell in the row + #I'm not sure if this is a sign for a further underlying bug. return values unless has_a_span_dummy?(cell.row) #fill up the values array