Abort SelectionPrompt and MultiSelectionPrompt with Escape key

[jariq]

Code in this PR adds possibility to abort SelectionPrompt and MultiSelectionPrompt with Escape key. When Escape key is pressed, prompt is aborted and its Aborted property is set to true. I believe this PR does not change the current default behavior in any way, because Escape key still does nothing unless AllowAbort property is explicitly set to true.

I tried to follow existing coding style as closely as possible. I did add some tests and made sure all existing tests are passing before opening this PR. As suggested by contribution guidelines I started the discussion first in #700 but then had to move on with the implementation before I received any reply. I'm really sorry for that but I couldn't continue with a soon-to-be-released^TM Pkcs11AdminConsole project without being able to abort these prompts.

If implementation in this PR is not suitable for inclusion I am willing to work on any suggested alternative.

[CLAassistant]

All committers have signed the CLA.

[phil-scott-78]

I haven't been able to get on my PC for the past few days, but I was thinking about the syntax and one thought I had was perhaps this syntax would be better over setting some properties

public static bool TryPrompt<T>(IPrompt<T> prompt, out T result)

[danielklecha]

I think that syntax AnsiConsole.TryPrompt is not necessary.
SelectionPrompt.ShowAsync should return default(T) if prompt was aborted.
MultiSelectionPrompt.ShowAsync should return empty list.

[danielklecha]

@phil-scott-78 Sorry, you have right. If we add Aborted property to IPrompt then we can add AnsiConsole.TryPrompt and this should not break existing logic.

[jariq]

TryPrompt could be easily added on top of the existing code for example this way:

namespace Spectre.Console;

/// <summary>
/// A console capable of writing ANSI escape sequences.
/// </summary>
public static partial class AnsiConsole
{
    /// <summary>
    /// Displays abortable prompt to the user.
    /// </summary>
    /// <typeparam name="T">The prompt result type.</typeparam>
    /// <param name="prompt">The prompt to display.</param>
    /// <param name="result">The prompt input result.</param>
    /// <returns>True if prompt processed normally; false if prompt aborted.</returns>
    public static bool TryPrompt<T>(SelectionPrompt<T> prompt, out T result) where T : notnull
    {
        prompt.AllowAbort = true;
        result = prompt.Show(Console);
        return !prompt.Aborted;
    }

    /// <summary>
    /// Displays abortable prompt to the user.
    /// </summary>
    /// <typeparam name="T">The prompt result type.</typeparam>
    /// <param name="prompt">The prompt to display.</param>
    /// <param name="result">The prompt input result.</param>
    /// <returns>True if prompt processed normally; false if prompt aborted.</returns>
    public static bool TryPrompt<T>(MultiSelectionPrompt<T> prompt, out List<T> result) where T : notnull
    {
        prompt.AllowAbort = true;
        result = prompt.Show(Console);
        return !prompt.Aborted;
    }
}

However in this case TryPrompt would work directly with SelectionPrompt<T> and MultiSelectionPrompt<T> instead of more general IPrompt<T> that was proposed by @phil-scott-78.

I currently see three options on how to proceed with this idea:

1. Add TryPrompt methods mentioned above.
   It's tempting because the solution is just one commit away but it somehow does not feel right.
2. Modify IPrompt to support aborting.
   This would most likely be an API breaking change and I'm not sure if that's acceptable in this project.
   I'm also not quite sure that every prompt implementation needs to / should support aborting.
3. Introduce IAbortablePrompt with AllowAbort and Aborted properties to support prompt aborting in general.
   I personally find this to be the best option.

What do others think?

[patriksvensson]

I'm not a fan of mutating the Prompt item.
I'm thinking that maybe it would be a good idea to have a method called ShowAbortable or similar, and set the AllowAbort as part of the context instead?

@phil-scott-78 @nils-a What do you think?

[danielklecha]

If we use ShowSortable then it should contain ShowSortableAsync too. I more like solution with AllowAbort method.

var item = await new SelectionPrompt<string>()
    .AllowAbort(true)
    .AddChoices( chocies )
    .ShowAsync( AnsiConsole.Console, CancellationToken.None );

Also, It will be interesting if this feature could be a part of IPrompt. I think that option to abort TextPrompt could be usefull. But I know that is bigger change. Adding it to selection without interface is still one step forward.

[jariq]

I want to be sure I did not miss any important feedback (English is not my native language) so let's sum up this merge request from my point of view:

• @phil-scott-78 suggested TryPrompt method to be added.
  I suggested three possible implementations but did not receive any hint on which one (if any) should I implement.
  I'm not sure how to proceed. Should I do my best and decide myself or should I wait for an additional feedback?

• @patriksvensson suggested ShowAbortable method to be added.
  He asked @phil-scott-78 and @nils-a for their opinions but they didn't provide any yet.
  I'm not sure how to proceed. Should I do anything or should I wait for an additional feedback?

I'm still here. I still want to continue with this PR. I'm just not sure how.

[jouniheikniemi]

Nobody asked for my opinion, but since I hit the same issue in another project of mine, I'll chip in my €0.02 anyway. :-)

Let me start with a philosophical statement: I think supporting escape is a worthy goal, and I might even go as far as to state that practically every prompt actually should be cancelable, as non-cancelable prompts just lead to the user hitting Ctrl-C if they panic. It would be better for the application to deal with cancellation according to its best possible understanding of the situation - this is much similar to GUIs having close buttons in dialog boxes even if a choice is logically mandatory. This was true when I wrote my first console UX library for BBSes in 1992, and I think it still mostly is. That may still obviously not be the scope under consideration here.

I agree with @patriksvensson that adding a result property into the Prompt object confuses things - that's the way e.g. Winforms did things with Form.DialogResult, and I don't think it was a good choice. But I'm not super-excited by a separate ShowAbortable method either: Sometimes the "abortability" (with whatever definition it has, given what I said above) is just another property of the prompt, and having a separate method for it makes things more complex at the callsite, especially if you have a structure where a generic UI framework (say, a wizard wrapper) is just passed a bunch of prompts. With separate ShowAbortable method, you'd end up passing (Prompt, bool callShowAbortable) tuples around which quickly turns into a mess. Also, if you ever need another separate Show* family, the combos quickly add up.

What I'm saying is leading into the direction of a breaking change; perhaps having Show<T>() methods return a PromptResult<T> instead of a T, but perhaps that could have an implicit conversion to T. The value T would be one field of the PromptResult, the "dialog state" (i.e. aborted/OK) would be another. That's not trivial, but it could have potential for future extensibility as well.

From past experience, I'd guess some likely extensibility points to come up are supporting tab/shift-tab to navigate between prompts (i.e. encapsulating several prompts into a form), which might be implementable as a "AllowTabNavigation" property and a couple of new "dialog states" (MovePrev/MoveNext), which a form controller might handle - but no, I don't think something like ShowNavigableAbortableAsync is going to fly. Another one is a combobox control, which may be used to pick from a list of customers (objects), but also enter a new customer name, in which scenario the result will never fit into a single field (as it may be either an object or a string).

None of these issues is fully solved by complicating the result type of a Prompt.Show call, but I do think composability of complex UI (whether console or GUI) benefits from having result objects that can return not just the value, but also some information about the user's intent. That should be the part that makes complex things possible. Simple things should then be kept simple by having the Abortability be false by default and having the PromptResult be implicitly convertible to the result type.

It will still break var foo = p.Show() scenarios, but hey, isn't this what 0.x versions are for?

[phil-scott-78]

I'm really digging the feedback from @jouniheikniemi. I'd actually prefer to break things than introduce properties to force it into working with the current model.

Sorry to keep hemming and hawing over this, @jariq. I think this is a really good feature and one that is needed for Spectre to reach a 1.0 release so it means a lot that we land it with something we all feel good about support long term.

[jariq]

Just a quick reminder I'm still waiting for the input from maintainers willing to merge this feature.

[jsiegmund]

Stumbled across this PR and wanted to pitch in. I'm looking for similar functionality, but my use case is a tad different. I'm using this project as a rudimentary UI for a dialog API. Basically it's a form which can be filled in on per question basis. So we render a given question, the user answers it and we move on to the next. The question types range from text input to numeric, date, choice, boolean, etc.

What I wanted to acheive is the ability to navigate back and forth between questions. The API behind this allows the user to navigate forward as long as the questions have already been answered. The user can also go back in order to revisit their answer to a given question.

I was looking to use the arrow keys for navigating back and forth, so my first comment would be: do not tie cancelling a prompt to specific keys, but allow for the ability to specify which keystrokes should cause the prompt to cancel.

If there's a need to implement this as a non-breaking change, you could consider a solution like this:

var item = await new SelectionPrompt<string?>()
    .AllowAbort(true, Keys.Left, Keys.Right)
    .AddChoices( choices )
    .ShowAsync( AnsiConsole.Console, CancellationToken.None );

Setting AllowAbort to true would only be available for nullable types (extension method probably?) and would have the ability to specify the keys to abort. Any of these keystrokes would abort the prompt and return null as a result. Then I would have the need to find out which key actually caused the prompt to cancel, could be done using a callback.

Even better imho would be to not return the typed response directly, but have a typed Result object which would either contain the requested typed value, or an abort indication along with the pressed keys. Such a Result object would allow for a broader set of usecases than just this one. That would probably be a breaking change although you might be able to get around that using an explicit operator to convert Result<T> to T.

[danielklecha]

SelectionPrompt.ShowAsync must return instance of T.
If we allow to abort then there will be a problem what should be returned because default(T) could be null.

I created INullablePrompt in #1084.
Maybe we should use AllowAbort in combination with that interface.

var item = await new SelectionPrompt<string?>()
    .AllowAbort(true) //or .AllowAbort(true, Keys.Left, Keys.Right)
    .AddChoices( choices )
    .ShowNullableAsync( AnsiConsole.Console, CancellationToken.None );

I'm not sure if we need wrapper around result (like Result<T>). Each choice can be wrapped with custom class so choice will be always an instance. In that case we can dectect null as abort action.

[jariq]

I'm closing this PR as I'm no longer interested in working on this feature.