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

Apply "moveToCancel" preference to all movement commands. #955

Merged
merged 3 commits into from Feb 28, 2018

Conversation

Projects
None yet
4 participants
@phyllisstein
Contributor

phyllisstein commented Feb 16, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Following #838, this PR updates the movement hooks for Page Up/Down and move-to-top/-bottom such that when the moveToCancel preference is enabled, the suggestion list is dismissed and the movement command is applied to the editor.

Here's a quick screencast showing the changes in action:

screenflow

Alternate Designs

N/A

Benefits

The principle behind the moveToCancel preference was to emulate the behavior of Sublime Text's autocompletion module. Dismissing the suggestion list when the user attempts to navigate "above" or "below" it was one part of that, but my first pass missed the edge cases where the user might be attempting to move to the top or bottom of the editor or to its next or previous screen. One justification for adding this behavior is therefore to conform more exactly with ST3, which dismisses the suggestion list in these cases; another is that it seems like a reasonable extension of the user expectation that movement commands should be applied to her editor primarily and to her suggestion list as an afterthought.

Possible Drawbacks

It's possible that I'm misjudging user expectations, in which case modifying the behavior could result in a frustrating tendency to inadvertently dismiss Autocomplete Plus' suggestions.

Applicable Issues

N/A

@iolsen

This comment has been minimized.

iolsen commented Feb 21, 2018

Thanks for the contribution, @phyllisstein. We're philosophically cool with this change but it needs tests first.

@phyllisstein

This comment has been minimized.

Contributor

phyllisstein commented Feb 23, 2018

Done! Let me know if the tests need any refining.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Feb 23, 2018

You have some linting errors.

@phyllisstein

This comment has been minimized.

Contributor

phyllisstein commented Feb 23, 2018

D'oh, butterfingers. Thanks for flagging that---should be cleaned up now.

@daviwil

This comment has been minimized.

Member

daviwil commented Feb 28, 2018

Thanks @phyllisstein!

@daviwil daviwil merged commit d6558d4 into atom:master Feb 28, 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