FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

re-base of font fallback for pdf and eps output + SVG support by tacaswell · Pull Request #23559 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

re-base of font fallback for pdf and eps output + SVG support - #23559

Merged
efiring merged 10 commits into
matplotlib:mainfrom
tacaswell:pdf-fallback
Aug 11, 2022
ConversationCommits (10)ChecksFiles changed
Merged

re-base of font fallback for pdf and eps output + SVG support#23559
efiring merged 10 commits into
matplotlib:mainfrom
tacaswell:pdf-fallback

Conversation

tacaswell commented Aug 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

This is a rebase of #20804 and ##20832, given that there was not a lot of discussion on either PR, I am making a new PR rather than force-pushing to @aitikgupta 's branches.

My process here was to discard all changes to the c++ code and keep all changes to the Python code from #20804 and then implement the c++ layer needed to make the Python run (with some small changes to the Python + fixing up rebase errors.).
The current status of this:

  • the Agg tests still pass 🎉
  • almost all the non-fallback tests pass 🎉 (I am seeing an svg bold font failure locally, not sure if it is related)
  • the pdf test only shows the Chinese characters 😞 This was a "re use of names" bug 🎉
  • c++ needs more error checking 😞 This could still be better, but the one obvious place that needed it has it
  • recursive fallback is not implemented yet 😞 (but I think this is low-priority)
  • minimal test of get_fontmap 🎉
  • the c++ style is terrible 😞 acceptable now.

tacaswell added this to the v3.6.0 milestone Aug 5, 2022

tacaswell commented Aug 5, 2022

Copy link
Copy Markdown
Member Author

I have debugged this to something going wrong in writing the fonts and only one font being embedded in the PDF, but not why yet.

tacaswell requested a review from aitikgupta August 5, 2022 03:07
tacaswell changed the title re-base of font fallback for pdf output re-base of font fallback for pdf and eps output Aug 7, 2022

tacaswell commented Aug 7, 2022

Copy link
Copy Markdown
Member Author

I have now squashed all of @aitikgupta 's work (+ adapting to the new c++ API) into two commits + a c++ commit + 2 extra commits.

I think this is ready to review. The c++ needs the most attention. There is likely also some un-needed work in the PDF backend.

tacaswell marked this pull request as ready for review August 7, 2022 21:07

tacaswell commented Aug 7, 2022

Copy link
Copy Markdown
Member Author

Also looks like I accidentally used too-new of c++.

QuLogic reviewed Aug 9, 2022
View reviewed changes
Comment thread src/ft2font.cpp Outdated
Comment thread lib/matplotlib/backends/backend_pdf.py Outdated
Comment thread lib/matplotlib/backends/backend_pdf.py
Comment thread lib/matplotlib/backends/backend_ps.py Outdated
Comment thread lib/matplotlib/backends/backend_ps.py Outdated
QuLogic reviewed Aug 9, 2022
View reviewed changes
Comment thread lib/matplotlib/backends/backend_pdf.py Outdated
QuLogic reviewed Aug 9, 2022
View reviewed changes
Comment thread src/ft2font_wrapper.cpp Outdated
tacaswell changed the title re-base of font fallback for pdf and eps output re-base of font fallback for pdf and eps output + SVG support Aug 9, 2022
oscargus reviewed Aug 10, 2022
View reviewed changes
Comment thread lib/matplotlib/testing/_markers.py
oscargus reviewed Aug 10, 2022
View reviewed changes
Comment thread src/ft2font_wrapper.cpp Outdated
oscargus reviewed Aug 10, 2022
View reviewed changes
Comment thread lib/matplotlib/tests/test_ft2font.py
"family_name, file_name",
[
("WenQuanYi Zen Hei", "wqy-zenhei.ttc"),
("Noto Sans CJK JP", "NotoSansCJK-Regular.ttc"),

oscargus Aug 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not sure I fully get this (probably not), but for OSX the font seems to be NotoSansCJKsc-Regular.otf. Not that it may be required to add that...

tacaswell Aug 10, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I guess we need to extend this to be a set of possible names?

MNT: match capitalization of argument name
5d6ca83 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
tacaswell mentioned this pull request Aug 10, 2022
Closed
6 tasks
efiring merged commit a947b38 into matplotlib:main Aug 11, 2022
tacaswell deleted the pdf-fallback branch August 12, 2022 13:52
This was referenced Aug 12, 2022
Closed
Closed
Merged
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

oscargus oscargus left review comments
QuLogic QuLogic approved these changes
aitikgupta Awaiting requested review from aitikgupta

Assignees

No one assigned

Labels

backend: pdf backend: ps topic: text/fonts

Projects

None yet

Milestone

v3.6.0

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL