py-pdf / fpdf2

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

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

Open Lucas-C opened 3 weeks ago

Lucas-C commented 3 weeks ago

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:

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Thank you for your feedback @andersonhc 👍