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

Stop font properties from leaking #384

Merged
merged 62 commits into from
Apr 12, 2022
Merged

Stop font properties from leaking #384

merged 62 commits into from
Apr 12, 2022

Conversation

gmischler
Copy link
Collaborator

Fixes #359 (and possibly others)

Checklist:

  • The GitHub pipeline is OK (green),
    meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • A unit test is covering the code added / modified by this PR

  • This PR is ready to be merged

  • [na] In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

  • The PR description or comment contains a picture of a cute animal (not mandatory but encouraged 🐸)

This solves the problems with leaking state after certain combinations of text output. It replaces several previous attempts to solve similar problems, and does so in a more straightforward and complete way.

  • Using qQ when the last fragment uses a different font/style than the current setting sometimes increases the size, sometimes just moves the q to a different location, but often also reduces it because the following text doesn't need to set the font.
  • In some cases, Tw was added seperately to the rest of the content. Fixing that moved it to a different location, but usually didn't change the length (sometimes it elminated a newline).
  • Found a way to save 3 bytes per word with unicode fonts (merging the leading space with the following word).

Since there were worries about increasing the file size of the resulting PDFs, here's a rundown of the affected test files:

same size:

  • moving Tw - render_styled_newpos.pdf
  • moving Tw - multi_cell_newpos.pdf
  • moving Tw - multi_cell_ln_newpos.pdf
  • moving Tw - multi_cell_markdown_with_fill_color.pdf
  • moving Tw - multi_cell_markdown_justified.pdf
  • moving Tw - simple_outline.pdf
  • moving qQ - graphics_context.pdf
  • moving qQ - links.pdf
  • moving qQ - link_with_zoom_and_shift.pdf
  • moving qQ - html_features.pdf
  • moving qQ - html_justify_paragraph.pdf
  • moving qQ - flextemplate_rotation.pdf
  • moving qQ - flextemplate_multipage.pdf
  • moving qQ - flextemplate_elements.pdf
  • moving qQ - template_nominal_hardcoded.pdf
  • moving qQ - template_nominal_csv.pdf
  • moving qQ - template_nominal_multipage.pdf
  • moving qQ - template_nominal_justify.pdf

shorter

  • moving Tw - 2_pages_outline.pdf
  • moving Tw - ln_positioning_and_page_breaking_for_multicell.pdf
  • replace F with qQ - cell_markdown_bleeding.pdf
  • replace F with qQ - cell_markdown_right_aligned.pdf
  • ttf words - multi_cell_j_paragraphs
  • moving qQ - test_img_inside_html_table_centered_with_align.pdf

longer

  • add qQ - multi_cell_markdown
  • add qQ /ttf words - multi_cell_justified_with_unicode_fonts
  • add qQ /ttf words - multi_cell_markdown_with_ttf_fonts

@gmischler gmischler requested a review from Lucas-C as a code owner April 9, 2022 14:25
@codecov
Copy link

codecov bot commented Apr 9, 2022

Codecov Report

Merging #384 (b1c877c) into master (2df5409) will increase coverage by 0.06%.
The diff coverage is 97.95%.

❗ Current head b1c877c differs from pull request most recent head a1f581d. Consider uploading reports for the commit a1f581d to get more accurate results

@@            Coverage Diff             @@
##           master     #384      +/-   ##
==========================================
+ Coverage   90.76%   90.82%   +0.06%     
==========================================
  Files          21       20       -1     
  Lines        6097     6041      -56     
  Branches     1247     1231      -16     
==========================================
- Hits         5534     5487      -47     
+ Misses        331      326       -5     
+ Partials      232      228       -4     
Impacted Files Coverage Δ
fpdf/fpdf.py 86.65% <97.95%> (+0.21%) ⬆️
fpdf/html.py 95.21% <0.00%> (ø)
fpdf/recorder.py 88.88% <0.00%> (ø)
fpdf/graphics_state.py 100.00% <0.00%> (ø)
fpdf/enums.py
fpdf/util.py 86.20% <0.00%> (+5.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2df5409...a1f581d. Read the comment docs.

@Lucas-C
Copy link
Member

Lucas-C commented Apr 10, 2022

This PR includes 88 commits:
you probably should rebase your branch on the master branch of this repo

@gmischler
Copy link
Collaborator Author

This PR includes 88 commits: you probably should rebase your branch on the master branch of this repo

I've tried, but Git Desktop repeatedly got confused and I had to cancel the attempt...

@Lucas-C
Copy link
Member

Lucas-C commented Apr 10, 2022

I performed the rebase. Could you check that all is right @gmischler please?

Also, seems like many reference PDF files have been impacted by this change:
could you explain how / why?

@gmischler
Copy link
Collaborator Author

I performed the rebase. Could you check that all is right @gmischler please?

Also, seems like many reference PDF files have been impacted by this change: could you explain how / why?

I was just doing another rebase attempt myself again, which may have conflicted with yours...

There now seem to be a few files included that shouldn't be.
I'll have to comb through everything to make sure it's actually in a useable state.

And PDFs that aren't listed and explained at the top here should probably not be included.

@gmischler
Copy link
Collaborator Author

Ok, I think it's clean now.

Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

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

The code seems quite clearer after this PR, nice and good job!

Thank you very much for the rundown on the affected test files sizes.

However it does not seem totally accurate...
render_styled_newpos.pdf for example is 15bytes larger.
Would you know why?

fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
test/text/test_multi_cell.py Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Show resolved Hide resolved
@gmischler
Copy link
Collaborator Author

Thank you very much for the rundown on the affected test files sizes.

However it does not seem totally accurate... render_styled_newpos.pdf for example is 15bytes larger. Would you know why?

Interesting.
I had compared the uncompressed files, which are the same length. Apparently in this case the compression is slightly less effective for the new version. I would expect such an effect to average out in total, though.

@Lucas-C
Copy link
Member

Lucas-C commented Apr 12, 2022

I'm OK to merge this once the conflicts are resolved

@Lucas-C
Copy link
Member

Lucas-C commented Apr 12, 2022

Just a heads-up: I'm planning to remove the useless \n characters from the calls to FPDF._out() in local_context() and rect_clip() as part of #386 This will generate several minor changes in reference PDF files, so I'll wait for your PR to be merged first

@Lucas-C Lucas-C merged commit cdc2ce7 into py-pdf:master Apr 12, 2022
@Lucas-C
Copy link
Member

Lucas-C commented Apr 12, 2022

Merged: thank you @gmischler !

@gmischler gmischler deleted the leak_stop branch April 12, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants