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

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

Merged
merged 35 commits into from
Dec 12, 2023
Merged

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

merged 35 commits into from
Dec 12, 2023

Conversation

JustAnotherCodemonkey
Copy link
Contributor

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.

…ns from ColoredString into CopyColorize, and add StyleTemplate struct to assist with CopyColorize.
…'s. Also, add doc for Styles and improve tests.
… None' and generally improve ColorizedMut docs.
…cer and change the wording on the first section (renamed to "Creation")
Merge branch 'master' into JustAnotherCodemonkey/enable-coloredstring-inplace-modification
…olorize as they are now unnesecary, and modify docs to reflect the new idioms.
@kurtlawrence
Copy link
Collaborator

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.

…ngelog to reflect this. Also, mark ColoredString as non_exhaustive.
@JustAnotherCodemonkey
Copy link
Contributor Author

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 kurtlawrence added the breaking Will need a major version bump label Dec 9, 2023
@kurtlawrence
Copy link
Collaborator

The changes to Colorize will be breaking.

@JustAnotherCodemonkey
Copy link
Contributor Author

The changes to Colorize will be breaking.

Wait I don't understand. How so?

@kurtlawrence
Copy link
Collaborator

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

@JustAnotherCodemonkey
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Collaborator

Just need to fix the formatting

@JustAnotherCodemonkey
Copy link
Contributor Author

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
Copy link
Collaborator

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

@JustAnotherCodemonkey
Copy link
Contributor Author

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

@kurtlawrence kurtlawrence merged commit 949f601 into colored-rs:master Dec 12, 2023
10 of 14 checks passed
@kurtlawrence
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Will need a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants