Skip to content
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

Decoding all URLs #87

Closed
neocotic opened this issue Mar 14, 2012 · 10 comments
Closed

Decoding all URLs #87

neocotic opened this issue Mar 14, 2012 · 10 comments
Assignees
Labels
bug
Milestone

Comments

@neocotic
Copy link
Member

@neocotic neocotic commented Mar 14, 2012

Copying the URL (for example)

http://sourceforge.net/projects/octave/files/Octave%20MacOSX%20Binary/2011-04-21%20binary%20of%20Octave%203.4.0/

as Markdown results in %20 being replaced with actual spaces rather than the percent-encodings, rendering the link broken.

Ticket: #2

After further testing it appears that even {url} replicates this. I'll look in to this ASAP.

@ghost ghost assigned neocotic Mar 14, 2012
@neocotic
Copy link
Member Author

@neocotic neocotic commented Mar 15, 2012

This appears to be an issue with the library used to dissect the URL decoding everything. One solution would be to use the original URL instead of the parsed URL for the {url} and other similar variables. However, should variables derived from the URL (e.g. path, segments, directory) be in their original or decoded form? If the latter, then I'll have to either find a replacement library that allows this or modify the existing one, which wouldn't be a problem but I just want to make sure I get it right.

@slhck
Copy link

@slhck slhck commented Mar 15, 2012

I'd say they should be in their original form, because everything else would break pasting links somewhere. Optionally, you could maybe add a checkbox to enable or disable URL decoding for every template.

@neocotic
Copy link
Member Author

@neocotic neocotic commented Mar 15, 2012

I've already identified a quick and easy fix that will make so that nothing is decoded;

jquery.url.js:32

// var str = decodeURI( url ),
   var str = url,

So for the original example the following will variables will be derived;

directory = "/projects/octave/files/Octave%20MacOSX%20Binary/2011-04-21%20binary%20of%20Octave%203.4.0/"
path = "/projects/octave/files/Octave%20MacOSX%20Binary/2011-04-21%20binary%20of%20Octave%203.4.0/"
relative = "/projects/octave/files/Octave%20MacOSX%20Binary/2011-04-21%20binary%20of%20Octave%203.4.0/"
segments = ["projects", "octave", "files", "Octave%20MacOSX%20Binary", "2011-04-21%20binary%20of%20Octave%203.4.0"]
url = "http://sourceforge.net/projects/octave/files/Octave%20MacOSX%20Binary/2011-04-21%20binary%20of%20Octave%203.4.0/"

You can still make these values prettier using the decode operation. For example;

<!-- Input -->
{#decode}{path}{/decode}
<!-- Output -->
/projects/octave/files/Octave MacOSX Binary/2011-04-21 binary of Octave 3.4.0/

However, if you only really want the {url} variable to be pretty then that is also possible by primarily using the tab URL instead of the parsed URL within the buildStandardData method (see background.coffee:707).

@slhck
Copy link

@slhck slhck commented Mar 15, 2012

Okay, perfect, that makes sense :)

Thanks for checking that out so quickly.

@neocotic
Copy link
Member Author

@neocotic neocotic commented Mar 15, 2012

@slhck I think we're in agreement as this is the behaviour I expected but didn't test. I'll make the "decode nout" change now and consider adding the option later. I would like to get this fix in today as I really see this as a key bug.

@neocotic
Copy link
Member Author

@neocotic neocotic commented Mar 15, 2012

Thanks for using the new feedback system and congrats on being the first! :)

neocotic added a commit that referenced this issue Mar 15, 2012
…ution (file name will be updated once it has been renamed)
@neocotic
Copy link
Member Author

@neocotic neocotic commented Mar 15, 2012

This is working fine for me so I'll include this in today's 1.0.6 release. I still want to rename the file (remove .min from its name) so the release will have to wait until this is done. Unfortunately, c9 isn't letting me use the git command today so I'll have to wait till after work to do this.

@neocotic
Copy link
Member Author

@neocotic neocotic commented Mar 15, 2012

It appears c9.io is working again. The file has been renamed so I'm now going to release 1.0.6.

@neocotic neocotic closed this Mar 15, 2012
@slhck
Copy link

@slhck slhck commented Mar 15, 2012

Works as expected now — can't comment on the UserVoice thing, you've probably deleted it?

Thanks for the quick fix. Love the extension.

@neocotic
Copy link
Member Author

@neocotic neocotic commented Mar 15, 2012

That's great and thanks for the feedback.

Not sure why it wouldn't let you comment on the ticket as this is the first time I've used it. The ticket is still open though and I've definitely not deleted but I'll close it off now since it's resolved.

https://template.uservoice.com/admin/tickets/#!search?id=2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.