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 "none" option to trigger keys #987

Merged
merged 3 commits into from Oct 1, 2018

Conversation

Projects
None yet
3 participants
@Aerijo
Contributor

Aerijo commented Aug 18, 2018

Description of the Change

This adds another option to the keybindings, none, which prevents any keys being set by the package.

This lets the user specify shortcuts themselves, without quirky behaviour with tab and enter; when using "enter": "unset!" I cannot make a new line with the enter key.

(Note: the change itself just adds an option that won't be picked up by the keybindings construction logic)

Alternate Designs

The setting could be turned into "custom", and add another string entry for the custom key command. This was not done, because supporting the only-if-selected behaviour would have been difficult.

Alternatively, we could default to the regular always confirm there, and ignore the setting if blank (indicating the user has set their own bindings)

Benefits

The enter and tab keys work when a completion pops up.

Possible Drawbacks

It can be confusing what "none" is for. This can be solved with documentation, or expanding the name to something like

none (use custom user bindings)
@daviwil

This comment has been minimized.

Member

daviwil commented Aug 28, 2018

Hey @Aerijo, this looks good! I think you're right, the setting needs a little bit more clarification and possibly some documentation so that users know what to expect. Do you mind adding that? Afterward you should feel free to merge this PR

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Sep 25, 2018

@daviwil Can you take another look?

@daviwil

This comment has been minimized.

Member

daviwil commented Oct 1, 2018

Sorry for the delay, looks great, thanks @Aerijo!

@daviwil daviwil merged commit db9265c into atom:master Oct 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment