Enable to change the background color of dialogs. by pocari · Pull Request #413 · ruby/reline

pocari · 2021-12-29T07:45:17Z

Reline in its current state also has the ability to change the background color using the bg_color attribute of DialogRenderInfo, but I couldn't seem to use that feature right now.

I changed it so that in addition to the bg_color, the background color of the item being selected for completion can also be set, and then it can be defined in "inputrc" so that the user can choose the color.

default color:

customize sample:

You can try with the following settings

% cat Gemfile
# frozen_string_literal: true

source "https://rubygems.org"

gem 'reline',  github: 'pocari/reline', branch: 'support-for-changing-the-background-color-of-dialogs'
gem 'irb',  github: 'pocari/irb', branch: 'feature/add-config-params-for-reline'

% bundle install
% cat ~/.irbrc

IRB.conf[:DIALOG_DEFAULT_BG_COLOR] = 40
IRB.conf[:DIALOG_POINTER_BG_COLOR] = 47

% bundle exec irb

elfham · 2022-01-09T06:06:14Z

Readline complains about new unknown keywords in inputrc.

% echo "set dialog-default-bg-color 40" >> ~/.inputrc
% echo "set dialog-pointer-bg-color 47" >> ~/.inputrc
% bash
readline: ~/.inputrc: line 14: dialog-default-bg-color: unknown variable name
readline: ~/.inputrc: line 15: dialog-pointer-bg-color: unknown variable name
$

pocari · 2022-01-10T14:31:59Z

@elfham If Reline is mandatory to be compatible with readline, my implementation this time may not be good.
But we can't customize the background color if we can't pass some value to the reline. If you have a better idea for this, please let me know.

elfham · 2022-01-10T16:25:20Z

Since inputrc is a common configuration file for Readline and Reline, I'm afraid that apps that use Readline will suffer the side effects of adding this keyword.

I'm not an expert on Reline, so I don't know if this is the right way to do it, but how about setting it up in irbrc?

pocari · 2022-01-10T16:29:27Z

@elfham I did not know that irbrc existed. I'll try to see if I can use it. Thank you.

pocari · 2022-01-17T14:08:10Z

@elfham I modified irbrc so that it can pass configuration items. (ruby/irb#337)

You can try this fix with a Gemfile like the one below.

% cat Gemfile
# frozen_string_literal: true

source "https://rubygems.org"

gem 'reline',  github: 'pocari/reline', branch: 'support-for-changing-the-background-color-of-dialogs'
gem 'irb',  github: 'pocari/irb', branch: 'feature/add-config-params-for-reline'

% bundle install
% cat ~/.irbrc

IRB.conf[:DIALOG_DEFAULT_BG_COLOR] = 40
IRB.conf[:DIALOG_POINTER_BG_COLOR] = 47

% bundle exec irb

elfham · 2022-01-18T00:23:31Z

I have tried this fix and it seems to be working well.
Thanks for the fix.

joeyparis · 2022-01-31T16:05:18Z

@pocari also confirmed working with no problems

pocari · 2022-02-14T07:49:43Z

I have also made the foreground color changeable.
ruby/irb#337 (comment)

sshock · 2022-03-05T07:22:12Z

Would love to see this feature! Is there any reason it hasn't been approved and merged yet?

elfham · 2022-03-06T02:25:48Z

I too see no problem with this feature.

BTW, the main developer, @aycabta, does not seem to be very active since the end of last year.
He tweeted that he suffered a mild stroke and I am concerned ......

st0012 · 2022-05-16T21:32:58Z

lib/reline/line_editor.rb

+      fg_color = 37
      if i == pointer
-        bg_color = '45'
+        if dialog_render_info.pointer_fg_color


How about setting the default values when initializing the reline like other configs?

reline/lib/reline.rb

Lines 546 to 551 in 5b3fc02

core.basic_word_break_characters = " \t\n`><=;|&{("

core.completer_word_break_characters = " \t\n`><=;|&{("

core.basic_quote_characters = '"\''

core.completer_quote_characters = '"\''

core.filename_quote_characters = ""

core.special_prefixes = ""

Then we don't need these if statements because the dialog_render_info should always have values from the config default. It will also help users getting the default by just calling Reline.dialog_default_fg_color.

If we really worry about the values being nil by accident, we should validate them at the setter.

pocari · 2022-05-22T16:07:00Z

@st0012 Thanks for the comments. Those points seemed right to me. However, the properties I added this time were defined in the below section, so I wasn't sure if I should treat them the same way.

reline/lib/reline.rb

Line 36 in 5b3fc02

    
           DialogRenderInfo = Struct.new(:pos, :contents, :bg_color, :width, :height, :scrollbar, keyword_init: true)

st0012 · 2022-05-23T17:48:18Z

Since we want them to be exposed externally, I think it doesn't matter where they'll be used. The point is to make the values more easily accessible.

pocari · 2022-05-28T14:16:45Z

@st0012 Thank you. I have added the code as you mentioned. 9bf4979

Maumagnaguagno · 2022-05-29T02:16:44Z

lib/reline.rb

      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 # Maggenta


Maggenta => Magenta

@Maumagnaguagno Thanks. I have fixed it.

st0012 · 2022-05-29T21:31:54Z

@pocari Thanks for the update. It looks good to me 👍
Do you think you can update the description example to a version that doesn't require the irb patch? I think directly setting reline's color with the exposed APIs should work too? Something like:

# .irbrc
Reline.dialog_default_bg_color = 40
Reline.dialog_pointer_bg_color = 47

@hsbt Would you mind giving this a look? I know @aycabta is the maintainer but he seems to have been away for a while.

I think being able to change the background color of the dialog is essential for good user-experience. And as shown in ruby/irb#328, the default color doesn't look good with some themes (for example, mine). And many users decided to turn off the autocomplete feature altogether because of the color.

Of course, to completely fix the irb's issue will require some more work (releasing this change in a minor release + bump irb's dependency requirement + cut another irb minor release). But having this merged will already allow some users to workaround the issue themselves.

pocari · 2022-05-30T00:29:38Z

@st0012 As for irbrc, I was advised by another person and made the following changes.
ruby/irb#337 (comment)

Do you think you can update the description example to a version that doesn't require the irb patch?

That may be possible.

pocari · 2022-05-30T02:32:17Z

@st0012 As you said it was possible. Thanks! You can try it with the following

% cat Gemfile
# frozen_string_literal: true

source "https://rubygems.org"

gem 'reline',  github: 'pocari/reline', branch: 'support-for-changing-the-background-color-of-dialogs'

% bundle install
% cat ~/.irbrc
if Reline
  Reline.dialog_default_bg_color = 40
  Reline.dialog_pointer_bg_color = 47
  Reline.dialog_pointer_fg_color = 30
  Reline.dialog_pointer_bg_color = 45
end

% bundle exec irb

I have added a condition because there must be some environments where Reline is not installed.

st0012 · 2022-05-30T08:41:58Z

@pocari To be clear, I know the irb change is necessary. But having reline-only example will make this change easier to understand and test 🙂

st0012 · 2022-06-13T16:47:42Z

@pocari Can you rebase your branch to see if the CI passes?

…ckground color of the selected item in inputrc.

Co-authored-by: Peter Zhu <peter@peterzhu.ca>

…f github.com:pocari/reline into support-for-changing-the-background-color-of-dialogs

peterzhu2118

Thank you for your contribution!

…/reline#413) ruby/reline@bd49537964

st0012 · 2022-06-27T19:37:03Z

@hsbt @peterzhu2118 Do you think we can cut a 0.4.0 release for this change? Here are the changes since v0.3.1

There are a few reasons:

We can test these new APIs early.
irb can start requiring reline >= 0.4.0, which can make the implementation cleaner and also make users adopting it easier.
- Instead of installing both irb and reline on GH source, the user will only need to install the latest irb.

Or do you have any concerns/thoughts?

nobu · 2022-06-27T23:30:23Z

46, 37 and so on are not “colors”, but attributes including where they are applied to, foreground or background.
That means you can set “background” to a foreground attribute 37, for instance.
So I think “attribute” string would be more proper here.

pocari · 2022-06-28T00:54:38Z

@nobu
Just to confirm, are you saying that dialog_default_bg_attribute is better than dialog_default_bg_color, for example?

st0012 · 2022-06-28T09:26:32Z

I feel "color" is still a better name because that's what we want them to be used for.
But the APIs can indeed do more to prevent misuse.

How about this

Reline.dialog_default_bg_color = :black
Reline.dialog_pointer_bg_color = :white
Reline.dialog_pointer_fg_color = :black
Reline.dialog_pointer_bg_color = :white

And the color assignment logic can do:

# lib/reline/config.rb
def dialog_default_bg_color=(color)
  @dialog_default_bg_color = select_color_code(:bg, color)
end

def select_color_code(type, color)
  if :bg
    case color
    when :black
      40
    when :white
      47
      # ...... the rest
    else
      raise ColorNotFound
    end
  else
    case color
    when :black
      30
    when :white
      37
      # ...... the rest
    else
      raise ColorNotFound
    end
  end
end

nobu · 2022-06-28T12:14:25Z

To set colors only, dialog_default_bg_color etc make sense, and passing color names would be better.
However attributes are not only colors, but highlight, reverse video, underline, etc too.

Another concern is "\e[#{bg_color}m\e[#{fg_color}m#{str}" will reset bg_color if fg_color is set to 0.
The setters don't check the arguments.

BTW, your select_color_code can be:

def select_color_code(type, color)
  case type
  when :bg; 40
  when :fg; 30
  else raise ColorNotFound
  end +
  case color
  when :black; 30
  when :white; 37
    # ...... the rest
  else raise ColorNotFound
end

or even Hash#fetch may be useful.

st0012 · 2022-06-28T13:00:53Z

However attributes are not only colors, but highlight, reverse video, underline, etc too.

That's true. But from what I've seen so far, the only request is on color and not other text effects. I'm sure some users would want that but perhaps we can design better APIs for those cases when we get more feedback?

Another concern is "\e[#{bg_color}m\e[#{fg_color}m#{str}" will reset bg_color if fg_color is set to 0.

The reason I want to pass color symbol instead of integer is to prevent cases like this, as we won't have 0 available in the case statement.

BTW, your select_color_code can be:

That's a great suggestion 👍 I'll open a PR shortly.

james-caresnap · 2022-06-28T18:21:47Z

46, 37 and so on are not “colors”, but attributes including where they are applied to, foreground or background.
That means you can set “background” to a foreground attribute 37, for instance.
So I think “attribute” string would be more proper here.

The linux man page calls them "color sequences".

However, the ANSI escape code standard calls them "control sequences".

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.

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_sequence APIs to access the raw code.

nobu · 2022-06-29T02:46:42Z

lib/reline/line_editor.rb

      str_width = dialog.width - (dialog.scrollbar_pos.nil? ? 0 : @block_elem_width)
      str = padding_space_with_escape_sequences(Reline::Unicode.take_range(item, 0, str_width), str_width)
-      @output.write "\e[#{bg_color}m#{str}"
+      @output.write "\e[#{bg_color}m\e[#{fg_color}m#{str}"


This can be "\e[#{bg_color};#{fg_color}m#{str}".

nobu · 2022-06-29T02:47:10Z

lib/reline/line_editor.rb

-      @output.write "\e[#{bg_color}m#{str}"
+      @output.write "\e[#{bg_color}m\e[#{fg_color}m#{str}"
      if dialog.scrollbar_pos and (dialog.scrollbar_pos != old_dialog.scrollbar_pos or dialog.column != old_dialog.column)
        @output.write "\e[37m"


Maybe scrollbar color too?

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_sequence APIs to access the raw code.

…r APIs As pointed out in the [comment](ruby/reline#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_sequence APIs to access the raw code. ruby/reline@b32a977766

pocari changed the title ~~Make it possible to set the background color of the dialog and the ba…~~ Enable to change the background color of dialogs. Dec 29, 2021

osyo-manga mentioned this pull request Jan 8, 2022

Change default completion background color ruby/irb#328

Closed

pocari mentioned this pull request Jan 17, 2022

Changed to add parameters in irbrc to pass to Reline. ruby/irb#337

Closed

st0012 reviewed May 16, 2022

View reviewed changes

Maumagnaguagno reviewed May 29, 2022

View reviewed changes

pocari added 7 commits June 15, 2022 01:59

Make it possible to set the background color of the dialog and the ba…

cc438de

…ckground color of the selected item in inputrc.

Remove from the configuration items in inputrc

0bfeda1

add config parameters

dec9289

add config parameters

e61c8d4

support for foreground color

a91c03d

add default fg, bg color settings

4d1e684

fix typo

e427619

pocari and others added 3 commits June 25, 2022 23:56

Update lib/reline.rb

99e90d3

Co-authored-by: Peter Zhu <peter@peterzhu.ca>

add space

8bf9ca2

Merge branch 'support-for-changing-the-background-color-of-dialogs' o…

6ccdcc5

…f github.com:pocari/reline into support-for-changing-the-background-color-of-dialogs

st0012 mentioned this pull request Jun 25, 2022

doesn't support some terminal control chars (eg ^u = kill) ruby/debug#650

Closed

pocari added 3 commits June 26, 2022 14:55

use forwardable

28a64c6

use attr_accessor

3b72875

add dialog configuration test

2302293

pocari force-pushed the support-for-changing-the-background-color-of-dialogs branch from d0559f8 to 2302293 Compare June 26, 2022 05:56

peterzhu2118 approved these changes Jun 27, 2022

View reviewed changes

peterzhu2118 merged commit bd49537 into ruby:master Jun 27, 2022

matzbot pushed a commit to ruby/ruby that referenced this pull request Jun 27, 2022

[ruby/reline] Enable to change the background color of dialogs. (ruby…

8c6c3e3

…/reline#413) ruby/reline@bd49537964

st0012 mentioned this pull request Jun 28, 2022

Improve dialog color api #454

Merged

nobu added the enhancement New feature or request label Jun 29, 2022

nobu reviewed Jun 29, 2022

View reviewed changes

st0012 mentioned this pull request Jul 27, 2022

Drop Ruby 2.5 support ruby/irb#383

Merged

	core.basic_word_break_characters = " \t\n`><=;\|&{("
	core.completer_word_break_characters = " \t\n`><=;\|&{("
	core.basic_quote_characters = '"\''
	core.completer_quote_characters = '"\''
	core.filename_quote_characters = ""
	core.special_prefixes = ""

Conversation

pocari commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elfham commented Jan 9, 2022

Uh oh!

pocari commented Jan 10, 2022

Uh oh!

elfham commented Jan 10, 2022

Uh oh!

pocari commented Jan 10, 2022

Uh oh!

pocari commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elfham commented Jan 18, 2022

Uh oh!

joeyparis commented Jan 31, 2022

Uh oh!

pocari commented Feb 14, 2022

Uh oh!

sshock commented Mar 5, 2022

Uh oh!

elfham commented Mar 6, 2022

Uh oh!

st0012 May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pocari commented May 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st0012 commented May 23, 2022

Uh oh!

pocari commented May 28, 2022

Uh oh!

Maumagnaguagno May 29, 2022

Choose a reason for hiding this comment

Uh oh!

pocari May 29, 2022

Choose a reason for hiding this comment

Uh oh!

st0012 commented May 29, 2022

Uh oh!

pocari commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pocari commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st0012 commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st0012 commented Jun 13, 2022

Uh oh!

peterzhu2118 left a comment

Choose a reason for hiding this comment

Uh oh!

st0012 commented Jun 27, 2022

Uh oh!

nobu commented Jun 27, 2022

Uh oh!

pocari commented Jun 28, 2022

Uh oh!

st0012 commented Jun 28, 2022

Uh oh!

nobu commented Jun 28, 2022

Uh oh!

st0012 commented Jun 28, 2022

Uh oh!

james-caresnap commented Jun 28, 2022

Uh oh!

nobu Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

nobu Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

pocari commented Dec 29, 2021 •

edited

Loading

pocari commented Jan 17, 2022 •

edited

Loading

st0012 May 16, 2022 •

edited

Loading

pocari commented May 22, 2022 •

edited

Loading

pocari commented May 30, 2022 •

edited

Loading

pocari commented May 30, 2022 •

edited

Loading

st0012 commented May 30, 2022 •

edited

Loading