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

Improve dialog color api #454

Merged
merged 1 commit into from
Jul 15, 2022
Merged

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Jun 28, 2022

As pointed out in the 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 PR, 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 read the raw color sequences.

Example

Reline.dialog_default_bg_color = :black
puts Reline.dialog_default_bg_color_sequence #=> 40
Reline.dialog_default_fg_color = :white
puts Reline.dialog_default_fg_color_sequence #=> 37
Reline.dialog_pointer_bg_color = :blue
puts Reline.dialog_pointer_bg_color_sequence #=> 34
Reline.dialog_pointer_fg_color = :black
puts Reline.dialog_pointer_fg_color_sequence #=> 30

截圖 2022-06-30 23 35 32

@st0012 st0012 force-pushed the improve-dialog-color-api branch 4 times, most recently from 1f8d765 to 16d09bd Compare June 28, 2022 22:17
@st0012 st0012 marked this pull request as ready for review June 28, 2022 22:21
@st0012
Copy link
Member Author

st0012 commented Jun 28, 2022

cc @nobu

@nobu nobu added the enhancement New feature or request label Jun 29, 2022
@nobu
Copy link
Member

nobu commented Jun 29, 2022

Do you think fg and bg should be able to set separately?
The (default, pointer) x (fg, bg) combination is too much a little, I think.
And scrollbar may be too.
How about set dialog-default-color cyan:white?

@st0012
Copy link
Member Author

st0012 commented Jun 30, 2022

I think having separate APIs that only take 1 argument is easier to understand. If we take 2 colors, the API name will be dialog_default_colors. But then users need to learn these implicitly:

  1. They should only provide 2 colors
  2. The first color is for foreground, and the second is for background

And because reline currently doesn't have its own API documentations, it'll be hard for them to learn them.
So reducing APIs will save a bit code duplication, but the cost will be transferred to the user's cognitive load, which imo is worse.

@st0012
Copy link
Member Author

st0012 commented Jul 1, 2022

And because reline currently doesn't have its own API documentations, it'll be hard for them to learn them.

After thinking about it, I think the lack of documentation shouldn't affect how we design an API. So please ignore this.

With that being said, other configs don't seem to take multiple values yet? If that's true, I think it's another reason we should keep it to a single value API.

@st0012
Copy link
Member Author

st0012 commented Jul 4, 2022

@nobu Let me know what you think. If you still think 2-value API is better, I'll change it.

lib/reline/config.rb Outdated Show resolved Hide resolved
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.
@st0012
Copy link
Member Author

st0012 commented Jul 11, 2022

@hsbt Do you think the updated API looks good?

@nobu
Copy link
Member

nobu commented Jul 12, 2022

Looks much better than bare integers. 😃

@peterzhu2118 peterzhu2118 merged commit b6ffa17 into ruby:master Jul 15, 2022
@st0012 st0012 deleted the improve-dialog-color-api branch July 15, 2022 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

3 participants