macvim url scheme with line number is broken

[rahul342]

Describe the bug
mvim url scheme with line number is not working on the latest release (Snapshot 162)

To Reproduce
Detailed steps to reproduce the behavior:

1. open a url with line number param (e.g. mvim://open?url=file://%2FUsers%2Frahulagrawal%2FCode%2Flimebike-web%2Fspec%2Fservices%2Fbike_scan_processor_spec.rb&line=87)
2. With 162 installed, the file opens but not set to the line number
3. Downgrade to 161 and try the same url, it works correctly this time.

Expected behavior
File opens at the passed line number

Screenshots

Environment (please complete the following information):

• Latest snapshot (162)
• OS: macOS 10.15.1
• Terminal: iTerm2, browser firefox.

Additional context
Potentially related to #1002 ?

[rahul342]

cc @frarees (Sorry if this is not related to your commit)

[frarees]

It is related to #1002 definitively. So I'm not sure if RFC 2396 defines what happens when a query value is a URI itself, if it need to be re-encoded or not. Is there somewhere a statement on this specific scenario?

In your case, not re-encoding (e.g. mvim://open?url=file:///Users/rahulagrawal/Code/limebike-web/spec/services/bike_scan_processor_spec.rb&line=87) works on 8.2.319+.

This is what I do on bash to detect the version. Not bulletproof, enough for my use case:

function urlenc
{
	/usr/bin/python -c "import sys, urllib as ul; print ul.quote(sys.argv[1],'/')" "$1"
}

FILE=$1
LINE=$2

MVIMVERSION=`/usr/local/bin/mvim --version | grep 'VIM - Vi IMproved' | cut -d ' ' -f 5`
# not 100% accurate, we could have several patch defs e.g. 1-200, 205-300, 302-500
MVIMPATCH=`/usr/local/bin/mvim --version | grep 'Included patches' | cut -d '-' -f 2`

ENC_FILE=`urlenc "$FILE"`

# pre 8.2.319
#   re-encode, since MacVim mistakenly decodes it
# see https://github.com/macvim-dev/macvim/pull/1002
if [[ $MVIMVERSION < '8.2' || $MVIMVERSION == '8.2' && $MVIMPATCH < '319' ]]; then
	ENC_FILE=`urlenc "$ENC_FILE"`
fi

URI="mvim://open?url=file://$ENC_FILE&line=$LINE"

[ychin]

Yeah that seems directly responsible for the issue. I’ll take a look tonight. We should add a regression test as well (which a lot of MacVim-specific features don’t have, unfortunately).

[frarees]

@rahul342 can you check #1021 see if it works for you?

[rahul342]

@frarees Would I need to compile and build your branch to test? What's the quickest way to try your change?

[ychin]

Hmm I don’t think this needs additional testing. I tried it and it does work and it’s possible to test locally as well

[frarees]

Sorry for the late response. There are build instruction on the wiki. Snapshot 163 will contain the changes, so you could wait for it to be released too.