ddengler / prawn-grouping

A gem to add a more flexible grouping option to the prawn gem
https://github.com/ddengler/prawn-grouping
MIT License
14 stars 21 forks source link

Use of "Destructive Functions" / Counters inside of group block #7

Closed jeffreylammers closed 10 years ago

jeffreylammers commented 10 years ago

I was attempting to create a dynamic grid of "n" number of rows with 2 columns each. I wanted each row to be wrapped in a group so that if the content of that row does not fit on the page, then the whole row will get pushed to the next page. The issue I ran into was that I was processing my rows / columns as a 2 dimensional array and using the "shift" function to remove each logical cell as I processed it. This did not work. Now that I understand how the code is working, it makes sense because each time you use "group" your block is called a minimum of 2 times. So the "destructive" shift function was emptying my array on the first pass and the second time the block got called, the array was empty.

I was able to work around this by not using "shift" and instead just maintaining a logical pointer to which "cell" in the 2-D array had been processed. However, that was challenging as well because my pointer would be wrong on subsequent recursions of the group block. I ended up having to save off the pointer before processing each row and then restoring the pointer within the group block.

Here's roughly what my code looks like:

save_cell_pointer = cell_pointer = 0

num_rows.times do |row|
  #new row -update saved pointer
  save_cell_pointer = cell_pointer
  # IMPORTANT! - The "group" block below will get re-executed multiple times by the prawn_grouping gem.  Because of that
  # do not use "desctructive" methods in the group block such as "shift" or "delete_at" - they won't work
  # since those objects will have the wrong state upon subsequent invocations.
  # Also, if you have any counters, logical pointers etc. you need to keep track of those and save off their value
  # and restore those at the beginning of the group block or they also will have the wrong values upon
  # subsequent invocation.
  group :too_tall => lambda { start_new_page } do |g|
    #restore saved pointer
    cell_pointer = save_cell_pointer
    bounding_box([bounds.left, cursor], width: bounds.width) do
      num_columns.times do |col|
        if col == 0
          x = bounds.left
        else
          x = bounds.width/2
        end
        bounding_box([x, bounds.top], width: (bounds.width/2) - 15) do
          #render cell content here...
        end
        cell_pointer += 1
        if cell_pointer >= num_cells
          break
        end
      end
    end
    move_down 10
  end
end

This worked ok, but was less than intuitive. I would like to suggest that at minimum this be documented. Beyond documenting it, I'm not sure if there is a simple solution to this issue.

BTW, thanks for getting this going - it is an essential component for generating dynamic / grouped content. Please let me know how I can help.

ddengler commented 10 years ago

I do not see a simple solution to this. I will try a solution with block params for custom vars, that could be copied. Any other suggestions? This would have to be documented as well, but might simplify things.

jeffreylammers commented 10 years ago

No - I don't see a simple solution either. Cloning the block variables could get messy if there are complex objects and they don't all handle deep copying. However, if you could pull that off, it might be the best you can do. Short of that, avoiding the issue as I did above is not horrible, but being aware of it was most of the battle. Honestly it is a small price to pay to get the grouping functionality - it is essential.

ddengler commented 10 years ago

As we both feared, I got into the whole deep copy mess with this again. As the deep copy problem in prawn transactions was the reason I created this plugin the way it is right now, I will not pursue this any further. I will keep the ticket open until this gets proper documentation.

ddengler commented 10 years ago

Added a reference to this in the readme