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

REPL: Add ^P and ^N to modal prefix searching #23319

Merged
merged 5 commits into from
Sep 5, 2017

Conversation

dalum
Copy link
Contributor

@dalum dalum commented Aug 18, 2017

I don't know if these bindings were left out intentionally or not (there is one mention, #8879 (comment), which may suggest it), but I never use the arrow keys, so didn't know that this functionality was in the REPL already. I assume I'm not the only Emacs user with this habit, and most other people don't use ^P or ^N anyway, I guess?

@rfourquet
Copy link
Member

I think at least one set of bindings must do normal history navigation (what ^P/^N currently does); I don't mind exchanging ^P/^N with Up/Down arrows though.

@dalum
Copy link
Contributor Author

dalum commented Aug 18, 2017

FWIW, IPython does prefix searching with both up/down and ^P/^N.

@fredrikekre
Copy link
Member

Should also collapse the rows describing these hotkeys in https://github.com/JuliaLang/julia/blob/master/doc/src/manual/interacting-with-julia.md#key-bindings

@dalum
Copy link
Contributor Author

dalum commented Aug 18, 2017

I fixed some mistakes in the manual entry.

Page-up/down does non-prefix scrolling through history already, maybe meta-P, meta-N should be added to that? Then ^P, ^N can do exactly the same as the arrow keys.

@phaverty
Copy link
Contributor

For people with ESS/emacs muscle memory, meta-p and meta-n move up and down in command history with or without a prefix. Ctrl-p and Ctrl-n move up and down in the console/command buffer (above the prompt), which is useful for grabbing printed output from previous commands. I'd be very excited to have that meta-p and meta-n behavior in a julia REPL. I suppose Ctrl-p and Ctrl-n could do the same thing, or perhaps just go up and down in the history ignoring any text already typed at the prompt. I was very happy to see this PR this morning, thanks!

@rfourquet rfourquet added the stdlib:REPL Julia's REPL (Read Eval Print Loop) label Aug 23, 2017
"^P" => (s,o...)->(history_prev(s, mode(s).hist)),
"^N" => (s,o...)->(history_next(s, mode(s).hist)),
"^P" => (s,o...)->(edit_move_up(s) || history_prev(s, mode(s).hist)),
"^N" => (s,o...)->(edit_move_down(s) || history_next(s, mode(s).hist)),
Copy link
Member

Choose a reason for hiding this comment

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

Aren't those bindings overwritten anyway? or is there a way to activate them easily?

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 can't really remember now, but I think that something funky happened when they weren't there. If they don't do anything, then the similar bindings for up/down arrows should probably be removed as well.

@rfourquet
Copy link
Member

I'm not 100% conviced with having arrow keys and ^N/^P doing the same thing, but this PR seems nevertheless an improvement over the status-quo. I'm tempted to merge. Anyone against this change?

@rfourquet
Copy link
Member

cc @blakejohnson

@rfourquet
Copy link
Member

Given the expressed support here, would you mind @eveydee rebasing this (easy to fix conflict) to get this merged?

@dalum
Copy link
Contributor Author

dalum commented Sep 4, 2017

I think it should be fixed now.

@rfourquet rfourquet merged commit be83cac into JuliaLang:master Sep 5, 2017
@dalum dalum deleted the evey/replhist branch September 5, 2017 10:18
@blakejohnson
Copy link
Contributor

I'm late to the party, but LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib:REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants