-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
modal prefix searching in REPL #8879
Conversation
1bf0ba2
to
4d0c344
Compare
I've found some cases where the keypress passthru is not working, yet. Time to investigate. |
23db392
to
18ecb90
Compare
Travis failure is unrelated. |
18ecb90
to
2ea358d
Compare
Bump. |
2ea358d
to
6b4cb34
Compare
@vtjnash does this adequately address your concerns? |
Finally managed to try this, and it is great! Seems like there is still left some work to make it work with the |
@ivarne good catch. I'll take a look. |
@ivarne that should fix it. |
bump |
needs a rebase? |
7bb2e9b
to
1b1d6ba
Compare
@tkelman Indeed. Merge conflicts should be fixed now. |
Seems to be a very nice improvement. I really appreciate how you can get back down to your originally typed (incomplete) line. It's a little tricky with multi-line inputs, but I think this is as good as it can possibly get, and it works fairly well in practice. I've not dug into it, but I can trigger an EOF error with the sequence up, down, page-down:
|
@mbauman I cannot seem to reproduce the error. Is that with any input currently in the prompt? Also maybe relevant... what is your most recent history item? |
Nevermind... I can reproduce it. |
@mbauman that issue should now be fixed. |
The appveyor timeout on win64 (https://ci.appveyor.com/project/StefanKarpinski/julia/build/1.0.46/job/xvjollvk9d5j67cx) was most likely #7942, I triggered a rebuild of the PR |
Looks great. I can no longer trigger the error. |
188ab99
to
d2c1ae1
Compare
d527a09
to
0bfc341
Compare
@rfourquet the cursor movement thing was responding to #8577. After reading that issue, I did a few tests and thought I found that @PythonNut was correct, that moving the cursor to the end was "standard behavior". Since then I have found that several REPL's choose to do something else. For instance, ipython moves the cursor to the end when navigating history with no prefix. With a prefix, it leaves the cursor in its current location. I see similar behavior in bash. So, is that a better choice? I guess I don't know. It certainly would be straightforward to implement. In any case, I've squashed some commits. I can squash it a little further if you prefer. |
iPython leaves the cursor be in prefix history search, because it's more important to remind the user what the prefix is. I.e. the text before the cursor is your prefix, and the rest is subject to change. This seems like a sound UI choice. I was actually unaware that Julia supported prefix search at the time of #8577. If you adopt this then the cursor will stay put on prefix search, and go to the end in normal history search ( |
@PythonNut that's the idea. Not implemented yet, but perhaps it would be better. On the other hand, there seemed to be fair number of people who disabled prefix searching in their REPL keymap after it landed, precisely because it changed the cursor movement behavior. |
I think your idea of unifying prefix/non-prefix search is great; my prefered UI would be best explained with adding a conceptual (and invisible)
Is it possible that the cursor is changed (shape or color) to visually indicate that it is in prefix-search mode? Sorry if this sounds complicated. Anyway, that's just me and not a requirement; as @mbauman said, I see the current state of the PR as an improvement over previous functioning and would like to see it merged with or without these modifications. |
Try out this last commit and let me know what you think (its implementation is quite a bit simpler than what you describe). It doesn't give 3) exactly as you describe, but you can get there with up, left, right, up (in most cases, up, left, up would be just as good). |
It also doesn't do your 4), but what you describe seems somewhat unintuitive to me. We have separate keybindings that always do a normal history search: |
Yes for 3) I first proposed the exact thing you said as an alternative, and then deleted this part to not overload too much the text. My thinking with returning in normal-mode with "End" is that by default "up" from empty line starts in normal-mode, so there need a way to get back from prefix-mode to normal-mode again usable with "up"/"down", but "Home" key does exactly that, so that's now perfect for me! |
I just played with it, and I agree, I think this latest change reduces the cognitive load when moving around. I like it. I also tried as hard as I could to break it, to no avail. Merge it! |
I tried to dive in the code but am too unfamiliar with it to really review... but if no-one opposes, I won't resist the merge button by tomorrow. |
Too late @rfourquet 😄 |
I haven't been paying attention here, but just yesterday I was noticing some issues with history. Looking forward to experiencing this---thanks @blakejohnson! |
😄 |
I like this a lot better than what we had before. The one thing I just now noticed that I don't like is the following:
I would have expected the empty line to cancel prefix searching. |
I think this appeared with this PR: "up" then "Ctrl-Left" (or right, or...) prints "5D" first, and if this is repeated, goes one word left as expected. Adding the entry |
This change has broken the old
|
Sorry, disregard the previous message, I misread the docs, they don't actually mention
? |
Unfortunately, the new prefix method |
I suspected as much, thanks for looking into this. For the time being, I solved my problem by grabbing the old version of |
From the discussions in #6377, #8577, and #8468, it is clear that we need a better solution for the interaction between prefix history searching and multi-line history entries. This PR implements one such solution which is a modal prefix searching behavior. The idea is that pressing up/down arrow will start a prefix search, and then will continue to do prefix searching on subsequent presses until some key other than up/down is pressed. In this way, history searching is not "interrupted" by multi-line history entries. It also allows us to put the cursor at the end of the line, like the old history traversal behavior (i.e. this fixes #8577).
Reasons that this is WIP:
1) I seem to have re-broken down-arrow behavior in terms of continuing from a previously accepted history item. i.e. #8468 is back with this PR.2) I need to figure out how to 'forward' a keypress to the parent REPL mode. At present, except for the arrow keys, enter/return, and ctrl+C, all other keys simply exit prefix searching mode without performing any other action. I am exploring alternatives to simply pulling in all the key bindings from the default keymap.3) I need to rebase into a smaller number of commits.