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

Conversation

tailhook
Copy link
Contributor

@tailhook tailhook commented May 8, 2020

  1. This commit allows keybinding for forcing accept line (even if
    validator fails). This is useful if you have a separate keybinding
    for accepting, such as Alt-Enter. It makes sense to show an error
    to user rather than insert an unexpected newline.
  2. Forcing Newline is also accepted
  3. AcceptLine meaning is changed (now it forces accept),
    but default Enter keybinding is not
  4. AcceptOrInsertLine now has a flag that modifies it's behavior in
    the middle of the line (at the end it always submits, when validator
    returned false it's always a newline)
  5. Fixed bug with exiting readline (both on success and error) when
    cursor is not on the last line

I'm not sure that current command-names are the best possible, so let me know if we can do better. The primary motivator was (1) from above.

This conflicts with #342. I'll rebase other one when either of them is merged.

src/lib.rs Outdated
// 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()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If line is empty, there is no need to move the cursor, no?

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. But we need to clear the hint. Fixed.

* This commit allows keybinding for forcing accept line (even if
  validator fails). This is useful if you have a separate keybinding
  for accepting, such as `Alt-Enter`. It makes sense to show an error
  to user rather than insert an unexpected newline.
* Forcing `Newline` is also accepted
* `AcceptLine` meaning is changed (not it forces accept),
   but default `Enter` keybinding is not
* `AcceptOrInsertLine` not has a flag that modifies it's behavior in
  the middle of the line (at the end it always submits, when validator
  returned false it's always a newline)
* Fixed bug with exiting readline (both on success and error) when
  cursor is not on the last line
Copy link
Collaborator

@gwenn gwenn left a comment

Choose a reason for hiding this comment

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

Seems good to me. I will do some tests...

src/lib.rs Outdated
@@ -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

src/lib.rs Outdated
@@ -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".

=> {
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)

src/lib.rs Outdated
// 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!

@@ -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).

@gwenn
Copy link
Collaborator

gwenn commented Oct 3, 2020

Thanks you.
I will try to review your PR as soon as possible.

}
if s.line.is_empty() {
return Err(error::ReadlineError::Eof);
} else if !input_state.is_emacs_mode() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Always true

@gwenn gwenn merged commit e9408c9 into kkawakam:master Oct 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants