-
Notifications
You must be signed in to change notification settings - Fork 371
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
Fix UI glitches and do not pre-write the answer to questions with the default anwser #6290
base: master
Are you sure you want to change the base?
Conversation
668b1b4
to
57cf486
Compare
@@ -824,12 +824,13 @@ let confirm ?(require_unsafe_yes=false) ?(default=true) fmt = | |||
then | |||
(formatted_msg "%sn\n" prompt; false) | |||
else | |||
short_user_input ~prompt ~default:(if default then "y" else "n") | |||
short_user_input ~prompt ~default:' ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason behind removing the pre-written answer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that it is more standard and expected from regular unix users. All the commands with interactive y/n questions i can think of do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, i don't have a strong opinion on that :) lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR content, lgtm!
I'm wondering if we should split the PR as ther is 2 purposes : the fix and the change (back) of the output of confirm questions.
Queued on top of #6289This implements what was described in #6289 (review) with the addition of a UI bugfix:
In this example you can see the
Press enter ...
line is missing the end of line character. This happens becauseOpamConsole.pause
(used to write this line) usesshort_user_input
with~default:""
, but this function wasn't made to have its default be anything but a string of length 1 in interactive mode. This is marked evident as the prompt subfunction will move the cursor left then remove one character, which happens to be the issue here.