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

Fix path completion when UTF8 char is occurring before match characters. #9227

Merged
merged 1 commit into from
Dec 4, 2014

Conversation

dhoegh
Copy link
Contributor

@dhoegh dhoegh commented Dec 2, 2014

Small fix to #8838 so completion works again on: "β $dir_space\\space", @tkelman, @ivarne

@ivarne ivarne added REPL Julia's REPL (Read Eval Print Loop) regression Regression in behavior compared to a previous version labels Dec 2, 2014
@ivarne
Copy link
Member

ivarne commented Dec 2, 2014

Looks good to me.

@@ -188,7 +188,7 @@ function completions(string, pos)
inc_tag = Base.incomplete_tag(parse(partial , raise=false))
if inc_tag in [:cmd, :string]
m = match(r"[\t\n\r\"'`@\$><=;|&\{]| (?!\\)", reverse(partial))
startpos = length(partial) - (m == nothing ? 1 : m.offset) + 2
startpos = nextind(partial, nextind(partial, length(partial) - m.offset))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you have to handle the m == nothing case?

See also my comment. It's not clear to me why the original code is incorrect, since m.offset is the offset of an ASCII character (a single code unit).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not handle nothing because the regex should be garented a match due to the ´if´ at line 189.
About the ´nextind´

julia>s="α C:/"
julia>start = match(r" ",reverse(s)).offset
julia>s[length(s)-start]
'α'

So I need to step past the UTF-xx char. It could be changed to:

nextind(partial, length(partial) - m.offset) + 1

because I know the match char is ASCII.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't s[endof(s) - start + 1] work fine? Hmm, no, that's not right either because endof gives the last character index, not the last byte index. Seems like it should be s[nextind(s,endof(s)) - start]

The real problem here seems to be the use of length instead of endof, which is wrong because offset is a byte index not a character index. e.g. your patch gives the wrong result 'β' for s="αβ C:/αβ"; start = match(r" ",reverse(s)).offset; s[nextind(s, nextind(s, length(s) - start))]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like searching strings in reverse is common enough and so error-prone that we should export a reverseind(s,i) = nextind(s,endof(s)) - i function from Base, and document it along with reverse. Although the implementation would be more complicated if i is not the index of an ASCII character.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevengj thank you for the pointer to endof, it was wrong to use length. But it still seems like it should be start = nextind(s, endof(s) - match(r" ",reverse(s)).offset) + 1 to get from C.

julia>s="αβ C:/αβ"
julia>start = nextind(s, endof(s) - match(r" ",reverse(s)).offset) + 1
julia>s[start:end]
"C:/αβ"

I have updated my comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextind(s,endof(s)) - offset gives the index of the matched (ASCII) character in s. If you want the index of the character after that, you can use nextind(s,endof(s)) - offset + 1 because the matched character is ASCII.

Your code does not work if the character before the matched character is ASCII:

julia> s="αβd C:/αβ";
julia> start = nextind(s, endof(s) - match(r" ",reverse(s)).offset) + 1;
julia> s[start:end]
" C:/αβ"

(Notice that the matched character is included in this case, whereas it was not included for s="αβ C:/αβ".)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevengj I have found the solution, it converts to charecter index and the back:

julia> s="αβ C:/αβ";
julia> start = chr2ind(s, length(s) - ind2chr(reverse(s), match(r" ",reverse(s)).offset) + 2);
julia> s[start:end]
"C:/αβ"

I have updated.
I fogot to reverse the string

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My solution is faster: ind2chr and chr2ind are O(n) operations. Why don't you want to use nextind(s,endof(s)) - offset + 1?

@dhoegh dhoegh force-pushed the fix_path_completion_unicode branch from 864ef0d to 9b3a83b Compare December 4, 2014 13:51
@dhoegh
Copy link
Contributor Author

dhoegh commented Dec 4, 2014

@stevengj I have updated it with you proposal, and added a UTF-xx string to the path. Sorry I did not realized it worked you solution worked, I was pretty busted when I looked at it yesterday.

@stevengj
Copy link
Member

stevengj commented Dec 4, 2014

LGTM, thanks!

stevengj added a commit that referenced this pull request Dec 4, 2014
Fix path completion when UTF8 char is occurring before match characters.
@stevengj stevengj merged commit 87e9ee1 into JuliaLang:master Dec 4, 2014
@dhoegh
Copy link
Contributor Author

dhoegh commented Dec 4, 2014

It should be backported @JuliaBackports.

@ivarne
Copy link
Member

ivarne commented Dec 6, 2014

Backported in 967b37a

@dhoegh dhoegh deleted the fix_path_completion_unicode branch January 13, 2015 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression in behavior compared to a previous version REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants