joplin icon indicating copy to clipboard operation
joplin copied to clipboard

desktop: Fixes #7521 Svg images are correctly sized when exported as png

Open adarsh-sgh opened this issue 2 years ago • 18 comments

Mermaid images when exported as png were having very low resolution.

Before

using rendered image's dimensions as exported image dimension image

After

using SVG's viewBox attribute as exported image's dimensions image

Fixes #7521

adarsh-sgh avatar Dec 28 '22 08:12 adarsh-sgh

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

github-actions[bot] avatar Dec 28 '22 08:12 github-actions[bot]

I have read the CLA Document and I hereby sign the CLA

adarsh-sgh avatar Dec 28 '22 08:12 adarsh-sgh

~~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

adarsh-sgh avatar Dec 28 '22 10:12 adarsh-sgh

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?

laurent22 avatar Dec 28 '22 13:12 laurent22

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 image image

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.

adarsh-sgh avatar Dec 28 '22 16:12 adarsh-sgh

So what would be the solution then?

laurent22 avatar Dec 28 '22 17:12 laurent22

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$.

The viewport is the dimension in user space

it's viewBox actually 🥲.

adarsh-sgh avatar Dec 28 '22 17:12 adarsh-sgh

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, and img.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

adarsh-sgh avatar Dec 28 '22 17:12 adarsh-sgh

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 ?

adarsh-sgh avatar Dec 29 '22 10:12 adarsh-sgh

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(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?

laurent22 avatar Dec 29 '22 11:12 laurent22

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

adarsh-sgh avatar Dec 29 '22 14:12 adarsh-sgh

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)?

laurent22 avatar Dec 30 '22 23:12 laurent22

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

adarsh-sgh avatar Dec 31 '22 11:12 adarsh-sgh

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.

adarsh-sgh avatar Dec 31 '22 11:12 adarsh-sgh

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.

laurent22 avatar Dec 31 '22 19:12 laurent22

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

small-fixed mermaid with a single block (39 * 55)


mermaid-1672247198591 bigger mermaid diagram (389*303)

adarsh-sgh avatar Jan 01 '23 17:01 adarsh-sgh

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?

laurent22 avatar Jan 01 '23 17:01 laurent22

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.

setupGraphViewbox fxn of mermaid

  1. 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.

adarsh-sgh avatar Jan 02 '23 19:01 adarsh-sgh

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?

laurent22 avatar Jan 03 '23 10:01 laurent22

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.

adarsh-sgh avatar Jan 04 '23 07:01 adarsh-sgh

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?

laurent22 avatar Jan 05 '23 17:01 laurent22

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

adarsh-sgh avatar Jan 06 '23 13:01 adarsh-sgh

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.

laurent22 avatar Jan 06 '23 16:01 laurent22

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

  1. Parse the svg using DOMParser to get id of svg
  2. getBoundingClientRect by selecting the svg from dom (like mermaid live editor does)

adarsh-sgh avatar Jan 06 '23 17:01 adarsh-sgh

  1. 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.

laurent22 avatar Jan 06 '23 17:01 laurent22

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.

adarsh-sgh avatar Jan 06 '23 18:01 adarsh-sgh

Please run yarn run linter ./ since I've updated the linter rules

laurent22 avatar Jan 11 '23 18:01 laurent22

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 ?

adarsh-sgh avatar Jan 13 '23 09:01 adarsh-sgh

Ok let's drop the testing. Thanks for implementing this

laurent22 avatar Feb 05 '23 11:02 laurent22