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

Added possibility of cancelling prompts #1046

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

quintesse
Copy link
Contributor

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

@mattirn
Copy link
Collaborator

mattirn commented Jul 26, 2024

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.

@quintesse
Copy link
Contributor Author

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?

@mattirn
Copy link
Collaborator

mattirn commented Jul 26, 2024

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;
                }

@quintesse
Copy link
Contributor Author

quintesse commented Jul 26, 2024

But how would you exit? And how would the caller detect that the user canceled?

@mattirn
Copy link
Collaborator

mattirn commented Jul 26, 2024

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());

@quintesse
Copy link
Contributor Author

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 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.

@gnodet
Copy link
Member

gnodet commented Jul 27, 2024 via email

@quintesse
Copy link
Contributor Author

quintesse commented Jul 27, 2024

@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)

@mattirn
Copy link
Collaborator

mattirn commented Jul 28, 2024

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.

@quintesse
Copy link
Contributor Author

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?

@mattirn
Copy link
Collaborator

mattirn commented Jul 28, 2024

But just to confirm: does that mean that you are okay with adding ESC to cancel by default to all key bindings?

Yes

@quintesse
Copy link
Contributor Author

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.

@quintesse
Copy link
Contributor Author

@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.

@mattirn
Copy link
Collaborator

mattirn commented Aug 14, 2024

  1. There is no need to modify prompt builders. You should rollback all modifications made on package org.jline.consoleui.prompt.builder.
  2. Modification on ConsolePrompt.UiConfig(...) constructor breaks existing implementations. We should not modify UiConfig constructor but implement setter and getter for cancellable property field.
  3. Rename cancellable property field to cancellableFirstPrompt .
  4. No modifications are required on test classes. You should rollback modifications done on classes: Issue1025, Basic and LongList.
  5. Create a new example test/org/jline/consoleui/examples/BasicDynamic.java, which can be later improved when resolving issue Dynamic console-ui prompts #1051. In BasicDynamic we will build two/three PromptBuilders: 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()).

@quintesse
Copy link
Contributor Author

quintesse commented Aug 14, 2024

@mattirn

  1. Oops forgot about that, sorry. Done now.
  2. Switched from constructor arg to setter
  3. Field renamed
  4. Examples reverted
  5. New example created (Edit3: the output isn't very pretty because we can't 100% control what the prompts display. This is something that could perhaps be handled as part of Dynamic console-ui prompts #1051 as well)

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.

@mattirn
Copy link
Collaborator

mattirn commented Aug 27, 2024

@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.

@quintesse
Copy link
Contributor Author

@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)

@mattirn
Copy link
Collaborator

mattirn commented Sep 2, 2024

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.

Cancelling a prompt by hitting the ESC key makes it go back to the
previous prompt. By default, if we're already at the fist prompt we'll
repeat the question until the user either answers it or forcefully quits
the application. If on the other hand the `cancellableFirstPrompt` option
of `UiConfig` has been set to `true`, the `prompt()` method will return
`null`.

Fixes jline#1035
@quintesse
Copy link
Contributor Author

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.

@mattirn mattirn merged commit 5283218 into jline:master Sep 10, 2024
4 checks passed
@quintesse quintesse deleted the consoleui_cancelop branch September 10, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to cancel console-ui prompts
3 participants