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

Add 'interact_text_opt' and 'interact_text_on_opt' for 'Input' prompts #278

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
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
52 changes: 36 additions & 16 deletions src/prompts/input.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::{
cmp::Ordering,
fmt::Debug,
io, iter,
str::FromStr,
sync::{Arc, Mutex},
Expand Down Expand Up @@ -281,20 +280,46 @@ where
impl<T> Input<'_, T>
where
T: Clone + ToString + FromStr,
<T as FromStr>::Err: Debug + ToString,
<T as FromStr>::Err: ToString,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you removing this bound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the bound was only required by the call to unwrap (in the dead code I removed in the same commit : 05f6a3b)
I figured I might as well remove it since it seems unnecessary (interact and interact_on didn't have this constraint).

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it's definitely worth fixing. I suggest making a separate PR for this and removal of the redundant check.

{
/// Enables the user to enter a printable ascii sequence and returns the result.
///
/// Its difference from [`interact`](Self::interact) is that it only allows ascii characters for string,
/// while [`interact`](Self::interact) allows virtually any character to be used e.g arrow keys.
///
/// The dialog is rendered on stderr.
/// This unlike [`interact_text_opt`](Self::interact_text_opt) does not allow to quit with 'Esc'
pub fn interact_text(self) -> Result<T> {
self.interact_text_on(&Term::stderr())
}

/// Enables the user to enter a printable ascii sequence and returns the result.
///
/// Its difference from [`interact_opt`](Self::interact_opt) is that it only allows ascii characters for string,
tbergerd marked this conversation as resolved.
Show resolved Hide resolved
/// while [`interact_opt`](Self::interact_opt) allows virtually any character to be used e.g arrow keys.
///
/// The dialog is rendered on stderr.
/// Result contains `Some(T)` if user hit 'Enter' or `None` if user cancelled with 'Esc'
#[inline]
pub fn interact_text_opt(self) -> Result<Option<T>> {
self.interact_text_on_opt(&Term::stderr())
}

/// Like [`interact_text`](Self::interact_text) but allows a specific terminal to be set.
pub fn interact_text_on(mut self, term: &Term) -> Result<T> {
#[inline]
pub fn interact_text_on(self, term: &Term) -> Result<T> {
Ok(self
._interact_text_on(term, false)?
.ok_or_else(|| io::Error::new(io::ErrorKind::Other, "Quit not allowed in this case"))?)
}

/// Like [`interact_text_opt`](Self::interact_text_opt) but allows a specific terminal to be set.
#[inline]
pub fn interact_text_on_opt(self, term: &Term) -> Result<Option<T>> {
self._interact_text_on(term, true)
}

fn _interact_text_on(mut self, term: &Term, allow_quit: bool) -> Result<Option<T>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should follow a builder pattern here, so the signature of this function shouldn't change.
Instead, add a new private field to the Input, and a corresponding setter for it.
Access it here via self.

if !term.is_term() {
return Err(io::Error::new(io::ErrorKind::NotConnected, "not a terminal").into());
}
Expand All @@ -313,11 +338,6 @@ where
},
)?;

// Read input by keystroke so that we can suppress ascii control characters
if !term.features().is_attended() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note : removed this because the check is already performed at the beginning of the function (and returns an error instead which makes more sense)

return Ok("".to_owned().parse::<T>().unwrap());
}

let mut chars: Vec<char> = Vec::new();
let mut position = 0;
#[cfg(feature = "history")]
Expand All @@ -332,6 +352,12 @@ where

loop {
match term.read_key()? {
Key::Escape if allow_quit => {
render.clear()?;
term.flush()?;
term.show_cursor()?;
return Ok(None);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's instead add a new variant for the Error like UserInterrupted and return it here.

}
Key::Backspace if position > 0 => {
position -= 1;
chars.remove(position);
Expand Down Expand Up @@ -591,7 +617,7 @@ where
render.input_prompt_selection(&self.prompt, &default.to_string())?;
}
term.flush()?;
return Ok(default.clone());
return Ok(Some(default.clone()));
} else if !self.permit_empty {
continue;
}
Expand Down Expand Up @@ -620,7 +646,7 @@ where
}
term.flush()?;

return Ok(value);
return Ok(Some(value));
}
Err(err) => {
render.error(&err.to_string())?;
Expand All @@ -629,13 +655,7 @@ where
}
}
}
}

impl<T> Input<'_, T>
where
T: Clone + ToString + FromStr,
<T as FromStr>::Err: ToString,
{
/// Enables user interaction and returns the result.
///
/// Allows any characters as input, including e.g arrow keys.
Expand Down