Add 'interact_text_opt' and 'interact_text_on_opt' for 'Input' prompts#278
Add 'interact_text_opt' and 'interact_text_on_opt' for 'Input' prompts#278tbergerd wants to merge 6 commits intoconsole-rs:mainfrom
Conversation
tbergerd
changed the title
| )?; | ||
|
|
||
| // Read input by keystroke so that we can suppress ascii control characters | ||
| if !term.features().is_attended() { |
There was a problem hiding this comment.
note : removed this because the check is already performed at the beginning of the function (and returns an error instead which makes more sense)
Gordon01
left a comment
Gordon01
left a comment
There was a problem hiding this comment.
Thanks for the PR, Please restructure the code a little and it's good to go.
| self._interact_text_on(term, true) | ||
| } | ||
|
|
||
| fn _interact_text_on(mut self, term: &Term, allow_quit: bool) -> Result<Option<T>> { |
There was a problem hiding this comment.
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.
| render.clear()?; | ||
| term.flush()?; | ||
| term.show_cursor()?; | ||
| return Ok(None); |
There was a problem hiding this comment.
Let's instead add a new variant for the Error like UserInterrupted and return it here.
| where | ||
| T: Clone + ToString + FromStr, | ||
| <T as FromStr>::Err: Debug + ToString, | ||
| <T as FromStr>::Err: ToString, |
There was a problem hiding this comment.
Why are you removing this bound?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Then it's definitely worth fixing. I suggest making a separate PR for this and removal of the redundant check.
|
Oh, I see why you took this approach. Other prompt implementations are like this. @pksunkara what do you think? Should "cancel" implementations be identical in all prompts? |
Yes, but IIRC there were some complications on the |
|
Thank you for the review. Yes I basically copied what was done for other prompts. One thing to note is this wouldn't work with interact and interact_on (as is currently the case with 'completion_with' and 'history_with') though. I'm not quite sure what the use case for those methods is actually (unicode input probably ?) If it were up to me I'd say consistency should prevail, I'll wait for both your feedbacks. |
3 participants
Hello,
This adds 'interact_text_opt' and 'interact_text_on_opt' for 'Input' prompts (see #160)
Note I did not do the same for 'interact' methods, I don't see how this would work with escape sequences being accepted.
Additionally, unlike selection prompts, I only allowed cancellation via the 'Esc' key, considering 'q' is a valid input
P.S: Is there a specific process to contribute or is opening a pull request fine ?