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

Infinite loop when adding multi_cell twice #389

Closed
skawouter opened this issue Apr 14, 2022 · 8 comments
Closed

Infinite loop when adding multi_cell twice #389

skawouter opened this issue Apr 14, 2022 · 8 comments

Comments

@skawouter
Copy link

When creating a simple pdf page and calling multi_cell twice in a row an infinite loop occurs.

Minimal code

from fpdf import FPDF
pdf = FPDF()
pdf.add_page()
pdf.set_font('Helvetica', '', 10)
pdf.multi_cell(w=0, h=5, txt='test')
pdf.multi_cell(w=0, h=5, txt='test')

I've traced the error to this piece of code in fpdf.py line 2860

        if w == 0:
            w = self.w - self.r_margin - self.x
        if h is None:
            h = self.font_size
        maximum_allowed_emwidth = (w - 2 * self.c_margin) * 1000 / self.font_size

where the resulting maximum_allowed_emwidth becomes negative. This is the result of self.x being almost equal to self.w

Environment

  • Linux Mint 20
  • Python 3.8.10
  • fpdf2 2.5.2 -> bug also present on master
@skawouter skawouter added the bug label Apr 14, 2022
@Lucas-C
Copy link
Member

Lucas-C commented Apr 14, 2022

Wow, this is a very annoying bug 😢
Thank you for reporting it @skawouter
I see, the infinite loop occurs in the while (text_line) is not None: loop just after the line you mentioned.
I'm going to try to fix this during the evening.
Ping @gmischler for information, as you worked on that part of the code too.

@Lucas-C
Copy link
Member

Lucas-C commented Apr 14, 2022

I opened #390 to fix this

@Lucas-C
Copy link
Member

Lucas-C commented Apr 14, 2022

Now your example would raise the following error:

$ ./issue_389.py
Traceback (most recent call last):
  File "./issue_389.py", line 13, in <module>
    pdf.multi_cell(w=0, h=5, txt='test')
  File "/opt/fpdf2/fpdf/fpdf.py", line 265, in wrapper
    return fn(self, *args, **kwargs)
  File "/opt/fpdf2/fpdf/fpdf.py", line 2864, in multi_cell
    "Not enough horizontal space to render cell. Consider introducing a line break."
fpdf.errors.FPDFException: Not enough horizontal space to render cell. Consider introducing a line break.

Note that you probably want to use new_x=XPos.LEFT:

pdf.multi_cell(w=0, h=5, txt='test', new_x=XPos.LEFT)
pdf.multi_cell(w=0, h=5, txt='test', new_x=XPos.LEFT)

@gmischler
Copy link
Collaborator

Makes you wonder about the wisdom of defaulting new_y to RIGHT...

@Lucas-C, Have you checked what happens if w ends up larger than c_margin but still smaller than the width of the next character?

@Lucas-C
Copy link
Member

Lucas-C commented Apr 17, 2022

Good catch, I'm going to test that

@Lucas-C
Copy link
Member

Lucas-C commented Apr 18, 2022

I have found another case where an infinite loop can still occur:

def test_multi_cell_with_limited_horizontal_space(tmp_path):  # issue #389
    pdf = FPDF()
    pdf.add_page()
    pdf.set_font("Helvetica", "", 10)
    pdf.multi_cell(w=pdf.epw - 2 * pdf.c_margin - 1, h=5, txt="test")
    assert pdf.x == pdf.l_margin + pdf.epw - 2 * pdf.c_margin - 1
    pdf.multi_cell(w=0, h=5, txt="test")
    assert_pdf_equal(pdf, HERE / "multi_cell_with_limited_horizontal_space.pdf", tmp_path, generate=True)

I think we should raise an error in MultiLineBreak.get_line_of_given_width() when this method return twice a manual_break() without consuming a character:
https://github.com/PyFPDF/fpdf2/blob/2.5.2/fpdf/line_break.py#L262

My instinct would be to simply add a dummy boolean state attribute to this class...
As you wrote it @gmischler, what do you think?

@gmischler
Copy link
Collaborator

As you wrote it @gmischler, what do you think?

@Lucas-C, I only modified that code a bit, it was really written by @oleksii-shyman.

The approach of checking for repeated lines with no content sounds about right.
It might be helpful if we can make sure that the new_x|y settings still get handled, even when an exception is raised.

Unfortunately, it gets a little more convoluted now that we need to handle the problem deeper down in the call chain...
The later phases of #339 might result in further tweeks (eg. changing the boolean to a counter), but let's cross that bridge when we get to it. 😉

Lucas-C added a commit that referenced this issue Apr 18, 2022
Lucas-C added a commit that referenced this issue Apr 18, 2022
@Lucas-C
Copy link
Member

Lucas-C commented Apr 18, 2022

Thank you for your answer @gmischler

I submitted #392 to fix this

Lucas-C added a commit that referenced this issue Apr 18, 2022
Lucas-C added a commit that referenced this issue Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants