Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ console = "0.13.0"
lazy_static = "1"
tempfile = "3"
zeroize = "1.1.1"
enigo = "0.0.14"
24 changes: 19 additions & 5 deletions src/prompts/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub struct Select<'a> {
default: usize,
items: Vec<String>,
prompt: Option<String>,
before: Box<dyn FnMut() + 'a>,
clear: bool,
theme: &'a dyn Theme,
paged: bool,
Expand Down Expand Up @@ -77,6 +78,7 @@ impl<'a> Select<'a> {
default: !0,
items: vec![],
prompt: None,
before: Box::new(|| ()),
clear: true,
theme,
paged: false,
Expand Down Expand Up @@ -180,7 +182,7 @@ impl<'a> Select<'a> {
/// Similar to [interact_on](#method.interact_on) except for the fact that it does not allow selection of the terminal.
/// The dialog is rendered on stderr.
/// Result contains index of a selected item.
pub fn interact(&self) -> io::Result<usize> {
pub fn interact(&mut self) -> io::Result<usize> {
self.interact_on(&Term::stderr())
}

Expand All @@ -189,10 +191,15 @@ impl<'a> Select<'a> {
/// This method is similar to [interact_on_opt](#method.interact_on_opt) except for the fact that it does not allow selection of the terminal.
/// The dialog is rendered on stderr.
/// Result contains `Some(index)` if user selected one of items or `None` if user cancelled with 'Esc' or 'q'.
pub fn interact_opt(&self) -> io::Result<Option<usize>> {
pub fn interact_opt(&mut self) -> io::Result<Option<usize>> {
self.interact_on_opt(&Term::stderr())
}

pub fn set_before(&mut self, f: impl FnMut() + 'a) -> &mut Select<'a> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this fn?

Copy link
Author

Choose a reason for hiding this comment

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

I might be able to avoid it if I modify and create a custom Term. However, I can promise you it won't be pretty and goes against basic design principles. I agree that we should minimize any fn's that does not benefit the user. So it might be an idea to find an use-case for it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please explain a bit on why exactly does enigo stuff need to happen after rendering the prompt and not before the whole interact fn?

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't and I think it should be prior to any rendering. I just did it as a test for the sake of getting visual feedback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case, we don't need this function. Please setup enigo before calling interact in the test.

Copy link
Author

@BlackPhlox BlackPhlox Dec 30, 2020

Choose a reason for hiding this comment

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

Done.
Next step is:

  • We have some test in bottom of prompts/select.rs, do we want that to stay or should it also be moved to tests/select.rs?
  • Configure CI so that the integration test passes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Those are unit tests. We don't want to move them to tests folder.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, I've renamed the pr to integration test (based on this).
Regarding CI, we currently use cargo test --lib. We have to use cargo test --tests to also accommodate the integration tests. Seen as I don't have the rights to cancel a workflow, I would rather have you monitoring the workflow with the changes. As it previously created an infinite loop when calling the 'before' fn after the rendering step, but it might still do it (It only happens in the workflow).

Either way, this doesn't have to be rushed by any means, especially given the date, Happy New Year Pavan! 🎆

self.before = Box::new(f);
self
}

/// Like [interact](#method.interact) but allows a specific terminal to be set.
///
/// ## Examples
Expand All @@ -211,7 +218,7 @@ impl<'a> Select<'a> {
/// Ok(())
/// }
///```
pub fn interact_on(&self, term: &Term) -> io::Result<usize> {
pub fn interact_on(&mut self, term: &Term) -> io::Result<usize> {
self._interact_on(term, false)?
.ok_or_else(|| io::Error::new(io::ErrorKind::Other, "Quit not allowed in this case"))
}
Expand All @@ -238,12 +245,12 @@ impl<'a> Select<'a> {
/// }
/// ```
#[inline]
pub fn interact_on_opt(&self, term: &Term) -> io::Result<Option<usize>> {
pub fn interact_on_opt(&mut self, term: &Term) -> io::Result<Option<usize>> {
self._interact_on(term, true)
}

/// Like `interact` but allows a specific terminal to be set.
fn _interact_on(&self, term: &Term, allow_quit: bool) -> io::Result<Option<usize>> {
fn _interact_on(&mut self, term: &Term, allow_quit: bool) -> io::Result<Option<usize>> {
let mut page = 0;

if self.items.is_empty() {
Expand Down Expand Up @@ -280,6 +287,8 @@ impl<'a> Select<'a> {
size_vec.push(*size);
}

let mut is_first_update = true;

loop {
for (idx, item) in self
.items
Expand All @@ -294,6 +303,11 @@ impl<'a> Select<'a> {
term.hide_cursor()?;
term.flush()?;

if is_first_update {
(self.before)();
is_first_update = false;
}

match term.read_key()? {
Key::ArrowDown | Key::Char('j') => {
if sel == !0 {
Expand Down
31 changes: 31 additions & 0 deletions tests/select.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
use dialoguer::{theme::ColorfulTheme, Select};

use enigo::{Enigo, Key, KeyboardControllable};
use std::thread;
use std::time::Duration;

#[test]
fn basic_navigation_produces_correct_selection() {
let selections = &[
"Ice Cream",
"Vanilla Cupcake",
"Chocolate Muffin",
"A Pile of sweet, sweet mustard",
];

let selection = Select::with_theme(&ColorfulTheme::default())
.with_prompt("Optionally pick your flavor")
.set_before(|| {
let mut enigo = Enigo::new();
enigo.key_click(Key::Layout('j'));
enigo.key_down(Key::Return);
thread::sleep(Duration::from_millis(10));
enigo.key_up(Key::Return);
})
.default(0)
.items(&selections[..])
.interact_opt()
.unwrap();

assert_eq!(Some(1), selection);
}