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

Allow SVGWriter to avoid having the background element #211

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

fra87
Copy link
Contributor

@fra87 fra87 commented Aug 26, 2023

With this PR I modified SVGWriter to be able to avoid having the background element on SVG images

If you set the "background" to None or (255,255,255,0) (i.e. transparent) in the writer_options parameter when calling render then the background will not be written in the SVG (instead of having a rectangle).

This is because transparent background does not seem to work in the current library (it is black)

}
_set_attributes(background, **attributes)
self._group.appendChild(background)
if self.background and self.background != (255, 255, 255, 0):
Copy link
Owner

Choose a reason for hiding this comment

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

I think it makes sense to exclude the background rectangle if self.background is None.

But I'm not convinced of excluding it if self.background == (255, 255, 255, 0). Why exclude it instead of respecting the provided input?

Using None sounds like the correct approach for somebody who wants it entirely excluded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this derives from my first usages: I wanted to have a transparent background, but using a 4-items tuple resulted in a black background. I did not go deeper in this (maybe there is a need to write background = (255, 255, 255) and then pass opacity 0), but definitely setting background to (255, 255, 255, 0) does not produce a transparent background.

I do, however, agree with you that this is more "sketchy", so maybe leaving only "None" as option and finding the correct way to manage transparency is better

Will you do the modification or I shall modify the PR?

Copy link
Owner

Choose a reason for hiding this comment

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

setting background to (255, 255, 255, 0) does not produce a transparent background.

Do you have a quick example of this? Did you check the background= property on the SVG XML itself? Maybe it was the program that rendered it that didn't handle alpha properly?

I do, however, agree with you that this is more "sketchy", so maybe leaving only "None" as option and finding the correct way to manage transparency is better

Will you do the modification or I shall modify the PR?

Please update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a quick example of this? Did you check the background= property on the SVG XML itself? Maybe it was the program that rendered it that didn't handle alpha properly?

Honestly I don't know where I got the hint to write (255,255,255,0), since I did it a long time ago, but it appears that CSS (and consequently SVG) do not support that notation.

The library correctly wrote fill:(255, 255, 255, 0), but this is wrong for two reasons: first you must write rgb(r, g, b) if you want to use three separate values, and then the fill parameter does not support alpha channel. The correct way to set transparency is using fill-opacity, so the style for the background rect shall be:

fill:rgb(255, 255, 255);fill-opacity:0

Up to now there is no way to set the fill-opacity parameter (maybe this can be added?), but with this PR maybe it will not be so much necessary.

In any case I'll update the PR

@WhyNotHugo WhyNotHugo merged commit 3a82036 into WhyNotHugo:main Oct 3, 2023
13 checks passed
@WhyNotHugo
Copy link
Owner

Thanks!

WhyNotHugo added a commit that referenced this pull request Oct 3, 2023
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