Add key binding for Delete (save Insert, PgUp, PgDn for later)

[sshock]

This fixes the delete key (delete on pc keyboard; fn+delete on mac keyboard) to work on macOS in Terminal and iTerm2.

It also fixes the insert, pgup, and pgdn keys to no longer spew out ugly escape codes. (do these later in a separate MR)

Helps with issue ruby/irb#330.

[dirkjonker]

Can confirm that this fixes the following issue I have with the delete key:

• it inputs ^[[3~ instead of deleting a character

My situation:

• I'm on macOS 12.2.1
• with an external keyboard (Logitech MX keys)
• issue occurs with both the default Terminal.app and with Kitty terminal

Edit: can confirm that this fixes the other keys mentioned (pgup/pgdn) as well.

[nobu]

Could you add tests?

[sshock]

Could you add tests?

I'm not used to Test::Unit, only rspec, but I'll see what I can do.

I think the tricky part is that in order to test these key bindings that works regardless of the current terminfo of whoever is running the test, I would need to stub out Reline::Terminfo.tigetstr with a double, and I don't know if Test::Unit supports stubs / mocks / doubles.

[sshock]

I spent some time on this today, but I got stuck for a while due to ncurses library not even getting loaded, so Reline::Terminfo.enabled? was false and as a result test_terminfo.rb wasn't even doing anything.

(It's because of some Linux distros using linker scripts in .so files and the workaround got reverted because there is a fix to fiddle itself now, but ruby still needs to sync / pull in the latest fiddle code to get this fix.)

Anyways, so after stumbling on that for a while, I noticed that there are exactly 0 tests for Reline::ANSI, and in fact most tests call Reline.send(:test_mode), which forces Reline::IOGate to be Reline::GeneralIO, presumably so they don't have to deal with any special ANSI stuff getting in the way.

[sshock]

Ok, I think I know how to best test this now.

At first I thought I would add a parameter to test_mode that would allow it to set the the IOGate to ANSI and then proceed to use the input_keys test method followed by assert_line and assert_cursor like the key actor tests do, but this actually won't work because the keys don't get expanded when using input_keys.

But that's overkill anyways, so next I thought I would add some tests to test_key_stroke.rb, using simple tests just like test_expand but with the config key bindings set up by calling Reline::ANSI.set_default_key_bindings(config).

That would work, but it's still overkill because it tests both 1. "did the correct key bindings get put into the config?", and 2. "did expand turn them into the proper symbol keys?", and we only want to test 1 not 2 (we already have key stroke tests).

So I think the right approach is to simply add a test_ansi.rb and in there add tests for the ANSI set_default_key_bindings by simply calling it and then testing the config.editing_mode.default_key_bindings hash has what it should in there. I can switch the config.editing_mode to each of the three editing modes to make sure key bindings are correct for all of them.

So yeah, this is gonna be pretty simple. My only question is what to do about the fact that the key bindings are different depending on whether Reline::Terminfo.enabled? is true or false. I'd prefer to test both the true and false scenarios if I can, but I don't know if I can/should just override/redefine that method, because if I did then I also have to figure out how to undo that. So I need to figure out if it's better to maybe change that to return a global variable (ugh) or change
Reline::ANSI somehow to allow that to be passed in, hmm...

So that is where I am stuck now, but I'll take another look maybe next weekend and it's still gonna be easy, at least once I figure out how to make it so I can test both the with and without terminfo scenarios.

[sshock]

@nobu I added tests!

[sshock]

@nobu do you need anything else for this PR?

[st0012]

I can also confirm that it fixes the fn+delete key for me. And the solution seems to be well-covered by tests.
@ima1zumi would you mind giving it a look?

[ima1zumi]

@sshock I would like to run CI. Can you push again?

[ima1zumi]

@sshock
Thank you! Can you please squash into one commit if you have time?

I could confirm this patch works well on:

OS	TERM	Terminal	Ruby	irb
macOS 12.4	xterm-256color	iTerm2 Build 3.4.10	3.2.0-dev(a66a69865d)	HEAD(bede04c)
macOS 12.4	screen-256color	Alacritty 0.10.0-dev (8cda3d14) + tmux 3.2	3.2.0-dev(a66a69865d)	HEAD(bede04c)

If TERM is ANSI or VT100 or VT102, it does not work well.
But perhaps I think it could be another capability issue. We can manage this as a separate issue.

As hasumikin commented at #458 (comment), we don't merge any PR (except emergency ones) before releasing reline 0.3.2 to keep the next Ruby 3.2 stable. I understand the seriousness of this issue. I hope to include it in the next release.

@hasumikin
Could you review this PR when you have time?

[sshock]

@sshock Thank you! Can you please squash into one commit if you have time?

@ima1zumi done!

[hasumikin]

Really sorry for the late confirmation.
I couldn't find any bad effect in the patch on my Windows and Linux.
So you'd be able to merge it at any time @ima1zumi

OSVersion/uname -a	Terminal emulator	Ruby (with the bundled IRB)
Windows 11 Pro (10.0.22623 N/A Build 22623)	PowerShell	ruby 3.2.0 (2022-12-25 revision a528908271) [x64-mingw-ucrt]
Linux DESKTOP-FOEG8S2 5.15.79.1-microsoft-standard-WSL2 ＃1 SMP Wed Nov 23 01:01:46 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux	Windows Terminal	ruby 3.3.0dev (2023-01-10T00:45:50Z master df76c54fc2) [x86_64-linux]
Linux hasumi-latitude3420 5.15.0-56-generic ＃62-Ubuntu SMP Tue Nov 22 19:54:14 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux	Konsole	ruby 3.3.0dev (2023-01-10T04:39:31Z master 89fb61f9a3) [x86_64-linux]

[peterzhu2118]

Hello! It looks like this patch is causing flaky tests in ruby/ruby with failures that look like the following:

  1) Failure:
TestIRB::MeasureTest#test_measure_with_proc [/home/chkbuild/chkbuild/tmp/build/20230110T133007Z/ruby/test/irb/test_cmd.rb:366]:
Expected /\A=> 3\nBLOCK is added\.\n=> nil\naaa\n=> 3\nBLOCK is added.\naaa\n=> nil\nbbb\n=> 3\n=> nil\n=> 3\n/ to match "(Object doesn't support #inspect)\n" +
"=> \n" +
"BLOCK is added.\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"aaa\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"BLOCK is added.\n" +
"aaa\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"bbb\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n".

  2) Failure:
TestIRB::MeasureTest#test_measure_enabled_by_rc_with_custom [/home/chkbuild/chkbuild/tmp/build/20230110T133007Z/ruby/test/irb/test_cmd.rb:295]:
Expected /\Acustom processing time: .+\n=> 3\n=> nil\n=> 3\n/ to match "custom processing time: 0.000138s\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n".

  3) Failure:
TestIRB::MeasureTest#test_measure_with_custom [/home/chkbuild/chkbuild/tmp/build/20230110T133007Z/ruby/test/irb/test_cmd.rb:328]:
Expected /\A=> 3\nCUSTOM is added\.\n=> nil\ncustom processing time: .+\n=> 3\n=> nil\n=> 3\n/ to match "(Object doesn't support #inspect)\n" +
"=> \n" +
"CUSTOM is added.\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"custom processing time: 0.000105s\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n".

  4) Failure:
TestIRB::MeasureTest#test_measure [/home/chkbuild/chkbuild/tmp/build/20230110T133007Z/ruby/test/irb/test_cmd.rb:238]:
Expected /\A=> 3\nTIME is added\.\n=> nil\nprocessing time: .+\n=> 3\n=> nil\n=> 3\n/ to match "(Object doesn't support #inspect)\n" +
"=> \n" +
"TIME is added.\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"processing time: 0.000084s\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n".

  5) Failure:
TestIRB::MeasureTest#test_measure_enabled_by_rc [/home/chkbuild/chkbuild/tmp/build/20230110T133007Z/ruby/test/irb/test_cmd.rb:264]:
Expected /\Aprocessing time: .+\n=> 3\n=> nil\n=> 3\n/ to match "processing time: 0.000116s\n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n" +
"(Object doesn't support #inspect)\n" +
"=> \n".

Here are some example failing builds:

• http://rubyci.s3.amazonaws.com/openbsd-current/ruby-master/log/20230110T133007Z.fail.html.gz
• http://rubyci.s3.amazonaws.com/osx1200arm/ruby-master/log/20230110T125004Z.fail.html.gz
• http://rubyci.s3.amazonaws.com/solaris10-gcc/ruby-master/log/20230110T130004Z.fail.html.gz

Could you take a look at it?

[sshock]

Hello! It looks like this patch is causing flaky tests in ruby/ruby with failures that look like the following:

Maybe it doesn't like the Reline::IOGate getting changed to Reline::ANSI. @ima1zumi I think we should try changing Reline::IOGate back to Reline::GeneralIO in the tests' teardown.

[sshock]

@ima1zumi here is a PR that I think will fix it to no longer cause side effects to other tests in ruby/ruby:

#497