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

Font fallback #669

Merged
merged 34 commits into from
Mar 1, 2023
Merged

Font fallback #669

merged 34 commits into from
Mar 1, 2023

Conversation

andersonhc
Copy link
Collaborator

@andersonhc andersonhc commented Jan 26, 2023

This is a draft PR for handling #637
Sending it now so you can take a look and validate that's the path you want to take.

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

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

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

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 overall approach seems good to me!

Ping @gmischler if he wants to get a look at this 😊

fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
test/fonts/test_font_fallback.py Outdated Show resolved Hide resolved
test/fonts/test_font_fallback.py Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
@@ -1981,6 +1982,12 @@ def set_stretching(self, stretching):
if self.page > 0:
self._out(f"BT {stretching:.2f} Tz ET")

def set_fallback_fonts(self, fallback_fonts):
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe add_fallback_fonts() ?
That would give the users more flexibility, in case they discover they need to add another font later on.

Copy link
Member

Choose a reason for hiding this comment

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

add_fallback_fonts(), as I understand it, could be a bit limiting, as it would not allow to remove fallback fonts, for example in some sections of a document.
In that sense set_fallback_fonts() would provide a bit more control to the users: they could call it with more or less fonts, and even with [] to remove all fallback fonts.

@Lucas-C
Copy link
Member

Lucas-C commented Feb 16, 2023

Hi @andersonhc!

I hope our feedbacks on this PR did not discourage you 😊
Can we help you to finish this work?
This would be a nice new feature for fpdf2!

@andersonhc
Copy link
Collaborator Author

Hi @andersonhc!

I hope our feedbacks on this PR did not discourage you 😊 Can we help you to finish this work? This would be a nice new feature for fpdf2!

I got sidetracked working on #696.
I'll get back to this soon.

andersonhc and others added 9 commits February 16, 2023 21:51
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
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.

Great work!
I gave you some final feedbacks / suggestions.
I'll be happy to merge this PR as soon as those points are addressed 😊

test/fonts/test_font_fallback.py Outdated Show resolved Hide resolved
test/fonts/test_font_fallback.py Outdated Show resolved Hide resolved
test/fonts/test_font_fallback.py Outdated Show resolved Hide resolved
test/fonts/test_font_fallback.py Outdated Show resolved Hide resolved
test/fonts/test_font_fallback.py Outdated Show resolved Hide resolved
fpdf/output.py Show resolved Hide resolved
txt="hello world 😄 😁 😆 😅 ✌ 🤞 🌭 🍔 🍟 🍕", w=50, new_x=XPos.LMARGIN, new_y=YPos.NEXT
)
pdf.write(txt="hello world 😄 😁 😆 😅 ✌ 🤞 🌭 🍔 🍟 🍕")
assert_pdf_equal(pdf, HERE / "font_fallback.pdf", tmp_path)
Copy link
Member

Choose a reason for hiding this comment

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

Finally, could you please add a check that the warning you added in #661 is not raised in that scenario?

Copy link
Member

Choose a reason for hiding this comment

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

I think something along those lines should do the trick:

    assert len(caplog.text) == 0

@Lucas-C
Copy link
Member

Lucas-C commented Feb 27, 2023

You may want to get rid of PYTHONMALLOCSTATS as part of this PR to avoid the GitHub Actions logs being bloated, cf. #657 (comment)

docs/Fallback_fonts.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Patch coverage: 97.93% and project coverage change: +0.08 🎉

Comparison is base (2f062b2) 93.66% compared to head (1115aeb) 93.75%.

❗ Current head 1115aeb differs from pull request most recent head 3f6c8aa. Consider uploading reports for the commit 3f6c8aa to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #669      +/-   ##
==========================================
+ Coverage   93.66%   93.75%   +0.08%     
==========================================
  Files          26       26              
  Lines        6868     7008     +140     
  Branches     1226     1258      +32     
==========================================
+ Hits         6433     6570     +137     
- Misses        259      260       +1     
- Partials      176      178       +2     
Impacted Files Coverage Δ
fpdf/output.py 96.65% <92.30%> (-0.20%) ⬇️
fpdf/fpdf.py 91.92% <98.66%> (+0.26%) ⬆️
fpdf/enums.py 100.00% <100.00%> (ø)
fpdf/image_parsing.py 83.25% <100.00%> (+1.48%) ⬆️
fpdf/line_break.py 99.60% <100.00%> (+0.03%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Lucas-C Lucas-C merged commit 89c528d into py-pdf:master Mar 1, 2023
@Lucas-C
Copy link
Member

Lucas-C commented Mar 1, 2023

Merged! Thank you @andersonhc !

@Lucas-C
Copy link
Member

Lucas-C commented Mar 1, 2023

There was a minor bug remaining in this PR when markdown=True;
I opened #712 to fix it.
Could you please give it a look @andersonhc when you'll have some time?
I basically got rid of the style parameter of _get_fallback_font(),
and I'l like to know if you think that's fine 😊

@andersonhc
Copy link
Collaborator Author

There was a minor bug remaining in this PR when markdown=True; I opened #712 to fix it. Could you please give it a look @andersonhc when you'll have some time? I basically got rid of the style parameter of _get_fallback_font(), and I'l like to know if you think that's fine 😊

The 2 missing glyphs were by design... it was "bold" on markdown and I didn't set any bold emoji font, so they were missing just like it does when you don't have a fallback font.

The solution in #712 disregard style, so bold text can be replaced by a non-bold fallback font.

@Lucas-C
Copy link
Member

Lucas-C commented Mar 3, 2023

The 2 missing glyphs were by design... it was "bold" on markdown and I didn't set any bold emoji font, so they were missing just like it does when you don't have a fallback font.

Oh OK! I did not realize that it was "by design" 😊

The solution in #712 disregard style, so bold text can be replaced by a non-bold fallback font.

Indeed.

What would you think about introducing a new ignore_style optional argument to set_font_fallback()?
It would be False by default, and you original behaviour would be implemented, but settng it to True
would allow bold/italics text to be replaced by a non-bold fallback font.

(I suggested that we continue this discusion in comments on #712)

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.

3 participants