fpdf2 icon indicating copy to clipboard operation
fpdf2 copied to clipboard

Add Cell Level Controll for Table Borders

Open bbrenter opened this issue 1 year ago • 10 comments

Intent It would be great to have control over table borders by defining them for each cell.

Describe the solution you'd like This could for example be implemented by adding the border string slot to the Cell Class and use it inside the get_cell_border function to determine the border for each cell. A possible implementation could look like this...

image

image

image

bbrenter avatar Jun 04 '24 07:06 bbrenter

@bbrenter are you interested in implementing the feature and submitting a PR?

andersonhc avatar Jun 06 '24 13:06 andersonhc

Hi I'm new to open source, but I'm eager to dive in and help out. I came across this issue and I think I can tackle it. Can I work on it?

Kaustbh avatar Aug 26 '24 17:08 Kaustbh

Yes, given the history on this issue, you are very welcome to tackle this @Kaustbh 🙂

As a minor change to the interface suggested by @bbrenter, I think it would be better if the new border property of Cell was typed with a new subclass of CoerciveEnum, maybe named CellBordersLayout (in a similar fashion as TableBordersLayout). It could have those values:

  • NONE = 0
  • LEFT = 1
  • RIGHT = 2
  • TOP = 4
  • BOTTOM = 8
  • ALL = LEFT | RIGHT | TOP | BOTTOM = 15
  • INHERIT = 16 would be the default value, meaning to inherit from the table layout scheme

A starting point for you @Kaustbh could to be to read our development guidelines.

Then, you could add a new unit test to test/table/test_table.py.

Please ask away if you have any question! 🙂

Lucas-C avatar Aug 26 '24 17:08 Lucas-C

Hi @Lucas-C ,

I'm currently working on implementing the CellBordersLayout as suggested. However, I'm having trouble understanding table rendering logic and get_cell_border function, could you please help me?

Kaustbh avatar Aug 29 '24 20:08 Kaustbh

I understood the logic of rendering and get_cell_border function, can you please tell me how should I write CellBordersLayout class whether it should be of type CoerciveEnum or CoerciveIntFlag ?

Kaustbh avatar Sep 09 '24 05:09 Kaustbh

Please reply on how I should implement CellBordersLayout.

Kaustbh avatar Sep 29 '24 03:09 Kaustbh

Hi @Kaustbh ,

The CellBordersLayout enum could be a good use case for CoerciveIntFlag if it wasn't for the INHERIT option. INHERIT can't be combined with the other values, so I believe CoerciveEnum is your better option.

andersonhc avatar Sep 30 '24 01:09 andersonhc

But Enum does not support bitwise operations, which we can do using IntFlag , and please also tell me what should be the input format to the new border parameter in cell() function?

Kaustbh avatar Oct 04 '24 17:10 Kaustbh

@Lucas-C can you please tell me the right way to do it?

Kaustbh avatar Oct 10 '24 15:10 Kaustbh

Hi @Kaustbh

A quick test revealed that with CoerciveIntFlag as a parent class it works fine:

class CellBordersLayout(CoerciveIntFlag):
    NONE = 0
    LEFT = 1
    RIGHT = 2
    TOP = 4
    BOTTOM = 8
    ALL = 15
    INHERIT = 16

    @classmethod
    def coerce(cls, value):
        if isinstance(value, int) and value > 16:
            raise ValueError("INHERIT cannot be combined with other values")
        return super(cls, cls).coerce(value)

    def __and__(self, value):
        value = super(CellBordersLayout, self).__and__(value)
        if value > 16:
            raise ValueError("INHERIT cannot be combined with other values")
        return value

    def __or__(self, value):
        value = super(CellBordersLayout, self).__or__(value)
        if value > 16:
            raise ValueError("INHERIT cannot be combined with other values")
        return value

print(CellBordersLayout.NONE)
print(CellBordersLayout.BOTTOM)
print(CellBordersLayout.coerce("LEFT"))
print(CellBordersLayout.INHERIT)
print(CellBordersLayout.ALL)
print(CellBordersLayout.LEFT | CellBordersLayout.RIGHT | CellBordersLayout.TOP | CellBordersLayout.BOTTOM)
print(CellBordersLayout.coerce(12))
print(CellBordersLayout.NONE | 1)
print(CellBordersLayout.coerce(17))  # <- this raises an error, as expected
print(CellBordersLayout.INHERIT | 1)  # <- this raises an error, as expected

Lucas-C avatar Oct 11 '24 15:10 Lucas-C

Hi @Kaustbh

Did my last answer help you to move forward on this implementation? Do you have other questions? Do you plan to continue working on this issue?

Lucas-C avatar Oct 14 '24 07:10 Lucas-C

Yes, I am working on this issue, and yes your last answer helped me move forward.

Kaustbh avatar Oct 14 '24 07:10 Kaustbh

Yes, I am working on this issue, and yes your last answer helped me move forward.

Great 🙂

I'm assigning this issue to you then!

Lucas-C avatar Oct 14 '24 08:10 Lucas-C

This feature has been included in the new release published today: https://github.com/py-pdf/fpdf2/releases/tag/2.8.2

Lucas-C avatar Dec 16 '24 12:12 Lucas-C