fix repl rsearch with one-char query #9352
Conversation
rfourquet
added
REPL
| @@ -514,12 +514,12 @@ function history_search(hist::REPLHistoryProvider, query_buffer::IOBuffer, respo | |||
| return true | |||
| end | |||
|
|
|||
| searchfunc,delta = backwards ? (rsearch,0) : (search,1) | |||
| searchfunc, searchstart = backwards ? (rsearch, b-1) : (search, a+1) | |||
There was a problem hiding this comment.
Is this Unicode safe, or should we rather use nextind and prevind?
There was a problem hiding this comment.
I honestly don't know and I can't test right now. I kind of assumed it was safe before, and I think this patch doesn't make things less safe.
There was a problem hiding this comment.
Great! Julia makes UTF-8 standard, and does it right, but makes it somewhat too easy to make bugs like this. Maybe #9297, and hiding the efficient interface behind a wrapped type has something to it anyway?
There was a problem hiding this comment.
Really I never had to think on utf8 things before today, but I already noticed that I don't like the getindex interface with Int, with invalid positions, and there are inconsistencies, like: a="éé"; a[1:1] == a[1:2]; a[3] == a[3:4]: the last indexing is a BoundsError, but not a[1:2], I'll read the thread you linked to, thanks.
There was a problem hiding this comment.
You'll get even more confused if you look at #7811, or inadvertantly finds out that SubString behaves differently. I hope someone will fix that, (or clean up after my most likely failed attempt).
rfourquet
force-pushed
the
rf/repl-rsearch
branch
from
55f45e0 to
e7bb2b9
Compare
|
This should work better now. |
|
Nice! Do you have anything in your |
|
Yes, I'll do that, but maybe the best here is to wait for #9365 to be fixed first, as I did my tests with that other PR merged locallly. |
|
Yes of course. |
|
"maybe the best here is to wait for #9365 to be fixed first", that one is closed, I'm not sure, it seemed to have been fixed with, see there "Fix #9365 rsearch/rsearchindex with UTF-8 #11533" This isn't important to me.. I just stumbled on this and see a commit and "All checks have passed" and wander if this was just forgotten, and/or if something happens automatically if a "dependency" is merged (but not if only closed..). |
|
No, there's nothing automated--thanks for surfacing this. (appears to be an 0.4 backport candidate as well) |
|
If this is backported, then the fix #11533 should probably be backported too. While writing additional tests for this PR involving UTF-8, I found the following error, which is indirectly fixed by this PR, but I'm not sure if there is an independant bug worth opening an issue, please help me decide. At REPL do: "ù ùù Ctrl-r ù Ctrl-r Ctrl-s Ctrl-s" crashes with in particular "type SearchState has no field p". |
|
#11533 was merged well before 0.4 was branched, so it's included in the 0.4.0 release. This hasn't been merged into master yet, after it does it could be potentially backported for a future 0.4.x release, but the unicode and regex code has diverged significantly between release-0.3 and release-0.4 so it is not worth the trouble to backport to release-0.3 at this point. (p.s. good to see you back again) |
|
I cannot update this branch, can someone pick up the commit at rfourquet@ab8445e and apply it here? (this adds a test). I will open an issue for the aforementioned bug (in my previous comment). (p.s. thanks Tony!) |
The backward i-search needs to set the "start" parameter of rsearch to the end (minus 1) of the current search window. This fixes e.g.: * searching backwards only one character remains stuck on the same match * searching backwards 'aaa' skips one of the matches in 'aaaa'
tkelman
force-pushed
the
rf/repl-rsearch
branch
from
e7bb2b9 to
773fb3b
Compare
|
Done. I also rebased since I think Travis would otherwise have issues on this pretty-old branch. Do double-check that it looks right now. |
|
Thanks, yes it looks right (a git diff with my locally rebased branch shows nothing). |
|
One of the travis failures is an intermittent parser thing. Will merge this if no one has any other feedback on it. |
fix repl rsearch with one-char query
The backward i-search needs to set the "start" parameter of rsearch to the end (minus 1) of the current search window. This fixes e.g.: * searching backwards only one character remains stuck on the same match * searching backwards 'aaa' skips one of the matches in 'aaaa' (cherry picked from commit b3a57c6) ref #9352
The backward i-search needs to set the "start" parameter of rsearch
to the end (minus 1) of the current search window.
This fixes e.g.: