Use em_delete in key_delete
#504
Conversation
lib/reline/line_editor.rb
Outdated
| @@ -2664,7 +2666,7 @@ def finish | |||
| alias_method :kill_whole_line, :em_kill_line | |||
|
|
|||
| private def em_delete(key) | |||
| if (not @is_multiline and @line.empty?) or (@is_multiline and @line.empty? and @buffer_of_lines.size == 1) | |||
| if @line.empty? and (not @is_multiline or @buffer_of_lines.size == 1) and key == 4 | |||
There was a problem hiding this comment.
We could change 4 to "\C-D".ord for more clarity.
There was a problem hiding this comment.
Good idea, thanks! Also rebased the branch.
etiennebarrie
force-pushed
the
use-em_delete-in-key_delete
branch
from
f4857c8
to
3b93464
Compare
| ed_delete_next_char(key) | ||
| elsif @config.editing_mode_is?(:emacs) | ||
| em_delete(key) |
There was a problem hiding this comment.
Could you add a test for this?
I think this part is trying to test key_delete but missing the scenario you reported
reline/test/reline/test_key_actor_emacs.rb
Lines 452 to 460 in 3aeea2a
There was a problem hiding this comment.
Thank you for pointing the tests. I've added a test in the third commit (making this change), that checks that the cursor is preserved. I also renamed this test in the first commit to clarify it's testing key_delete.
lib/reline/line_editor.rb
Outdated
| @@ -2651,7 +2653,7 @@ def finish | |||
| alias_method :kill_whole_line, :em_kill_line | |||
|
|
|||
| private def em_delete(key) | |||
| if (not @is_multiline and @line.empty?) or (@is_multiline and @line.empty? and @buffer_of_lines.size == 1) | |||
| if @line.empty? and (not @is_multiline or @buffer_of_lines.size == 1) and key == "\C-D" | |||
There was a problem hiding this comment.
"\C-d".ord(lowercase d) is better because lowercase is used in this file.
Could you add a test to check that \C-d with empty line will end the input?
I think we can use assert_line(nil)
There was a problem hiding this comment.
Thank you for pointing that out, I found a test in test/reline/test_key_actor_vi.rb that was testing the same case: \C-d with an empty line ends the input, so I added it to test/reline/test_key_actor_emacs.rb as well.
Typing Ctrl-D ends editing but typing <Del> does not. Also renamed a test that is not testing ed_delete_next_char but key_delete.
By distributivity of AND over OR, we can factor out this condition. This will make the next commit simpler.
When the editing mode is emacs, use `em_delete` in `key_delete`. We need to add a condition though to `em_delete`, because it implements both `delete-char` and `end-of-file`. We only want the `end-of-file` behavior is the key is really Ctrl-D. This matches the behavior of the <Del> key with readline, i.e. deleting the next character if there is one, but not moving the cursor, while not finishing the editing if there are no characters.
etiennebarrie
force-pushed
the
use-em_delete-in-key_delete
branch
from
3b93464
to
c0d4086
Compare
|
I added some tests. As a separate first commit, tests that were testing the existing behavior that we want to preserve, namely Ctrl-D ends editing with an empty line, but that Del does not. Then there's the small refactor, then for the last commit changing the behavior, I added a test checking the state of the cursor to demonstrate the readline behavior for Del. That test fails when it runs on main. |
|
Thank you for your contribution 👍 🎉 |
Since #434, the Del key is bound to
:key_delete, but:key_deletedoesn't behave like readline does in emacs mode. It moves the cursor left if a character was deleted under the cursor, while in readline, it just deletes the character under the cursor.key_deleteem_deleteabc|abc|ab|ca|bab|ab|c|aab|:em_deleteis almost Del like readline, except it also handles the end-of-file situation. Which is annoying because if you type Del on an empty line, you get EOF instead of no changes.This fixes it by using
:em_deletein emacs mode, but also tweaking the behavior of:em_deleteto only handle EOF when the key received is Ctrl-D. I hardcoded this but we could revisit that, see #501.