Skip to content

Conversation

@TheMDev
Copy link

@TheMDev TheMDev commented Jan 22, 2025

When attempting to add SVG images to connectors and cables, they failed to show up even though the base64 content was correct in the HTML and SVG outputs.
Checking https://www.w3.org/TR/SVG11/intro.html reveals the mime type should be image/svg+xml, not image/svg.
As the code already handles mime subtype replacements for jpeg and tiff, adding svg here made for a simple fix.

@kvid
Copy link
Collaborator

kvid commented Feb 8, 2025

Thank you for detecting and suggesting a fix for this bug. I tried your fix and I can verify that the HTML output looked correct, but I noticed that the SVG image for a connector is still not shown in the PNG output. I understand that such a problem is outside the scope of this PR, but I wonder if this is also observed by others. JPG, PNG, and GIF images in connectors seems to work fine, but a TIF image was rejected by Graphviz. It would be nice to create a test use case with all image types expected to work, and perhaps a separate issue for unexpected outputs for such a test use case.

The automatic checks are currently failing due to #441, that now should be fixed for PRs with dev as target, but I probably need to cherry-pick a commit into master to also fix PRs with master as target.

@kvid
Copy link
Collaborator

kvid commented Feb 15, 2025

The commit mentioned above is now cherry-picked into master, but the automatc checks seem not to be re-executed automatically. Maybe we need to rebase this PR branch on top of the latest master (there should not be any conflicts) to re-execute the latest versions of these automatc checks?

@TheMDev - do you want to try doing this yourself, or should I do it?

@TheMDev
Copy link
Author

TheMDev commented Feb 17, 2025

I'm sorry for not getting back to you sooner. I cannot replicate your issue where an SVG image does not appear in a PNG output, but I did notice that the PNG output does not follow the same scaling as the HTML and SVG outputs. It seems this is because it uses a different render function that would likely require a rewrite. A more straightforward solution is to convert the SVG output to PNG at export. If you can provide more detailed information on the issue you noticed, I would be more than happy to look into it in this PR, but the scaling issue warrants a separate issue with more discussion on how to resolve it.

Ill rebase my branch onto master here shortly.

@kvid
Copy link
Collaborator

kvid commented Feb 22, 2025

@TheMDev wrote:

I'm sorry for not getting back to you sooner.

Nobody are contributing to this project as a payed full-time work and expected to answer quickly.

I cannot replicate your issue where an SVG image does not appear in a PNG output, but I did notice that the PNG output does not follow the same scaling as the HTML and SVG outputs. It seems this is because it uses a different render function that would likely require a rewrite. A more straightforward solution is to convert the SVG output to PNG at export. If you can provide more detailed information on the issue you noticed, I would be more than happy to look into it in this PR, but the scaling issue warrants a separate issue with more discussion on how to resolve it.

I just created #445 with my preliminary test use case where I also made a comment with my outputs. I don't observe any scaling differences.

Feel free to add a similar comment there with your environment and outputs, and optionally suggest improvements to the test use case.

I'll rebase my branch onto master here shortly.

Thank's. That helped. All automatic checks have passed now.

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

I'm sorry that I forgot to formally review this PR when testing it several months ago. In my opinion, this is a solid fix to a verified bug, and I find it impossible to find a simpler implementation. 😄

I recommend this as a good candidate for a hot fix release. If there are a couple of other such candidates ready, they might be combined.

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