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

Add text-only prompt element for console-ui #1138

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

quintesse
Copy link
Contributor

@quintesse quintesse commented Dec 19, 2024

Fixes #1136

First stab at implementing this new text-only element. It won't win any beauty contests, but it works. WDYT @mattirn ?

NB: I considered creating a TextPrompt and using ConsoleUIItemIF for the lines of the Text element, but between the fact that TextPrompt would be mostly empty (because we're not actually asking for input) , the fact that items only deal with Strings, not AttributedStrings and the fact that each XxxxPrompt only displays a single line of result it just wasn't a good fit.

@mattirn mattirn added this to the 3.28.1 milestone Dec 19, 2024
@mattirn
Copy link
Collaborator

mattirn commented Dec 20, 2024

Looks good!
I'd like to make two small changes:

  1. from the resultMap that is returned from ConsolePrompt.prompt(...) method I would remove textPrompt results (i.e. _text_X=NoResult{})
  2. in BasicDynamic example catch UserInterruptException in place of IOError, fixed in Move catch to proper place #1133.

I think we can open an other ticket/pull request for ConsolePrompt's methods visibility changes

@quintesse
Copy link
Contributor Author

  1. from the resultMap that is returned from ConsolePrompt.prompt(...) method I would remove textPrompt results (i.e. _text_X=NoResult{})

Good idea!

  1. in BasicDynamic example catch UserInterruptException in place of IOError, fixed in Move catch to proper place #1133.

I actually tried that and couldn't get it to work right. Let me try again and if I can't figure it out I'll give a shout for help :-)

I think we can open an other ticket/pull request for ConsolePrompt's methods visibility changes

Ok, btw, do you perhaps see a way to "retrofit" the now deprecated methods so they can be used with this new prompt method? I've been racking my brain but I can't think of a good solution. Sure, we can add them with a different name, eg promptItems() or whatever, but it seems a pity. It probably also depends on how much backwards compatibility you want to maintain with older code.

@mattirn
Copy link
Collaborator

mattirn commented Dec 23, 2024

  1. from the resultMap that is returned from ConsolePrompt.prompt(...) method I would remove textPrompt results (i.e. _text_X=NoResult{})

Good idea!

textPrompt results should be removed also here
return resultMap;

  1. in BasicDynamic example catch UserInterruptException in place of IOError, fixed in Move catch to proper place #1133.

I actually tried that and couldn't get it to work right. Let me try again and if I can't figure it out I'll give a shout for help :-)

Update your project. I have merged the fix #1133.

I think we can open an other ticket/pull request for ConsolePrompt's methods visibility changes

Ok, btw, do you perhaps see a way to "retrofit" the now deprecated methods so they can be used with this new prompt method?

I have created the ticket #1141 with some guide lines...

@quintesse
Copy link
Contributor Author

@mattirn updated the PR with your suggestions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add text-only prompt element for console-ui
2 participants