Closed quintesse closed 3 days ago
I agree with you that cancellable prompts configuration is better to be done in globally rather than per-item basis.
I think that when cancelling the first prompt it would better not to exit from ConsolePrompt.prompt(...)
method returning null
but we should redraw the first prompt (i.e. to go back to the first prompt). It is the null
response of the ConsolePrompt.prompt(...)
method that is breaking the backward compatibility requiring for developers to handle annoying null
response. If we avoid the null
response we can set the default configuration value true
for cancellable prompts.
About the implementation. There is no need to modify prompt builders. You should rollback your modifications done in package org.jline.consoleui.prompt.builder
.
The prompt classes have global configuration property config
(ConsolePrompt.UiConfig) that we should use for cancellable prompts configuration. We should add cancellable
property with getter and setter methods in UiConfig
class. We do not need any new properties in prompt classes and also ConsolePrompt.cancellable(boolean)
method should be removed.
About the implementation
Ok, I assumed the UiConfig was only for things related to look & feel, not to behaviour. That will make things easier indeed.
But I'm not so sure about the other comment. I agree that returning null
breaks backward compatibility, but how would you suggest going back to previous prompts if there's no way to detect that a prompt was cancelled?
But I'm not so sure about the other comment. I agree that returning
null
breaks backward compatibility, but how would you suggest going back to previous prompts if there's no way to detect that a prompt was cancelled?
I'm referring to the line ConsolePrompt.java#L199. Here we should not return null
. I was thinking to replace if statement starting on line 190 with
if (result == null) {
// Prompt was cancelled by the user
if (i > 0) {
// Remove last result
header.remove(header.size() - 1);
// Go back to previous prompt
i -= 2;
} else {
i--;
}
continue;
}
But how would you exit? And how would the caller detect that the user canceled?
I guess I am missing something you would like to achieve.
In order to exit the user has to answer all the prompts or to cancel he can press ctrl-c
.
The caller can detect that the user has cancelled by catching java.io.InterruptedIOException
. To achieve this you have to register signal handler to your terminal:
Thread executeThread = Thread.currentThread();
terminal.handle(Terminal.Signal.INT, signal -> executeThread.interrupt());
Well first of all requiring ctrl-c to cancel is not very user-friendly, second a user would expect ctrl-c to exit the application, so what if the UI is just part of a larger application that you do not want to exit? And finally, I do not want to exit the application, I want to exit the current question to go back to the previous one so I can re-answer it without having to start the entire process over. Which is something that you suggested in https://github.com/jline/jline3/issues/1042 which this PR (also) fixes.
Perhaps there's an issue of wording here, when I created this issue I was referring to cancelling individual prompt items, not the entire prompt (with a single keystroke). Cancelling the entire prompt would be handled by repeatedly hitting ESC until you reach the first question, then hitting ESC one final time. Which is what this PR implements.
When inside a shell, ctrl+c usually gets back to the shell. If you’re inside a prompt, for example a readline call, the call will exit and get back to the shell. Just try the gogo demo for example…
Guillaume Nodet
Le sam. 27 juil. 2024 à 18:06, Tako Schotanus @.***> a écrit :
Well first of all requiring ctrl-c to cancel is not very user-friendly, second a user would expect ctrl-c to exit the application, so what if the UI is just part of a larger application that you do not want to exit? And finally, I do not want to exit the application, I want to exit the current question to go back to the previous one so I can re-answer it without having to start the entire process over. Which is something that you suggested in #1042 https://github.com/jline/jline3/issues/1042 which this PR (also) fixes.
Perhaps there's an issue of wording here, when I created this issue I was referring to cancelling individual prompt items, not the entire prompt (with a single keystroke). Cancelling the entire prompt would be handled by repeatedly hitting ESC until you reach the first question, then hitting ESC one final time. Which is what this PR implements.
— Reply to this email directly, view it on GitHub https://github.com/jline/jline3/pull/1046#issuecomment-2254187915, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUQNRUVOQLFSOPRRXUCPLZOPASBAVCNFSM6AAAAABLMSAL3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGE4DOOJRGU . You are receiving this because you are subscribed to this thread.Message ID: @.***>
@gnodet this is about not wanting to go back to the shell, but simply backing up a single question. For example, the user selects an option, in the next prompt (item) they realize they've made the wrong selection and want to back out to the previous question. You might say "just ctrl-c and restart" but there might be a whole bunch of reasons the user doesn't want to do that (one reason might be that they've already answered a dozen other prompts before and don't want to repeat all of them)
Ok, I think I have understood the point. For example you can wrote an app that prompt question(s) before you have called ConsolePrompt.prompt(...)
method. In that case cancelling the first ConsolePrompt
's prompt question should go back to the question asked just before the ConsolePrompt.prompt(...)
method call. Of course we want to cancel all the prompts in the same way.
IMHO it is enough to have configuration option only for the first prompt cancellation. As a default the first prompt is not cancellable in order to keep the backward compatibility. If the first prompt is not cancellable we will redraw the first prompt when trying to cancel it. In a case the first prompt is cancellable ConsolePrompt.prompt(...)
will return null
when the first prompt is cancelled.
Indeed! And the idea of repeating the first question (if cancellable is false) to maintain backward compatibility is a good options as well. But just to confirm: does that mean that you are okay with adding ESC to cancel by default to all key bindings?
But just to confirm: does that mean that you are okay with adding ESC to cancel by default to all key bindings?
Yes
Updated the PR according to the feedback. cancellable
is now a field of UiConfig
(defaults to false for backward compatibility). ESC will now always cancel a prompt and go back to the previous question, unless we're already at the first question. In which case the behavior depends on the cancellable
field: false
means we'll repeat asking the first question indefinitely, true
means the prompt will return null
.
@mattirn have you had some time to look at the new code?
Btw, my idea was to flatten the commits into a single one, I haven't done so for now so you can look at the changes I made. Once you're okay with the new code I can either flatten manually or you can do so at the moment of merging, whatever you prefer.
org.jline.consoleui.prompt.builder
.ConsolePrompt.UiConfig(...)
constructor breaks existing implementations. We should not modify UiConfig
constructor but implement setter and getter for cancellable
property field.cancellable
property field to cancellableFirstPrompt
.test/org/jline/consoleui/examples/BasicDynamic.java
, which can be later improved when resolving issue #1051. In BasicDynamic
we will build two/three PromptBuilder
s: promptPizzaOrHamburger
, promptPizza
and promptHamburger
. promptPizza
is like promptBuilder
in Basic.java
and promptHamburger
is equivalent for hamburger ordering. The logic of BasicDynamic
should be something like Result result = consolePrompt.prompt(header, promptPizzaOrHumbuger.build())
if (result == pizza) {
result = consolePrompt.prompt(header, promptPizza.build())
} else {
result = consolePrompt.prompt(header, promptHumburger.build())
}
When cancelling the first prompt of promptPizza
(or promptHumburger
) the program flow should return to the line Result result = consolePrompt.prompt(header, promptPizzaOrHumbuger.build())
.
@mattirn
The only thing is that all of a sudden I have to hit ESC twice to have it registered. It's almost as if the ambiguous timeout doesn't work anymore. Investigating...
Edit: the code doesn't seem to honor the timeout. In this line https://github.com/jline/jline3/blob/master/reader/src/main/java/org/jline/keymap/BindingReader.java#L83 the peek simply blocks forever even though the value passed is 150 (checked with debugger).
Edit2: this only seems to happen in a Git Bash console on WIndows, it doesn't happen in a CMD window (using jansi), nor on my Fedora Linux system.
@quintesse , your pull request looks good to me.
Please, could you add in BasicDynamic
also the possibility to cancel the request by pressing ctrl-c
i.e. register signal handler to your terminal:
Thread executeThread = Thread.currentThread();
terminal.handle(Terminal.Signal.INT, signal -> executeThread.interrupt());
and in try-block catch java.io.InterruptedIOException
.
I agree that the problems you have found in BasicDynamic
example can be fixed in #1051.
@mattirn I'm not sure if I understand what you want me to do exactly? Do you want to make it possible to treat CTRL+C as an alternative to ESC to cancel a prompt item instead of exiting the application?
If so, shouldn't that be configurable? Because that seems like a behaviour most people wouldn't want by default. (Which other application behaves like that? I don't know of any. In fact I would be very angry with any CLI application doing that, if I hit CTRL+C I want an app to exit)
I would like you to register signal handler to your terminal and catch java.io.InterruptedIOException
on your try-block (BasicDynamic.main(...)
). So we will enhance the example to show also how to manage ctrl-c
interrupts.
The desired behavior of ctrl-c
depends on app. For example if you execute consolePrompt command from REPL-app pressing ctrl-c
should not exit from app but only from consolePrompt command.
There is no need additional configurations because the developer already has a full control to implement ctrl-c
actions.
Ok, so it's only to show how to handle CTRL+C in an app. That confused me a bit because no CTRL+C handling is being done in Basic
either, so I thought you meant something inside the library itself. Changes pushed.
Cancelling a prompt by hitting the ESC key makes it go back to the previous prompt. If we're already at the fist prompt the result returned will be
null
.Fixes #1035