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

Expose image data as String with to_b64 and to_svg using Kaleido #251

Merged
merged 10 commits into from
Nov 25, 2024

Conversation

mchant
Copy link
Contributor

@mchant mchant commented Nov 22, 2024

No description provided.

@andrei-ng andrei-ng changed the title added to_b64 and to_svg Expose image data as String with to_b64 and to_svg using Kaleido Nov 23, 2024
@andrei-ng
Copy link
Collaborator

Hi @mchant, it looks good. I will like to do some small refactoring on your code changes. I will push on top of your branch if that is OK with you.

 - kaleido seems to be flaky on windows and macos CI even for newly
   added tests
 - target new kaleido unittests for linux only

Signed-off-by: Andrei Gherghescu <[email protected]>
@andrei-ng
Copy link
Collaborator

@mchant , I refactored the code a bit.
Also, thought it would be better to have to_base64 return just a String and an empty String for invalid image formats as this will make the API be consistent for the 3 functions save ,to_base64 and to_svg. If on the other hand you think it is better for your use case to have it return an Err, I will change it back.

Let me know what is your opinion. Besides that discussion, the PR is ready for merging IMO.

@mchant
Copy link
Contributor Author

mchant commented Nov 25, 2024

@andrei-ng, that works. I had it return an error if ImageFormat was not JPEG, PNG, or WEBP; however, I don't think it's harmful if an empty string is returned.

@andrei-ng
Copy link
Collaborator

@andrei-ng, that works. I had it return an error if ImageFormat was not JPEG, PNG, or WEBP; however, I don't think it's harmful if an empty string is returned.

OK. Let's return an empty string for now and if that doesn't work for you or other, I am going to refactor it to make all those functions return an error.

@andrei-ng andrei-ng merged commit f925085 into plotly:main Nov 25, 2024
19 checks passed
let expected = "<svg class=\"main-svg\" xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlink\" width=\"200\" height=\"150\" style=\"\" viewBox=\"0 0 200 150\"><rect x=\"0\" y=\"0\" width=\"200\" height=\"150\" style=\"fill: rgb(255, 255, 255); fill-opacity: 1;\"/><defs id=\"defs-2dc70a\"><g class=\"clips\"><clipPath id=\"clip2dc70axyplot\" class=\"plotclip\"><rect width=\"40\" height=\"2\"/></clipPath><clipPath class=\"axesclip\" id=\"clip2dc70ax\"><rect x=\"80\" y=\"0\" width=\"40\" height=\"150\"/></clipPath><clipPath class=\"axesclip\" id=\"clip2dc70ay\"><rect x=\"0\" y=\"82\" width=\"200\" height=\"2\"/></clipPath><clipPath class=\"axesclip\" id=\"clip2dc70axy\"><rect x=\"80\" y=\"82\" width=\"40\" height=\"2\"/></clipPath></g><g class=\"gradients\"/></defs><g class=\"bglayer\"/><g class=\"layer-below\"><g class=\"imagelayer\"/><g class=\"shapelayer\"/></g><g class=\"cartesianlayer\"><g class=\"subplot xy\"><g class=\"layer-subplot\"><g class=\"shapelayer\"/><g class=\"imagelayer\"/></g><g class=\"gridlayer\"><g class=\"x\"><path class=\"xgrid crisp\" transform=\"translate(100,0)\" d=\"M0,82v2\" style=\"stroke: rgb(238, 238, 238); stroke-opacity: 1; stroke-width: 1px;\"/><path class=\"xgrid crisp\" transform=\"translate(114.25,0)\" d=\"M0,82v2\" style=\"stroke: rgb(238, 238, 238); stroke-opacity: 1; stroke-width: 1px;\"/></g><g class=\"y\"/></g><g class=\"zerolinelayer\"><path class=\"xzl zl crisp\" transform=\"translate(85.75,0)\" d=\"M0,82v2\" style=\"stroke: rgb(68, 68, 68); stroke-opacity: 1; stroke-width: 1px;\"/></g><path class=\"xlines-below\"/><path class=\"ylines-below\"/><g class=\"overlines-below\"/><g class=\"xaxislayer-below\"/><g class=\"yaxislayer-below\"/><g class=\"overaxes-below\"/><g class=\"plot\" transform=\"translate(80,82)\" clip-path=\"url('#clip2dc70axyplot')\"><g class=\"scatterlayer mlayer\"><g class=\"trace scatter trace86f735\" style=\"stroke-miterlimit: 2; opacity: 1;\"><g class=\"fills\"/><g class=\"errorbars\"/><g class=\"lines\"><path class=\"js-line\" d=\"M5.75,1L20,0L34.25,2\" style=\"vector-effect: non-scaling-stroke; fill: none; stroke: rgb(31, 119, 180); stroke-opacity: 1; stroke-width: 2px; opacity: 1;\"/></g><g class=\"points\"><path class=\"point\" transform=\"translate(5.75,1)\" d=\"M3,0A3,3 0 1,1 0,-3A3,3 0 0,1 3,0Z\" style=\"opacity: 1; stroke-width: 0px; fill: rgb(31, 119, 180); fill-opacity: 1;\"/><path class=\"point\" transform=\"translate(20,0)\" d=\"M3,0A3,3 0 1,1 0,-3A3,3 0 0,1 3,0Z\" style=\"opacity: 1; stroke-width: 0px; fill: rgb(31, 119, 180); fill-opacity: 1;\"/><path class=\"point\" transform=\"translate(34.25,2)\" d=\"M3,0A3,3 0 1,1 0,-3A3,3 0 0,1 3,0Z\" style=\"opacity: 1; stroke-width: 0px; fill: rgb(31, 119, 180); fill-opacity: 1;\"/></g><g class=\"text\"/></g></g></g><g class=\"overplot\"/><path class=\"xlines-above crisp\" d=\"M0,0\" style=\"fill: none;\"/><path class=\"ylines-above crisp\" d=\"M0,0\" style=\"fill: none;\"/><g class=\"overlines-above\"/><g class=\"xaxislayer-above\"><g class=\"xtick\"><text text-anchor=\"middle\" x=\"0\" y=\"97\" transform=\"translate(85.75,0)\" style=\"font-family: 'Open Sans', verdana, arial, sans-serif; font-size: 12px; fill: rgb(68, 68, 68); fill-opacity: 1; white-space: pre;\">0</text></g><g class=\"xtick\"><text text-anchor=\"middle\" x=\"0\" y=\"97\" transform=\"translate(100,0)\" style=\"font-family: 'Open Sans', verdana, arial, sans-serif; font-size: 12px; fill: rgb(68, 68, 68); fill-opacity: 1; white-space: pre;\">1</text></g><g class=\"xtick\"><text text-anchor=\"middle\" x=\"0\" y=\"97\" transform=\"translate(114.25,0)\" style=\"font-family: 'Open Sans', verdana, arial, sans-serif; font-size: 12px; fill: rgb(68, 68, 68); fill-opacity: 1; white-space: pre;\">2</text></g></g><g class=\"yaxislayer-above\"><g class=\"ytick\"><text text-anchor=\"end\" x=\"79\" y=\"4.199999999999999\" transform=\"translate(0,84)\" style=\"font-family: 'Open Sans', verdana, arial, sans-serif; font-size: 12px; fill: rgb(68, 68, 68); fill-opacity: 1; white-space: pre;\">2</text></g><g class=\"ytick\"><text text-anchor=\"end\" x=\"79\" y=\"4.199999999999999\" transform=\"translate(0,83.5)\" style=\"font-family: 'Open Sans', verdana, arial, sans-serif; font-size: 12px; fill: rgb(68, 68, 68); fill-opacity: 1; white-space: pre;\">4</text></g><g class=\"ytick\"><text text-anchor=\"end\" x=\"79\" y=\"4.199999999999999\" transform=\"translate(0,83)\" style=\"font-family: 'Open Sans', verdana, arial, sans-serif; font-size: 12px; fill: rgb(68, 68, 68); fill-opacity: 1; white-space: pre;\">6</text></g><g class=\"ytick\"><text text-anchor=\"end\" x=\"79\" y=\"4.199999999999999\" transform=\"translate(0,82.5)\" style=\"font-family: 'Open Sans', verdana, arial, sans-serif; font-size: 12px; fill: rgb(68, 68, 68); fill-opacity: 1; white-space: pre;\">8</text></g><g class=\"ytick\"><text text-anchor=\"end\" x=\"79\" y=\"4.199999999999999\" transform=\"translate(0,82)\" style=\"font-family: 'Open Sans', verdana, arial, sans-serif; font-size: 12px; fill: rgb(68, 68, 68); fill-opacity: 1; white-space: pre;\">10</text></g></g><g class=\"overaxes-above\"/></g></g><g class=\"polarlayer\"/><g class=\"ternarylayer\"/><g class=\"geolayer\"/><g class=\"funnelarealayer\"/><g class=\"pielayer\"/><g class=\"treemaplayer\"/><g class=\"sunburstlayer\"/><g class=\"glimages\"/><defs id=\"topdefs-2dc70a\"><g class=\"clips\"/></defs><g class=\"layer-above\"><g class=\"imagelayer\"/><g class=\"shapelayer\"/></g><g class=\"infolayer\"><g class=\"g-gtitle\"/><g class=\"g-xtitle\"/><g class=\"g-ytitle\"/></g></svg>";
// Limit the test to the first LEN characters
const LEN: usize = 100;
assert_eq!(expected[..LEN], image_svg[..LEN]);

Choose a reason for hiding this comment

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

The first 100 characters are mostly (or just) SVG metadata that I suspect will make the test less robust.
Is the reason for this to avoid expensive computation or is it maybe to just test that in fact an SVG was produced?
If the latter, then I totally understand. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @emilbratt. Magical number indeed 😅. Well, when I added the test , I found out that the SVG ends up containing some element ids that are new with each invocation. Didn't have the time to go down the rabbit hole to find out how those get generated. So I ended up limiting the test to the beginning of the string that does not contain any such id. I agree with you, that probably it would be wiser to limit the assert to the first 10ish characters or so. Will probably change it to that before releasing the new version.

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.

3 participants