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

Flexible AcceptLine, fix bug in multi-line accept #379

Merged
merged 3 commits into from
Oct 11, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions src/keymap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ pub enum Cmd {
/// abort
Abort, // Miscellaneous Command
/// accept-line
///
/// See also AcceptOrInsertLine
AcceptLine,
/// beginning-of-history
BeginningOfHistory,
Expand Down Expand Up @@ -99,9 +101,23 @@ pub enum Cmd {
/// moves cursor to the line below or switches to next history entry if
/// the cursor is already on the last line
LineDownOrNextHistory(RepeatCount),
/// accepts the line when cursor is at the end of the text (non including
/// trailing whitespace), inserts newline character otherwise
AcceptOrInsertLine,
/// Inserts a newline
Newline,
/// Either accepts or inserts a newline
///
/// Always inserts newline if input is non-valid. Can also insert newline
/// if cursor is in the middle of the text
///
/// If you support multi-line input:
/// * Use `accept_in_the_middle: true` for mostly single-line cases,
/// for example command-line.
/// * Use `accept_in_the_middle: false` for mostly multi-line cases,
/// for example SQL or JSON input.
AcceptOrInsertLine {
/// Whether this commands accepts input if the cursor not at the end
/// of the current input
accept_in_the_middle: bool
},
}

impl Cmd {
Expand Down Expand Up @@ -913,7 +929,9 @@ impl InputState {
}
}
KeyPress::Ctrl('J') |
KeyPress::Enter => Cmd::AcceptLine,
KeyPress::Enter => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

AcceptLine == AcceptOrInsertLine { accept_in_the_middle: true } ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, accept whatever is fully formed command or insert a newline otherwies. That's for backwards compatibility. As described in the docstring above this is what makes most sense for mostly-single-line cases (i.e. a command-line like in nushell).

Cmd::AcceptOrInsertLine { accept_in_the_middle: true }
}
KeyPress::Down => Cmd::LineDownOrNextHistory(1),
KeyPress::Up => Cmd::LineUpOrPreviousHistory(1),
KeyPress::Ctrl('R') => Cmd::ReverseSearchHistory,
Expand Down
45 changes: 35 additions & 10 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,15 +281,16 @@ fn page_completions<C: Candidate, H: Helper>(
&& cmd != Cmd::SelfInsert(1, ' ')
&& cmd != Cmd::Kill(Movement::BackwardChar(1))
&& cmd != Cmd::AcceptLine
&& cmd != Cmd::AcceptOrInsertLine
&& cmd != Cmd::Newline
&& matches!(cmd, Cmd::AcceptOrInsertLine { .. })
Copy link
Collaborator

Choose a reason for hiding this comment

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

&& !matches!(cmd, Cmd::AcceptOrInsertLine { .. }) (not matches)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

{
cmd = s.next_cmd(input_state, rdr, false)?;
}
match cmd {
Cmd::SelfInsert(1, 'y') | Cmd::SelfInsert(1, 'Y') | Cmd::SelfInsert(1, ' ') => {
pause_row += s.out.get_rows() - 1;
}
Cmd::AcceptLine | Cmd::AcceptOrInsertLine => {
Cmd::AcceptLine | Cmd::Newline | Cmd::AcceptOrInsertLine { .. } => {
pause_row += 1;
}
_ => break,
Expand Down Expand Up @@ -427,6 +428,7 @@ fn readline_edit<H: Helper>(
editor.reset_kill_ring(); // TODO recreate a new kill ring vs Arc<Mutex<KillRing>>
let ctx = Context::new(&editor.history);
let mut s = State::new(&mut stdout, prompt, helper, ctx);

let mut input_state = InputState::new(&editor.config, Arc::clone(&editor.custom_bindings));

s.line.set_delete_listener(editor.kill_ring.clone());
Expand Down Expand Up @@ -509,8 +511,12 @@ fn readline_edit<H: Helper>(
s.edit_overwrite_char(c)?;
}
Cmd::EndOfFile => {
if s.has_hint() || !s.is_default_prompt() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only if line is empty or in vi mode ?
That is when the action is not edit_delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. But I'm not sure that if !input_state.is_emacs_mode() && !s.line.is_empty() makes sense. Ctrl+D should be "scroll downwards" in vim, not "submit".

// Force a refresh without hints to leave the previous
// line as the user typed it after a newline.
s.refresh_line_with_msg(None)?;
}
if !input_state.is_emacs_mode() && !s.line.is_empty() {
s.edit_move_end()?;
break;
} else if s.line.is_empty() {
return Err(error::ReadlineError::Eof);
Expand Down Expand Up @@ -575,7 +581,7 @@ fn readline_edit<H: Helper>(
kill_ring.kill(&text, Mode::Append)
}
}
Cmd::AcceptLine | Cmd::AcceptOrInsertLine => {
Cmd::AcceptLine | Cmd::AcceptOrInsertLine { .. } => {
#[cfg(test)]
{
editor.term.cursor = s.layout.cursor.col;
Expand All @@ -585,13 +591,23 @@ fn readline_edit<H: Helper>(
// line as the user typed it after a newline.
s.refresh_line_with_msg(None)?;
}
// Only accept value if cursor is at the end of the buffer
if s.validate()? && (cmd == Cmd::AcceptLine || s.line.is_end_of_input()) {
break;
} else {
s.edit_insert('\n', 1)?;
let valid = s.validate()?;
let end = s.line.is_end_of_input();
match (cmd, valid, end) {
| (Cmd::AcceptLine, _, _)
| (Cmd::AcceptOrInsertLine { .. }, true, true)
| (Cmd::AcceptOrInsertLine { accept_in_the_middle: true }, true, _)
=> {
break;
}
| (Cmd::Newline, _, _)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unreachable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

(this is actually why I'd like the top-level match to be exhaustive, as this error would have been spotted by the compiler)

| (Cmd::AcceptOrInsertLine { .. }, false, _)
| (Cmd::AcceptOrInsertLine { .. }, true, false)
=> {
s.edit_insert('\n', 1)?;
}
_ => unreachable!(),
}
continue;
}
Cmd::BeginningOfHistory => {
// move to first entry in history
Expand Down Expand Up @@ -656,6 +672,10 @@ fn readline_edit<H: Helper>(
}
}
Cmd::Interrupt => {
// Move to end, in case cursor was in the middle of the
// line, so that next thing application prints goes after
// the input
s.edit_move_end()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

edit_move_buffer_end ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's better. Thanks!

return Err(error::ReadlineError::Interrupted);
}
#[cfg(unix)]
Expand All @@ -671,6 +691,11 @@ fn readline_edit<H: Helper>(
}
}
}

// Move to end, in case cursor was in the middle of the line, so that
// next thing application prints goes after the input
s.edit_move_buffer_end()?;

if cfg!(windows) {
let _ = original_mode; // silent warning
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ fn complete_line() {
let keys = vec![KeyPress::Enter];
let mut rdr: IntoIter<KeyPress> = keys.into_iter();
let cmd = super::complete_line(&mut rdr, &mut s, &mut input_state, &Config::default()).unwrap();
assert_eq!(Some(Cmd::AcceptLine), cmd);
assert_eq!(Some(Cmd::AcceptOrInsertLine { accept_in_the_middle: true }), cmd);
assert_eq!("rust", s.line.as_str());
assert_eq!(4, s.line.pos());
}
Expand Down