Replace invalid regex for storage placeholder the export functions

[dredav]

This pull request will fix the issue #2884. The match to ("|]) doesn't make sense for me. The or match ] should match ). I also removed the group match at the replacement with DESTINATION_FOLDER. The group is not needed.

Issue fixed

Fix wrong test
#2884

Type of changes

• 🔵 Bug fix (Change that fixed an issue)
• ⚪ Breaking change (Change that can cause existing functionality to change)
• ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
• ⚪ Feature (Change that adds new functionality)
• ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

• 🔵 My code follows the project code style
• ⚪ I have written test for my code and it has been tested
• 🔵 All existing tests have been passed
• ⚪ I have attached a screenshot/video to visualize my change if possible

[ehhc]

@dredav i did the same work as you (because i didn't look if somebody else also did.. (stupid me)). Can you have a look at the tests here: #2901 and merge them into your PR? In my PR the invalid markdown syntax for the tests is replaced by a valid one...

[dredav]

@dredav i did the same work as you (because i didn't look if somebody else also did.. (stupid me)). Can you have a look at the tests here: #2901 and merge them into your PR? In my PR the invalid markdown syntax for the tests is replaced by a valid one...

@ehhc Thanks for your feedback with your pull request. I've merged the tests into this pull request. I'm not sure but I think that the markdown with the curly bracket at the image name ![image}]( is deliberate.
In your pull request I'm sure that the regex match to or ] is obsolete. I can't find an image syntax that contains an ] after the image address.

[ehhc]

hi @dredav why do you think the } in the path of attchments is intended?

[ehhc]

@Rokt33r what do you think about this? :)

[Rokt33r]

@ehhc I need more context to understand the current situation. But the current changes in this pr look reasonable to me. URL of image should be inside of ().

[dredav]

hi @dredav why do you think the } in the path of attchments is intended?

Sorry for the late response. I suppose that the } is a negative distinction to test some special chars at the image name.

[ehhc]

hi @dredav why do you think the } in the path of attchments is intended?

Sorry for the late response. I suppose that the } is a negative distinction to test some special chars at the image name.

you might be correct, but i can't remember if thats the reason why it was done - and according to the "blame-view" most of the cases are from me and have not been modified later.. :)
I think it might be an error and i copied and pasted it everywhere...

But i don't mind, if it stays in the tests :)

@ehhc I need more context to understand the current situation. But the current changes in this pr look reasonable to me. URL of image should be inside of ().

@Rokt33r:
the problem is stated here #2884 and it was caused by an invalid regex and wrong tests (my fault -.- stupid me -.-)

[incredibleweirdo]

@ehhc I was able to test this - in Windows 7 - by applying the changes to browser/main/lib/dataApi/attachmentManagement.js onto a branch for PR #3048 (I need to start from that branch because the fix there is needed for Export to work at all).
This PR fixes PDF export, but HTML export suffers from a %5C in the path instead of a slash:

Edited to add: this apparently does not affect HTML export on macOS - there, the path comes through fine with a slash. I'm not readily seeing how these should be different so I must admit to feeling a bit confused by it.

[incredibleweirdo]

@ehhc @dredav apologies if I'm making a bunch of noise with all these comments, hopefully I'm being helpful :) I'm not an experienced contributor to open source and only have limited development experience with NodeJS (none with React or Electron).
In stepping through the export of HTML, I have identified that the change of the \ to %5C on Windows is not due to this section where your changes are, but is instead a result of the markdown render to HTML.
After this fixed function is called, the path has \ as expected. Once the content of the note is passed through the markdown render though, the slash is replaced with %5C.
Hopefully this is helpful :)

[incredibleweirdo]

Because path.sep is platform specific, but HTML image source paths are not/should not be platform specific, I think this is the change that needs to be made for this fix to fully work for all cases.
The backslash is also an escape character in markdown and treated specially, so this avoids the issue of it being later replaced with %5C on markdown render to HTML.
I've tested this on Windows 7 to work with HTML, PDF, text, and markdown export, as well as printing.

Apologies that the suggestions are in separate comments, but that is the only way github would allow me to make them and have it work properly.

[incredibleweirdo]

Suggested change

	.replace(new RegExp(mdurl.encode(path.win32.sep), 'g'), path.sep)
	.replace(new RegExp(mdurl.encode(path.win32.sep), 'g'), path.posix.sep)

[incredibleweirdo]

Suggested change

	.replace(new RegExp(mdurl.encode(path.posix.sep), 'g'), path.sep)
	.replace(new RegExp(mdurl.encode(path.posix.sep), 'g'), path.posix.sep)

[incredibleweirdo]

Suggested change

	.replace(new RegExp(escapeStringRegexp(path.win32.sep), 'g'), path.sep)
	.replace(new RegExp(escapeStringRegexp(path.win32.sep), 'g'), path.posix.sep)

[incredibleweirdo]

Suggested change

	.replace(new RegExp(escapeStringRegexp(path.posix.sep), 'g'), path.sep)
	.replace(new RegExp(escapeStringRegexp(path.posix.sep), 'g'), path.posix.sep)

[incredibleweirdo]

Suggested change

	return storagePath.replace(new RegExp(STORAGE_FOLDER_PLACEHOLDER + escapeStringRegexp(path.sep) + noteKey, 'g'), DESTINATION_FOLDER)
	return storagePath.replace(new RegExp(STORAGE_FOLDER_PLACEHOLDER + escapeStringRegexp(path.posix.sep) + noteKey, 'g'), DESTINATION_FOLDER)

[Rokt33r]

@dredav @ehhc Sorry, I forgot to review this pr. Could you review @incredibleweirdo 's suggestion? I'll try to merge this asap.

[ehhc]

@Rokt33r i fear, i don't have the time to test the changes.. nontheless: i'm not sure if changing it to using posix sep everywhere might cause other problems.. i would have to be tested a lot..

i hope @dredav has time to do that :)

[Rokt33r]

@ehhc It's okay. I'll try it by myself too in a week.

[incredibleweirdo]

@Rokt33r i fear, i don't have the time to test the changes.. nontheless: i'm not sure if changing it to using posix sep everywhere might cause other problems.. i would have to be tested a lot..

To be sure, it's not everywhere, it's only in these 5 lines in browser/main/lib/dataApi/attachmentManagement.js which replace the :storage references when prepping the markdown for render to HTML, PDF, and such. HTML image paths are not platform specific, they must be forward slashes, is the gist of it.
The other option is to replace the links after render of the markdown, as is being done in the duplicate fix in PR #2937. I can't test that one to see if it works, though, yarn errors out on that one.

[dredav]

Sorry for my late response. This and next week I'm very busy - but I will check your input @incredibleweirdo when I find some time.

[Flexo013]

@dredav @Rokt33r What is the status on this PR?

[Rokt33r]

I'm going to fix this by myself in this week. I'll directly commit to this pr.

[Rokt33r]

Fixed by #3549