Add encodeFilePath to FileUtils to convert file paths into encoded strings (replace metacharacters such as %, @, $, #, etc.)

[Mark-Simulacrum]

Cannot use encodeURI/encodeURIComponent because they replace # with %25, which is incorrect - should be replaced by %23.

I'm not sure if there is a Brackets' utility for this already (I searched for it but couldn't find it).

I've tested with test#tt.jpg and test%tt.jpg, but nothing else - are there any other special characters that should be added to this replace scheme?

Should this be moved to a module in utils/ and loaded from ImageViewer?

Fixes #9117.

[Mark-Simulacrum]

It's possible that this will also be fixed by the Project Manager Revamp (#9015)

[redmunds]

@Mark-Simulacrum Yes, this should be in a reusable function in file/FileUtils.js. encodeURI/encodeURIComponent assume that # is used for a page anchor and is not part of the file name.

[marcelgerber]

Well, in Chrome 37, encodeURIComponent("#"); actually returns %23 as expected...

[redmunds]

In #9117, # appears in filename so encodeURI() should be used (not encodeURIComponent()).

[marcelgerber]

Oops, didn't think about that. So those are the chars encodeURI() doesn't encode: #$&+,/:;=?@
We should test all of them.

[Mark-Simulacrum]

Ready for another review.

[peterflynn]

@Mark-Simulacrum Can you add some unit tests to this?

[Mark-Simulacrum]

Sure - they should go in FileUtils-test.js, right?

[peterflynn]

Yep!

[Mark-Simulacrum]

Should we be replacing / with %2F? Since this is not a valid symbol in a file name (directory separator), perhaps this should not be replaced?

[Mark-Simulacrum]

Is it standard Brackets policy to squash commits? I currently have 6 in this branch, should I squash them (not now, but when final review is completed)?

[ingorichter]

We don't require them to be squashed, but you can do it, if you like. I'm fine with not having them squashed.

[Mark-Simulacrum]

I don't like squashing commits myself - since it effectively erases history.

[peterflynn]

@Mark-Simulacrum Remind me -- why do we need a custom utility for this instead of just using the built-in encodeURI()/encodeURIComponent()? Your note at top mentions "#" being handled wrong, but if I do encodeURIComponent("#") in our dev tools console today, I get "%23" which seems correct.

[marcelgerber]

@peterflynn We don't want to replace /.

[Mark-Simulacrum]

@marcelgerber @peterflynn Perhaps the correct fix for that would be to use encodeURIComponent/encodeURI, and then replace %2F with what it represents, /. Maybe this causes other issues, though.

[RaymondLim]

@Mark-Simulacrum I think what you need is only encodeURIComponent, but you must first split the path into sub path array before calling encodeURIComponent.

var pathArray = path.split("/");
    pathArray.forEach(function (subPath, index, encodedPath) {
        encodedPath[index] = encodeURIComponent(subPath);
    });
    encodedPath.join("/");

[Mark-Simulacrum]

@RaymondLim Okay. I've implemented the suggested change in 592febc, and merged with master. Ready for another review.

[marcelgerber]

You could use encodedPath = pathArray.map(...) instead.
That would actually make the line above unnecessary.

[Mark-Simulacrum]

Should I use LoDash _.map or native map?

[Mark-Simulacrum]

I think that using LoDash _.map is preferable, due to the speed increase, since this is a utility method that may be called quite often.

[Mark-Simulacrum]

@RaymondLim Implemented with LoDash.

[Mark-Simulacrum]

Committed test changes and fixed my mistake with _.map assigning to the array.

[peterflynn]

It's more canonical to use native pathArray.map(...) here. We usually only use the Lodash versions when traversing Object key-value pairs instead of Arrays.

[peterflynn]

And once that's fixed I think you'll need to remove the import above too, else JSHint will complain about the unused var

[Mark-Simulacrum]

Done.

[peterflynn]

Also: It would be good to put up a squashed version of the PR at this point, since 12 commits is a lot for a relatively small change like this.

[Mark-Simulacrum]

Didn't see the point of making a new PR -- I've rebased onto master here.

[peterflynn]

Thanks!

[Mark-Simulacrum]

Fixed merge conflict, squashed.

[RaymondLim]

LGTM. Merging.