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

feat: add custom_key_binding argument to add custom keyboard handlings #282

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

Conversation

baturayo
Copy link
Contributor

What is the problem that this PR addresses?
Users were not able to customize keyboard inputs. Thanks to this PR, we can now customize the keyboard input as shown below thanks to new custom_key_binding argument. For now, custom_key_binding field has been added for select, text, checkbox, and confirm.

We will add a custom key handling for the s button in this example. Whenever user press s, we will return _skipped_ string.custom_key_binding argument takes a dictionary where the key is either a string or prompt_toolkit.keys.Keys object the value is a callable that accepts a prompt_toolkit.key_binding.KeyBinding event.

questionary.select(
    "What do you want to do?",
    choices=["Order a pizza", "Make a reservation", "Ask for opening hours"],
    instruction="(<s> to skip the question)",
    custom_key_binding= {
            "s": lambda event: event.app.exit(result="_skipped_")
        }
).ask()

or the same example can be written as

def custom_handler(event):
     return event.app.exit(result="_skipped_")

questionary.select(
    "What do you want to do?",
    choices=["Order a pizza", "Make a reservation", "Ask for opening hours"],
    instruction="(<s> to skip the question)",
    custom_key_binding= {
            "s": custom_handler
        }
).ask()

Checklist

  • I have read the Contributor's Guide.
  • I will check that all automated PR checks pass before the PR gets reviewed.

Copy link
Collaborator

@kiancross kiancross left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I've done a quick pass of code changes and made some suggestions. I think you've disabled the option for reviewers to apply changes themselves, so can you look over these changes and then apply them? Once you've done that, I'll do a more comprehensive review.

FYI, I might have missed changing custom_key_binding to custom_key_bindings in some places - please fix if I have!

questionary/prompts/checkbox.py Outdated Show resolved Hide resolved
questionary/prompts/checkbox.py Outdated Show resolved Hide resolved
questionary/prompts/confirm.py Outdated Show resolved Hide resolved
questionary/prompts/confirm.py Outdated Show resolved Hide resolved
questionary/prompts/confirm.py Outdated Show resolved Hide resolved
questionary/prompts/text.py Outdated Show resolved Hide resolved
tests/prompts/test_checkbox.py Outdated Show resolved Hide resolved
tests/prompts/test_confirm.py Outdated Show resolved Hide resolved
tests/prompts/test_select.py Outdated Show resolved Hide resolved
tests/prompts/test_text.py Outdated Show resolved Hide resolved
baturayo and others added 14 commits June 30, 2023 15:46
@baturayo
Copy link
Contributor Author

Thanks for your review @kiancross, I fixed your feedbacks :)

Copy link
Collaborator

@kiancross kiancross left a comment

Choose a reason for hiding this comment

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

I wonder if a better interface might be

{"c": lambda: "custom"}

or even just

{"c":  "custom"}

The other code would be modified to something like this

    if custom_key_bindings is not None:
        for key, func in custom_key_bindings.items():
            bindings.add(key, eager=True)(lamba event: event.app.exit(result=func()))
            # or, if `func` is just a string
            bindings.add(key, eager=True)(lamba event: event.app.exit(result=func))

The reason for this is that I don't think we expose event.app anywhere else, and in most (all?) places, we just return a value, which is handled by internal code, rather than requiring users to call event.app.exit explicitly.

What do you think?

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.

2 participants