[ruby/reline] Improve key binding match/matching check

(https://github.com/ruby/reline/pull/709)

* Improve key binding match/matching check

* Rename key_actors to default_key_bindings

* Make key_stroke.expand always return a value

* Update add_default_key_binding to use a add_default_key_binding_by_keymap internally

Co-authored-by: Stan Lo <stan001212@gmail.com>

---------

https://github.com/ruby/reline/commit/353ec236e2

Co-authored-by: Stan Lo <stan001212@gmail.com>

7 files changed, 89 insertions, 116 deletions

diff --git a/test/reline/helper.rb b/test/reline/helper.rb
index a5f850e838..9a346a17a1 100644
--- a/test/reline/helper.rb
+++ b/test/reline/helper.rb
@@ -168,7 +168,7 @@ class Reline::TestCase < Test::Unit::TestCase
   def assert_key_binding(input, method_symbol, editing_modes = [:emacs, :vi_insert, :vi_command])
     editing_modes.each do |editing_mode|
       @config.editing_mode = editing_mode
-      assert_equal(method_symbol, @config.editing_mode.default_key_bindings[input.bytes])
+      assert_equal(method_symbol, @config.editing_mode.get(input.bytes))
     end
   end
 end
diff --git a/test/reline/test_config.rb b/test/reline/test_config.rb
index 8c4b513600..16727c9bc9 100644
--- a/test/reline/test_config.rb
+++ b/test/reline/test_config.rb
@@ -22,6 +22,15 @@ class Reline::Config::Test < Reline::TestCase
     @config.reset
   end
 
+  def additional_key_bindings(keymap_label)
+    @config.instance_variable_get(:@additional_key_bindings)[keymap_label].instance_variable_get(:@key_bindings)
+  end
+
+  def registered_key_bindings(keys)
+    key_bindings = @config.key_bindings
+    keys.to_h { |key| [key, key_bindings.get(key)] }
+  end
+
   def test_read_lines
     @config.read_lines(<<~LINES.lines)
       set bell-style on
@@ -97,14 +106,17 @@ class Reline::Config::Test < Reline::TestCase
     assert_equal nil, @config.convert_meta
   end
 
-  def test_comment_line
-    @config.read_lines([" #a: error\n"])
-    assert_not_include @config.key_bindings, nil
-  end
-
   def test_invalid_keystroke
-    @config.read_lines(["a: error\n"])
-    assert_not_include @config.key_bindings, nil
+    @config.read_lines(<<~LINES.lines)
+      #"a": comment
+      a: error
+      "b": no-error
+    LINES
+    key_bindings = additional_key_bindings(:emacs)
+    assert_not_include key_bindings, 'a'.bytes
+    assert_not_include key_bindings, nil
+    assert_not_include key_bindings, []
+    assert_include key_bindings, 'b'.bytes
   end
 
   def test_bind_key
@@ -242,8 +254,8 @@ class Reline::Config::Test < Reline::TestCase
       "\xC": "O"
     LINES
     keys = [0x1, 0x3, 0x4, 0x6, 0x7, 0xC]
-    key_bindings = keys.to_h { |k| [[k.ord], ['O'.ord]] }
-    assert_equal(key_bindings, @config.instance_variable_get(:@additional_key_bindings)[:emacs])
+    key_bindings = keys.to_h { |k| [[k], ['O'.ord]] }
+    assert_equal(key_bindings, additional_key_bindings(:emacs))
   end
 
   def test_unclosed_if
@@ -282,9 +294,9 @@ class Reline::Config::Test < Reline::TestCase
       $endif
     LINES
 
-    assert_equal({[5] => :history_search_backward}, @config.instance_variable_get(:@additional_key_bindings)[:emacs])
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:vi_insert])
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:vi_command])
+    assert_equal({[5] => :history_search_backward}, additional_key_bindings(:emacs))
+    assert_equal({}, additional_key_bindings(:vi_insert))
+    assert_equal({}, additional_key_bindings(:vi_command))
   end
 
   def test_else
@@ -296,9 +308,9 @@ class Reline::Config::Test < Reline::TestCase
       $endif
     LINES
 
-    assert_equal({[6] => :history_search_forward}, @config.instance_variable_get(:@additional_key_bindings)[:emacs])
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:vi_insert])
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:vi_command])
+    assert_equal({[6] => :history_search_forward}, additional_key_bindings(:emacs))
+    assert_equal({}, additional_key_bindings(:vi_insert))
+    assert_equal({}, additional_key_bindings(:vi_command))
   end
 
   def test_if_with_invalid_mode
@@ -310,9 +322,9 @@ class Reline::Config::Test < Reline::TestCase
       $endif
     LINES
 
-    assert_equal({[6] => :history_search_forward}, @config.instance_variable_get(:@additional_key_bindings)[:emacs])
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:vi_insert])
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:vi_command])
+    assert_equal({[6] => :history_search_forward}, additional_key_bindings(:emacs))
+    assert_equal({}, additional_key_bindings(:vi_insert))
+    assert_equal({}, additional_key_bindings(:vi_command))
   end
 
   def test_mode_label_differs_from_keymap_label
@@ -327,9 +339,9 @@ class Reline::Config::Test < Reline::TestCase
         "\C-e": history-search-backward
       $endif
     LINES
-    assert_equal({[5] => :history_search_backward}, @config.instance_variable_get(:@additional_key_bindings)[:emacs])
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:vi_insert])
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:vi_command])
+    assert_equal({[5] => :history_search_backward}, additional_key_bindings(:emacs))
+    assert_equal({}, additional_key_bindings(:vi_insert))
+    assert_equal({}, additional_key_bindings(:vi_command))
   end
 
   def test_if_without_else_condition
@@ -340,9 +352,9 @@ class Reline::Config::Test < Reline::TestCase
       $endif
     LINES
 
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:emacs])
-    assert_equal({[5] => :history_search_backward}, @config.instance_variable_get(:@additional_key_bindings)[:vi_insert])
-    assert_equal({}, @config.instance_variable_get(:@additional_key_bindings)[:vi_command])
+    assert_equal({}, additional_key_bindings(:emacs))
+    assert_equal({[5] => :history_search_backward}, additional_key_bindings(:vi_insert))
+    assert_equal({}, additional_key_bindings(:vi_command))
   end
 
   def test_default_key_bindings
@@ -353,7 +365,7 @@ class Reline::Config::Test < Reline::TestCase
     LINES
 
     expected = { 'abcd'.bytes => 'ABCD'.bytes, 'ijkl'.bytes => 'IJKL'.bytes }
-    assert_equal expected, @config.key_bindings
+    assert_equal expected, registered_key_bindings(expected.keys)
   end
 
   def test_additional_key_bindings
@@ -363,7 +375,7 @@ class Reline::Config::Test < Reline::TestCase
     LINES
 
     expected = { 'ef'.bytes => 'EF'.bytes, 'gh'.bytes => 'GH'.bytes }
-    assert_equal expected, @config.key_bindings
+    assert_equal expected, registered_key_bindings(expected.keys)
   end
 
   def test_additional_key_bindings_with_nesting_and_comment_out
@@ -375,7 +387,7 @@ class Reline::Config::Test < Reline::TestCase
     LINES
 
     expected = { 'ef'.bytes => 'EF'.bytes, 'gh'.bytes => 'GH'.bytes }
-    assert_equal expected, @config.key_bindings
+    assert_equal expected, registered_key_bindings(expected.keys)
   end
 
   def test_additional_key_bindings_for_other_keymap
@@ -390,7 +402,7 @@ class Reline::Config::Test < Reline::TestCase
     LINES
 
     expected = { 'cd'.bytes => 'CD'.bytes }
-    assert_equal expected, @config.key_bindings
+    assert_equal expected, registered_key_bindings(expected.keys)
   end
 
   def test_additional_key_bindings_for_auxiliary_emacs_keymaps
@@ -412,7 +424,7 @@ class Reline::Config::Test < Reline::TestCase
       "\C-xef".bytes => 'EF'.bytes,
       "\egh".bytes => 'GH'.bytes,
     }
-    assert_equal expected, @config.key_bindings
+    assert_equal expected, registered_key_bindings(expected.keys)
   end
 
   def test_key_bindings_with_reset
@@ -424,7 +436,7 @@ class Reline::Config::Test < Reline::TestCase
     LINES
     @config.reset
     expected = { 'default'.bytes => 'DEFAULT'.bytes, 'additional'.bytes => 'ADDITIONAL'.bytes }
-    assert_equal expected, @config.key_bindings
+    assert_equal expected, registered_key_bindings(expected.keys)
   end
 
   def test_history_size
diff --git a/test/reline/test_key_actor_emacs.rb b/test/reline/test_key_actor_emacs.rb
index a2ea060ab9..36fbfd4040 100644
--- a/test/reline/test_key_actor_emacs.rb
+++ b/test/reline/test_key_actor_emacs.rb
@@ -1,6 +1,6 @@
 require_relative 'helper'
 
-class Reline::KeyActor::Emacs::Test < Reline::TestCase
+class Reline::KeyActor::EmacsTest < Reline::TestCase
   def setup
     Reline.send(:test_mode)
     @prompt = '> '
diff --git a/test/reline/test_key_actor_vi.rb b/test/reline/test_key_actor_vi.rb
index 4deae2dd83..1288b9aaa5 100644
--- a/test/reline/test_key_actor_vi.rb
+++ b/test/reline/test_key_actor_vi.rb
@@ -1,6 +1,6 @@
 require_relative 'helper'
 
-class Reline::KeyActor::ViInsert::Test < Reline::TestCase
+class Reline::ViInsertTest < Reline::TestCase
   def setup
     Reline.send(:test_mode)
     @prompt = '> '
@@ -13,69 +13,73 @@ class Reline::KeyActor::ViInsert::Test < Reline::TestCase
     @line_editor.reset(@prompt, encoding: @encoding)
   end
 
+  def editing_mode_label
+    @config.instance_variable_get(:@editing_mode_label)
+  end
+
   def teardown
     Reline.test_reset
   end
 
   def test_vi_command_mode
     input_keys("\C-[")
-    assert_instance_of(Reline::KeyActor::ViCommand, @config.editing_mode)
+    assert_equal(:vi_command, editing_mode_label)
   end
 
   def test_vi_command_mode_with_input
     input_keys("abc\C-[")
-    assert_instance_of(Reline::KeyActor::ViCommand, @config.editing_mode)
+    assert_equal(:vi_command, editing_mode_label)
     assert_line_around_cursor('ab', 'c')
   end
 
   def test_vi_insert
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
     input_keys('i')
     assert_line_around_cursor('i', '')
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
     input_keys("\C-[")
     assert_line_around_cursor('', 'i')
-    assert_instance_of(Reline::KeyActor::ViCommand, @config.editing_mode)
+    assert_equal(:vi_command, editing_mode_label)
     input_keys('i')
     assert_line_around_cursor('', 'i')
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
   end
 
   def test_vi_add
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
     input_keys('a')
     assert_line_around_cursor('a', '')
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
     input_keys("\C-[")
     assert_line_around_cursor('', 'a')
-    assert_instance_of(Reline::KeyActor::ViCommand, @config.editing_mode)
+    assert_equal(:vi_command, editing_mode_label)
     input_keys('a')
     assert_line_around_cursor('a', '')
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
   end
 
   def test_vi_insert_at_bol
     input_keys('I')
     assert_line_around_cursor('I', '')
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
     input_keys("12345\C-[hh")
     assert_line_around_cursor('I12', '345')
-    assert_instance_of(Reline::KeyActor::ViCommand, @config.editing_mode)
+    assert_equal(:vi_command, editing_mode_label)
     input_keys('I')
     assert_line_around_cursor('', 'I12345')
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
   end
 
   def test_vi_add_at_eol
     input_keys('A')
     assert_line_around_cursor('A', '')
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
     input_keys("12345\C-[hh")
     assert_line_around_cursor('A12', '345')
-    assert_instance_of(Reline::KeyActor::ViCommand, @config.editing_mode)
+    assert_equal(:vi_command, editing_mode_label)
     input_keys('A')
     assert_line_around_cursor('A12345', '')
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
   end
 
   def test_ed_insert_one
@@ -901,11 +905,11 @@ class Reline::KeyActor::ViInsert::Test < Reline::TestCase
     assert_line_around_cursor('abc', '')
     input_keys("\C-[0C")
     assert_line_around_cursor('', '')
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
   end
 
   def test_vi_motion_operators
-    assert_instance_of(Reline::KeyActor::ViInsert, @config.editing_mode)
+    assert_equal(:vi_insert, editing_mode_label)
 
     assert_nothing_raised do
       input_keys("test = { foo: bar }\C-[BBBldt}b")
diff --git a/test/reline/test_key_stroke.rb b/test/reline/test_key_stroke.rb
index cd205c7d9e..de67c7ad44 100644
--- a/test/reline/test_key_stroke.rb
+++ b/test/reline/test_key_stroke.rb
@@ -27,13 +27,11 @@ class Reline::KeyStroke::Test < Reline::TestCase
     assert_equal(:matching, stroke.match_status("a".bytes))
     assert_equal(:matching, stroke.match_status("ab".bytes))
     assert_equal(:matched, stroke.match_status("abc".bytes))
-    assert_equal(:matched, stroke.match_status("abz".bytes))
-    assert_equal(:matched, stroke.match_status("abx".bytes))
-    assert_equal(:matched, stroke.match_status("ac".bytes))
-    assert_equal(:matched, stroke.match_status("aa".bytes))
+    assert_equal(:unmatched, stroke.match_status("abz".bytes))
+    assert_equal(:unmatched, stroke.match_status("abcx".bytes))
+    assert_equal(:unmatched, stroke.match_status("aa".bytes))
     assert_equal(:matched, stroke.match_status("x".bytes))
-    assert_equal(:unmatched, stroke.match_status("m".bytes))
-    assert_equal(:matched, stroke.match_status("abzwabk".bytes))
+    assert_equal(:unmatched, stroke.match_status("xa".bytes))
   end
 
   def test_match_unknown
@@ -47,10 +45,13 @@ class Reline::KeyStroke::Test < Reline::TestCase
       "\e[1;1R", # Cursor position report
       "\e[15~", # F5
       "\eOP", # F1
-      "\e\e[A" # Option+Up
+      "\e\e[A", # Option+Up
+      "\eX",
+      "\e\eX"
     ]
     sequences.each do |seq|
       assert_equal(:matched, stroke.match_status(seq.bytes))
+      assert_equal(:unmatched, stroke.match_status(seq.bytes + [32]))
       (1...seq.size).each do |i|
         assert_equal(:matching, stroke.match_status(seq.bytes.take(i)))
       end
@@ -61,16 +62,18 @@ class Reline::KeyStroke::Test < Reline::TestCase
     config = Reline::Config.new
     {
       'abc' => '123',
+      'ab' => '456'
     }.each_pair do |key, func|
       config.add_default_key_binding(key.bytes, func.bytes)
     end
     stroke = Reline::KeyStroke.new(config)
-    assert_equal('123'.bytes, stroke.expand('abc'.bytes))
+    assert_equal(['123'.bytes.map { |c| Reline::Key.new(c, c, false) }, 'de'.bytes], stroke.expand('abcde'.bytes))
+    assert_equal(['456'.bytes.map { |c| Reline::Key.new(c, c, false) }, 'de'.bytes], stroke.expand('abde'.bytes))
     # CSI sequence
-    assert_equal([:ed_unassigned] + 'bc'.bytes, stroke.expand("\e[1;2;3;4;5abc".bytes))
-    assert_equal([:ed_unassigned] + 'BC'.bytes, stroke.expand("\e\e[ABC".bytes))
+    assert_equal([[], 'bc'.bytes], stroke.expand("\e[1;2;3;4;5abc".bytes))
+    assert_equal([[], 'BC'.bytes], stroke.expand("\e\e[ABC".bytes))
     # SS3 sequence
-    assert_equal([:ed_unassigned] + 'QR'.bytes, stroke.expand("\eOPQR".bytes))
+    assert_equal([[], 'QR'.bytes], stroke.expand("\eOPQR".bytes))
   end
 
   def test_oneshot_key_bindings
@@ -88,17 +91,14 @@ class Reline::KeyStroke::Test < Reline::TestCase
   def test_with_reline_key
     config = Reline::Config.new
     {
-      [
-        Reline::Key.new(100, 228, true), # Alt+d
-        Reline::Key.new(97, 97, false) # a
-      ] => 'abc',
+      "\eda".bytes => 'abc', # Alt+d a
       [195, 164] => 'def'
     }.each_pair do |key, func|
       config.add_oneshot_key_binding(key, func.bytes)
     end
     stroke = Reline::KeyStroke.new(config)
     assert_equal(:unmatched, stroke.match_status('da'.bytes))
-    assert_equal(:matched, stroke.match_status("\M-da".bytes))
+    assert_equal(:matched, stroke.match_status("\eda".bytes))
     assert_equal(:unmatched, stroke.match_status([32, 195, 164]))
     assert_equal(:matched, stroke.match_status([195, 164]))
   end
diff --git a/test/reline/test_reline.rb b/test/reline/test_reline.rb
index c664ab74b0..deff411232 100644
--- a/test/reline/test_reline.rb
+++ b/test/reline/test_reline.rb
@@ -303,12 +303,12 @@ class Reline::Test < Reline::TestCase
 
   def test_vi_editing_mode
     Reline.vi_editing_mode
-    assert_equal(Reline::KeyActor::ViInsert, Reline.core.config.editing_mode.class)
+    assert_equal(:vi_insert, Reline.core.config.instance_variable_get(:@editing_mode_label))
   end
 
   def test_emacs_editing_mode
     Reline.emacs_editing_mode
-    assert_equal(Reline::KeyActor::Emacs, Reline.core.config.editing_mode.class)
+    assert_equal(:emacs, Reline.core.config.instance_variable_get(:@editing_mode_label))
   end
 
   def test_add_dialog_proc
diff --git a/test/reline/test_reline_key.rb b/test/reline/test_reline_key.rb
index 7f9a11394a..1e6b9fcb6c 100644
--- a/test/reline/test_reline_key.rb
+++ b/test/reline/test_reline_key.rb
@@ -2,53 +2,10 @@ require_relative 'helper'
 require "reline"
 
 class Reline::TestKey < Reline::TestCase
-  def setup
-    Reline.test_mode
-  end
-
-  def teardown
-    Reline.test_reset
-  end
-
-  def test_match_key
-    assert(Reline::Key.new(1, 2, false).match?(Reline::Key.new(1, 2, false)))
-    assert(Reline::Key.new(1, 2, false).match?(Reline::Key.new(nil, 2, false)))
-    assert(Reline::Key.new(1, 2, false).match?(Reline::Key.new(1, 2, nil)))
-
-    assert(Reline::Key.new(nil, 2, false).match?(Reline::Key.new(nil, 2, false)))
-    assert(Reline::Key.new(1, nil, false).match?(Reline::Key.new(1, nil, false)))
-    assert(Reline::Key.new(1, 2, nil).match?(Reline::Key.new(1, 2, nil)))
-
-    assert(Reline::Key.new(nil, 2, false).match?(Reline::Key.new(nil, 2, false)))
-    assert(Reline::Key.new(1, nil, false).match?(Reline::Key.new(1, nil, false)))
-    assert(Reline::Key.new(1, 2, nil).match?(Reline::Key.new(1, 2, nil)))
-
-    assert(!Reline::Key.new(1, 2, false).match?(Reline::Key.new(3, 1, false)))
-    assert(!Reline::Key.new(1, 2, false).match?(Reline::Key.new(1, 3, false)))
-    assert(!Reline::Key.new(1, 2, false).match?(Reline::Key.new(1, 3, true)))
-  end
-
-  def test_match_integer
-    assert(Reline::Key.new(1, 2, false).match?(2))
-    assert(Reline::Key.new(nil, 2, false).match?(2))
-    assert(Reline::Key.new(1, nil, false).match?(1))
-
-    assert(!Reline::Key.new(1, 2, false).match?(1))
-    assert(!Reline::Key.new(1, nil, false).match?(2))
-    assert(!Reline::Key.new(nil, nil, false).match?(1))
-  end
-
   def test_match_symbol
-    assert(Reline::Key.new(:key1, :key2, false).match?(:key2))
-    assert(Reline::Key.new(:key1, nil, false).match?(:key1))
-
-    assert(!Reline::Key.new(:key1, :key2, false).match?(:key1))
-    assert(!Reline::Key.new(:key1, nil, false).match?(:key2))
-    assert(!Reline::Key.new(nil, nil, false).match?(:key1))
-  end
-
-  def test_match_other
-    assert(!Reline::Key.new(:key1, 2, false).match?("key1"))
-    assert(!Reline::Key.new(nil, nil, false).match?(nil))
+    assert(Reline::Key.new(:key1, :key1, false).match?(:key1))
+    refute(Reline::Key.new(:key1, :key1, false).match?(:key2))
+    refute(Reline::Key.new(:key1, :key1, false).match?(nil))
+    refute(Reline::Key.new(1, 1, false).match?(:key1))
   end
 end