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

Implement changing editing mode #681

Merged
merged 1 commit into from Apr 16, 2024

Conversation

ima1zumi
Copy link
Member

@ima1zumi ima1zumi commented Apr 15, 2024

I implemented Readline’s emacs-editing-mode (C-e) and vi-editing-mode (M-C-j, M-C-m) to switch editing modes. The keymap was aligned with Readline's behavior. But M-C-m was used in the yamatanooroti tests, so I decided not to map it.

@ima1zumi ima1zumi added the enhancement New feature or request label Apr 15, 2024
@ima1zumi ima1zumi force-pushed the implement-change-editing-mode branch from 5d55556 to 2a75980 Compare April 15, 2024 14:43
@st0012
Copy link
Member

st0012 commented Apr 15, 2024

Since both mappings are currently used for other operations, this feels like a breaking change.
Do we know if people use the current mappings and did we receive request for the new behaviour?

@ima1zumi
Copy link
Member Author

The original key mapping is different from Readline, so I believe few people use it.
On second thoughts, not many people, other than maintainers, want to change between Vi mode and Emacs mode using the default key bindings. I think I should stop binding them by default. Thank you for your comment!

@ima1zumi ima1zumi marked this pull request as draft April 15, 2024 15:55
@tompng
Copy link
Member

tompng commented Apr 15, 2024

Just for information, GNU Bash source code comment also says it's confusing.

  /* In Bash, the user can switch editing modes with "set -o [vi emacs]",
     so it is not necessary to allow C-M-j for context switching.  Turn
     off this occasionally confusing behaviour. */
  ...
  if (func == rl_vi_editing_mode)
    rl_unbind_key_in_map (CTRL('J'), emacs_meta_keymap);
  ...
  if (func == rl_vi_editing_mode)
    rl_unbind_key_in_map (CTRL('M'), emacs_meta_keymap);
  ...
  if (func == rl_emacs_editing_mode)
    rl_unbind_key_in_map (CTRL('E'), vi_movement_keymap);

@ima1zumi ima1zumi force-pushed the implement-change-editing-mode branch from 2a75980 to 92ef148 Compare April 15, 2024 16:52
@@ -1436,4 +1436,9 @@ def test_unix_line_discard
input_keys("\C-f\C-u", false)
assert_line_around_cursor('', '')
end

def test_vi_editing_mode
@line_editor.__send__(:vi_editing_mode, nil)
Copy link
Member Author

Choose a reason for hiding this comment

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

When key binding is set, key.char is passed as symbol to LineEditor#update.
I can't seem to test it properly with input_keys. If you have a better way to do this, please let me know.

@ima1zumi ima1zumi marked this pull request as ready for review April 15, 2024 16:56
@ima1zumi ima1zumi merged commit 501b9a6 into ruby:master Apr 16, 2024
40 checks passed
@ima1zumi ima1zumi deleted the implement-change-editing-mode branch April 16, 2024 11:58
artur-intech pushed a commit to artur-intech/ruby that referenced this pull request Apr 26, 2024
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.

None yet

3 participants