Skip to content

Conversation

LuciferInLove
Copy link

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

@CLAassistant
Copy link

CLAassistant commented Jul 2, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


LuciferInLove seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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!

@EdanBrooke
Copy link

Is this dead?

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.

9 participants