fix for #906, 1) windows paths like C:\a\b weren't accepted because b…

[moltenform]

1. windows paths like C:\a\b weren't accepted because backslashes were treated as escape sequences,
2. common paths like C:\Program Files\Foo\Foo.exe weren't accepted because of the space in the path

[laurent22]

Thanks for the patch. Do you know what part of the code was treating "" as an escape character? Maybe, rather than escaping the path, whatever was incorrectly treating \ as escape character should be fixed?

[moltenform]

It's splitCommandString() that is both treating \ as an escape character and treating spaces as separating arguments. This is right for Linux, where paths don't usually have spaces, but isn't expected in Windows.

True, a simpler approach could be to simply skip splitCommandString() on Windows and pass the string as one argument. I doubt that Windows users would need to pass flags to a .exe. This would be a simpler ~5 line patch, along with a new string, since the documentation string says "The editor command (may include arguments) that will be used..." would have a Windows version that says "The editor command that will be used..."

If that would be preferred, I'll update the pr.

[laurent22]

It's splitCommandString() that is both treating \ as an escape character and treating spaces as separating arguments. This is right for Linux, where paths don't usually have spaces, but isn't expected in Windows.

Hmm, yes I think the bug is indeed in splitCommandString(). I've converted this function from some of my Golang code but it was used in a different context where escaping needed to be supported. However in this case we don't ever need escaping - the app should just take whatever string the user provided and run this.

So yes I think if you could fix this function by making its ability to escape option it should be fine. Like the signature would be this:

function splitCommandString(command, options =null) {
    options = {...options};
    if (!('handleEscape' in options)) options.handleEscape = true;
    /// etc.
}

For the spaces in Windows, the splitCommandString function supports quotes around paths so we don't need to do something special for this. Perhaps we could add a description below the field though - something like "If you path contains spaces, add double quotes around it".

[moltenform]

When I update joplin.pot, which script should I run to regenerate locales/*.js ? (I'm guessing something related to gettext)

[laurent22]

You don't need to regenerate the .pot file, I take care of it when I rebuild all the translations. If you want a string to be translated, simply wrap it in _().

[laurent22]

Is there any reason we still need this, since in splitCommandString you are now disabling escape?

[moltenform]

Thanks, good catch.

[laurent22]

Sorry I missed your new commits from a few days ago. I think it's almost ready to merge but I just have one question on one of the changes. If you could just confirm on this that would be great.

[moltenform]

Sounds good.

[laurent22]

Thanks @downpoured!

[moltenform]

Thanks!

By the way, I'm thinking of changing my username on here to 'moltenjs' within the next few days, GitHub seems to handle this fine, but letting you know just in case.