Strange Multi-Page Table Wrapping when a cell has multi-rowspan on the last row
Closed this issue · 18 comments
When having a multi page table, and the last row of the table has a rowspan > 1 then the following output is obtained.
@Bharat311 pull request #3 fixed the test case, can you confirm it also fixes your issue in your production environment?
@hbrandl, thanks for the fix. It certainly looks much better than earlier. However, this fix is not exactly optimal solution for this.
Currently it is preventing the page break but i feel it should break the cell content(s) to the second page. Here's what the above pdf after the fix should look like:
The above fix is causing some more issues:
And in another one, we have a lot of space left unused on the first page of the pdf because of the rule that cells should not break across page. In some cases this uses up a lot more pages than are needed.
I will provide you with appropriate test case(s) for the above issues in a separate PR.
Breaking cell content can be the right behavior for certain circumstances, but won't always be desirable, and I'm not even sure about whether it should be default behavior.
So it'd be a useful thing to have, but it's a pretty complicated feature to support.
@sandal I work with @Bharat311. Other platforms which have tables (Word, Excel) all break merged cells across the page. I would think this should be the default behavior for most use cases.
@johnnyshields I suppose that could make sense if we limit the behavior to cells with rowspan > 1, though I don't generally like to compare PDF to other software because of the difference in purpose.
Excel is not really designed for print, and Word isn't really designed for high-precision print layouts (it's possible, of course). In those systems, a sensible default that isn't precise is fine, but it could be a deal-breaker for Prawn's users. It may well still be a good default, but I'd want to think about that before we just assumed it to be the case.
What's surprising to me here is that you're using tables to render such a complex document. If it were me I would have probably built some custom code one level down the Prawn stack (i.e. using text-boxes), because then you could have full control over the layout. But I guess that has its own set of drawbacks, too.
I can understand that "Prawn is different" than Word, Excel, printed HTML, etc. However, when all these other platforms follow identical logic for drawing table page-breaks and Prawn does something different on its own, it's quite counter-intuitive.
Also, note that Prawn's current table page-break logic leaves excessive whitespace--in our screenshots, half the page is simply blank!
While the example document we provided is indeed complex, I've also encountered this issue on more traditional tables, for example on multi-page tables when a cell spans the entire height of the table. (Things get really wonky when one cell spans more than one page's height)
Even if we don't make our proposed page-break logic the default (though I do think it should be) how might we implement this?
@johnnyshields: Prawn's table functionality currently has lots of bugs. That's the main reason why it's now in its own gem.
The current behavior is clearly broken, and so we're going to need to find a way to fix it. One way is to split the content across cells as @Bharat311 suggested. This is reasonable behavior for usual text flow, though I'm unsure how we handle things like images, cells with rotated text, etc. I also am not familiar enough with our rowspan support to even begin to know how to implement this, though maybe @hbrandl might have some ideas.
The other option, which we may end up needing anyway for the edge cases, is to move as many rows as necessary onto the next page to make the full row spanned cell fit on one page. This will guarantee unbroken cells, but has the cost of leaving lots of empty space at the bottom of a page, possibly resulting in ugly output.
Ideally we'd support both behaviors, and then we'd rely on practical use cases to determine what would be the best default.
Prawn currently has a ton of undefined and underspecified behavior, and also has its share of functionality that behaves in a non-conventional way. All of this leads to confusion, and it's undesirable. But it's worth noting that a good portion of these problems came from us saying "let's just do this the way (Some Tool X) does it" and then implementing a half-baked version of that functionality without realizing all the corner cases that could complicate it. It's my experience that things that almost work the way people would expect can be just as harmful as things that are completely ignorant of existing conventions.
It's easy to say "this should work like X by default", but unless you're going to do the analysis, write the patch, and be willing to maintain it over the long haul, including any support requests and follow-up fixes for corner case... I'd appreciate if you weren't so assertive about your opinions on how Prawn ought to work.
@sandal apologies if I come across as overly assertive. My team and I have built many client reports on Prawn, and this table page-break issue is a constant obstacle we face--we've had to avoid using multi-rowspan cells altogether.
We'd be glad to contribute code/tests and help maintain it, if we can agree on the approach.
@johnnyshields: Work on a patch that fixes the problems for your own needs, and then submit a pull request. Even if it doesn't get merged immediately, it'll give us a concrete solution to the problem to experiment with.
Keep in mind that in Prawn, there are show-stopping issues affecting many people, across dozens of different features. At best, we've got a few hours of volunteer time a week to spend on all of these issues, usually working on code we didn't implement, code that we weren't actively involved in the design decisions around. The project was literally dead for over a year before I returned to do very basic maintenance work on it. Tables would still be dead (similar to templates) if @hbrandl didn't step up.
So while I can empathize with the severity of this issue for you, please try to be more mindful of what the experience is like doing maintenance on our end.
We certainly do appreciate the great work you're doing. We will look into a patch for this.
@hbrandl what are your thoughts on how to resolve this? Is it worth attempting @Bharat311's suggestion? Any pointers on how we might do it?
I definitely see your problem. I think your solution might also resolve this long standing bug prawnpdf/prawn#562.
My main goal for this gem is to get it bug free and make the code more readable including reducing complexity, because as @sandal already said this code is very difficult to maintain since I didn't write it and parts include very complex logic.
That said I'm all open for feature additions. However I must be sure that any pull request of that magnitude doesn't introduce lots of bugs for edge cases down the road.
This definitely includes the mentioned images and rotated text that must be handled in a way that results in a kind of expected output.
I wouldn't want to merge a pull request, even as a non default behaviour, that results in broken layouts for rotated texts for example.
Given a pull request that
- can split cell contents into multiple rows
- can handle images, rotated text and anything else we all can think of
- includes documentation for this feature
- includes lots of tests fully covering any possible variations we all can think of
- is easy to read and low in complexity for the individual functions (codeclimate 4.0 metric)
I'd definitely be willing to accept it.
But please do note that even just reviewing a pull request of this magnitude and working with you to ensure it really is doing all of the above is a lot of work.
But my time is limited for this project, so I can't promise you quick turn around times for reviewing it.
That all said, I'd love to include this feature. But please ensure I need to devote as little development time for it as possible.
I've spent quite some time on the issue linked above to no avail. What I can give you is the functions changed by pull request #5. The affected lines in table.rb and the ones below them are what trigger line breaks and draw cells. You're definitely gonna have to change them.
In fact the whole draw method should probably be refactored in a separate pull request before you even start to try and implement any changes (at least starting with row 318). Otherwise we're all gonna have a hard time trying to find out what the existing and your new code really does.
The draw function is rated as the most complex method of this gem by codeclimate. And I'd definitely agree with codeclimate here. https://codeclimate.com/github/prawnpdf/prawn-table/issues
@Bharat311 Do wait with the test case for the line splitting until we've agreed on how to proceed. But please do file that pull request for the rendering error that I'm seeing in your screenshots.
@hbrandl thank you for your detailed comments. To recap visually, it would be something like:
In other words, we'd need to iterate line-by-line for the text within the cell and evaluate whether to push it onto the next page? (I realize there's a lot of complexity in doing this, rotation, images, vertical centering...)
Also, your work in #5 is certainly an improvement. Thank you very much for your time!
Closing this issue as it is being addressed in PR #16
Any update on this ?