technomancy / swank-clojure

Swank/slime support for clojure
Eclipse Public License 1.0
412 stars 83 forks source link

Fix an issue where a space in the source filename prevented CDT opening the correct file when viewing source. #58

Closed GrumpyLittleTed closed 12 years ago

GrumpyLittleTed commented 13 years ago

Fix an issue where a space in the source filename prevented CDT opening the correct file when viewing source.

Tested only on OS X.

purcell commented 13 years ago

Judging by the discussion to which you linked, it looks like the canonical solution to this would instead be:

(java.net.URLDecoder/decode (.getFile uri))

-Steve

GrumpyLittleTed commented 13 years ago

I’m afraid that that code wouldn’t work: the function slime-file-resource is being passed a filename encoded using octet encoding, so spaces in the filepath and filename are being passed as %20 octets. The (poorly named) java.net.URLDecoder/decode is intended for decoding URLs which are in the application/x-www-form-urlencoded MIME format, such as those in HTML forms. A filename encoded this way would have a + substituted for the space character, rather than a %20 URL octet style encoding. So if we used your suggested code any filenames containing + would get mis-decoded as having spaces where there should be a +.

The unary constructor version of the decode method you used is deprecated in favour of the dyadic version which requires getting involved with more encoding which has plenty of scope for going horribly wrong. My fix of creating a new URI and then using getPath looks very hacky but avoids getting involved in any extra explicit decoding/encoding. I believe all getPath actually does is a simple octet to UTF-8 mapping.

Clearly encoding/decoding is a minefield so I'd appreciate any feedback on a prettier way to do this. Also note that I haven't touched any other getFile calls outside the slime-file-resource function, so there may be other places that the decoding should happen where it currently isn't happening.

purcell commented 13 years ago

Thanks for the detailed feedback! That makes sense; my reading of the java bug thread was obviously incomplete.