Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a new class Reline::Face to configure character attributes #552

Merged
merged 34 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
1224863
Reine::Face
hasumikin Jun 12, 2023
17114b9
fix test_yamatanooroti
hasumikin Jun 13, 2023
e033f88
Define singleton methods to make accessors to attributes of a face
hasumikin Jun 15, 2023
629db24
s/display/foreground/
hasumikin Oct 10, 2023
a993e52
s/default/default_style/ && s/normal_line/default/ && s/enhanced_line…
hasumikin Oct 10, 2023
cab307f
fix typo
hasumikin Oct 10, 2023
e4d61dd
FaceConfig.new now takes keyword arguments
hasumikin Oct 10, 2023
0e7acfb
Update lib/reline/face.rb
hasumikin Oct 14, 2023
b442c57
Update test/reline/test_face.rb
hasumikin Oct 14, 2023
389ff41
Fix to correspond to frozen_string_literal
hasumikin Oct 14, 2023
c3f92f1
Face::FaceConfig -> Face::Config
hasumikin Oct 14, 2023
d652fc0
Merge branch 'master' into reline_face
hasumikin Oct 14, 2023
1550519
ref https://github.com/ruby/reline/pull/552#pullrequestreview-1677282576
hasumikin Oct 14, 2023
6e4b4e9
delete unused ivar
hasumikin Oct 14, 2023
e8eb85b
ref https://github.com/ruby/reline/pull/552#discussion_r1358783723
hasumikin Oct 14, 2023
7ea200e
insert "\e[0m" into all SGR
hasumikin Oct 16, 2023
26cb115
tiny fix
hasumikin Oct 23, 2023
9286c59
ESSENTIAL_DEFINE_NAMES
hasumikin Oct 23, 2023
f001348
Change to Hash-accessor style
hasumikin Oct 23, 2023
28197be
Cache array method call in local variable
hasumikin Oct 24, 2023
efb75ed
Tests for Face configuration variations
hasumikin Nov 2, 2023
e7a6d56
resolve https://github.com/ruby/reline/pull/552#pullrequestreview-171…
hasumikin Nov 3, 2023
5a53084
amend to
hasumikin Nov 3, 2023
67a8497
check invalid SGR parameter in :style
hasumikin Nov 3, 2023
c02205c
The order of define values should be preserved
hasumikin Nov 3, 2023
728ca2d
Update test/reline/test_face.rb
hasumikin Nov 4, 2023
cde902e
Update test/reline/test_face.rb
hasumikin Nov 4, 2023
23d2360
Add methods: load_initial_config and reset_to_initial_config. And tea…
hasumikin Nov 5, 2023
2eee3b0
omission in amending "style: :default" to "style: :reset"
hasumikin Nov 5, 2023
76dd7b0
refs https://github.com/ruby/reline/issues/598
hasumikin Nov 6, 2023
1ce4818
Fix link
hasumikin Nov 6, 2023
7c5133c
amend method name
hasumikin Nov 6, 2023
a33109f
Merge branch 'master' into reline_face
hasumikin Nov 6, 2023
544f3d1
Update lib/reline/face.rb
hasumikin Nov 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions lib/reline.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
require 'reline/line_editor'
require 'reline/history'
require 'reline/terminfo'
require 'reline/face'
require 'rbconfig'

module Reline
Expand Down Expand Up @@ -37,10 +38,8 @@ def match?(other)
DialogRenderInfo = Struct.new(
:pos,
:contents,
:bg_color,
:pointer_bg_color,
:fg_color,
:pointer_fg_color,
:face,
:bg_color, # For the time being, this line should stay here for the compatibility with IRB.
:width,
:height,
:scrollbar,
Expand Down Expand Up @@ -261,10 +260,7 @@ def get_screen_size
contents: result,
scrollbar: true,
height: [15, preferred_dialog_height].min,
bg_color: 46,
pointer_bg_color: 45,
fg_color: 37,
pointer_fg_color: 37
face: :completion_dialog
)
}
Reline::DEFAULT_DIALOG_CONTEXT = Array.new
Expand Down Expand Up @@ -604,4 +600,16 @@ def self.line_editor
io
end

Reline::Face.config(:default) do |face|
face.define :normal_line, :default
face.define :enhanced_line, :default
face.define :scrollbar, :default
end

Reline::Face.config(:completion_dialog) do |face|
face.define :normal_line, :white_display, :cyan_background
face.define :enhanced_line, :white_display, :magenta_background
face.define :scrollbar, :white_display, :cyan_background
end

Reline::HISTORY = Reline::History.new(Reline.core.config)
120 changes: 120 additions & 0 deletions lib/reline/face.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
class Reline::Face
hasumikin marked this conversation as resolved.
Show resolved Hide resolved
SGR_PARAMETERS = {
# Display
black_display: 30,
red_display: 31,
green_display: 32,
yellow_display: 33,
blue_display: 34,
magenta_display: 35,
cyan_display: 36,
white_display: 37,
bright_black_display: 90,
gray_display: 90,
bright_red_display: 91,
bright_green_display: 92,
bright_yellow_display: 93,
bright_blue_display: 94,
bright_magenta_display: 95,
bright_cyan_display: 96,
bright_white_display: 97,
# Background
black_background: 40,
red_background: 41,
green_background: 42,
yellow_background: 43,
blue_background: 44,
magenta_background: 45,
cyan_background: 46,
white_background: 47,
bright_black_background: 100,
gray_background: 100,
bright_red_background: 101,
bright_green_background: 102,
bright_yellow_background: 103,
bright_blue_background: 104,
bright_magenta_background: 105,
bright_cyan_background: 106,
bright_white_background: 107,
# Style
default: 0,
bold: 1,
faint: 2,
italicized: 3,
underlined: 4,
slowly_blinking: 5,
blinking: 5,
rapidly_blinking: 6,
negative: 7,
concealed: 8,
crossed_out: 9
}.freeze

class FaceConfig
hasumikin marked this conversation as resolved.
Show resolved Hide resolved
def initialize(name, &block)
@name = name
block.call(self)
define(:default) unless self.respond_to?(:default)
end

def define(name, *sgr_values)
sgr_values.each do |value|
sgr_valid?(value) or raise ArgumentError, "invalid SGR parameter: #{value.inspect}"
end
sgr = "\e[" + sgr_values.map { |value|
case value
when Symbol
SGR_PARAMETERS[value]
when Hash
key, v = value.first
sgr_rgb(key, v)
end
}.join(';') + "m"
define_singleton_method(name) { sgr }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too sure about the singleton method approach. For example, it means we don't have a way to list all configured variations. I think it'll be easier to debug and maintain if we can use a hash here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it means we don't have a way to list all configured variations.

If I don't misunderstand what you say, we do.
@configs would be the hash that you say:

Reline::Face.instance_variable_get :@configs
=>
{:default=>#<Reline::Face::Config:0x00007fb0a0305400>,
 :completion_dialog=>#<Reline::Face::Config:0x00007fb0a03f6d50>}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But individual configs have different states/types (e.g. enhanced), how can we know what's defined and what are the values?

For example,

Reline::Face.config(:my_config) do |face|
  face.define :default, foreground: :blue
  face.define :enhanced, foreground: :red
end
face = Reline::Face[:my_config]

I'd like to see something like this when I call face.inspect:

#<Reline::Face::Config:0x0000000105fc1d30 @states={:default=>{:foreground=>:blue}, :enhanced=>{:foreground=>:red}}>

There should also be something like face.states to list all registered states.

And to achieve this, the code would look like:

    def initialize(name, &block)
      @states = {}
      block.call(self)
      define(:default) unless @states[:default]
    end

    def define(name, foreground: nil, background: nil, style: nil)
      values = {}
      values[:foreground] = foreground if foreground
      values[:background] = background if background
      values[:style] = style if style
      sgr = format_to_sgr(values)
      @states[name] = values
    end

end

private

def sgr_rgb(key, value)
case key
when :display
"38;2;"
hasumikin marked this conversation as resolved.
Show resolved Hide resolved
when :background
"48;2;"
end + value[1, 6].scan(/../).map(&:hex).join(";")
end

def sgr_valid?(sgr_value)
case sgr_value
when Symbol
SGR_PARAMETERS.keys.include?(sgr_value)
when Hash
sgr_value.count == 1 or return false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API usage is like this

face.define :enhanced_line, { display: '#abcdef' }, { background: '#fedcba' }, :bold, :italicized
face.define :enhanced_line, { display: '#abcdef' }, :bright_blue_background, :underlined

I think it's good. I especialy like that YAML format will be simple.

However, I thought if I can define it like this

# Single hash or keyword parameter
face.define :enhanced_line, display: '#abcdef', background: '#fedcba'
# Use both hex-color and color name
face.define :enhanced_line, display: '#012345', background: :bright_blue
# Need to be able to specify other styles, so
face.define :enhanced_line, display: :white, background: :blue, styles: [:bold, :italicized]

What do you think?

key, value = sgr_value.first
%i(display background).include?(key) or return false
rgb?(value) or return false
else
false
end
end

def rgb?(color)
color.respond_to?(:match?) and color.match?(/\A#[0-9a-fA-F]{6}\z/)
end
end

CONFIGS = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO using a constant to host mutable data is a bit anti-pattern. Do you think we can use singleton instance variable instead?

Copy link
Collaborator Author

@hasumikin hasumikin Oct 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this what you mean?
1550519


private_constant :SGR_PARAMETERS, :FaceConfig, :CONFIGS

def self.[](name)
CONFIGS[name]
end

def self.config(name, override = true, &block)
hasumikin marked this conversation as resolved.
Show resolved Hide resolved
return if CONFIGS[name] && !override
CONFIGS[name] = FaceConfig.new(name, &block)
end

end

22 changes: 9 additions & 13 deletions lib/reline/line_editor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -827,27 +827,23 @@ def add_dialog_proc(name, p, context = nil)
dialog.column = 0
dialog.width = @screen_size.last
end
face = Reline::Face[dialog_render_info.face || :default]
scrollbar_sgr = face.scrollbar
normal_line_sgr = face.normal_line
dialog.contents = contents.map.with_index do |item, i|
if i == pointer
fg_color = dialog_render_info.pointer_fg_color
bg_color = dialog_render_info.pointer_bg_color
else
fg_color = dialog_render_info.fg_color
bg_color = dialog_render_info.bg_color
end
line_sgr = i == pointer ? face.enhanced_line : normal_line_sgr
str_width = dialog.width - (scrollbar_pos.nil? ? 0 : @block_elem_width)
str = padding_space_with_escape_sequences(Reline::Unicode.take_range(item, 0, str_width), str_width)
colored_content = "\e[#{bg_color}m\e[#{fg_color}m#{str}"
colored_content = "#{line_sgr}#{str}"
if scrollbar_pos
color_seq = "\e[37m"
if scrollbar_pos <= (i * 2) and (i * 2 + 1) < (scrollbar_pos + bar_height)
colored_content + color_seq + @full_block
colored_content + scrollbar_sgr + @full_block
elsif scrollbar_pos <= (i * 2) and (i * 2) < (scrollbar_pos + bar_height)
colored_content + color_seq + @upper_half_block
colored_content + scrollbar_sgr + @upper_half_block
elsif scrollbar_pos <= (i * 2 + 1) and (i * 2) < (scrollbar_pos + bar_height)
colored_content + color_seq + @lower_half_block
colored_content + scrollbar_sgr + @lower_half_block
else
colored_content + color_seq + ' ' * @block_elem_width
colored_content + scrollbar_sgr + ' ' * @block_elem_width
end
else
colored_content
Expand Down
106 changes: 106 additions & 0 deletions test/reline/test_face.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
require_relative 'helper'
hasumikin marked this conversation as resolved.
Show resolved Hide resolved

class Reline::Face::Test < Reline::TestCase
class WithSetupTest < self
def setup
Reline::Face.config(:my_config) do |face|
face.define :normal_line, :blue_display
face.define :enhanced_line, { display: "#FF1020" }, :black_background, :bold, :underlined
end
Reline::Face.config(:another_config) do |face|
face.define :another_label, :red_display
end
@face = Reline::Face[:my_config]
end

def test_my_config_line
assert_equal "\e[34m", @face.normal_line
end

def test_my_config_enhanced_line
assert_equal "\e[38;2;255;16;32;40;1;4m", @face.enhanced_line
end

def test_my_config_default
assert_equal "\e[m", @face.default
end

def test_not_respond_to_another_label
assert_equal false, @face.respond_to?(:another_label)
end

hasumikin marked this conversation as resolved.
Show resolved Hide resolved
def test_existing_confit_override_default
Reline::Face.config(:my_config) do |face|
face.define :normal_line, :red_display
end
assert_equal "\e[31m", Reline::Face[:my_config].normal_line
end

def test_existing_config_override_false
Reline::Face.config(:my_config, false) do |face|
face.define :normal_line, :red_display
end
assert_equal "\e[34m", Reline::Face[:my_config].normal_line
end

def test_new_config_override_false
Reline::Face.config(:new_config, false) do |face|
face.define :normal_line, :red_display
end
assert_equal "\e[31m", Reline::Face[:new_config].normal_line
end
end

class WithoutSetupTest < self
def test_invalid_display_name
assert_raise ArgumentError do
Reline::Face.config(:invalid_config) do |face|
face.define :normal_line, :invalid_display
end
end
end

def test_invalid_background_name
assert_raise ArgumentError do
Reline::Face.config(:invalid_config) do |face|
face.define :normal_line, :invalid_background
end
end
end

def test_private_constants
[:SGR_PARAMETER, :FaceConfig, :CONFIGS].each do |name|
assert_equal false, Reline::Face.constants.include?(name)
end
end
end
hasumikin marked this conversation as resolved.
Show resolved Hide resolved

class FaceConfigTest < self
def setup
@face_config = Reline::Face.const_get(:FaceConfig).new(:my_config) { }
end

def test_rgb?
assert_equal true, @face_config.send(:rgb?, "#FFFFFF")
end

def test_invalid_rgb?
assert_equal false, @face_config.send(:rgb?, "FFFFFF")
assert_equal false, @face_config.send(:rgb?, "#FFFFF")
end

def test_sgr_valid?
assert_equal true, @face_config.send(:sgr_valid?, :white_display)
assert_equal true, @face_config.send(:sgr_valid?, { display: "#ffffff" })
end

def test_invalid_sgr_valid?
assert_equal false, @face_config.send(:sgr_valid?, { invalid_key: "#ffffff" })
end

def test_sgr_rgb
assert_equal "38;2;255;255;255", @face_config.send(:sgr_rgb, :display, "#ffffff")
assert_equal "48;2;18;52;86", @face_config.send(:sgr_rgb, :background, "#123456")
end
end
end
2 changes: 1 addition & 1 deletion test/reline/yamatanooroti/multiline_repl
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ opt.on('--dialog VAL') { |v|
if v.include?('alt-scrollbar')
scrollbar = true
end
Reline::DialogRenderInfo.new(pos: cursor_pos, contents: contents, height: height, scrollbar: scrollbar)
Reline::DialogRenderInfo.new(pos: cursor_pos, contents: contents, height: height, scrollbar: scrollbar, face: :completion_dialog)
})
if v.include?('alt-scrollbar')
ENV['RELINE_ALT_SCROLLBAR'] = '1'
Expand Down