New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed file url issue #1054

Merged
merged 1 commit into from Jan 9, 2019

Conversation

Projects
None yet
3 participants
@tfinnberg
Copy link
Contributor

tfinnberg commented Dec 19, 2018

fix to issue #1012. File URLs should work now.

@tessus
Copy link
Collaborator

tessus left a comment

This change worked for me.

However, space is not the only character that needs to be urlencoded (as mentioned in your screenshot). e.g. ' (single quote) has to be encoded as well. If you don't, you break the app.

Also, I suggest you add the text in your screenshot to either the commit message or the PR comment section.

@@ -212,7 +212,7 @@ function toTitleCase(string) {
}

function urlDecode(string) {
return decodeURIComponent((string+'').replace(/\+/g, '%20'));

This comment has been minimized.

@laurent22

laurent22 Dec 21, 2018

Owner

What's the reason for removing this?

This comment has been minimized.

@tfinnberg

tfinnberg Dec 21, 2018

Contributor

well, I can't think of a reason to replace a '+' character with a space in string. Am I missing something?

@tessus

This comment has been minimized.

Copy link
Collaborator

tessus commented Dec 21, 2018

I'm not sure what is happening, but my comments in the review are pending. Therefore I don't know, if you have even seen my first comment:

The reason for replacing a + with %20 is so that you can write a local file with + instead of a space and it still works. e.g.: file:///tmp/file+with+space.jpg

I believe some wikis do the same. confluence maybe?

@tfinnberg

This comment has been minimized.

Copy link
Contributor

tfinnberg commented Dec 21, 2018

@tessus you are absolutely right! The ' (single quote) is a problem. So far I have not find another critical character, but I will have a closer look soon.
Thank you for explaining the reason for replacing the + with a space. But we will run into a problem if the filename actually contains a +, right?

@tessus

This comment has been minimized.

Copy link
Collaborator

tessus commented Dec 21, 2018

But we will run into a problem if the filename actually contains a +, right?

Yes and no. The + character is just a short form for %20. Theoretically URLs are always supposed to be encoded, making + just an exception.

If a file really had a + in its name, you'd have to encode it with %2B.

P.S.: The chance of having a space in a filename is much higher than having a +...

@tfinnberg

This comment has been minimized.

Copy link
Contributor

tfinnberg commented Dec 22, 2018

There are a lot of rather heated discussions revolving around the subject of using the plus sign in URLs.

The best description I could find is

For HTTP URLs, a space in a path fragment part has to be encoded to "%20" (not, absolutely not "+"), while the "+" character in the path fragment part can be left unencoded.

Now in the query part, spaces may be encoded to either "+" (for backwards compatibility: do not try to search for it in the URI standard) or "%20" while the "+" character (as a result of this ambiguity) has to be escaped to "%2B". REF

There is no query part in file-URLs, using the '+' in the path section to represent space is maybe not a good idea.

File-URLs are described in RFC8089 referring to REF3986 for the encoding part.

All non US-ASCII characters has to be percent encoded. All unreserved US-ASCII characters can and should be used unescaped: ALPHA / DIGIT / "-" / "." / "_" / "~"

There are a couple of reserved characters in RFC3986. These are ":" / "/" / "?" / "#" / "[" / "]" / "@""!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="

The purpose of reserved characters is to provide a set of delimiting characters that are distinguishable from other data within a URI.

But given the fact that we don't have/need delimiters in file-URLs these characters can be (and for the sake of readability in my opinion should be) used unescaped.

To sum it up, I am not in favor of using + as placeholder of space in the file-URL path. But I think it is up to you @tessus and @laurent22 to decide how to proceed in this matter.


As a side note:

An easy way to get a well defined file-URL reference of a file is by creating a context menu extension of your file browser. I defined mine in MAC-OS finder with the help of a script I found in https://stackoverflow.com/questions/9617029/how-to-get-the-a-file-url-in-osx-with-applescript-or-a-shell-script.

EDIT: A very interesting file URL blog entry from Microsoft can be found in https://blogs.msdn.microsoft.com/ie/2006/12/06/file-uris-in-windows/. Especially the proposed handling of non US-ASCII characters is different in Microsoft. They suggest leaving non US-ASCII characters untouched, replacing only the spaces with %20. A file-URL extension for Microsoft Explorer can be found in https://superuser.com/questions/297060/generate-file-uris-in-windows-explorer (I have not tried it yet)

Is it possible to slightly alter the drag and drop mechanism of joplin to enable the choice of either embedding the file or inserting a file-url instead?

@tessus

This comment has been minimized.

Copy link
Collaborator

tessus commented Dec 23, 2018

But I think it is up to you @tessus and @laurent22 to decide how to proceed in this matter.

Well, I think it's @laurent22's decision alone. I'm not a developer of Joplin and it's his code.

Is it possible to slightly alter the drag and drop mechanism of joplin to enable the choice of either embedding the file or inserting a file-url instead?

I like this idea, maybe by clicking ctrl while dragging the file into the ediitor pane to insert a link instead of attaching the file.

@tessus

This comment has been minimized.

Copy link
Collaborator

tessus commented Dec 24, 2018

@tfinnberg I'm not sure though, if we should discuss this in this PR. maybe we should open a separate issue for it. I can convert #1054 (comment) to a new issue, if you are ok with that.

In this case, the fix can be merged sooner and the discussion can be more public. Let me know what you think.

@tfinnberg

This comment has been minimized.

Copy link
Contributor

tfinnberg commented Dec 24, 2018

@tessus yes, moving the discussion to a new issue is a good idea. I have changed the PR to only cover the basic module export part, leaving the + to %20 swap untouched. Thanks!

@tessus

This comment has been minimized.

Copy link
Collaborator

tessus commented Dec 30, 2018

@tfinnberg I've opened a new issue #1074

Unfortunately the code has changed since you submitted the PR and there's now a conflict. Please rebase and squash, or create a new PR, if that's easier.

@tfinnberg tfinnberg force-pushed the tfinnberg:fix_file_url branch 2 times, most recently from a736c92 to 3c37870 Dec 30, 2018

@tfinnberg tfinnberg force-pushed the tfinnberg:fix_file_url branch from 3c37870 to 818ef81 Dec 30, 2018

@tfinnberg

This comment has been minimized.

Copy link
Contributor

tfinnberg commented Jan 1, 2019

@tessus ok, I have fixed the PR

@tessus

This comment has been minimized.

Copy link
Collaborator

tessus commented Jan 1, 2019

@laurent22 This PR is ready to be merged. Any objections, if I do so?

@tessus tessus added the bug label Jan 4, 2019

@laurent22 laurent22 merged commit ffda04f into laurent22:master Jan 9, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@laurent22

This comment has been minimized.

Copy link
Owner

laurent22 commented Jan 9, 2019

Looks good, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment