manifoldco / promptui

Interactive prompt for command-line applications
https://www.manifold.co
BSD 3-Clause "New" or "Revised" License
6.07k stars 336 forks source link

Adding key for quit in SelectKeys #155

Open LuciferInLove opened 4 years ago

LuciferInLove commented 4 years ago

Adding key for quit (default is 'q') in SelectKeys

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

LuciferInLove commented 4 years ago

@blast-hardcheese, thanks for your comment. I've added variable for an optional value that can be returned on exit.

blast-hardcheese commented 4 years ago

what's blocking this from getting merged?

HurSungYun commented 3 years ago

I like this feature and I hope this PR would be merged soon. :) thank you.


Additionally, I want to leave some comments below. :)

I doubt both os.Exit(0) and q as default value is good decision.

  1. I believe it would be better to return errors like ErrSelectionQuitted because it gives users freedom of choices. Currently, if a user want to emit os.Exit(1) when quitted, there's nothing users can do about it.

  2. I am wondering that ESC can be a better choice comparing with q because Select have search feature.

LuciferInLove commented 3 years ago

@HurSungYun, thanks for your comment.

  1. I believe it would be better to return errors like ErrSelectionQuitted because it gives users freedom of choices. Currently, if a user want to emit os.Exit(1) when quitted, there's nothing users can do about it.

There is an ExitValue. You can use it instead of exit, then intercept the value in your main module and emit whatever you want. I hope it'll help. Maybe, os.Exit(0) from this module isn't a good idea, and I'm waiting for advice from reviewers.

2. I am wondering that ESC can be a better choice comparing with q because Select have search feature.

readline.CharEsc and readline.CharEscapeEx don't work on my Linux laptop at all... You could try to redefine promptui.SelectKeys in your main module like this:

prompt := promptui.Select{
        Label:        "Select an item (or press \"Esc\" for exit)",
        Items:        items,
        Templates:    &templates,
        Size:         20,
        Searcher:     searcher,
        HideSelected: true,
        Keys: &promptui.SelectKeys{
            Next: promptui.Key{
                Code:    readline.CharNext,
                Display: "↓",
            },
            Prev: promptui.Key{
                Code:    readline.CharPrev,
                Display: "↑",
            },
            PageUp: promptui.Key{
                Code:    readline.CharForward,
                Display: "→",
            },
            PageDown: promptui.Key{
                Code:    readline.CharBackward,
                Display: "←",
            },
            Search: promptui.Key{
                Code:    '/',
                Display: "/",
            },
            Exit: promptui.Key{
                Code:    readline.CharEsc,
                Display: "Esc",
            },
        },
    }
blast-hardcheese commented 3 years ago

Happy early first birthday for this PR!

HurSungYun commented 3 years ago

@LuciferInLove Thanks for replying!

1.

There is an ExitValue. You can use it instead of exit, then intercept the value in your main module and emit whatever you want. I hope it'll help. Maybe, os.Exit(0) from this module isn't a good idea, and I'm waiting for advice from reviewers.

As you said, ExitValue would be helpful if user want to intercept cancel signal, but I believe it could be a better interface to return some kind of error (ErrSelectionQuitted) instead of ExitValue because ExitValue shares same parameter with normal results.

I think waiting for advice from reviewers is a good idea as well :)

2.

readline.CharEsc and readline.CharEscapeEx don't work on my Linux laptop at all... You could try to redefine promptui.SelectKeys in your main module like this:

Oh, I didn't consider that there are keyboards w/o escape key. I believe there are plenty of people having keyboard w/o escape key, so q as default value might be better :) thank you for letting me know.

1xyz commented 3 years ago

Hi Folks; Not sure if this project is active and/or maintained (over a year. This PR for example is super useful, but has been sitting here. I ended up forking the project and implementing a similar thing, before I stumbled upon this.

Any ideas why PR velocity is slow. Thanks folks!

jroper commented 2 years ago

@jbowes You seem to be the only person maintaining this project at the moment, is this an orphaned project that would better off be forked so that popular PRs like this that have been waiting for over a year can be merged, or is the project going to be maintained?

rr-nick-tan commented 2 years ago

this is a super useful feature which can improve the user experience a lot, hope it can be merged soon.

rr-nick-tan commented 2 years ago

@jbowes , second to @jroper 's question, is this project still maintained? otherwise, can you add some collaborators to keep this project going? or shall we just fork it?

HurSungYun commented 2 years ago

I hope this project not be abandoned as well. 🙏 This project is really useful and helps lots of other CLI projects.