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

adarsh-sgh · 2022-12-28T08:34:01Z

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

github-actions · 2022-12-28T08:34:17Z

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

adarsh-sgh · 2022-12-28T08:38:24Z

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

adarsh-sgh · 2022-12-28T10:14:43Z

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

personalizedrefrigerator

Thank you for working on this! I've left a few comments.

personalizedrefrigerator · 2022-12-28T11:02:10Z

packages/app-desktop/gui/NoteEditor/utils/contextMenuUtils.test.ts

+
+describe('contextMenuUtils',()=>{
+ test('should return the correct dimensions for svg with space seperated viewBox values',()=>{
+ const svg = '<svg viewBox="0 0 45.9375 60" "></svg> ';


Suggested change

const svg = '<svg viewBox="0 0 45.9375 60" "></svg> ';

const svg = '<svg viewBox="0 0 45.9375 60"></svg> ';

Consider removing the extra ".

personalizedrefrigerator · 2022-12-28T11:08:35Z

packages/app-desktop/gui/NoteEditor/utils/contextMenuUtils.test.ts

+ expect(width).toBe(undefined);
+ expect(height).toBe(undefined);
+ });
+


Please also consider the following cases:

viewBox has the form viewBox='1,2,3,4' or viewBox = "1,2,3,4" instead of viewBox="1,2,3,4"

viewBox has the form

viewBox=' 1, 2, 3 , 4 , '

personalizedrefrigerator · 2022-12-28T11:18:51Z

packages/app-desktop/gui/NoteEditor/utils/contextMenu.ts

@@ -106,8 +106,12 @@ export function menuItems(dispatch: Function): ContextMenuItems {
 if (!options.filename) {
 throw new Error('Filename is needed to save as png');
 }
+ const [width,height] = svgDimensions(options.textToCopy);
+ if (!Number.isInteger(width) || !Number.isInteger(height)) {
+ bridge().showErrorMessageBox(_('failed to get width and height of image'));


I think, with this change, <svg xmlns="http:https://www.w3.org/2000/svg"><rect x='5' y='5' width='50' height='50' fill='red' stroke='black' style='stroke-width: 5;'/></svg> would show an error message when it wouldn't before, because of the missing viewBox attribute.

laurent22 · 2022-12-28T13:43:14Z

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?

adarsh-sgh · 2022-12-28T16:54:16Z

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.

laurent22 · 2022-12-28T17:08:53Z

So what would be the solution then?

personalizedrefrigerator · 2022-12-28T17:39:05Z

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

adarsh-sgh · 2022-12-28T17:45:30Z

The viewport is the dimension in user space

it's viewBox actually 🥲.

adarsh-sgh · 2022-12-28T17:51:47Z

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 · 2022-12-29T10:38:14Z

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 ?

laurent22 · 2022-12-29T11:33:09Z

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?

adarsh-sgh · 2022-12-29T14:57:34Z

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

laurent22 · 2022-12-30T23:23:35Z

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

adarsh-sgh · 2022-12-31T11:55:26Z

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 · 2022-12-31T11:57:10Z

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.

laurent22 · 2022-12-31T19:00:15Z

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.

adarsh-sgh · 2023-01-01T17:35:29Z

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)

laurent22 · 2023-01-01T17:54:40Z

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?

adarsh-sgh · 2023-01-02T19:15:08Z

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

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.

laurent22 · 2023-01-03T10:26:20Z

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?

adarsh-sgh · 2023-01-04T07:03:55Z

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.

laurent22 · 2023-01-05T17:53:17Z

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?

adarsh-sgh · 2023-01-06T13:48:10Z

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 #7546 (comment)

laurent22 · 2023-01-06T16:52:52Z

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.

adarsh-sgh · 2023-01-06T17:09:57Z

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)

laurent22 · 2023-01-06T17:18:19Z

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.

adarsh-sgh · 2023-01-06T18:30:14Z

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.

laurent22 · 2023-01-11T18:26:36Z

packages/app-desktop/gui/NoteEditor/utils/contextMenuUtils.ts

+ const id = parser.parseFromString(svg, 'text/html').querySelector('svg').id;
+ ({ width , height } = document.querySelector<HTMLIFrameElement>('.noteTextViewer').contentWindow.document.querySelector(`#${id}`).getBoundingClientRect());
+ } catch {
+ // do nothing


We don't hide errors, so at least logger.warn(...). Please search how loggers are created

laurent22 · 2023-01-11T18:29:01Z

packages/app-desktop/gui/NoteEditor/utils/contextMenu.ts

@@ -106,8 +106,10 @@ export function menuItems(dispatch: Function): ContextMenuItems {
 if (!options.filename) {
 throw new Error('Filename is needed to save as png');
 }
+ // double dimensions to make sure it's always big enough even on hdpi screens
+ const [width,height] = svgDimensions(document,options.textToCopy).map((x: number) => x * 2);


The dimensions can be undefined, and you'll end up with NaN as width and height.

Also when it's undefined, what dimensions do we end up using?

when it's undefined, what dimensions do we end up using?

The dimensions that used to be before this fix.
anything better we can choose as default ? 🤔

How about the NaN dimensions, what do you plan to do about it?

The dimensions that used to be before this fix.

Which are?

How about the NaN dimensions, what do you plan to do about it?

assigned dimensions as undefined if it's NaN

Which are?

almost of zero use, but since we have no better fallback we have to use them if we can't find any sensible default value.
Dimensions were being calculated as follows,:
we are making an 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.
different sized mermaids i exported to check size were having height 150px each. and different heights all less than 300.

Ok thanks for clarifying. The default dimensions indeed aren't useful so let's make them 1024. Is it possible at least to respect the diagram aspect ratio? Otherwise make it 1024x1024

let's make them 1024

when we have the aspect ratio, should the bigger/smaller dimension be 1024 or make width = 1024 ?

The max dimension would be 1024. For example 600x1024, or 1024x800

laurent22 · 2023-01-11T18:57:16Z

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

adarsh-sgh · 2023-01-13T09:01:14Z

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 ?

laurent22 · 2023-02-05T11:39:46Z

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

desktop: fix 'save as png' image dimensions

23692b2

github-actions bot added a commit that referenced this pull request Dec 28, 2022

@adarsh-sgh has signed the CLA from Pull Request #7546

caaac5c

adarsh-sgh marked this pull request as draft December 28, 2022 09:48

personalizedrefrigerator reviewed Dec 28, 2022

View reviewed changes

adarsh-sgh marked this pull request as ready for review December 28, 2022 11:25

Merge branch 'dev' into svg-png-fix

4d33d5a

replace regex parsing of svg

22d7bbc

laurent22 reviewed Jan 11, 2023

View reviewed changes

Merge branch 'dev' into svg-png-fix

bd1d8ad

add logger & handle NaN

ec92269

fix: set sensible default dimensions

c8d6705

tessus added the desktop All desktop platforms label Jan 22, 2023

Merge branch 'dev' into svg-png-fix

bfdfdb6

laurent22 merged commit 8aad67c into laurent22:dev Feb 5, 2023

github-actions bot locked and limited conversation to collaborators Feb 5, 2023

adarsh-sgh deleted the svg-png-fix branch February 8, 2023 06:24

	const svg = '<svg viewBox="0 0 45.9375 60" "></svg> ';
	const svg = '<svg viewBox="0 0 45.9375 60"></svg> ';

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

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

Conversation

adarsh-sgh commented Dec 28, 2022 • edited Loading

Before

After

github-actions bot commented Dec 28, 2022 • edited Loading

adarsh-sgh commented Dec 28, 2022

adarsh-sgh commented Dec 28, 2022 • edited Loading

personalizedrefrigerator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Dec 28, 2022

adarsh-sgh commented Dec 28, 2022 • edited Loading

laurent22 commented Dec 28, 2022

personalizedrefrigerator commented Dec 28, 2022

adarsh-sgh commented Dec 28, 2022

adarsh-sgh commented Dec 28, 2022 • edited Loading

adarsh-sgh commented Dec 29, 2022

laurent22 commented Dec 29, 2022

adarsh-sgh commented Dec 29, 2022

laurent22 commented Dec 30, 2022

adarsh-sgh commented Dec 31, 2022

adarsh-sgh commented Dec 31, 2022

laurent22 commented Dec 31, 2022

adarsh-sgh commented Jan 1, 2023

laurent22 commented Jan 1, 2023

adarsh-sgh commented Jan 2, 2023 • edited Loading

laurent22 commented Jan 3, 2023

adarsh-sgh commented Jan 4, 2023 • edited Loading

laurent22 commented Jan 5, 2023

adarsh-sgh commented Jan 6, 2023

laurent22 commented Jan 6, 2023

adarsh-sgh commented Jan 6, 2023

laurent22 commented Jan 6, 2023

adarsh-sgh commented Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Jan 11, 2023

adarsh-sgh commented Jan 13, 2023

laurent22 commented Feb 5, 2023

adarsh-sgh commented Dec 28, 2022 •

edited

Loading

github-actions bot commented Dec 28, 2022 •

edited

Loading

adarsh-sgh commented Dec 28, 2022 •

edited

Loading

adarsh-sgh commented Dec 28, 2022 •

edited

Loading

adarsh-sgh commented Dec 28, 2022 •

edited

Loading

adarsh-sgh commented Jan 2, 2023 •

edited

Loading

adarsh-sgh commented Jan 4, 2023 •

edited

Loading

adarsh-sgh commented Jan 6, 2023 •

edited

Loading