googlefonts / picosvg

Helps simplify SVG files. Intended for use as part of a font build.
Apache License 2.0
142 stars 12 forks source link

Made picosvg ignore <?xpacket?> tags. #288

Closed zond closed 1 year ago

zond commented 1 year ago
anthrotype commented 1 year ago

regarding the CI lint job failing, you can fix it by installing the black python auto-formatter (pip install black) and running it on the files that you modified.

zond commented 1 year ago

Hey @anthrotype,

I moved the test to svg_test.py and removed xpacket_test.py.

The thing is - I only ignored ?xpacket the way comments are ignored - and that was evidently enough to not crash a call to simplify(), but the ?xpacket is still there.

WDYT?

anthrotype commented 1 year ago

Should comments actually be removed?

they are removed, but not in simplify. The SVG.topicosvg() is the main public entry point, and that one calls a bunch of other methods like remove_comments or remove_nonsvg_content or remove_title_meta_desc (BTW, if your xpacket elements are inside a metadata element, like they seem to be in the test svg file you uploaded, then they should be removed by the latter method).

The simplify method is about removing groups, applying clip-paths and transforms (stuff that needs to be done all at once as opposed to one at a time) and isn't really meant to be called in isolation from the rest, but as part of topicosvg public method, and it's not currently specifically tested on its own, as you can see. But I understand you would like to apply only some picosvg transformations and not all of them, hence your need to call directly the simplify method instead of the entire topicosvg method with all the accompanying transformations.

How about you add a new remove_processing_instructions method (similar to remove_comments, and should be also called inside topicosvg) whichdoes what you want and add a specific unit test for that (instead of calling simplify)?

zond commented 1 year ago

Ah, now it makes more sense. Thank you for explaining.

I updated the PR to add a function to remove processing instructions (like comments are removed), and made topicosvg use it. See what you think!

zond commented 1 year ago

Cool, thanks!

I am happy with this PR now, but I don't have write access, so please merge at will :)

anthrotype commented 1 year ago

Thank you!

zond commented 1 year ago

Likewise! Woohoo!

anthrotype commented 1 year ago

FYI, I made a minor fixup to the tests here

zond commented 1 year ago

Ah, that's much nicer - thanks!