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.

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

The FPDF.circle() parameter names & documentation is misleading #1245

Closed
Lucas-C opened this issue Aug 19, 2024 · 3 comments · Fixed by #1248
Closed

The FPDF.circle() parameter names & documentation is misleading #1245

Lucas-C opened this issue Aug 19, 2024 · 3 comments · Fixed by #1248

Comments

@Lucas-C
Copy link
Member

Lucas-C commented Aug 19, 2024

When the FPDF.circle() method was added, back in the summer of 2021 and version 2.4.2 of fpdf2, I think we missed a couple of details:

  • the value provided for the r parameter sets in fact the diameter of the circle! 😲
  • less problematic but still a bit annoying, the x & y parameters refer to the top-left corner of the circle, instead of its center, as it's the case in almost all libraries

Minimal code

from fpdf import FPDF

pdf = FPDF()
pdf.set_font("Helvetica", size=20)
pdf.add_page()

def draw_cross(x, y, size=5):
    pdf.set_draw_color(255, 0, 0)  # red
    pdf.line(x - size, y, x + size, y)
    pdf.line(x, y - size, x, y + size)

radius = pdf.w/2
pdf.cell(text="The radius is set to be the page half width.")

pdf.ln(); pdf.ln()

x, y = pdf.w/2, pdf.h/2
draw_cross(x, y)
pdf.cell(text="The red cross is the (x,y) position provided to FPDF.circle().")
pdf.ln()
pdf.cell(text="This is set to be the center of the page.")

pdf.set_draw_color(0)  # black
pdf.circle(x=x, y=y, r=radius)

pdf.output("issue_1245.pdf")

What do we do now?
Now, the question is: how should we fix this while preserving backward compatibility, as much as possible?

Given that we have a parameter named r that is in fact a diameter,
I suggest that for this specific case we do not bother too much with backward compatibility,
and just fix this issue (r becoming the actual radius & x/y becoming the circle center)
and inform our end-users by mentioning it in our CHANGELOG.md

What do you think @gmischler & @andersonhc?

@Lucas-C
Copy link
Member Author

Lucas-C commented Aug 19, 2024

For the record, there is the current workaround that I currently use:

from fpdf import FPDF, FPDF_VERSION

minor, patch = map(int, FPDF_VERSION.split(".")[1:])
if minor < 7 or (minor == 7 and patch <= 9):
    print("Using workaround for https://github.com/py-pdf/fpdf2/issues/1245")
    class CustomFPDF(FPDF):
        def circle(self, x, y, r, style=None):
            super().circle(x-r, y-r, 2*r, style)
else:
    CustomFPDF = FPDF

@andersonhc
Copy link
Collaborator

If it was only the r problem I would say create a new radius parameter and deprecate r to be removed in the future, but compounding with x and y behavior I'd say it's a case where it's not worth keeping backwards compatibility.

@Lucas-C
Copy link
Member Author

Lucas-C commented Aug 19, 2024

Thank you for your feedback @andersonhc 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants