Introduce a new class Reline::Face to configure character attributes #552
Conversation
hasumikin
changed the title
|
I'd be happy if it can fallback to nearest 256 palette color, maybe in the future, because macOS Terminal.app which I usually use does not support |
lib/reline/face.rb
Outdated
| when Symbol | ||
| SGR_PARAMETERS.keys.include?(sgr_value) | ||
| when Hash | ||
| sgr_value.count == 1 or return false |
There was a problem hiding this comment.
API usage is like this
face.define :enhanced_line, { display: '#abcdef' }, { background: '#fedcba' }, :bold, :italicized
face.define :enhanced_line, { display: '#abcdef' }, :bright_blue_background, :underlinedI 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?
|
Thank you for the proposal and the detailed description 🙏
Additional References
|
|
(I don't write this in a thread as I answer your both comments) @tompng But one concern was that "style" does not only represent "bold" and "underlined" in ECMA-48. In other words, a color also belongs to "style" (Although ECMA-48 doesn't use the word "style" but "graphic rendition aspects"). As in, there is no rule about the order of the attribute in ECMA-48. We can specify a "style" in advance of a display color. This also relates to @st0012's suggestion of preferring "foreground" to "display". Half of my reason not to adopt "style" will go away if we adopt "foreground" because we'd no longer stick to ECMA-48 ;) Therefore I propose that, first of all, we should decide to use "foreground" or "display". @ima1zumi Please let us know your idea, too |
|
@hasumikin
However, I find this idea fascinating. Could you please tell me more about how to make the API transparent to CSI and SGR? |
My word "transparent" in my mind is explained in the comment above (let me quote again ↓):
In short, introducing the Even if we use "foreground" instead of "display", the "transparent" concept can work because they are basically orthogonal to each other, I think. |
TBH I still don't know. I even feel we will now know for good... It's one of the reasons that I'm overall suggesting API which is "transparent" to ECMA-48 as much as possible.
It's the point. I believe the word "face" works well because it is not used very often. As you can see from our discussion, "style" and "display" are highly ambiguous and we should avoid those words in this case.
I suggested the word "enhanced" in this PR though, I am really open to discussing this concern. |
Honestly, I don't quite see it from that perspective. If you give me a class called The documentation seems to hint at this too, as it introduces
That being said, given that However, I do feel strongly about using the most commonly recognized names for attributes. So, to encapsulate my thoughts on naming conventions:
|
|
@st0012 Firstly I want us to decide whether we take 'Face' as the name of the class. Or should we choose another one? |
|
I agree with @hasumikin that One little concern I have for Face is that the naming of Face reminds me of font-family.
I'm not strongly against |
|
|
|
@hasumikin |
|
Thank you everyone for your comments. The class name is
|
I like singular
I like
I like
I prefer no suffix. Just |
|
Similar to @tompng:
face.define :default, foreground: :white, background: :blue
face.define :enhanced, foreground: :black, background: :white, style: [:bold, :italicized] |
|
|
One final point on which opinions are conflicted:
Maybe I will pick |
lib/reline/face.rb
Outdated
| sgr_rgb(key, value) | ||
| end | ||
| when :style | ||
| [ value ].flatten.map { |style_name| SGR_PARAMETERS[:style][style_name] } |
There was a problem hiding this comment.
If style contains :default, foreground and background is reset.
I think this is a little confusing
# red, cyan, bold
face.define :enhanced, background: :red, foreground: :cyan, style: :bold
# default background, default foreground
face.define :enhanced, background: :red, foreground: :cyan, style: :defaultOther part of this pull request looks good to me 👍
There was a problem hiding this comment.
Yap, that's why I originally named :normal_line instead of :default 🥺
There was a problem hiding this comment.
...How about style :reset ?
There was a problem hiding this comment.
RESET_SGR is always set at sgr == RESET_SGR ? RESET_SGR : RESET_SGR + sgr, so using SGR_PARAMETERS[:style][:default] or SGR_PARAMETERS[:style][:reset] here has no effect other than ignore foreground and background. I thought excluding :default is missing.
[ value ].flatten.flter_map do |style_name|
SGR_PARAMETERS[:style][style_name] if style_name != :default
endThere was a problem hiding this comment.
Okay, I got what you're saying.
Fortunately, I guess changing style: :default to style: :reset made the situation better.
5a53084
Parameters before :reset will be reset while parameters after :reset are not ignored because the order of the escape sequences will be the same as the order of the parameters in the format_to_sgr method.
example:
format_to_sgr({foreground: :white, style: :reset, background: :red})
=> "\e[0m\e[37;0;41m"
Since Reline users are programmers, I think there is no problem with them writing freely and taking responsibility for it.
There was a problem hiding this comment.
Looks good.
Perhaps face.define :default, style: :default in reline.rb should be face.define :default, style: :reset?
It seems that invalid SGR parameter check is skipped for style
|
|
||
| class WithInsufficientSetupTest < self | ||
| def setup | ||
| Reline::Face.config(:my_insufficient_config) do |face| |
There was a problem hiding this comment.
For tests that call Reline::Face.config, should we also set its @configs to the Reline default in teardown?
Once merged, these tests will be ran on Ruby CI with other gems' tests in the same process. So if Reline::Face.configs change over tests, it may cause other tests to fail. I broke Ruby CI many times because of things like this 😅
There was a problem hiding this comment.
Existing tests don't override configs of :default and :completion_dialog which are possible to be used by other tests, so I don't think Ruby CI will break without teardown...maybe
On the other hand, it is also good to make the teardown process in advance I carelessly write a test that overrides the default config in the future.
Will it look like this?:
# lib/reline/face.rb
class Reline::Face
def self.reset_config
@configs = {}
Reline::Face.config(:default) do |face|
face.define :default, style: :reset
face.define :enhanced, style: :reset
face.define :scrollbar, style: :reset
end
Reline::Face.config(:completion_dialog) do |face|
face.define :default, foreground: :white, background: :cyan
face.define :enhanced, foreground: :white, background: :magenta
face.define :scrollbar, foreground: :white, background: :cyan
end
end
end
# test/reline/test_face.rb
class Reline::Face::Test < Reline::TestCase
def teardown
Reline::Face.reset_config
end
endI can't say if I should do this now or not. What do you think?
There was a problem hiding this comment.
That's a good idea. But I also want to use this method in reline.rb to make sure the default there is always the same as what we reset.
How about:
class Reline::Face
def self.apply_default_configs
@configs = {}
Reline::Face.config(:default) do |face|
face.define :default, style: :reset
face.define :enhanced, style: :reset
face.define :scrollbar, style: :reset
end
Reline::Face.config(:completion_dialog) do |face|
face.define :default, foreground: :white, background: :cyan
face.define :enhanced, foreground: :white, background: :magenta
face.define :scrollbar, foreground: :white, background: :cyan
end
end
end
# In reline.rb
Reline::Face.apply_default_configsThere was a problem hiding this comment.
I separated the initializing method into reset_to_initial_config and load_initial_config so that a user can only reset the "initial" config (:default and :completion_dialog) without losing definitions by the user.
initial vs default
I chose the word "initial" instead of "default" to avoid ambiguity because "default" is used elsewhere.
config vs configs
In this case, no need for users to think about how many settings exist. So I think the singular "config" is enough and better.
There was a problem hiding this comment.
I chose the word "initial" instead of "default" to avoid ambiguity because "default" is used elsewhere.
👍
In this case, no need for users to think about how many settings exist. So I think the singular "config" is enough and better.
I'm not sure if I agree. Using config means explicitly "there's only 1 config", which is simply incorrect in this case. But using configs actually could mean 1 or more.
There was a problem hiding this comment.
I initially said "singular," but upon closer consideration, "uncountable" seems more appropriate. Therefore, I don't believe a plural form is necessary.
There was a problem hiding this comment.
I still don't get it. config is never uncountable. Otherwise, we wouldn't have Reline::Face.configs, no?
Even if we do want to consider it uncountable for this API, it's more confusing to have both countable and uncountable form of the same word in the same project, especially on the same component:
Reline::Face.load_initial_config #=> looks like it's loading a single config
Reline::Face.configs #=> returns 2 values?The fact is that we do load 2 configs when calling that method, and we do call configs when retrieving them. Why insisting it being called either singular or uncountable?
There was a problem hiding this comment.
I see, fixed 7c5133c
We may have to do a major rewrite of this document. However, since overall rewriting of the doc will be a big task, it may be practical to write a document in github.com/ruby/reline/doc/reline/face.md |
Co-authored-by: Stan Lo <[email protected]>
Co-authored-by: Stan Lo <[email protected]>
|
I don't think this should be documented on the IRB side because other
|
|
OK, let's discuss here |
|
Added a doc hasumikin@76dd7b0 |
| config(:default) do |conf| | ||
| conf.define :default, style: :reset | ||
| conf.define :enhanced, style: :reset | ||
| conf.define :scrollbar, style: :reset | ||
| end |
There was a problem hiding this comment.
What is this config required for? It does not appear to be used.
Sorry if I have missed it.
There was a problem hiding this comment.
In IRB, configs[:default] is used for all character output except the completion dialog.
Actually, :enhanced and :scrollbar are not used.
I was also unsure whether to keep them or not, but even if they were there, IRB wouldn't malfunction so I left it. But should they be deleted?
There was a problem hiding this comment.
It looks like IRB's document dialog (which does not specify face yet) use it.
There was a problem hiding this comment.
Thanks for letting me know.
Does it mean that a default value is used if the face argument is not provided in Reline::DialogRenderInfo.new?
Like this code https://github.com/ruby/irb/blob/v1.8.3/lib/irb/input-method.rb#L382
There was a problem hiding this comment.
As a result of ensuring that the absence of :enhanced and :scrollbar attributes are not broken even if the user forgets the scrollbar attribute in the completion_dialog config, enhanced and scrollbar attributes are automatically created in the :default config, too.
This is kind of a compromise with the existing implementation, where the concept of "completion_dialog" basically belongs to the IRB, but the SGR settings are set on the Reline side.
There was a problem hiding this comment.
Sorry, I overlooked tompng's comment.
default config is used here https://github.com/ruby/reline/pull/552/files#diff-9703d9141e08b96c2d616930b207873b52dc9eb68b81223818fe250cdb0e927fR834
Totally I made confusing this thread, lol
Co-authored-by: ima1zumi <[email protected]>
character attributes (ruby/reline#552) * Reine::Face * fix test_yamatanooroti * Define singleton methods to make accessors to attributes of a face * s/display/foreground/ * s/default/default_style/ && s/normal_line/default/ && s/enhanced_line/enhanced/ * fix typo * FaceConfig.new now takes keyword arguments * Update lib/reline/face.rb Co-authored-by: Stan Lo <[email protected]> * Update test/reline/test_face.rb Co-authored-by: Stan Lo <[email protected]> * Fix to correspond to frozen_string_literal * Face::FaceConfig -> Face::Config * ref ruby/reline#552 (review) * delete unused ivar * ref ruby/reline#552 (comment) * insert "\e[0m" into all SGR * tiny fix * ESSENTIAL_DEFINE_NAMES ref ruby/reline#552 (comment) * Change to Hash-accessor style - Reline::Face[:completion_dialog].enhanced -> Reline::Face[:completion_dialog][:enhanced] - Reline::Face.configs shows all defined values * Cache array method call in local variable * Tests for Face configuration variations * resolve ruby/reline#552 (review) * amend to * check invalid SGR parameter in :style * The order of define values should be preserved * Update test/reline/test_face.rb Co-authored-by: Stan Lo <[email protected]> * Update test/reline/test_face.rb Co-authored-by: Stan Lo <[email protected]> * Add methods: load_initial_config and reset_to_initial_config. And teardown in tests * omission in amending "style: :default" to "style: :reset" * refs ruby/reline#598 * Fix link * amend method name * Update lib/reline/face.rb Co-authored-by: ima1zumi <[email protected]> --------- ruby/reline@fdc1d3b1e5 Co-authored-by: Stan Lo <[email protected]> Co-authored-by: ima1zumi <[email protected]>
I made this PR to restart discussing the color configuration of Reline.
The original issue is https://bugs.ruby-lang.org/issues/18996
Summary of this PR
Reline::DialogRenderInfoReline::Facethat manages sets of character attributes based on Select Graphic Rendition (SGR)"#3300aa"(actual result depending on your terminal emulator):underlinedand:negativeBackground
Reline::DialogRenderInfocurrently accepts these text color attributes::bg_color:pointer_bg_color:fg_color:pointer_fg_colorDialogRenderInfoshould not worry about escape sequencesWhy "Face"?
My respected @shugo kindly advised me to refer to the
FaceofTextbringer(and Emacs): https://github.com/shugo/textbringer/blob/main/lib/textbringer/faces/programming.rbNaming attributes
I know that Ruby core members gently agreed with a suggestion about naming here:
https://bugs.ruby-lang.org/issues/18996#note-8
However, as this PR shows,
Reline::Facecan focus on only the character attribute of the terminal emulator which is described in "8.3.117 SGR - SELECT GRAPHIC RENDITION" of the ECMA-48 standard: https://www.ecma-international.org/wp-content/uploads/ECMA-48_5th_edition_june_1991.pdfI think it would make us pick the names again from the ECMA-48.
This is the reason that I use "display" instead of "foreground".
Likewise, the other attributes like
:italicizedand:underlinedwere picked from the ECMA-48 standard.To be honest, I also think that "foreground" is more widely used.
Textbringer also uses "foreground".
However, I myself am not aware of any authorized documents that provide a basis for us to adopt "foreground" instead of "display".
Even if so, you can still recommend "foreground".
Let me hear your opinion.
Other points where I want your opinion
Reline::Face::SGR_PARAMETERSReline::Facecurrently accepts are my really arbitrary choice. For example, I have omitted "doubly underlined" because I don't think it's widely supported:blinkingis an alias for:slowly_blinkingwhile no alias for:rapidly_blinking:gray_is an alias for:bright_black_"#ffffff"is strictly checked with/\A#[0-9a-fA-F]{6}\z/though, a value like"ffffff"(omitting#) might be valid as well:normal_lineand:enhanced_lineare perfectly right to the point. But at least I think that the word "selected" refers to a concept of "Completion dialog" and is not a universal concept among all kinds of dialogdefine, etc. Advise me on anything :)Reline::Facein.irbrcIRB's completion dialog in my terminal looks like this by default:
If you feel the color contrast of the dialog above is too low to see, you can write the following in
.irbrc:This looks nice, too:
Note
Making able to configure the character attributes in
.irbrcis not my original purpose.If you feel it's too rushed to make it a public API, let's discuss it.
Future plan (not by this PR)
As they say,
.irbrcis possibly dangerous.So you may want to put a file like
~/.irbrc.yamlthat looks like this:It'd be supposed to be done by another big discussion and some PRs to IRB.
Other remaining issues
:bg_colorinDialogRenderInfoso as not to break IRB: https://github.com/ruby/irb/blob/v1.7.0/lib/irb/input-method.rb#L407