Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
Lucas-C opened this issue Jul 11, 2024 · 3 comments · May be fixed by #1254
Open

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

Lucas-C opened this issue Jul 11, 2024 · 3 comments · May be fixed by #1254
Labels
emphasis enhancement font proposal-rfc

Comments

@Lucas-C
Copy link
Member

Lucas-C commented Jul 11, 2024 •
edited
Loading

Problem
As demonstrated in #1220 (comment),
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?
@Lucas-C Lucas-C mentioned this issue Jul 11, 2024
Closed
@gmischler
Copy link
Collaborator

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
Copy link
Member Author

Lucas-C commented Aug 20, 2024 •
edited
Loading

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:

  • grouping "font-styling" attributes together makes it clearer that those concepts are linked
  • we currently have a very large GraphicsStateMixin, with 23 attributes: reducing this count makes it a lot easier to debug parts of fpdf2 source code involving this class

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 Lucas-C mentioned this issue Aug 20, 2024
Merged
3 tasks
smilerightnow added a commit to smilerightnow/fpdf2 that referenced this issue Sep 1, 2024
@smilerightnow
Fixing issue py-pdf#1223: pdf.font_style now behaves like pdf.set_font
de5fb1c
smilerightnow added a commit to smilerightnow/fpdf2 that referenced this issue Sep 1, 2024
@smilerightnow
Fixing issue py-pdf#1223: pdf.font_style now behaves like pdf.set_font
e54583c
smilerightnow added a commit to smilerightnow/fpdf2 that referenced this issue Sep 1, 2024
@smilerightnow
Fixing issue py-pdf#1223: pdf.font_style now behaves like pdf.set_font
ab64006
@smilerightnow smilerightnow linked a pull request Sep 1, 2024 that will close this issue
Open
4 tasks
@Lucas-C
Copy link
Member Author

Lucas-C commented Oct 14, 2024 •
edited
Loading

A PR has been created by @smilerightnow in September: #1254

This issue can be considered as "assigned & in progress" while this PR is open 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
emphasis enhancement font proposal-rfc
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixing issue #1223: pdf.font_style now behaves like pdf.set_font smilerightnow/fpdf2
2 participants
@Lucas-C @gmischler