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

Better handling of text overflow in FPDF.write() & FPDF.write_html() - fix #847 #850

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

Lucas-C
Copy link
Member

@Lucas-C Lucas-C commented Jul 10, 2023

This PR also silence warnings in test/text/test_unbreakable.py by moving the with pytest.warns() block at the correct indentation level

Copy link

@stenci stenci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested these changes with my tests, and the result is now correct. I don't see any empty lines.

I'm not familiar with github, I don't know if it would be possible to pip install this branch, or to clone and use this in some way in my environment. I manually applied the same changes that I saw in this diff to the source code installed by pip on my environment and ran my own tests.

@Lucas-C Lucas-C requested a review from gmischler July 11, 2023 14:12
Copy link
Collaborator

@gmischler gmischler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Don't remember the exact details of why we introduced the wordsplit argument back then. But if we can get rid of it again then that visibly simplifies the code.

I see that test/text/write_soft_hyphen.pdf now wraps one line a bit later than before. Any idea why (or why it didn't do that before)? I hope we're not overshooting the boundaries there now. In a few other test files I've drawn vertical lines at the boundaries or painted the background yellow to explicitly show we're staying within. Maybe we should do that here as well to be sure.

old:
image

new:
image

@Lucas-C
Copy link
Member Author

Lucas-C commented Jul 12, 2023

I see that test/text/write_soft_hyphen.pdf now wraps one line a bit later than before. Any idea why (or why it didn't do that before)? I hope we're not overshooting the boundaries there now. In a few other test files I've drawn vertical lines at the boundaries or painted the background yellow to explicitly show we're staying within. Maybe we should do that here as well to be sure.

Yes I'm well aware of that 😊
I added an explanatory comment in this PR to explain that and keep a trace of this change of behaviour:
https://github.com/PyFPDF/fpdf2/blob/issue-847/test/text/test_write.py#L20

I think the idea at the time for this wordsplit argument was to avoid word-splitting in this particular case of calling FPDF.write() really close to the right edge of the page.

As this is the only reference PDF file in our test suite that is impacted by this change,
I think it's worth it. I'm going to merge this PR now.

@Lucas-C Lucas-C merged commit e9abc00 into master Jul 12, 2023
11 checks passed
@Lucas-C Lucas-C deleted the issue-847 branch July 12, 2023 18:23
Tolker-KU pushed a commit to Tolker-KU/fpdf2 that referenced this pull request Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants