joplin
joplin copied to clipboard
desktop: Fixes #7521 Svg images are correctly sized when exported as png
Mermaid images when exported as png were having very low resolution.
Before
using rendered image's dimensions as exported image dimension
After
using SVG's viewBox attribute as exported image's dimensions
Fixes #7521
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅
I have read the CLA Document and I hereby sign the CLA
~~converted to draft since added jest test fails when run together using yarn test
, they are passing when only that test file is run individually~~
TypeError: (0 , contextMenuUtils_1.svgDimensions) is not a function or its return value is not iterable
edit: was some hoisting err perhaps, solved by removing and reinstalling node_modules
The viewport is the dimension in user space - doesn't it mean that if the window is small, the exported PNG will be small too? And we wouldn't want that, instead it should be dimensions that make sense (although I don't know what that is), independent of the viewport.
Why is the PNG small to begin with? Isn't it one of those things where we can just double the exported dimensions and it will work?
if the window is small, the exported PNG will be small too
~yeah that would be a problem in this approach 🤔~ viewBox is not affected by resizing window
Why is the PNG small to begin with
cause we are making an <img>
element for the svg and using it's resolution to set exported image's resolution
img is a replaced element and it's default size is 300 * 150 a/c to google search.
we can just double the exported dimensions and it will work?
considering below two images
both have same height of 150px (default height of replaced elements) so if we double them these will still have same height which I guess is not desirable in this case.
So what would be the solution then?
It might make sense to have a minimum dimension when exporting SVGs. We could then increase the image's width and height if necessary.
For example, if the minimum dimension is 400, img.height = 20
, and img.width = 100
, we could multiply width and height by 20, making $\min(\texttt{img.width}, \texttt{img.height}) = \min(20\times 100, 20\times 20) = \min(1000, 400) = 400$.
yeah that would be a problem in this approach 🤔
my bad 😅 , the viewbox's values are not varying when I resize the window of joplin if that's what we are concerned with.
it might make sense to have a minimum dimension when exporting SVGs. We could then increase the image's width and height if necessary. For example, if the minimum dimension is 400,
img.height = 20
, andimg.width = 100
, we could multiply width and height by 20, making min(img.width,img.height)=min(20×100,20×20)=min(1000,400)=400.
yeah that would be nice
so what is the final implementation ? IMO combining viewbox information and minimum dimension approach explained by Henry would be sufficient for this ? should I implement and push that ?
It might make sense to have a minimum dimension when exporting SVGs. We could then increase the image's width and height if necessary. For example, if the minimum dimension is 400,
img.height = 20
, andimg.width = 100
, we could multiply width and height by 20, making min(img.width,img.height)=min(20×100,20×20)=min(1000,400)=400.
I don't think a hard coded minimum dimension would work, because 400 is way too big for certain tiny diagrams, or way too small for massive ones. Basically there can be all sizes of diagrams so the dimensions we choose need to be based on the diagram.
@adarsh-sgh, did you check the Mermaid API - do they provide some info about the diagram such as recommended width or similar?
did you check the Mermaid API
could not find anything like that there, but found about their live editor which also has PNG download
functionality.
they are using dimensions of rendered svg (on the preview side) as default resolution for downloaded image, and allowing user to override height or width by entering a value for it.
https://github.com/mermaid-js/mermaid-live-editor/blob/7c64a6549779435986739d48d5dbf3710725c281/src/lib/components/Actions.svelte#L35
they are using dimensions of rendered svg (on the preview side) as default resolution for downloaded image, and allowing user to override height or width by entering a value for it.
Sounds reasonable. How about using this technique but double the dimensions (just to be sure it's high res enough for printing)?
Sounds reasonable. How about using this technique but double the dimensions (just to be sure it's high res enough for printing)?
Dimensions of rendered Svg will depend on window size, That's something we want to avoid I guess ?
if the window is small, the exported PNG will be small too? And we wouldn't want that
How about using this technique
Yes, allowing user's to override auto detected dimensions would be a very helpful feature IMO not sure about other point.
Dimensions of rendered Svg will depend on window size, That's something we want to avoid I guess ?
But what's the alternative, do you have any suggestion?
We don't want to ask the user for dimensions, just double it and that should work in most cases.
But what's the alternative, do you have any suggestion?
using viewBox attribute's height and width values is a good plan IMO.
- it's independent of window size in which diagram is rendered.
- it's values are proportional to size of mermaid diagram.
small and big diagrams example
these are PNGs exported with viewBox
dimensions
mermaid with a single block (39 * 55)
bigger mermaid diagram (389*303)
But since those are user space dimensions, isn't it the same as using the window dimensions? Also what is setting the viewbox? Is that the mermaid lib? And how do they determine the dimensions then?
what is setting the viewbox? Is that the mermaid lib
Yes.
how do they determine the dimensions then?
They $draw^1$ the svg and use it's getBBox
method to get the smallest rectangle in which object fits.
- d3 lib is used to draw it
But since those are user space dimensions, isn't it the same as using the window dimensions?
sorry I could not get which window we are talking about.
But the part I don't get here is that it's vector graphics, so the vector dimensions are irrelevant and can't be of any help to export to png. And the coordinates are floating values, so for example you could fit an infinity of vector graphics in a 1.0 x 1.0 square. However if you export that to a 1x1 pixels image, that's not going to make any sense.
So how is it done normally, are there ways to get reasonable dimensions from some API or calculations? Does the DOM SVG API provides this maybe?
are there ways to get reasonable dimensions from some API or calculations? Does the DOM SVG API provides this maybe?
Could not find anything useful in DOM svg APIs. getBBox was already discussed above but it has same pros and cons as viewBox.
When we render a mermaid diagram using mermaid js in browser, then the size in which browser renders it, can we say that that size is a reasonable dimension ?
it's vector graphics, so the vector dimensions are irrelevant and can't be of any help to export to png. And the coordinates are floating values, so for example you could fit an infinity of vector graphics in a 1.0 x 1.0 square.
yes that is a problem, by using viewBox as exported dimensions we are making this assumption that viewBox size is also the recommended size of svg, which seems true in case of mermaid exported svgs since they render with same size as defined in viewBox not scaled up or down.
When we render a mermaid diagram using mermaid js in browser, then the size in which browser renders it, can we say that that size is a reasonable dimension ?
Yes that's what I meant by window dimensions.
So it seems we always go back to this, so let's do this. You don't need the XML regex parsing as that's too unreliable, I think you can get the width and height directly from the DOM. Then double those dimensions to make sure it's always big enough even on hdpi screens.
In fact, doesn't it mean that we keep the current code as it is but add * 2
for the dimensions?
You don't need the XML regex parsing as that's too unreliable
Yes already removed that and was using dom parser to get those values, didn't pushed since I was not sure if we will choose this way or not.
double those dimensions to make sure it's always big enough even on hdpi screens
Okay, on it.
doesn't it mean that we keep the current code as it is but add
* 2
for the dimensions?
No, That will be different, all images will have dimension 600 * 300. 2nd part of comment linked below explains why https://github.com/laurent22/joplin/pull/7546#issuecomment-1366789571
No, That will be different, all images will have dimension 600 * 300. 2nd part of comment linked below explains why #7546 (comment)
I don't know about the technical details, but the dimensions of all the exported diagrams should not be 600x300 - at a minimum the aspect ratio should match the diagram aspect ratio, which I assume we can get accurately. And the size should be based on the note viewer size.
dimensions of all the exported diagrams should not be 600x300 - at a minimum the aspect ratio should match the diagram aspect ratio, which I assume we can get accurately. And the size should be based on the note viewer size.
Sure, That's how it will be. 🙌🏼
we will
- Parse the svg using DOMParser to get id of svg
- getBoundingClientRect by selecting the svg from dom (like mermaid live editor does)
- Parse the svg using DOMParser to get id of svg
But why do you need to parse the SVG? In svgUriToPng there's full access to the DOM so you shouldn't need to do any XML regex parsing.
why do you need to parse the SVG there's full access to the DOM
we have access to the document but we need to select the svg element to get it's rendered size, I was thinking of getting the ID of svg using the svg string we are given, then select svg element from dom using querySelector
you shouldn't need to do any XML regex parsing.
instead of regex, DOMParser.parseFromString() can be used, it wouldn't have those drawbacks I think.
edit: I have pushed the proposed changes for discussion. will add tests if this approach is fine.
Please run yarn run linter ./
since I've updated the linter rules
I am facing a problem while trying to write jest tests , jsDom does not render elements so getBoundingClientRect() always returns all dimensions as 0.
I was trying to test svgDimensions
function. any suggestions how to do this ?
Ok let's drop the testing. Thanks for implementing this