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

Support conversion from String to Color #143

Closed
orhun opened this issue Apr 20, 2023 · 14 comments · Fixed by #180
Closed

Support conversion from String to Color #143

orhun opened this issue Apr 20, 2023 · 14 comments · Fixed by #180
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@orhun
Copy link
Sponsor Member

orhun commented Apr 20, 2023

Problem

In my projects I sometimes need a conversion like this:

"black" => TuiColor::Black,
"red" => TuiColor::Red,
"green" => TuiColor::Green,

Solution

Implement FromStr for ratatui::style::Color.

An example can be found here.

In the example, I used rgb crate for converting HEX values to Color as well. Since it introduces a new dependency, I'm not sure if we should do that as well.

Alternatives

None.

Additional context

I also did something like this in the past: https://github.com/orhun/kmon/blob/66f27006fed614c048469b2511b600d07c8912d6/src/style.rs#L110-L152

@orhun orhun added the enhancement New feature or request label Apr 20, 2023
@sophacles
Copy link
Collaborator

This might be a "good first issue" too (unless you're planning on knocking it out yourself)

@orhun orhun added the good first issue Good for newcomers label Apr 20, 2023
@orhun
Copy link
Sponsor Member Author

orhun commented Apr 20, 2023

Oh yeah, I was waiting for some comments, but sure. Anyone should be free to pick it up 🐻

@pythops
Copy link
Contributor

pythops commented Apr 21, 2023

I would like to give it a try :)

@orhun
Copy link
Sponsor Member Author

orhun commented Apr 21, 2023

Go for it!

@pythops
Copy link
Contributor

pythops commented Apr 21, 2023

@orhun

In the example, I used rgb crate for converting HEX values to Color as well. Since it introduces a new dependency, I'm not sure if we should do that as well.

what would be the ideal solution for you ?

@orhun
Copy link
Sponsor Member Author

orhun commented Apr 21, 2023

New dep ain't cool. I'm not sure how TrueColor conversion would work without it though. I haven't thought about it in depth.

But if you ask me I would blindly go with that solution. I simply created this issue to get more comments about it.

@sayanarijit
Copy link
Member

I wouldn't use strings if I have enums. Enums are more maintainable. If someone wants easy string to enum conversation, that'd be a separate feature flag, with someone dedicated to maintain it.

@orhun
Copy link
Sponsor Member Author

orhun commented Apr 22, 2023

I wouldn't use strings if I have enums. Enums are more maintainable.

The reason why I want this feature is because you need this conversion from the outside of ratatui, for example when you have a configuration file which have a color field and you need to implement De/Serialize for it. In that case, you either going to define your own color enum in your project and manually convert it to ratatui::style::Color OR you will have a string which will somehow support this conversion. I usually go with the latter approach since it has the advantage of true color support. When we use strings, the color can simply be hex code (#000) instead of a name (black) as well.

@sayanarijit
Copy link
Member

sayanarijit commented Apr 22, 2023

That's the thing... Some may prefer "red", for some it's "RED", while some may prefer hex. There's no "right" conversion system to use. The current serde implementation converts it to "Red", which I think is a good enough default.

EDIT: consider both to and from string.

EDIT 2: Ah... I see now, one good usecase of this would be to allow conversion from all these different conversion systems into the enum. That might be useful in the sense that the users can choose the style they prefer. But one (probably minor) drawback of this is that, when converting back to string (ser), we have to default to one of the systems. So, if a user writes it as "RED", and when viewing the config (serialized version) sees "Red" or hex code, they'll be pretty confused.

@sayanarijit
Copy link
Member

I think, we can add it as an optional feature and see how many people use it. This way, we don't have to worry about the dependency issue.

@sophacles
Copy link
Collaborator

I don't think it needs to be behind a feature:

  • Converting from a string based on the variant name is very common, and with the to_lower call to make it case-insensitive, it covers any capitalization preferences. (this is also how the fairly widely used strum crate does it).

  • having a constructor Color::parse_rbg(&str) -> Result<Self, ParseError> that just handles the most common case of #abcdef and doesn't use the Rgb crate as a dependency. This probably covers most TUI cases - I don't expect people to need color conversions or alternate representation (and if they do, they can source the Rgb crate for themselves).

@orhun
Copy link
Sponsor Member Author

orhun commented Apr 22, 2023

I agree with @sophacles 💯

@pythops
Copy link
Contributor

pythops commented May 2, 2023

is it okay to use regex or should we go with no additional depency at all ?

@sophacles
Copy link
Collaborator

If it's not to hard without regex, no dependency is the preference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants