Mermaid-Chart / vscode-mermaid-preview

Previews Mermaid diagrams
MIT License
142 stars 15 forks source link

Black rendering on Windows OS #30

Closed FrodgE closed 7 years ago

FrodgE commented 7 years ago

Hi, Thanks for this great extension !

I've noticed a rendering problem when using Windows OS. The entire graph is black, see image below: image

I thought this may be related to the other existing open issues but by chance I tried using OSX and it worked correctly: image

Linux also works as expected: image

I had a quick look and it seems that the extension path ${this.context.extensionPath} in content-provider.js on line 22 and line 31 is not being escaped correctly. If I manually edit the extension path string the graph will render as expected. I was going to submit a pull request but I didn't know the best way to fix the path.

Let me know if you need more information.

Thanks ! Michael

vstirbu commented 7 years ago

Thanks for opening the issue! I do not have access to a Windows machine so it is hard to test how it works there... :)

I've created a branch with a possible solution: https://github.com/vstirbu/vscode-mermaid-preview/tree/issues/30. Can you test it and let me know if there are any issues. Thanks!

FrodgE commented 7 years ago

Thanks for the quick reply ;)

Ok I've tested the changes but unfortunately the path is still not being escaped properly. As a debug I've added document.write('${base}');

The actual path should read D:\Programming\open source\vscode-mermaid-preview\node_modules\mermaid\dist

image

If I hard code the path as forward slashes it works fine: image

Interestingly if I try to manually escape the backslash "\\" it still does not work (you can see the commented const base in the editor in the background of the image). It has the same result as the first image.

FrodgE commented 7 years ago

It seems I spoke to soon, if I use 4 backslashes it works (again see the editor in the background of the image) image

vstirbu commented 7 years ago

Can you try with the new commit? vscode provide a method to resolve these relative paths as absolute. Maybe it works cross platform...

FrodgE commented 7 years ago

The asAbsolutePath does seem to work if you look at the console log output in the background but it doesn't fix the interpretation of the javascript literal. image

I found a similar question on stack overflow so I tried doing a similar replace on the string. image

This works!! But using const base = this.context.asAbsolutePath('node_modules/mermaid/dist/mermaid').replace(/\\/g,"\\\\"); feels like a bit of a hack !!

vstirbu commented 7 years ago

Yes, it feels hackish.

Can you try to convert the windows filepath returned by asAbsolutePath to a file url using file-url? This might be handled better by the webview that rendered the preview.

FrodgE commented 7 years ago

Hey this is looking better, nice find 👍 image

vstirbu commented 7 years ago

Can you test it on mac and linux and make a PR into this branch. I'll merge later and publish an update.

FrodgE commented 7 years ago

The OS test (virtual machines) looks good! For the point of the test I've left the path debug messages in. PR coming soon...

OSX image

Linux image

vstirbu commented 7 years ago

published as 0.4.4