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

Support menu-complete-backward command for upward navigation #677

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

mjgiarlo
Copy link
Contributor

@mjgiarlo mjgiarlo commented Apr 12, 2024

Fixes #675

This commit extracts the upward navigation condition in LineEditor#input_key to a new private method, and adds a new alias. This change allows Reline to support upward navigation in when a user has configured inputrc to map Shift-Tab to menu-complete-backward, a common setting in Bash (>= 4.x).

Instead of special-casing upward navigation in LineEditor#input_key, we now allow it to be processed by the branch that calls process_key. The extracted method no longer includes the editing mode check since this check is already made by #wrap_method_call by the time #completion_journey_up (or #menu_complete_backward) is called. Since upward navigation is happening in a method other than #input_key now, the completion_occurs variable that used to be local to #input_key is changed to an instance variable so that the new method can change its value. (I see many examples of mutating such instance variables in LineEditor, so I assumed this would be an uncontroversial change consistent with the coding practices already in place.)

Test coverage of this change has been added to the emacs and vi KeyActor tests. (They are a bit wordy, so I could try to DRY them up if it matters to y'all.)

Many thanks to @ima1zumi for their very helpful comments on #675 which encouraged me to contribute this work!

Copy link
Member

@ima1zumi ima1zumi left a comment

Choose a reason for hiding this comment

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

The changes generally look good. 👍

Could you add @completion_occurs = false to reset_variables?

@ima1zumi ima1zumi added the enhancement New feature or request label Apr 13, 2024
Fixes ruby#675

This commit extracts the upward navigation condition in `LineEditor#input_key` to a new private method, and adds a new alias. This change allows Reline to support upward navigation in when a user has configured `inputrc` to map Shift-Tab to `menu-complete-backward`, a common setting in Bash (>= 4.x).

Instead of special-casing upward navigation in `LineEditor#input_key`, we now allow it to be processed by the branch that calls `process_key`. The extracted method no longer includes the editing mode check since this check is already made by `#wrap_method_call` by the time `#completion_journey_up` (or `#menu_complete_backward`) is called. Since upward navigation is happening in a method other than `#input_key` now, the `completion_occurs` variable that used to be local to `#input_key` is changed to an instance variable so that the new method can change its value. (I see many examples of mutating such instance variables in `LineEditor`, so I assumed this would be an uncontroversial change consistent with the coding practices already in place.)

Test coverage of this change has been added to the emacs and vi `KeyActor` tests.

Many thanks to @ima1zumi for their very helpful comments on ruby#675 which encouraged me to contribute this work!
@mjgiarlo mjgiarlo force-pushed the support-menu-complete-backward#675 branch from daa9853 to 28b5629 Compare April 13, 2024 19:14
@mjgiarlo
Copy link
Contributor Author

@ima1zumi 💬

The changes generally look good. 👍

👏🏻

Could you add @completion_occurs = false to reset_variables?

Done, thanks!

Copy link
Member

@ima1zumi ima1zumi left a comment

Choose a reason for hiding this comment

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

LGTM 🌟

@ima1zumi ima1zumi merged commit 2ccdb37 into ruby:master Apr 14, 2024
40 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Apr 14, 2024
navigation
(ruby/reline#677)

Fixes ruby/reline#675

This commit extracts the upward navigation condition in `LineEditor#input_key` to a new private method, and adds a new alias. This change allows Reline to support upward navigation in when a user has configured `inputrc` to map Shift-Tab to `menu-complete-backward`, a common setting in Bash (>= 4.x).

Instead of special-casing upward navigation in `LineEditor#input_key`, we now allow it to be processed by the branch that calls `process_key`. The extracted method no longer includes the editing mode check since this check is already made by `#wrap_method_call` by the time `#completion_journey_up` (or `#menu_complete_backward`) is called. Since upward navigation is happening in a method other than `#input_key` now, the `completion_occurs` variable that used to be local to `#input_key` is changed to an instance variable so that the new method can change its value. (I see many examples of mutating such instance variables in `LineEditor`, so I assumed this would be an uncontroversial change consistent with the coding practices already in place.)

Test coverage of this change has been added to the emacs and vi `KeyActor` tests.

Many thanks to @ima1zumi for their very helpful comments on #675 which encouraged me to contribute this work!

ruby/reline@2ccdb374a4
@mjgiarlo mjgiarlo deleted the support-menu-complete-backward#675 branch April 15, 2024 15:06
tenderlove added a commit to tenderlove/ruby that referenced this pull request Apr 15, 2024
* master: (29 commits)
  Not all `nm`s support the `--help` option
  Emit a performance warning when redefining specially optimized methods
  YJIT: A64: Avoid intermediate register in `opt_and` and friends (ruby#10509)
  [ruby/reline] Remove not implemented method (ruby/reline#680)
  [ruby/reline] Fix vi_to_column which was broken (ruby/reline#679)
  Include more debug information in test_uplus_minus
  [Universal parser] DeVALUE of p->debug_lines and ast->body.script_lines
  Add more assertions in `test_uplus_minus`
  `super{}` doesn't use block
  fix incorrect warning.
  Update default gems list at fc36394 [ci skip]
  [ruby/optparse] bump up to 0.5.0
  show warning for unused block
  Emit `warn` event for duplicated hash keys on ripper
  [ruby/reline] Refactored Default Key Bindings (ruby/reline#678)
  [ruby/reline] Refactor waiting_proc and waiting_operator_proc (ruby/reline#649)
  [pty] Fix missing `or`
  [pty] Fix `ptsname_r` fallback
  [ruby/irb] Allow defining custom commands in IRB (ruby/irb#886)
  [ruby/reline] Support `menu-complete-backward` command for upward navigation (ruby/reline#677)
  ...
artur-intech pushed a commit to artur-intech/ruby that referenced this pull request Apr 26, 2024
navigation
(ruby/reline#677)

Fixes ruby/reline#675

This commit extracts the upward navigation condition in `LineEditor#input_key` to a new private method, and adds a new alias. This change allows Reline to support upward navigation in when a user has configured `inputrc` to map Shift-Tab to `menu-complete-backward`, a common setting in Bash (>= 4.x).

Instead of special-casing upward navigation in `LineEditor#input_key`, we now allow it to be processed by the branch that calls `process_key`. The extracted method no longer includes the editing mode check since this check is already made by `#wrap_method_call` by the time `#completion_journey_up` (or `#menu_complete_backward`) is called. Since upward navigation is happening in a method other than `#input_key` now, the `completion_occurs` variable that used to be local to `#input_key` is changed to an instance variable so that the new method can change its value. (I see many examples of mutating such instance variables in `LineEditor`, so I assumed this would be an uncontroversial change consistent with the coding practices already in place.)

Test coverage of this change has been added to the emacs and vi `KeyActor` tests.

Many thanks to @ima1zumi for their very helpful comments on ruby#675 which encouraged me to contribute this work!

ruby/reline@2ccdb374a4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Upward navigation through completion results not working as expected
2 participants