fix: use to_string to serialize Color

[SleepySwords]

Since deserialize now uses FromStr to deserialize color, serializing Color RGB values, as well as index values, would produce an output that would no longer be able to be deserialized without causing an error.

Color::Rgb will now be serialized as the hex representation of their value.
For example, with serde_json, Color::Rgb(255, 0, 255) would be serialized as "#FF00FF" rather than {"Rgb": [255, 0, 255]}.

Color::Indexed will now be serialized as just the string of the index.
For example, with serde_json, Color::Indexed(10) would be serialized as "10" rather than {"Indexed": 10}.

Other color variants remain the same.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.4%. Comparing base (b7778e5) to head (6e6cb13).

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #934   +/-   ##
=====================================
  Coverage   89.4%   89.4%           
=====================================
  Files         61      61           
  Lines      15430   15465   +35     
=====================================
+ Hits       13799   13834   +35     
  Misses      1631    1631           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kdheepak]

I think you don't need this in Ratatui. This works for me to serialize and deserialize Color.

#[serde_as]
#[derive(Debug, Deserialize, Serialize)]
pub struct Config {
    #[serde_as(as = "DisplayFromStr")]
    pub background_color: ratatui::style::Color,
}

If I export an instance of this struct to toml, I get something like this:

[config]
background_color = "#111827"

[Valentin271]

But it does make sense that a Ratatui color should be able to make a round trip. Plus #ff00ff makes much more sense to me (except in RON).

[kdheepak]

For context, the code block I shared above does round trip, I'm able to read and write to toml and I am able to read #ff00ff strings and write out #ff00ff strings.

[Valentin271]

For context, the code block I shared above does round trip, I'm able to read and write to toml and I am able to read #ff00ff strings and write out #ff00ff strings.

One missing thing from my comment was probably "by default". Of course you could make it do that, but why not make it the default.

[kdheepak]

Good point. One reason I can think of is that people may want to serialize into hsl(0, 100%, 50%) instead of #ff0000. I'm not sure if that would work if we provide a default (I'm fairly new to serde, so there may be some obvious way to do this that I don't know about).

[joshka]

Good point. One reason I can think of is that people may want to serialize into hsl(0, 100%, 50%) instead of #ff0000. I'm not sure if that would work if we provide a default (I'm fairly new to serde, so there may be some obvious way to do this that I don't know about).

My guess is that this would never be a default serialization format, only a deserialization. I could be wrong though, the only way I can think of to be able to make serialization work would be to keep the colors a Color::Hsl instead of converting to Color::Rgb during serialization. At that point, I'd advise the developer of such a problem to use a wrapper type or serialization helper that implements this and does the conversion for their app. This is because the only color schemes that consoles understand are indexed or RGB - not HSL.

I think hex is a reasonable default serialization format rather than "Color(r,g,b)" (I'm assuming this is the current default?)

[orhun]

Yes, with this change we're serializing as hex instead of Rgb { .. } which looks like the correct behavior.

e.g. without this change the test fails like so:

thread 'style::color::tests::serialize_then_deserialize' panicked at src/style/color.rs:585:9:
assertion `left == right` failed
  left: "{\"Rgb\":[255,0,255]}"
 right: "\"#FF00FF\""

[joshka]

How does this change existing serialized colors?

1. Can you write tests for the existing serialization format?
   If it makes the previous serialization format fail, then this is a breaking change, which we should:

a) work out whether we need to provide a serialization method that understands both formats (it's possible that we broke things when switching to using the from_str method - I'm not sure how many apps use serialization, it's likely a small amount but growing)
b) document the breaking change as such in the breaking changes doc

3. Can you please update the first comment in the PR to summarize the changes in behavior that users would notice (e.g. write a sucint message for the changelog).
   (Something like Color::Indexed is now serialized as ..., Color::Rgb ...)

[joshka]

(also, thanks for submitting your first PR to Ratatui!)

[kdheepak]

I like hex as a default, but I'm just not familiar enough with serde to know if supporting other formats would be possible after adding this default. For example, Base16 color schemes use ff00ff instead of #ff00ff, e.g.:

• https://github.com/Defman21/base16-github-scheme/blob/master/github.yaml
• https://github.com/edunfelt/base16-rose-pine-scheme/blob/main/rose-pine.yaml
• More here: https://github.com/chriskempson/base16

And I think it doesn't make sense to implement from_str("ff00ff"), right? How would one make a serialization and deserialization for these files into a struct containing Colors?

[SleepySwords]

You are still able to provide serealizers/deserializers other than the default using the with attribute

This would still be more difficult when wanting custom colour logic for something like Style for instance (I think you could achieve this with a wrapper, not too sure though). However, you would still face these issues, even without this change.

[SleepySwords]

This test does fail

#[cfg(feature = "serde")]
#[test]
fn deserialize_with_previous_format() -> Result<(), serde_json::Error> {
    assert_eq!(Color::White, serde_json::from_str::<Color>("\"White\"")?);
    assert_eq!(
        Color::Rgb(255, 0, 255),
        serde_json::from_str::<Color>("{\"Rgb\":[255,0,255]}")?
    );
    assert_eq!(
        Color::Indexed(10),
        serde_json::from_str::<Color>("{\"Indexed\":10}")?
    );
    Ok(())
}

Fixed thanks to https://bgrande.de/blog/custom-deserialization-of-multiple-type-field-from-json-in-rust/

[SleepySwords]

I have made the deserializer accept the existing serialized format. Are we still considering this a breaking change, since we changed how things are serialized?

[joshka]

A couple of small things to add regarding comments, but otherwise good to go.

[joshka]

Are we still considering this a breaking change, since we changed how things are serialized?

Nope - this might break people with tests for exact serialization to strings, but that's really everything. I don't consider that a breaking change worth worrying about.

[joshka]

Generally approving the code, still a couple of minor things to get it over the line.

[SleepySwords]

Would it be fine if the value being deserialized is not a string or one of the old formats (for example passing an empty array), it would create this error Failed to parse Colors: data did not match any variant of untagged enum Helper? Or should I rename Helper to Color?

[joshka]

Thanks for fixing up the requested changes (and apologies in the delay in response). Merging this.