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

Refactor: Organize _render_styled_text_line() by fragment #519

Merged
merged 13 commits into from
Sep 5, 2022
Merged

Refactor: Organize _render_styled_text_line() by fragment #519

merged 13 commits into from
Sep 5, 2022

Conversation

gmischler
Copy link
Collaborator

@gmischler gmischler commented Sep 3, 2022

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
  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder
  • [NA] A mention of the change is present in CHANGELOG.md

Refactoring _render_styled_text_line() turned out to be more straightforward than could have been. All text rendering is now done based on the information in the current fragment, eliminating quite a bit of redundant code in the process. The distinction between core and TTF fonts is now done at the lowest level of the loop, where it actually matters. An inconvenient consequence was that the "Ws" instruction now comes after setting the font in the output. This is otherwise inconsequential, but made it necessary to replace quite a few PDFs.

I kept detecting more text related settings that I needed to move inside the loop. It turned out to be rather inconvenient that font_stretching and char_spacing are written to the PDF immediately when being set. I will open a seperate issue about this topic in general. For the moment, I ended up wrapping each line of text in "q ... Q" wrappers when any parameters are modified in a fragment. Since self.ws is only used inside of _render_styled_text_line() for justified alignment, I was able to eliminate it.

At first I thought that I'd had to do the same thing in text(), but that turned out not to be necessary just yet. I only cleaned it up a bit to make it more efficient. In order not to break anything when doing that I went looking for text()-specific tests cases, and didn't find any. Apparently in the past text() was only tested incidentally because it happens to be used in tests about other topics. So I had to create a test suite for it first, checking all possible parameter permutations ("test_text.py").

The new file "test_varied_fragments.py" demonstrates that we can now vary most parameters (font, size, stretching, char spacing, etc.) between the fragments handed to _render_styled_text_line(), while still being able to align und justify the text. This is the necessary functional foundation for solving #245, and implements phase 1. as outlined in #339. The next phase will be to create a hierarchy of TextArea() classes implementing different outline shapes, columns, and probably even an automatic table.

The most prominent thing that doesn't work yet is changing the text color between fragments. This is a particular challenge, as setting colors within a text object ("TB ... TE") does not seem to be allowed. So whenever a fragment changes color, we need to end the current text object and start a new one afterwards. But that's a limitation I can live with for the moment.

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #519 (a6692d3) into master (1949838) will increase coverage by 0.10%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
+ Coverage   92.23%   92.34%   +0.10%     
==========================================
  Files          23       23              
  Lines        6855     6860       +5     
  Branches     1410     1405       -5     
==========================================
+ Hits         6323     6335      +12     
+ Misses        303      299       -4     
+ Partials      229      226       -3     
Impacted Files Coverage Δ
fpdf/fpdf.py 90.52% <100.00%> (+0.26%) ⬆️
fpdf/line_break.py 99.05% <100.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gmischler
Copy link
Collaborator Author

Would be nice if the checks would give an indication about which specific test failed...

On my system, all tests run through, except for the embedding stuff as mentioned in #515 (which should pass here).

@RedShy
Copy link

RedShy commented Sep 4, 2022

Would be nice if the checks would give an indication about which specific test failed...

Hi @gmischler, maybe I'm saying something obvious and excuse me for that
I clicked "details" on the check, scrolling down I found the summary that report this

=========================== short test summary info ====================
FAILED test/image/test_load_image.py::test_share_images_cache - assert 0.2913...
================= 1 failed, 969 passed, 18 warnings in 35.21s ==================

@gmischler
Copy link
Collaborator Author

gmischler commented Sep 4, 2022

Hi @gmischler, maybe I'm saying something obvious and excuse me for that I clicked "details" on the check, scrolling down I found the summary that report this

=========================== short test summary info ====================
FAILED test/image/test_load_image.py::test_share_images_cache - assert 0.2913...
================= 1 failed, 969 passed, 18 warnings in 35.21s ==================

Not obvious at all, if you don't actually see such a thing... 🙄

But now that I knew that something should be there, I could figure out why it wasn't. Turns out that github has moved this functionality to yet another domain name, so that I had to instruct my javascript blocker to allow that one as well.
Mille gracie, now I can see again! 👁️

added:
Also, now I can see that the error is in test_share_images_cache, which I haven't touched at all. So the solution is out of my hands, until @Lucas-C finds a fix.

And finally:
After adding a few more tests (unrelated to the previous problem), everything runs through without an issue.
Go figure... I guess the original tests were operating on a corrupted copy of something.

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.

Nice work, the code is clearer and you improved the tests, thanks!

test/text/test_varied_fragments.py Show resolved Hide resolved
test/text/test_text.py Show resolved Hide resolved
fpdf/fpdf.py Show resolved Hide resolved
fpdf/fpdf.py Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
@Lucas-C Lucas-C merged commit 57d88c9 into py-pdf:master Sep 5, 2022
@Lucas-C
Copy link
Member

Lucas-C commented Sep 5, 2022

Merged, thank you @gmischler!

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