[ruby/reline] Use color name instead of code (integer) in dialog color APIs

As pointed out in the [comment](https://github.com/ruby/reline/pull/413#issuecomment-1168033973), 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_sequence APIs to access the raw code. https://github.com/ruby/reline/commit/b32a977766
2022-11-09 12:17:21 -05:00 · 2022-06-28 14:39:56 +01:00 · 2022-06-28 14:39:56 +01:00 · 36ca0e58b6
commit 36ca0e58b6
parent 280b805d04
4 changed files with 130 additions and 56 deletions
--- a/lib/reline.rb
+++ b/lib/reline.rb
@ -46,6 +46,21 @@ module Reline
    keyword_init: true
  )

+  DIALOG_COLOR_APIS = [
+    :dialog_default_bg_color,
+    :dialog_default_bg_color_sequence,
+    :dialog_default_bg_color=,
+    :dialog_default_fg_color,
+    :dialog_default_fg_color_sequence,
+    :dialog_default_fg_color=,
+    :dialog_pointer_bg_color,
+    :dialog_pointer_bg_color_sequence,
+    :dialog_pointer_bg_color=,
+    :dialog_pointer_fg_color,
+    :dialog_pointer_fg_color_sequence,
+    :dialog_pointer_fg_color=
+  ]
+
  class Core
    ATTR_READER_NAMES = %i(
      completion_append_character
@ -73,14 +88,7 @@ module Reline
    def_delegators :config,
      :autocompletion,
      :autocompletion=,
-      :dialog_default_bg_color,
-      :dialog_default_bg_color=,
-      :dialog_default_fg_color,
-      :dialog_default_fg_color=,
-      :dialog_pointer_bg_color,
-      :dialog_pointer_bg_color=,
-      :dialog_pointer_fg_color,
-      :dialog_pointer_fg_color=
+      *DIALOG_COLOR_APIS

    def initialize
      self.output = STDOUT
@ -264,10 +272,10 @@ module Reline
        contents: result,
        scrollbar: true,
        height: 15,
-        bg_color: config.dialog_default_bg_color,
-        pointer_bg_color: config.dialog_pointer_bg_color,
-        fg_color: config.dialog_default_fg_color,
-        pointer_fg_color: config.dialog_pointer_fg_color
+        bg_color: config.dialog_default_bg_color_sequence,
+        pointer_bg_color: config.dialog_pointer_bg_color_sequence,
+        fg_color: config.dialog_default_fg_color_sequence,
+        pointer_fg_color: config.dialog_pointer_fg_color_sequence
      )
    }
    Reline::DEFAULT_DIALOG_CONTEXT = Array.new
@ -553,10 +561,7 @@ module Reline
  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_COLOR_APIS

  def_single_delegators :core, :readmultiline
  def_instance_delegators self, :readmultiline
@ -579,10 +584,10 @@ module Reline
      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

--- a/lib/reline/config.rb
+++ b/lib/reline/config.rb
@ -45,13 +45,11 @@ class Reline::Config
    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_sequence,
+    :dialog_default_fg_color_sequence,
+    :dialog_pointer_bg_color_sequence,
+    :dialog_pointer_fg_color_sequence

  def initialize
    @additional_key_bindings = {} # from inputrc
@ -77,10 +75,10 @@ class Reline::Config
    @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_sequence = nil
+    @dialog_pointer_bg_color_sequence = nil
+    @dialog_default_fg_color_sequence = nil
+    @dialog_pointer_fg_color_sequence = nil
  end

  def reset
@ -106,6 +104,65 @@ class Reline::Config
    (val.respond_to?(:any?) ? val : [val]).any?(@editing_mode_label)
  end

+  def dialog_default_bg_color=(color)
+    @dialog_default_bg_color_sequence = dialog_color_to_code(:bg, color)
+  end
+
+  def dialog_default_fg_color=(color)
+    @dialog_default_fg_color_sequence = dialog_color_to_code(:fg, color)
+  end
+
+  def dialog_pointer_bg_color=(color)
+    @dialog_pointer_bg_color_sequence = dialog_color_to_code(:bg, color)
+  end
+
+  def dialog_pointer_fg_color=(color)
+    @dialog_pointer_fg_color_sequence = dialog_color_to_code(:fg, color)
+  end
+
+  def dialog_default_bg_color
+    dialog_code_to_color(:bg, @dialog_default_bg_color_sequence)
+  end
+
+  def dialog_default_fg_color
+    dialog_code_to_color(:fg, @dialog_default_fg_color_sequence)
+  end
+
+  def dialog_pointer_bg_color
+    dialog_code_to_color(:bg, @dialog_pointer_bg_color_sequence)
+  end
+
+  def dialog_pointer_fg_color
+    dialog_code_to_color(:fg, @dialog_pointer_fg_color_sequence)
+  end
+
+  COLORS = [
+    :black,
+    :red,
+    :green,
+    :yellow,
+    :blue,
+    :magenta,
+    :cyan,
+    :white
+  ].freeze
+
+  private def dialog_color_to_code(type, color)
+    base = type == :bg ? 40 : 30
+    c = COLORS.index(color.to_sym)
+
+    if c
+      base + c
+    else
+      raise ArgumentError.new("Unknown color: #{color}.\nAvailable colors: #{COLORS.join(", ")}")
+    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 +396,13 @@ class Reline::Config
    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
    when 'dialog-default-fg-color'
-      @dialog_default_fg_color = value.to_i
+      self.dialog_default_fg_color = value
+    when 'dialog-pointer-bg-color'
+      self.dialog_pointer_bg_color = value
    when 'dialog-pointer-fg-color'
-      @dialog_pointer_fg_color = value.to_i
+      self.dialog_pointer_fg_color = value
    when *VARIABLE_NAMES then
      variable_name = :"@#{name.tr(?-, ?_)}"
      instance_variable_set(variable_name, value.nil? || value == '1' || value == 'on')
--- a/test/reline/test_config.rb
+++ b/test/reline/test_config.rb
@ -411,16 +411,16 @@ class Reline::Config::Test < Reline::TestCase

  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

--- a/test/reline/test_reline.rb
+++ b/test/reline/test_reline.rb
@ -48,19 +48,31 @@ class Reline::Test < Reline::TestCase

  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)
+    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 = 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)
+    Reline.dialog_default_bg_color = :black
+    assert_equal(:black, Reline.dialog_default_bg_color)
+    assert_equal(40, Reline.dialog_default_bg_color_sequence)
+
+    Reline.dialog_default_fg_color = :white
+    assert_equal(:white, Reline.dialog_default_fg_color)
+    assert_equal(37, Reline.dialog_default_fg_color_sequence)
+
+    Reline.dialog_pointer_bg_color = :white
+    assert_equal(:white, Reline.dialog_pointer_bg_color)
+    assert_equal(47, Reline.dialog_pointer_bg_color_sequence)
+
+    Reline.dialog_pointer_fg_color = :black
+    assert_equal(:black, Reline.dialog_pointer_fg_color)
+    assert_equal(30, Reline.dialog_pointer_fg_color_sequence)
+
+    # test value validation
+    assert_raise(ArgumentError) do
+      Reline.dialog_pointer_fg_color = :foo
+    end
  end

  def test_basic_word_break_characters