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

Adding key for quit in SelectKeys #155

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

Conversation

LuciferInLove
Copy link

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

@CLAassistant
Copy link

CLAassistant commented Jul 2, 2020

CLA assistant check
All committers have signed the CLA.

Copy link

@blast-hardcheese blast-hardcheese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to emit a signal that can be caught by the calling code, as a sentinel value from Run() in case there are multiple levels of promptui.

This PR does satisfy my needs as is though, but I can easily see random os.Exit(0) (hard-coded 0, uncatchable Exit) being undesirable without overrides.

Perhaps even just a configurable function that handles the Keys.Exit.Code, default Exit(0), optionally returning a sentinel value if the user doesn't want to exit.

@LuciferInLove
Copy link
Author

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

Copy link

@blast-hardcheese blast-hardcheese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worked as expected, thank you!

@blast-hardcheese
Copy link

what's blocking this from getting merged?

@HurSungYun
Copy link

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
Copy link
Author

@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
Copy link

Happy early first birthday for this PR!

@HurSungYun
Copy link

HurSungYun commented Jun 30, 2021

@LuciferInLove Thanks for replying!

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 :)

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
Copy link

1xyz commented Jun 30, 2021

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
Copy link

jroper commented Nov 11, 2021

@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
Copy link

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

@rr-nick-tan
Copy link

@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?

rr-nick-tan added a commit to rewards-guilds/promptui that referenced this pull request Aug 4, 2022
@HurSungYun
Copy link

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

Copy link

@asudarsanan asudarsanan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGT - Can we merge this!

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.

8 participants