py-pdf / fpdf2

Simple PDF generation for Python
https://py-pdf.github.io/fpdf2/
GNU Lesser General Public License v3.0
1.13k stars 253 forks source link

pdf.font_style = "B" and pdf.emphasis = "BOLD" should behave the same way as pdf.set_font(style="B") #1223

Open Lucas-C opened 4 months ago

Lucas-C commented 4 months ago

Problem As demonstrated in https://github.com/py-pdf/fpdf2/issues/1220#issuecomment-2221497527, currently setting pdf.font_style = "B" does not properly apply a bold emphasis to the following text:

pdf.font_style = ""
pdf.cell(text="Eins.")
pdf.font_style = "B"
pdf.cell(text="Zwei.")
pdf.font_style = "I"
pdf.cell(text="Drei.")

Describe the solution you'd like

    @font_style.setter
    def font_style(self, new_style: str):
        "Set the font style: bold and/or italics"
        self.set_font(style=new_style)

    @emphasis.setter
    def emphasis(self, value: TextEmphasis | str):
        "Set the text emphasis: bold, italics and/or underlined."
        value = TextEmphasis.coerce(value)
        self.underline = value & TextEmphasis.U
        self.set_font(style=value.style.replace("U", ""))

As part of this change, I suggest to replace the .underline & .font_style attributes of FPDF by getters & setters modifying an underlying ._emphasis property.

RFC: Whad fo you think of this proposal?

gmischler commented 4 months ago

At the moment, I think that proposed solution would result in infinite recursion. We'd have to move the part of set_font() that sets self.current_font into an internal method _set_font() to avoid this, and only use that one from the other property setters.

I agree that in the user facing API, we should try to treat "bold", "italic", and "underline" as similarly as possible. When storing them internally though, collecting them into a single "emphasis" string seems to make their use a bit cumbersome. Alternatively we could just as well store three booleans seperately, and only combine them into a string when the user requests one. It will take a bit of code analysis to figure out which strategy will result in the simpler code overall. Currently the font selection key is created by appendeing a "[b]?[i]?" string to the family, though. Is there a more straightforward way to do that?

Lucas-C commented 3 months ago

At the moment, I think that proposed solution would result in infinite recursion. We'd have to move the part of set_font() that sets self.current_font into an internal method _set_font() to avoid this, and only use that one from the other property setters.

Yes, you are right, we would have to take care of that.

When storing them internally though, collecting them into a single "emphasis" string seems to make their use a bit cumbersome.

Could you please develop a bit why?

This may be subjective, I find this actually a lot more elegant & handy than having 3 attributes ๐Ÿ˜…

I see the following "factual" benefits of having a single attribute (._emphasis) instead of 4 distinct ones:

However, I agree that having 4 distinct attributes would probably make the code often simpler to grasp, and would hence be better for the readibility of fpdf2 source code.

Lucas-C commented 1 month ago

A PR has been created by @smilerightnow in September: https://github.com/py-pdf/fpdf2/pull/1254

This issue can be considered as "assigned & in progress" while this PR is open ๐Ÿ™‚