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

Changed to add parameters in irbrc to pass to Reline. #337

Closed

Conversation

pocari
Copy link

@pocari pocari commented Jan 17, 2022

I wanted to change the color of the completion dialog in Reline, so I made the following modifications to Reline
ruby/reline#413

Initially, I was thinking of setting the color in inputrc.

However, after receiving a report from Readline that warned that it was an unknown setting, I received a suggestion to set it in irbrc.

So, I made a modification to pass the configuration items to Reline using irbrc.

This is an example of using irbrc.
スクリーンショット 2022-01-17 23 00 08

This is an example without irbrc settings.
スクリーンショット 2022-01-17 23 00 41

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

@joeyparis
Copy link

@pocari I've been using this for a little while now and haven't run into any problems.

@james-caresnap
Copy link

Would it be desirable to support at least 4 color settings?

  • default foreground
  • default background
  • highlighted foreground (pointer foreground?)
  • highlighted background (pointer background?)

For example, this is a screenshot on my machine with your config. The highlighted row of the autocomplete is completely illegible.

image

@pocari
Copy link
Author

pocari commented Feb 13, 2022

@james-caresnap

Like you said, I needed to be able to set up a foreground view.
I've implemented that. You can check it out by doing 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'
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
IRB.conf[:DIALOG_DEFAULT_FG_COLOR] = 31
IRB.conf[:DIALOG_POINTER_FG_COLOR] = 30

% bundle exec irb

You should be able to see something like the following

スクリーンショット 2022-02-13 15 39 38

@sshock
Copy link

sshock commented Mar 5, 2022

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

@BrianHawley
Copy link

BrianHawley commented May 19, 2022

Probably a good idea to only set the Reline settings if they are set in IRB.conf and supported in reline. Like this:

if IRB.conf[:DIALOG_DEFAULT_BG_COLOR] && Reline.respond_to?("dialog_default_bg_color=")
  Reline.dialog_default_bg_color = IRB.conf[:DIALOG_DEFAULT_BG_COLOR]
end

And similar for the other settings. Existing "~/.irbrc" files likely won't have those settings.

@pocari
Copy link
Author

pocari commented May 22, 2022

@BrianHawley Thank you. I have fixed it. ( 12e5a7a )

@pocari
Copy link
Author

pocari commented May 30, 2022

By making ~/.irbrc look like the following, this PR may be unnecessary for us.
(It was possible to include code in irbrc that directly executes the Reline api)
I think this is reasonable since what we want to set up is a feature that exists only in Reline.

% 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

ruby/reline#413 (comment)

% cat Gemfile
# frozen_string_literal: true

source "https://rubygems.org"
# Install only the PR on the Reline side and use the standard IRB as is.
gem 'reline',  github: 'pocari/reline', branch: 'support-for-changing-the-background-color-of-dialogs'

% bundle install
% bundle exec irb

@@ -299,6 +299,18 @@ def initialize
if IRB.conf[:USE_AUTOCOMPLETE]
Reline.add_dialog_proc(:show_doc, SHOW_DOC_DIALOG, Reline::DEFAULT_DIALOG_CONTEXT)
end
if IRB.conf[:DIALOG_DEFAULT_BG_COLOR] && Reline.respond_to?('dialog_default_bg_color=')
Copy link
Member

Choose a reason for hiding this comment

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

I think it's more idiomatic to use symbols rather than strings for respond_to?. E.g. Reline.respond_to?(:dialog_default_bg_color=).

@st0012
Copy link
Member

st0012 commented Jun 27, 2022

First of all, ruby/reline#413 has been merged! We're half way of fixing the issue 🎉

Ideally, changes like that will be released in a minor version bump, which will be 0.4.0 in this case.
So I think it's preferable to have

if Gem::Version.new(Reline::VERSION) >= Gem::Version.new("0.4.0")
  Reline.dialog_default_bg_color = IRB.conf[:DIALOG_DEFAULT_BG_COLOR]
  Reline.dialog_pointer_bg_color = IRB.conf[:DIALOG_POINTER_BG_COLOR]
  Reline.dialog_default_fg_color = IRB.conf[:DIALOG_DEFAULT_FG_COLOR]
  Reline.dialog_pointer_fg_color = IRB.conf[:DIALOG_POINTER_FG_COLOR] 
end

And then assigning the defaults in IRB.init_config.

Also, can you add some test cases for it?

@st0012
Copy link
Member

st0012 commented Jul 15, 2022

@pocari I've updated the reline APIs in ruby/reline#454. It doesn't require significant changes on this PR though. But I hope we can:

  1. Simplify the check to just Reline.respond_to?(: dialog_default_bg_color) (or to be safe, check all 4 of them in the single if statement) because those APIs go hand-in-hand
  2. We should provide defaults in IRB.init_config
    • I'm also thinking about providing 2 themes dark and light with jus black/white combinations so users don't need to pick colors one by one
  3. We need some tests and documents around it

Thanks again for initiating these changes. Let me know if you want to do the extra work or you want someone else to take over it.

@tompng
Copy link
Member

tompng commented Nov 29, 2023

Thank you for the pull request.
I'm closing this because Reline now has a dialog customization API Reline::Face released in Reline 0.4.0.
ruby/reline#552 document

@tompng tompng closed this Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants