Skip to content

Add in return for Abort on MultiSelectionPrompt#1001

Closed
BenjaminMichaelis wants to merge 3 commits intospectreconsole:mainfrom
BenjaminMichaelis:851-esc-key-to-exit-selection
Closed

Add in return for Abort on MultiSelectionPrompt#1001
BenjaminMichaelis wants to merge 3 commits intospectreconsole:mainfrom
BenjaminMichaelis:851-esc-key-to-exit-selection

Conversation

@BenjaminMichaelis
Copy link
Contributor

@BenjaminMichaelis BenjaminMichaelis commented Oct 6, 2022

Fixes #851

@BenjaminMichaelis
Copy link
Contributor Author

@patriksvensson To follow up with my question on this issue, is this all you are looking for a return of the Result? Or also some handling of some sort?

@patriksvensson
Copy link
Contributor

@BenjaminMichaelis

We call HandleInput from here: https://github.com/spectreconsole/spectre.console/blob/main/src/Spectre.Console/Prompts/List/ListPrompt.cs#L59

  1. We would need to add a check here for the Abort result, and in that case, break.
  2. We should probably add a new required boolean to the ListPrompt.Show method, and before aborting, check if we're allowed to, so we don't abort if the prompts are expected to produce some kind of result.

What do you think?

@BenjaminMichaelis
Copy link
Contributor Author

I think that makes sense, Let me check that I fully understand what you mean by trying to implement it

@BenjaminMichaelis
Copy link
Contributor Author

How does this look? @patriksvensson

@BenjaminMichaelis BenjaminMichaelis marked this pull request as ready for review October 13, 2022 05:04
@BenjaminMichaelis BenjaminMichaelis marked this pull request as draft October 13, 2022 05:04
@patriksvensson
Copy link
Contributor

@BenjaminMichaelis I think it looks good! Would it be possible to add a test as well?

@patriksvensson
Copy link
Contributor

The build problems should have been resolved now.
Could you rebase your PR branch against the main branch?

@BenjaminMichaelis BenjaminMichaelis force-pushed the 851-esc-key-to-exit-selection branch from 23e8d62 to 235e251 Compare October 13, 2022 16:40
@BenjaminMichaelis
Copy link
Contributor Author

Just rebased @patriksvensson, added in a small test, but as I am still getting used to this library, not positive if there are better things I should be testing for/or how to since this feels minor. I'm not positive how to get the ListPromptInputResult out from the test view since we return a state

@BenjaminMichaelis BenjaminMichaelis marked this pull request as ready for review October 13, 2022 17:05
@patriksvensson
Copy link
Contributor

@BenjaminMichaelis Your test actually showed that there is a flaw in the design.
We always return the selected item(s) and in this case, we want to inform the caller that the operation was aborted.

@nils-a @phil-scott-78 Any suggestions on how we could solve this in an extendable way?

@BenjaminMichaelis
Copy link
Contributor Author

Ahh, to make sure I am following, so the state index should still be 0 in fact like the test shows but in addition be letting the caller know that it didn't move because this was an aborted case?

@patriksvensson
Copy link
Contributor

@BenjaminMichaelis Correct, but I think we need to discuss how this should be done. Would like some input from the other maintainers about this.

@patriksvensson
Copy link
Contributor

@BenjaminMichaelis I'm sorry, but turns out there already is an active pull request for this item: #711

I'm sorry I didn't realize this before, I could have saved you from wasting your time 😞

@BenjaminMichaelis
Copy link
Contributor Author

@BenjaminMichaelis I'm sorry, but turns out there already is an active pull request for this item: #711

I'm sorry I didn't realize this before, I could have saved you from wasting your time 😞

Aw bummer, I'll keep looking for more to do :)

@Kamakazi152
Copy link

@patriksvensson Am I looking at this right and seeing #711 was never finished? If so, could @BenjaminMichaelis submit his changes again? This feature is still not implemented, and it looks like Benjamin already got this figured out.

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.

Esc Key to exit selection

3 participants