Use color name instead of code (integer) in dialog color APIs

As pointed out in the [comment](ruby#413 (comment)), the code is actually a control sequence and not only for colors. To make the dialog color APIs safer to use, we should restrict its usages and extract away the bg/fg concept from the input. So in this commit, I made these changes: 1. The dialog_*_bg/fg_color APIs only takes and returns color names (symbol): - :black - :red - :green - :yellow - :blue - :magenta - :cyan - :white 2. Add additional dialog_*_bg/fg_color_code APIs to access the raw code.
st0012 · Jun 28, 2022 · fb684d7 · fb684d7
1 parent 347a468
commit fb684d7
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 45 deletions.
diff --git a/lib/reline.rb b/lib/reline.rb
@@ -74,12 +74,16 @@ class Core
       :autocompletion,
       :autocompletion=,
       :dialog_default_bg_color,
+      :dialog_default_bg_color_code,
       :dialog_default_bg_color=,
       :dialog_default_fg_color,
+      :dialog_default_fg_color_code,
       :dialog_default_fg_color=,
       :dialog_pointer_bg_color,
+      :dialog_pointer_bg_color_code,
       :dialog_pointer_bg_color=,
       :dialog_pointer_fg_color,
+      :dialog_pointer_fg_color_code,
       :dialog_pointer_fg_color=
 
     def initialize
@@ -553,10 +557,10 @@ def self.insert_text(*args, &block)
   def_single_delegators :core, :add_dialog_proc
   def_single_delegators :core, :dialog_proc
   def_single_delegators :core, :autocompletion, :autocompletion=
-  def_single_delegators :core, :dialog_default_bg_color, :dialog_default_bg_color=
-  def_single_delegators :core, :dialog_pointer_bg_color, :dialog_pointer_bg_color=
-  def_single_delegators :core, :dialog_default_fg_color, :dialog_default_fg_color=
-  def_single_delegators :core, :dialog_pointer_fg_color, :dialog_pointer_fg_color=
+  def_single_delegators :core, :dialog_default_bg_color, :dialog_default_bg_color=, :dialog_default_bg_color_code
+  def_single_delegators :core, :dialog_pointer_bg_color, :dialog_pointer_bg_color=, :dialog_default_fg_color_code
+  def_single_delegators :core, :dialog_default_fg_color, :dialog_default_fg_color=, :dialog_pointer_bg_color_code
+  def_single_delegators :core, :dialog_pointer_fg_color, :dialog_pointer_fg_color=, :dialog_pointer_fg_color_code
 
   def_single_delegators :core, :readmultiline
   def_instance_delegators self, :readmultiline
@@ -579,10 +583,10 @@ def self.core
       core.filename_quote_characters = ""
       core.special_prefixes = ""
       core.add_dialog_proc(:autocomplete, Reline::DEFAULT_DIALOG_PROC_AUTOCOMPLETE, Reline::DEFAULT_DIALOG_CONTEXT)
-      core.dialog_default_bg_color = 46 # Cyan
-      core.dialog_default_fg_color = 37 # White
-      core.dialog_pointer_bg_color = 45 # Magenta
-      core.dialog_pointer_fg_color = 37 # White
+      core.dialog_default_bg_color = :cyan
+      core.dialog_default_fg_color = :white
+      core.dialog_pointer_bg_color = :magenta
+      core.dialog_pointer_fg_color = :white
     }
   end
 

diff --git a/lib/reline/config.rb b/lib/reline/config.rb
@@ -45,13 +45,11 @@ class InvalidInputrc < RuntimeError
     attr_accessor v
   end
 
-  attr_accessor(
-    :autocompletion,
-    :dialog_default_bg_color,
-    :dialog_default_fg_color,
-    :dialog_pointer_bg_color,
-    :dialog_pointer_fg_color,
-  )
+  attr_accessor :autocompletion
+  attr_reader :dialog_default_bg_color_code,
+    :dialog_default_fg_color_code,
+    :dialog_pointer_bg_color_code,
+    :dialog_pointer_fg_color_code
 
   def initialize
     @additional_key_bindings = {} # from inputrc
@@ -77,10 +75,10 @@ def initialize
     @test_mode = false
     @autocompletion = false
     @convert_meta = true if seven_bit_encoding?(Reline::IOGate.encoding)
-    @dialog_default_bg_color = nil
-    @dialog_pointer_bg_color = nil
-    @dialog_default_fg_color = nil
-    @dialog_pointer_fg_color = nil
+    @dialog_default_bg_color_code = nil
+    @dialog_pointer_bg_color_code = nil
+    @dialog_default_fg_color_code = nil
+    @dialog_pointer_fg_color_code = nil
   end
 
   def reset
@@ -106,6 +104,64 @@ def editing_mode_is?(*val)
     (val.respond_to?(:any?) ? val : [val]).any?(@editing_mode_label)
   end
 
+  def dialog_default_bg_color=(color)
+    @dialog_default_bg_color_code = dialog_color_to_code(:bg, color.to_sym)
+  end
+
+  def dialog_default_fg_color=(color)
+    @dialog_default_fg_color_code = dialog_color_to_code(:fg, color.to_sym)
+  end
+
+  def dialog_pointer_bg_color=(color)
+    @dialog_pointer_bg_color_code = dialog_color_to_code(:bg, color.to_sym)
+  end
+
+  def dialog_pointer_fg_color=(color)
+    @dialog_pointer_fg_color_code = dialog_color_to_code(:fg, color.to_sym)
+  end
+
+  def dialog_default_bg_color
+    dialog_code_to_color(:bg, @dialog_default_bg_color_code)
+  end
+
+  def dialog_default_fg_color
+    dialog_code_to_color(:fg, @dialog_default_fg_color_code)
+  end
+
+  def dialog_pointer_bg_color
+    dialog_code_to_color(:bg, @dialog_pointer_bg_color_code)
+  end
+
+  def dialog_pointer_fg_color
+    dialog_code_to_color(:fg, @dialog_pointer_fg_color_code)
+  end
+
+  COLORS = [
+    :black,
+    :red,
+    :green,
+    :yellow,
+    :blue,
+    :magenta,
+    :cyan,
+    :white
+  ].freeze
+
+  private def dialog_color_to_code(type, color)
+    base = type == :bg ? 40 : 30
+
+    if c = COLORS.index(color)
+      base + c
+    else
+      raise "Unknown color: #{color}"
+    end
+  end
+
+  private def dialog_code_to_color(type, code)
+    base = type == :bg ? 40 : 30
+    COLORS[code - base]
+  end
+
   def keymap
     @key_actors[@keymap_label]
   end
@@ -339,13 +395,13 @@ def bind_variable(name, value)
     when 'emacs-mode-string'
       @emacs_mode_string = retrieve_string(value)
     when 'dialog-default-bg-color'
-      @dialog_default_bg_color = value.to_i
-    when 'dialog-pointer-bg-color'
-      @dialog_pointer_bg_color = value.to_i
+      self.dialog_default_bg_color = value.to_sym
     when 'dialog-default-fg-color'
-      @dialog_default_fg_color = value.to_i
+      self.dialog_default_fg_color = value.to_sym
+    when 'dialog-pointer-bg-color'
+      self.dialog_pointer_bg_color = value.to_sym
     when 'dialog-pointer-fg-color'
-      @dialog_pointer_fg_color = value.to_i
+      self.dialog_pointer_fg_color = value.to_sym
     when *VARIABLE_NAMES then
       variable_name = :"@#{name.tr(?-, ?_)}"
       instance_variable_set(variable_name, value.nil? || value == '1' || value == 'on')

diff --git a/test/reline/test_config.rb b/test/reline/test_config.rb
@@ -411,16 +411,16 @@ def test_relative_xdg_config_home
 
   def test_dialog_configurations
     @config.read_lines(<<~LINES.lines)
-      set dialog-default-bg-color 1
-      set dialog-pointer-bg-color 2
-      set dialog-default-fg-color 3
-      set dialog-pointer-fg-color 4
+      set dialog-default-bg-color white
+      set dialog-pointer-bg-color black
+      set dialog-default-fg-color cyan
+      set dialog-pointer-fg-color magenta
     LINES
 
-    assert_equal 1, @config.dialog_default_bg_color
-    assert_equal 2, @config.dialog_pointer_bg_color
-    assert_equal 3, @config.dialog_default_fg_color
-    assert_equal 4, @config.dialog_pointer_fg_color
+    assert_equal :white, @config.dialog_default_bg_color
+    assert_equal :black, @config.dialog_pointer_bg_color
+    assert_equal :cyan, @config.dialog_default_fg_color
+    assert_equal :magenta, @config.dialog_pointer_fg_color
   end
 end
 
diff --git a/test/reline/test_reline.rb b/test/reline/test_reline.rb
@@ -48,19 +48,26 @@ def test_completion_append_character
 
   def test_dialog_color_configuration
     # defaults
-    assert_equal(46, Reline.dialog_default_bg_color)
-    assert_equal(37, Reline.dialog_default_fg_color)
-    assert_equal(45, Reline.dialog_pointer_bg_color)
-    assert_equal(37, Reline.dialog_pointer_fg_color)
-
-    Reline.dialog_default_bg_color = 40
-    assert_equal(40, Reline.dialog_default_bg_color)
-    Reline.dialog_default_fg_color = 47
-    assert_equal(47, Reline.dialog_default_fg_color)
-    Reline.dialog_pointer_bg_color = 37
-    assert_equal(37, Reline.dialog_pointer_bg_color)
-    Reline.dialog_pointer_fg_color = 30
-    assert_equal(30, Reline.dialog_pointer_fg_color)
+    assert_equal(:cyan, Reline.dialog_default_bg_color)
+    assert_equal(:white, Reline.dialog_default_fg_color)
+    assert_equal(:magenta, Reline.dialog_pointer_bg_color)
+    assert_equal(:white, Reline.dialog_pointer_fg_color)
+
+    Reline.dialog_default_bg_color = :black
+    assert_equal(:black, Reline.dialog_default_bg_color)
+    assert_equal(40, Reline.dialog_default_bg_color_code)
+
+    Reline.dialog_default_fg_color = :white
+    assert_equal(:white, Reline.dialog_default_fg_color)
+    assert_equal(37, Reline.dialog_default_fg_color_code)
+
+    Reline.dialog_pointer_bg_color = :white
+    assert_equal(:white, Reline.dialog_pointer_bg_color)
+    assert_equal(47, Reline.dialog_pointer_bg_color_code)
+
+    Reline.dialog_pointer_fg_color = :black
+    assert_equal(:black, Reline.dialog_pointer_fg_color)
+    assert_equal(30, Reline.dialog_pointer_fg_color_code)
   end
 
   def test_basic_word_break_characters