Increase flexibility of TableBordersLayout

[TedBrookings]

Add capability to precisely control thickness, color, and dash of table cell borders.

TableBordersLayout is changed from an enum to a more complex class with helper style classes that allow more complex custom style setting. The old enum is maintained as static members of the new class, so the previous invocation style works the same, with the added option to specify cell border styles by passing a custom function.
Fixes #957

Checklist:

• The GitHub pipeline is OK (green),
  meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

• A unit test is covering the code added / modified by this PR

• This PR is ready to be merged

• In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

• A mention of the change is present in CHANGELOG.md

I'm making a draft PR. There are many unit tests that fail. The reason is partly down to strategy, and rather than plowing ahead and changing all the reference PDFs for the tests, I wanted to first get agreement that the overall approach I've used is good.
The tests fail for one of two reasons

1. The reference implementation draws two identical lines where mine draws one. This is easily fixable by just double-drawing the lines, and I've started doing that, but wanted agreement on this decision.
2. More critically, since the new class manages color, thickness, and dash qualities, I didn't want to managing setting and restoring all of these, so instead I wrap the drawing commands inside a local context (q [set thickness/color/dash] [draw element] Q). The result is visually identical and generally more concise, but it (correctly) fails assert_pdf_equal. My preferred solution would be to just update the test PDFs, but if this is not desired, it would be possible (if tedious) to adopt a set and reset approach that should yield identical outputs.

It also occurred to me that you might not like the class structure I created, so again I'd rather fix that and deal with the implications now, rather than plow ahead.

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

[TedBrookings]

@Lucas-C @gmischler I've created a draft pull request in response to Issue #957. If you agree with the direction I've gone, it should be fairly quick to fix up remaining issues. Obviously I'm open to different direction if you see things you don't like.

[gmischler]

An interesting concept!
Although, looking at your documentation example and the test, creating a TableBordersLayout() seems to be quite a convoluted procedure. Having to provide several complicated lambdas in a constructor call is certainly not what I'd consider an intuitive API.
We try to keep fpdf2 straightforward and simple to use. The general idea of having a seperate class for managing borders and cell background may be helpful, but I think it needs to become much simpler and easier to understand how its rules are defined.

And a heads-up: I'm currently working on turning the table cells into text regions (see #339). To do that I'll have to rearrange quite a few parts of the table code, delegating tasks away from tha main Table() class. I hope this will simplify the overall architecture a bit. The borders code should not be directly affected by those changes, but I may still have to move it around.

[TedBrookings]

I understand the desire for simplicity.

To be fair, you only supply one lambda, and generally it would be quite simple (e.g. see the re-implementation of the existing styles). The demo was complicated to show all the things that could be done, but that could obviously be scaled back to provide a simple introduction, e.g. similar to MINIMAL but the line under headings is thick). The test needs to have every possible combination just in case a corner case breaks one of them.

[gmischler]

The specific number of lambdas is not the point.

Using query functions that the table elements can use to learn about their formatting is a useful concept. But that is an internal implementation detail, and not necessarily a good aproach for a public API.
What you're doing here is essentially tell the user: "Just implement the formatting yourself!"
That's hardly more convenient than Subclassing Table() and replacing the border drawing method.

Rather than in 5 abstract lambda arguments, users may rather think in terms like "outer left border", "border below last heading", "bottom row of cells" etc. If you allow them to define rules in those terms and build your lambda out of them internally, then that would be a lot more helpful in terms of useability.

Forcing users to think in terms of internal implemention details is an issue I see with a lot of software on a daily basis. I really hope we can avoid falling into the same trap here.

[tfenne]

@gmischler weighing in here as I opened the original issue. I wanted to share an example of a table style I'm trying to replicate. It's toy data to illustrate, but hopefully it gets the point across:

Because (as I currently understand it) there's no way to embed tables in tables, I'm constructing this as a single table even though there are three sub-tables. In this example I have:

• A major header row that labels each sub-table, with a single thin border below it
• A "minor" header row with column headers with a thick border below it
• A double-border above the summary/footer row
• Vertical divider lines between the "sub-tables" that start only at the "minor" header row

This is very similar to a style of table that is very frequently used in academic publications (at least in biology and I believe other sciences too) where people are used to seeing tables generated by latex.

I don't know the internals of the library nearly as well as you, but me sense is that if we only went with your suggestion (border styles settable for a specific categories of borders) I think either I wouldn't be able to do this or there would be a be-wildering array of categories.

How would you feel if this was the underlying mechanism and the public API supported?:

1. The current method of supplying a named configuration of borders
2. Something like you describe that provides a medium level of configurability by allowing the user to specify styles for a range of border categories
3. Approximately what Ted has implemented, as a last resort when people (like me) want to do things that can't be expressed by a more user-friendly API like (1) and (2)

[gmischler]

Thanks for your input, @tfenne, that is a wonderful example to discuss the topic.
Your "last resort" concept might be workable, but I'm not sure if it's really necessary.
While the "selectors" in my description above were overly verbose and generic, the concept can be applied with more concise specifications. An API to format your table could eg. look similar to this:

layout = TableLayout()
layout.bottom_border(rows=[0], start=0, end=-1)
layout.bottom_border(rows=[1], start=0, end=-1, width=2)
layout.top_border(rows=[-1], start=0, end=-1, width=2, color=DeviceGray(0.5))
layout.right_border(columns=[2, 4], start=1, end=-1)
layout.row_format(rows=[0], style=bold_blue_font_face, align="CENTER")
layout.row_format(rows=[1], style=bold_font_face, align="CENTER")

This is a rough idea very much off the cuff, so there are probably more refined ways to go about it. But I think you'd need an extemely weird table format to make the lambda solution easier to understand and handle than something along those lines.

How to translate this kind of input into a request reply for each individual cell is left as an excercise for the reader... 😉

[TedBrookings]

@gmischler I have been thinking about the best way to implement something along the lines of your suggestion. There are two issues that I see, and I see an easy work-around the first one, but I encountered some difficulties doing the second with a pure implementation.

1. It's entirely possible to define conflicting selectors with an API like the one you suggested. One thing to do would be to check for conflicts and throw an exception. Another would be to combine styles, treating later selectors as an override. This way you could e.g. specify gray color for all rows, but in a later call to the API make one of them thicker with a partially overlapping style that doesn't mention thickness.
2. I assume that its desirable to maintain existing TableBorderStyle values and the current method of specifying them, but to reimplement them in this new class. The problem being that some styles make use of the number of heading rows in the actual style definition. So for instance, I can't figure out a concise reimplementation of the MINIMAL table using an API similar to the one you propose, so that someone could create a minimal table like in this preexisting unit test test_table_with_minimal_layout_and_multiple_headings

The best way forward that I can see to provide the API you like and reimplement the old styles is a hybrid approach. I could write a TableBorderStyle class with an API like the one you want, which stores data from the called APIs, and then implements its own cell_style_getter function based on those styles. So a user could if they chose just work with the API you wanted. This would probably be fine for one-off custom styles where they likely know the number of heading rows in advance.
Then the existing styles would be implemented by overwriting cell_style_getter with their own function that can make user of num_header_rows. This could be done in either of two ways:

1. Have an optional getter function (that defaults to None) as a member of the dataclass
2. Overwrite the class method for specific instances. e.g. something like MINIMAL.cell_style_getter = some_lambda.

I think either approach would involve enough work that I'd like to get your thoughts before plowing ahead and coding.

[Lucas-C]

Hi @TedBrookings

I'm sorry that your last, detailed comment was left unanswered for almost a year 😢

Thank you very much for the effort you put in this PR 👍

I volunteer to take over @gmischler in assisting you on this PR 🙂

But first, are you still willing to continue this work, almost a year later?
Or do you prefer that we simply close this PR?

I agree with the points you made in your last comment, about potential conflicting selectors, and the issue with reproducing the MINIMAL layout.
However, I have to admit that I am not myself a big fan of the cell_style_getter lambda design, in terms of usability & code readability.

In release 2.7.9 of fpdf2 we added TableCellFillMode and the ability to override the should_fill_cell() method.
What do you think of this approach @TedBrookings?

Another fpdf2 user also started a new issue on the subject in #1192.
I think your hindsight would be very welcome on the solution suggested there, that resembles the TableCellFillMode approach🙂

[TedBrookings]

Hi @Lucas-C I think we are interested in seeing this to completion one way or another. I just got back from international travel, but I should have time today to review my old work, TableCellFillMode, and issue 1992. After that I'll propose an approach that hopefully meets with your preferences and if you agree I can try and get this finished soon.

[Lucas-C]

Hi @TedBrookings! 🙂

What are is the status on your side regarding this PR?

Wish you a happy holiday season 🎄☃️ 𐂂

[TedBrookings]

Hi, I'm sorry for dropping out for a bit, things got very hectic with urgent client commitments.

I did look over the other work, and I think that unfortunately it's unlikely that we are going to agree on a path forward for this PR (and obviously this is your repo, so we respect your need to maintain your own coding standards!). The other PR added control over per-cell formatting, which my PR also did. So I would have to rebase my work to get back to the same place and use the new system. But that would still leave us at an impasse for how to define per-cell format.

If your objection is really just to lambdas, and you would be fine if the lambdas were replaced with ordinary functions in def statements, then that is a pretty trivial change, but my understanding is that the objection is to using functions whatsoever to define the format. Unfortunately I don't see how to do it otherwise.

The other suggestion involved explicitly passing lists of rows or columns for specific format effects, but in addition to being quite verbose, it then results in something that isn't a style, but a set of drawing instructions for each table (you have to know table dimensions exactly in advance). I also made a brief attempt at implementing them, but quickly found that coordinating between the different proposed format operations became much more complicated than simply specifying a per-cell format function. By contrast, e.g. TableCellFillMode uses very simple functions to determine if a cell should be filled. Our original issue suggested allowing the definition of a new style that could be consistently applied to a different tables, and would hopefully be very succinct.

[Lucas-C]

If your objection is really just to lambdas, and you would be fine if the lambdas were replaced with ordinary functions in def statements, then that is a pretty trivial change, but my understanding is that the objection is to using functions whatsoever to define the format.

Actually I think methods are fine, as we already used this approach for TableCellFillMode.
So adding methods to TableBordersLayout seems really OK to me.

I'm sorry for the inconsistent feedbacks we are giving you regarding this PR.
@gmischler was initially overseeing the work on this PR, and I have the greatest respect for his analysis.
But he has been less active on fpdf2 for the pas few months, and I think that:

• this feature could benefit many users
• you produced some substantial work in this PR, and were willing to very constructively listen to our feedbacks, and I think it is important to welcome quality contributions like yours
• even if a function-based approach does not cover all the cases, it could be quite user-friendly and consistent with the existing usages in fpdf2

So if you are still willing to finish the work started in this PR, I would be happy to merge a new version of this PR based on TableBordersLayout methods.

Maybe something akin to this:

class TableBordersLayout:
    ...
    def border_style_for_cell(self, i, j, table):
        ...

Does it make sense / seem practical to you? 🙂

[TedBrookings]

I think that seems practical. Just to be sure we're on the same page, you're thinking an abstract base class / protocol, or something similar, and then the user defines a new class with an actual implementation of border_style_for_cell?

In that case, I think the bulk of the actual work would just be rebasing to use the already merged support for defining Cell styles. I am pretty slammed for the rest of December, but if you're willing to wait until January, I think I could finish it off then.

[Lucas-C]

Just to be sure we're on the same page, you're thinking an abstract base class / protocol, or something similar, and then the user defines a new class with an actual implementation of border_style_for_cell?

A base class yes, not necesserily an abstract one 🙂
We could provide a sane default class, as it's done with TableCellFillMode, but let users subclass this new class.

In that case, I think the bulk of the actual work would just be rebasing to use the already merged support for defining Cell styles. I am pretty slammed for the rest of December, but if you're willing to wait until January, I think I could finish it off then.

That's perfectly fine, take your time 👍