[ruby/reline] Support menu-complete-backward command for upward

navigation
(https://github.com/ruby/reline/pull/677)

Fixes https://github.com/ruby/reline/pull/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!

https://github.com/ruby/reline/commit/2ccdb374a4

lib/reline/line_editor.rb

@@ -233,6 +233,7 @@ class Reline::LineEditor
     @waiting_operator_vi_arg = nil
     @completion_journey_state = nil
     @completion_state = CompletionState::NORMAL
+    @completion_occurs = false
     @perfect_matched = nil
     @menu_info = nil
     @searching_prompt = nil
@@ -1118,42 +1119,35 @@ class Reline::LineEditor
     end
     old_lines = @buffer_of_lines.dup
     @first_char = false
-    completion_occurs = false
+    @completion_occurs = false
     if @config.editing_mode_is?(:emacs, :vi_insert) and key.char == "\C-i".ord
       if !@config.disable_completion
         process_insert(force: true)
         if @config.autocompletion
           @completion_state = CompletionState::NORMAL
-          completion_occurs = move_completed_list(:down)
+          @completion_occurs = move_completed_list(:down)
         else
           @completion_journey_state = nil
           result = call_completion_proc
           if result.is_a?(Array)
-            completion_occurs = true
+            @completion_occurs = true
             complete(result, false)
           end
         end
       end
-    elsif @config.editing_mode_is?(:emacs, :vi_insert) and key.char == :completion_journey_up
-      if not @config.disable_completion and @config.autocompletion
-        process_insert(force: true)
-        @completion_state = CompletionState::NORMAL
-        completion_occurs = move_completed_list(:up)
-      end
     elsif @config.editing_mode_is?(:vi_insert) and ["\C-p".ord, "\C-n".ord].include?(key.char)
       # In vi mode, move completed list even if autocompletion is off
       if not @config.disable_completion
         process_insert(force: true)
         @completion_state = CompletionState::NORMAL
-        completion_occurs = move_completed_list("\C-p".ord == key.char ? :up : :down)
+        @completion_occurs = move_completed_list("\C-p".ord == key.char ? :up : :down)
       end
     elsif Symbol === key.char and respond_to?(key.char, true)
       process_key(key.char, key.char)
     else
       normal_char(key)
     end
-
+    unless @completion_occurs
-    unless completion_occurs
       @completion_state = CompletionState::NORMAL
       @completion_journey_state = nil
     end
@@ -1164,7 +1158,7 @@ class Reline::LineEditor
     end
 
     modified = old_lines != @buffer_of_lines
-    if !completion_occurs && modified && !@config.disable_completion && @config.autocompletion
+    if !@completion_occurs && modified && !@config.disable_completion && @config.autocompletion
       # Auto complete starts only when edited
       process_insert(force: true)
       @completion_journey_state = retrieve_completion_journey_state
@@ -1433,6 +1427,14 @@ class Reline::LineEditor
     end
   end
 
+  private def completion_journey_up(key)
+    if not @config.disable_completion and @config.autocompletion
+      @completion_state = CompletionState::NORMAL
+      @completion_occurs = move_completed_list(:up)
+    end
+  end
+  alias_method :menu_complete_backward, :completion_journey_up
+
   # Editline:: +ed-unassigned+ This  editor command always results in an error.
   # GNU Readline:: There is no corresponding macro.
   private def ed_unassigned(key) end # do nothing

test/reline/test_key_actor_emacs.rb

@@ -789,6 +789,52 @@ class Reline::KeyActor::Emacs::Test < Reline::TestCase
     assert_line_around_cursor('foo_ba', '')
   end
 
+  def test_autocompletion_with_upward_navigation
+    @config.autocompletion = true
+    @line_editor.completion_proc = proc { |word|
+      %w{
+        Readline
+        Regexp
+        RegexpError
+      }.map { |i|
+        i.encode(@encoding)
+      }
+    }
+    input_keys('Re')
+    assert_line_around_cursor('Re', '')
+    input_keys("\C-i", false)
+    assert_line_around_cursor('Readline', '')
+    input_keys("\C-i", false)
+    assert_line_around_cursor('Regexp', '')
+    @line_editor.input_key(Reline::Key.new(:completion_journey_up, :completion_journey_up, false))
+    assert_line_around_cursor('Readline', '')
+  ensure
+    @config.autocompletion = false
+  end
+
+  def test_autocompletion_with_upward_navigation_and_menu_complete_backward
+    @config.autocompletion = true
+    @line_editor.completion_proc = proc { |word|
+      %w{
+        Readline
+        Regexp
+        RegexpError
+      }.map { |i|
+        i.encode(@encoding)
+      }
+    }
+    input_keys('Re')
+    assert_line_around_cursor('Re', '')
+    input_keys("\C-i", false)
+    assert_line_around_cursor('Readline', '')
+    input_keys("\C-i", false)
+    assert_line_around_cursor('Regexp', '')
+    @line_editor.input_key(Reline::Key.new(:menu_complete_backward, :menu_complete_backward, false))
+    assert_line_around_cursor('Readline', '')
+  ensure
+    @config.autocompletion = false
+  end
+
   def test_completion_with_indent
     @line_editor.completion_proc = proc { |word|
       %w{

test/reline/test_key_actor_vi.rb

@@ -627,6 +627,52 @@ class Reline::KeyActor::ViInsert::Test < Reline::TestCase
     assert_line_around_cursor('foo_bar', '')
   end
 
+  def test_autocompletion_with_upward_navigation
+    @config.autocompletion = true
+    @line_editor.completion_proc = proc { |word|
+      %w{
+        Readline
+        Regexp
+        RegexpError
+      }.map { |i|
+        i.encode(@encoding)
+      }
+    }
+    input_keys('Re')
+    assert_line_around_cursor('Re', '')
+    input_keys("\C-i", false)
+    assert_line_around_cursor('Readline', '')
+    input_keys("\C-i", false)
+    assert_line_around_cursor('Regexp', '')
+    @line_editor.input_key(Reline::Key.new(:completion_journey_up, :completion_journey_up, false))
+    assert_line_around_cursor('Readline', '')
+  ensure
+    @config.autocompletion = false
+  end
+
+  def test_autocompletion_with_upward_navigation_and_menu_complete_backward
+    @config.autocompletion = true
+    @line_editor.completion_proc = proc { |word|
+      %w{
+        Readline
+        Regexp
+        RegexpError
+      }.map { |i|
+        i.encode(@encoding)
+      }
+    }
+    input_keys('Re')
+    assert_line_around_cursor('Re', '')
+    input_keys("\C-i", false)
+    assert_line_around_cursor('Readline', '')
+    input_keys("\C-i", false)
+    assert_line_around_cursor('Regexp', '')
+    @line_editor.input_key(Reline::Key.new(:menu_complete_backward, :menu_complete_backward, false))
+    assert_line_around_cursor('Readline', '')
+  ensure
+    @config.autocompletion = false
+  end
+
   def test_completion_with_disable_completion
     @config.disable_completion = true
     @line_editor.completion_proc = proc { |word|