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

Dynamic console-ui prompt improvements, see #1051 #1132

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

quintesse
Copy link
Contributor

This adds an easier way to create complex dynamic prompts

This adds an easier way to create complex dynamic prompts
@quintesse quintesse force-pushed the consoleui-dynaimprov branch from c46b053 to 34650d9 Compare December 12, 2024 13:03
@quintesse
Copy link
Contributor Author

@mattirn and @gnodet , this might be a bit forward, but I was wondering if we could improve the ConsolePrompt somehow. Because I feel that with the latest changes some unnecessary complexity was introduced.

In the new code the old prompt() methods are deprecated and we have new ones that take a resultMap as an argument. Now, this is completely understandable so we can correctly restore previous state when cancelling prompts (basically when going backwards).

But this Map needs to be in a very specific state for this to work, it must either be empty or it must contain a value for all previously answered questions. On top of that, if the Map contains values, the header must contain 1 line for each of those values. Otherwise the system of cancelling prompts breaks. This, to me, smacks of too much internal implementation details leaking out into a public API.

In this PR I've introduced a new prompt() method that can be used for complex situations while not having to deal too much with states and multiple result maps etc (see the diff for BasicDynamic.java).

Just to be clear, this method is not for simple situations, just to make complex situations like the example in BasicDynamic.java easier to deal with! Although at the same time you could easily get the behaviour of the old deprecated methods by calling:

var resultMap = prompt.prompt(results - > results.isEmpty() ? promptBuilder.build() : null);

Still, I wouldn't want users to have to use that for simple cases, so for ease-of-use it might be desirable to add methods like the ones that were just deprecated to be able to simply pass a List of prompts. Unfortunately I wasn't able to come up with a solution where the old deprecated methods could be used in both situations (thereby un-deprecating them), so perhaps new methods could be added with a different name?

I would also suggest to make the new methods that take a resultMap parameter protected so people won't try to use them by accident without knowing all the ins and outs of their workings. I think everything that can be done with those methods can more easily be done with this PR and the other suggestions I made.

@mattirn
Copy link
Collaborator

mattirn commented Dec 17, 2024

Looks good!
I would like to change your implemented behavior of UiConfig.cancellableFirstPrompt configuration. If UiConfig.cancellableFirstPrompt = false, I would not allow to return previous prompt lists. I guess in this way UiConfig.cancellableFirstPrompt configuration could be more useful.

There is at least one thing what you cannot do with the new method. The methods that take a resultMap parameter you can modify the header list. For example you can add sub-header before starting a new prompt list.

@quintesse
Copy link
Contributor Author

quintesse commented Dec 17, 2024

Looks good! I would like to change your implemented behavior of UiConfig.cancellableFirstPrompt configuration. If UiConfig.cancellableFirstPrompt = false, I would not allow to return previous prompt lists. I guess in this way UiConfig.cancellableFirstPrompt configuration could be more useful.

Sure, we can do that. But is there a good use-case for that situation? I can understand requiring the user to at least give some answer, but once you get into multiple branching pathways I would imagine it's good to always be able to go back and redo your selections?

There is at least one thing what you cannot do with the new method. The methods that take a resultMap parameter you can modify the header list. For example you can add sub-header before starting a new prompt list.

True, but having the possibility to go back and forth in the list (really a tree) of questions would make managing headers rather hard anyways, I guess. 🤔

Perhaps it would be an idea to introduce a "text-only element"? That way the printing of headers would be handled by the system itself. A text-only element could be seen as a prompt that immediately returns a dummy/empty answer. (And when cancelling prompts they would be automatically be removed as well).

@mattirn
Copy link
Collaborator

mattirn commented Dec 18, 2024

Perhaps it would be an idea to introduce a "text-only element"? That way the printing of headers would be handled by the system itself. A test-only element could be seen as a prompt that immediately returns a dummy/empty answer. (And when cancelling prompts they would be automatically be removed as well).

Ok, please open an other ticket if you think you can do a pull request to resolve it.

@quintesse
Copy link
Contributor Author

Ok, please open an other ticket if you think you can do a pull request to resolve it.

Done #1136

I'll create a PR for it based on the code in this PR (I assume it will be merged before or at the same time)

@mattirn mattirn merged commit ffce720 into jline:master Dec 18, 2024
5 checks passed
@quintesse
Copy link
Contributor Author

Awesome, thanks!

@mattirn mattirn added this to the 3.28.1 milestone Dec 18, 2024
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.

2 participants