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

Made text elements optionally allowed #289

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

zond
Copy link
Contributor

@zond zond commented Feb 22, 2023

  • Added allow_text options to both topicosvg() and checkpicosvg()to enable simplifying SVGs with text elements. Note that the text elements will not be converted to paths, they will only pass through.
  • Added an allow_text boolean flag to the command line tool to control this behaviour.


self._update_etree()

for el in self.xpath("//*[name()='text']"):
Copy link
Contributor Author

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 SVGs don't allow me to use xpath to find "//text", but according to random bloggers on the web this is the way to do it instead.

@anthrotype
Copy link
Member

maybe we don't need to add a self.allow_text attribute, how about just add an allow_text=False parameter to topicosvg method instead?

also, maybe the command line tool could have an additional flag to control that. It's defined in this module

@anthrotype
Copy link
Member

anthrotype commented Feb 22, 2023

In general, I think we both agree that by default picosvg should not silently drop text elements but raise an error if these are present and the user didn't explicitly request to drop them. And then, it could have an optional mode where one decides to intentionally removekeep the text elements as part of the picosvg processing. And finally also a way to only remove the text elements (you did that already).
By not storing the allow_text in the SVG object state, but instead passing it down as a parameter to SVG.topicosvg, I think it makes the patch simpler/smaller (no need to add that _clone)

@anthrotype
Copy link
Member

the title "Added optional support for text elements" may be misunderstood as saying 'picosvg can now simplify/convert text elements to paths' but actually here we are additing support for dropping them, I suggest you reword the PR title accordingly

@anthrotype
Copy link
Member

actually wait... I got a bit confused.

I think what you want for your use case (from our private conversation) is to retain the text elements as they are, so you just need a way to tell picosvg to not raise an error if they are there. You don't need/want to remove them, and neither we do want to simply remove them. Eventually, one day we may wish to add support for transforming the text to outlines.

So I think we don't need to have a remove_text method at all. We only need a way to allow the text in (with a parameter to topicosvg which in turn passes it down to checkpicosvg). By default though, we should not allow it, and we can continue to raise an error that text elements are not proper 'pico' svg format.

@anthrotype
Copy link
Member

anthrotype commented Feb 22, 2023

technically we could also add support for removing text elements as in deleting them (not in the good sense of converting them to path which we haven't implemented yet), but what purpose would that serve, I'm not sure of its utility. Unlinke comments or other metadata, the text elements are proper data and picosvg in general doesn't offer ways to "remove this or that". It tries to retain the essence/visual appearance of the original svg while simplifying it's formatting.

@zond
Copy link
Contributor Author

zond commented Feb 22, 2023

Alright, I believe the best suggestion is to just pass an allow_text flag to topicosvg() and checkpicosvg(), and not have the allow_text attribute or remove_text method.

I updated the PR. WDYT?

@zond
Copy link
Contributor Author

zond commented Feb 22, 2023

Note that I still kept the _clone() method, since I feel like it's an improvement to the code, even if it doesn't feel as necessary anymore. WDYT?

@anthrotype anthrotype changed the title Added optional support for text elements. Add option to allow text elements Feb 22, 2023
@anthrotype
Copy link
Member

ok, thanks, looks good that way.
Mind adding a flag to the command line tool as well?

@anthrotype
Copy link
Member

also please edit the first message (PR description) as it's no longer in sync with the changes, thanks!

@zond zond changed the title Add option to allow text elements Made text elements optionally allowed Feb 22, 2023
- Added `allow_text` options to both `topicosvg()` and `checkpicosvg()`
  to enable simplifying SVGs with text elements. Note that the text
  elements will not be converted to paths, they will only pass through.
- Added an `allow_text` boolean flag to the command line tool to control
  this behaviour.
@zond
Copy link
Contributor Author

zond commented Feb 22, 2023

Ah, good catch, thanks. Also, I updated the command line tool.

@anthrotype anthrotype merged commit 155bd7d into googlefonts:main Feb 22, 2023
@anthrotype
Copy link
Member

Thanks

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.

2 participants