colored-rs / colored

(Rust) Coloring terminal so simple you already know how to do it !
Mozilla Public License 2.0
1.68k stars 82 forks source link

Add and Expose Methods of easily Creating and Mutating `ColoredString`'s and `Style`'s for Users #154

Closed JustAnotherCodemonkey closed 9 months ago

JustAnotherCodemonkey commented 9 months ago

I know this is a lot but most of it is either long documentation or impls for Style or Styles. I can easily downgrade certain parts if necessary or split them off into separate PRs as I am starting to think to do with one feature in particular.

Changes

The changes are all laid out in the changelog and I'd highly recommend looking at that as it will make it vastly easier to understand what the code is and is trying to do. If you see the point in all the changes listed, feel free to skip the Motivation section.

Motivation

This all started when I was trying to make an adventure game and wanted to have a function that took a Vec of ColoredString's and would return an iterator of new ColoredString's that were each a single character of their respective ColoredString and would maintain the coloring and style. Should be easy. The problem: I quickly realized that there are several things that cannot be done easily or at all in colored as it stands. This PR addresses a whole bunch of them with a cluster of solutions that open the door wide in terms of possibilities. Here are some of the problems:

  1. No ability to change the text of ColoredString's. They deref to str and thus you can make String's off of them, however, 1. there is no way of doing this in-place, which would be important for performance and 2. this means you now need to re-create foreground color, background color, and style from one ColoredString to another and that last one, style, is exceptionally difficult as it stands. I mean maybe I'm just stupid but just try to copy the style of one ColoredString to another.
  2. No easy ability to copy style from one ColoredString to another. This was kind of addressed in the previous bit but although you can get a Style from a ColoredString, you can't really use it other than to just check if any specific style switch is activated and compare them with ==.
  3. No ability to create Style's for the user. This kind of goes hand-in-hand with the previous point. Also, Style does not implement Default which was preventing a deriving of Default for ColoredString which was another PR (i.e. that one made a default for both).
  4. No way to manipulate existing Style's such as combining them or subtracting one from another.
  5. No way to easily modify the attributes of ColoredString stored in its fields. Those fields are now exposed.

There are likely other problems I had but can't remember atm. It's 3:45 AM in my time zone at time of writing.

Breaking Changes

There is one (1) breaking change and it's one of the features of this PR I'm willing to part with: ColoredString now derefs to String instead of str. Why this is breaking and also why it's very uncommon for it to actually break anything is explained in the changelog.

Overall, I think I'm pretty satisfied with the changes. There might be one or two I've left out but they can happen in their own PRs. I think it would enable so much, especially performance-wise as the workarounds, when possible, are very ugly. I sure hope this one doesn't die on Capital Hill.

kurtlawrence commented 9 months ago

I think we could leave the Deref<str> to avoid the breaking.

Since you are exposing input, mutating the inner string can simply be done via a field reference.

Probably should mark ColoredString as #[non_exhaustive] as well.

JustAnotherCodemonkey commented 9 months ago

I think we could leave the Deref<str> to avoid the breaking.

Since you are exposing input, mutating the inner string can simply be done via a field reference.

Probably should mark ColoredString as #[non_exhaustive] as well.

You got it. I kept DerefMut though as I didn't see any reason why not. Changes made in 94e12e7.

kurtlawrence commented 9 months ago

The changes to Colorize will be breaking.

JustAnotherCodemonkey commented 9 months ago

The changes to Colorize will be breaking.

Wait I don't understand. How so?

kurtlawrence commented 9 months ago

The addition of the two (non-default) methods is a breaking change.

JustAnotherCodemonkey commented 9 months ago

The addition of the two (non-default) methods is a breaking change.

Ah I now see. I didn't consider that we'd be considering that users would be implementing Colorize on their own types. I do like the new methods though and the amount of real-world code their inclusion would break would likely (in my opinion) be very small. Can we keep them? I can part with them too if necessary though.

kurtlawrence commented 9 months ago

Unfortunately, it is still a breaking change, no matter how small the impact might be. You could add them with default implementations (calling out to the appropriate methods). This way we would avoid a breaking change.

JustAnotherCodemonkey commented 9 months ago

Ok I basically just removed those from the PR. I'd really like a method that could do the with_style thing as I think that would be super useful but I don't see a way to do that and have it have a default implementation. Maybe we could add those in a separate PR that would go in v3 (which to my understanding has other planned breaking changes)

kurtlawrence commented 9 months ago

Yeah, go with another PR which merges targeting the v3 branch. I'll merge in these changes, thanks for the PR.

Please note that v3 will likely make a change to ColoredString which will generalise the backing str type.

kurtlawrence commented 9 months ago

Just need to fix the formatting

JustAnotherCodemonkey commented 9 months ago

Please note that v3 will likely make a change to ColoredString which will generalize the backing str type.

I saw that! Very exciting imo. I really like the idea that initially sparked the PR of being able to use Cow's.

kurtlawrence commented 9 months ago

If you'd like, you are welcome to open a PR with the changes for v3

JustAnotherCodemonkey commented 9 months ago

Don't know what to do about these last 2 tests I don't think it has anything to do with my PR...?

kurtlawrence commented 9 months ago

Don't know what to do about these last 2 tests I don't think it has anything to do with my PR...?

I think it may be a bug in the windows terminal environment code