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

Switch SVG path parsing to fonttools #545

Merged
merged 6 commits into from
Sep 20, 2022

Conversation

gmischler
Copy link
Collaborator

@gmischler gmischler commented Sep 17, 2022

e.g. Fixes #525

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 nice part is that the new parser resolves everything to absolute coordinates, and it does so in a very simple and elegant way. For us this means that many class definitions dealing with relative movements have become redundant. After removing them, sgv.py is now significantly shorter.

At first, I couldn't get the "exponentated numbers" to parse correctly. Then I found that my copy of fonttools was slightly outdated, and they had just recently fixed a bug quite similar to our own #376. I have now pinned fonttools to the current release 4.37.2 or newer as well to make sure we have this fix available.

Previously, the "comma seperation" test converted "M0,1l2,3" into [M(0, 1), L2, 3)]. I don't think this was correct, since a relative line_to should end up at absolute (2,4). I didn't analyze if this was our own bug or one in "svg.path", but now it's gone anyway.

When a path gets closed with "Z/z", the fonttools parser adds an explicit extra "L" back to the start point. They may need this internally, but it has no purpose for us. I had to add methods to drawing._graphics_context() and drawing.PaintedPath() for removing the last element of a path again to eliminate this.

I had to replace a few of the reference PDFs. They all looked identical visually, but the new ones were consistently a bit smaller.

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 17, 2022

Codecov Report

Base: 94.12% // Head: 93.94% // Decreases project coverage by -0.17% ⚠️

Coverage data is based on head (ef05f6c) compared to base (af12339).
Patch coverage: 100.00% of modified lines in pull request are covered.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
- Coverage   94.12%   93.94%   -0.18%     
==========================================
  Files          22       22              
  Lines        6225     5999     -226     
  Branches     1271     1213      -58     
==========================================
- Hits         5859     5636     -223     
+ Misses        191      189       -2     
+ Partials      175      174       -1     
Impacted Files Coverage Δ
fpdf/drawing.py 92.43% <100.00%> (-0.57%) ⬇️
fpdf/svg.py 96.63% <100.00%> (+0.68%) ⬆️
fpdf/prefs.py 90.90% <0.00%> (-1.95%) ⬇️
fpdf/syntax.py 95.37% <0.00%> (-0.05%) ⬇️
fpdf/fpdf.py 92.59% <0.00%> (-0.02%) ⬇️
fpdf/sign.py 100.00% <0.00%> (ø)
fpdf/enums.py 100.00% <0.00%> (ø)
fpdf/__init__.py 100.00% <0.00%> (ø)

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.

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.

Good job!
Nice code shrinking here, thank you @gmischler!

By the way, don't hesitate to ping me by email whenever you want to get the role of a maintainer on this project, I still think fpdf2 would benefit from it and you clearly deserve it

CHANGELOG.md Show resolved Hide resolved
docs/index.md Show resolved Hide resolved
fpdf/svg.py Outdated Show resolved Hide resolved
test/drawing/test_drawing.py Show resolved Hide resolved
@Lucas-C
Copy link
Member

Lucas-C commented Sep 19, 2022

$ pylint fpdf test tutorial/tuto*.py
************* Module fpdf.svg
fpdf/svg.py:552:0: W0223: Method 'addComponent' is abstract in class 'AbstractPen' but is not overridden (abstract-method)
fpdf/svg.py:558:4: W0237: Parameter 'pt' has been renamed to 'end' in overridden 'PathPen.moveTo' method (arguments-renamed)
fpdf/svg.py:564:4: W0237: Parameter 'pt' has been renamed to 'end' in overridden 'PathPen.lineTo' method (arguments-renamed)
fpdf/svg.py:570:4: W0221: Number of parameters was 2 in 'AbstractPen.curveTo' and is now 4 in overridden 'PathPen.curveTo' method (arguments-differ)
fpdf/svg.py:570:4: W0221: Variadics removed in overridden 'PathPen.curveTo' method (arguments-differ)
fpdf/svg.py:583:4: W0221: Number of parameters was 2 in 'AbstractPen.qCurveTo' and is now 3 in overridden 'PathPen.qCurveTo' method (arguments-differ)
fpdf/svg.py:583:4: W0221: Variadics removed in overridden 'PathPen.qCurveTo' method (arguments-differ)

@Lucas-C
Copy link
Member

Lucas-C commented Sep 19, 2022

To me, this looks ready to be merged 😊

setup.py Outdated Show resolved Hide resolved
@Lucas-C Lucas-C merged commit bea5d39 into py-pdf:master Sep 20, 2022
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.

fonttools.svgPath.path.parse_path() to replace svg.path?
2 participants