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

updates to SVG conversion: defs, shapes and clipping paths #968

Merged
merged 13 commits into from
Oct 24, 2023
Merged

updates to SVG conversion: defs, shapes and clipping paths #968

merged 13 commits into from
Oct 24, 2023

Conversation

afriedman412
Copy link

@afriedman412 afriedman412 commented Oct 18, 2023

Fixes #858

  • SVG conversion now handles clipping paths
  • various shapes (rect, circle etc) now work in defs
  • defs tags get read first instead of sequentially
    SVGs allow you pre-define objects anywhere, which means they can be cross-referenced before they are created. Our code didn't do that, but now it does.

ie fpdf can accurately convert this file now:

<svg>
<use href="#square" x="0" y="0" />
<defs>
  <rect id="square" width="100" height ="100" />
</defs>
</svg>

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
Collaborator

@gmischler gmischler left a comment

Choose a reason for hiding this comment

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

Looking good!

You probably want to remove those "actual.pdf", "actual_qpdf.pdf", and "expected_qpdf.pdf" files. I assume they are left over from your testing experiments?

@gmischler
Copy link
Collaborator

A few points more to round this up:

  • I just noticed another left over file "output_audit.py".
  • Your changes to ".gitignore" also don't look like they are of general use outside this PR, so please remove them as well.
  • Is there a test that loads all the possible shapes through defs and inserts them into the drawing? You might think that if rect and circle work, then they all should. But to avoid regressions with any possible changes in the future, it's best to just systematically verify that.
  • Please add a mention of the new capabilities to the *added* section of "CHANGELOG.md" as well as to the "supported SVG features" list in "SVG.md".

Don't worry that some of the test runs failed, that is an annoyance we need to address separately.

@afriedman412
Copy link
Author

above is done!

I also added code for making reference pdfs for testing -- I know it was already mentioned, but I figure putting the code in there explicitly will save someone time down the line

@gmischler
Copy link
Collaborator

Alright, one last thing that didn't previously catch my eye:
Your entry in CHANGELOG.md seems to be in the section for release 2.7.6, which is already out the door...

@codecov-commenter
Copy link

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (99e4d6d) 93.63% compared to head (fc6efe3) 93.70%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
+ Coverage   93.63%   93.70%   +0.06%     
==========================================
  Files          28       28              
  Lines        8248     8276      +28     
  Branches     1508     1514       +6     
==========================================
+ Hits         7723     7755      +32     
+ Misses        325      324       -1     
+ Partials      200      197       -3     
Files Coverage Δ
fpdf/svg.py 98.03% <96.00%> (+0.52%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gmischler
Copy link
Collaborator

Thanks for the excellent contribution, @afriedman412 !

Merging it now.

@gmischler gmischler merged commit eac6120 into py-pdf:master Oct 24, 2023
10 checks passed
@Lucas-C
Copy link
Member

Lucas-C commented Oct 30, 2023

Thank you for your excellent contribution @afriedman412,
and thanks for @gmischler for the review!

@Lucas-C
Copy link
Member

Lucas-C commented Nov 2, 2023

@allcontributors please add @afriedman412 for code

Copy link

@Lucas-C

I've put up a pull request to add @afriedman412! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

svg exported from matplotlib not inserted correctly - add support for clip-path & clipPath
4 participants