-
Notifications
You must be signed in to change notification settings - Fork 323
Support WOFF and WOFF2 fonts #1660
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
base: master
Are you sure you want to change the base?
Conversation
|
inform me if there is any problem |
andersonhc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR.
I like the solution you found for harfbuzz.
Did you try producing a PDF with a WOFF font? Does the subset at output already decompress the font without any additional parameter?
Please create some tests on test/fonts/test_add_fonts.py. You can use WOFF and WOFF2 fonts, with and without text shaping.
Let us know if you have any question.
| if TYPE_CHECKING: # Help static type checkers / language servers locate optional deps | ||
| try: # pragma: no cover - typing-only | ||
| import uharfbuzz as hb # type: ignore | ||
| except Exception: | ||
| hb = None # type: ignore | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you added this block of code, since right below the code already does the exact same thing (import uharfbuzz as hb or default to None if there is an error)
fpdf/fpdf.py
Outdated
| if TYPE_CHECKING: # Help static type checkers / language servers locate optional deps | ||
| try: # pragma: no cover - typing-only | ||
| import endesive # type: ignore | ||
| except Exception: | ||
| pass | ||
| try: # pragma: no cover - typing-only | ||
| import uharfbuzz # type: ignore | ||
| except Exception: | ||
| pass | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will soon add the optional dependencies on the pyproject.toml
I am not convinced this block of code is really needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok ,I will try to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sir I am just beginner for contributing for open source, if any mistakes tell me I will try fix that
- fpdf/fonts.py: * Added explanatory comment for TYPE_CHECKING block clarifying it's for static type checkers only * Optimized hbfont property to avoid performance hit for non-WOFF fonts by checking file extension * For WOFF/WOFF2: uses byte buffer decompression (required for HarfBuzz) * For TTF/OTF: loads directly from file path (faster, no extra serialization) * Removed invalid fallback for WOFF/WOFF2 that would fail anyway * Added logging for failed temp file cleanup to track orphaned files - fpdf/fpdf.py: * Removed unnecessary TYPE_CHECKING block for optional dependencies * Fixed table() method return type from Iterator[Table] to ContextManager[Table]
BharathPESU
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have changed some part of code,review it and inform me ,if any changes required
Can you please create some tests on
Look the |
| # If the user passed a WOFF2 file but brotli is not installed, fontTools | ||
| # raises an ImportError/RuntimeError during parsing. Provide a clearer hint | ||
| # only for that specific situation. Allow other exceptions (e.g. FileNotFoundError, | ||
| # OSError, parsing errors) to propagate normally so they aren't masked here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you manage to create a unit test for this case, please?
|
Hey @BharathPESU |
e.g. Fixes #0
Checklist:
A unit test is covering the code added / modified by this PR
In case of a new feature, docstrings have been added, with also some documentation in the
docs/folderA mention of the change is present in
CHANGELOG.mdThis PR is ready to be merged
By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.