More colors and some minor changes

[hanmertens]

I was in the need for 3/4-bit ANSI escapes for a no_std project, so I decided to try to add them to this crate even though they're technically not rgb. I'm curious if you're nevertheless open to extending the crate to more color types.

While working on this, I also did some unrelated minor changes that I've included:

• Supertrait Colorable for easy importing (reverted because it does not work; only merging the Background and Foreground traits would make it possible get the fg and bg functions both with a single import)
• Convenience functions fg_by_ref and bg_by_ref that borrow instead of consume the object they're coloring. I was looking for a way to make var.fg(color) work for non-copy owned items without running into "temporary value dropped while borrowed" #3 and this is the best thing I could think of that worked. I think var.fg_by_ref(color) looks slightly nicer than having to do (&var).fg(color), but it's not the elegant solution I was hoping for. (removed, see later comments)
• Update of rgb crate without requiring features that are not used
• Formatting changes

Let me know if I should cherry pick only some of these or if they are better split off into a separate PR.

[matthew-a-thomas]

@hanmertens Thanks so much for making this PR! I appreciate your willingness to spend time making this better. I have a couple of thoughts I'd like to discuss with you.

First, I'm not sure I want to include escape sequences for anything but 24 bit colors. Mainly because I put "RGB" in the name and have tried to make it as small and focused as possible. You can see me discussing the name and purpose of this crate a while back with the owner of the rust-osdev organization here. But at the same time I'm not dead set against it because its original purpose was "support RGB colors" not "support only RGB colors".

So let's continue under the assumption we'll add more than RGB. In that case I'm not sure this PR has the right abstractions. Allow me to explain. Everything boils down to this code:

impl fmt::Display for EscapeCode {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        let base = match self.canvas {
            Canvas::Background => 40,
            Canvas::Foreground => 30,
        };
        match self.color {
            Color::Color4(color4) => write!(
                f,
                "\x1B[{}{}m",
                if color4.bright { "1;" } else { "" },
                base + color4.color3 as i32
            ),
            Color::Rgb(rgb) => write!(f, "\x1B[{};2;{};{};{}m", base + 8, rgb.r, rgb.g, rgb.b),
        }
    }
}

That match expression is setting off little alarms in my head because neither match expressions nor enums are open for extension/closed for modification. Now I don't want to be too hard on you because neither was my code. This PR is proof you had to modify my code instead of being able to just implement a trait for a new struct haha. But now that we're talking about extending support I think this is worth considering. So what will happen when we inevitably decide to support 8 bit colors too? The Color enum and the above code would both have to change. Is there a better way?

I see why you brought the concept of foreground/background into the logic that fabricates the escape codes. That's a handy abstraction that allows all the escape code logic to be in one place—let's keep that concept. So to make the code open for extension let's draw a box around that match expression and turn it into a function/trait:

pub trait Color {
  fn fmt(&self, f: &mut fmt::Formatter, canvas: Canvas) -> fmt::Result
}

(I think the Color enum could go away and this could take its place. EscapeCode might also go away)

Now to add a new type of color we can just:

pub struct Color8Bit {...}

impl Color for Color8Bit {
  fn fmt(&self, f: &mut fmt::Formatter, canvas: Canvas) -> fmt::Result {
    // todo: implement https://en.wikipedia.org/wiki/ANSI_escape_code#8-bit
  }
}

Hooray! Extension without modification!

To make this work we'll have to tweak WithForeground and WithBackground (I think they'll need a type parameter that implements this Color trait) along with some other stuff (like the impl_me macros).

Now, this PR also reduces the RGB crate footprint. That's great! Thanks for that.

Formatting changes. Great! I'm paid to program in another language so I'm learning how the Rust community does things.

fg/bg_by_ref... good idea but I'm not sure this is necessary to include in this crate. Anyone who wants this convenience can create their own trait with the x_by_ref function and implement it for Foreground/Background. I guess it doesn't hurt to have the convenience functions in this crate, but I would at least move them out of the Foreground/Background traits.

Thoughts? Questions?

[matthew-a-thomas]

@hanmertens I ended up putting into master the open/closed principle modifications I was talking about. That's why there are now conflicts. Let me know your thoughts.

[hanmertens]

The reason I implemented Color as an enum was because there's only a limited number of color ANSI escapes as far as I'm aware (3, 4, 8 and 24 bit), so it would be possible to include all color escapes that way. I indeed wanted to change the enum for that later, but I hadn't gotten around to implementing it yet. Regardless, I think a trait also works fine; I'm going to have a look how to implement 3/4-bit colors this way and update the PR later. It does mean the crate infrastructure then basically allows users to implement generic formatting (which may or may not be what you want), e.g. for bold something like

struct Bold();

impl Color for Bold {
    fn prelude(&self, f: &mut fmt::Formatter, _canvas: Canvas) -> fmt::Result {
        f.write_str("\x1B[1m")
    }
    fn epilogue(&self, f: &mut fmt::Formatter, _canvas: Canvas) -> fmt::Result {
        f.write_str("\x1B[0m")
    }
}

About the *_by_ref functions: I wasn't super convinced by them either, I was hoping for a more elegant solution but I think I'll just drop them.

Another thing: what are your thoughts on merging the Background and Foreground traits into a single one (something like Colorize or Colorable)? There's no types that only support one of the two, and it would cut back on importing (I don't like to do use ansi_rgb::*). The main drawback I can think of is name clashing, e.g. if a crate user already has a bg associated function and only want to use this crate's fg function.

[matthew-a-thomas]

It does mean the crate infrastructure then basically allows users to implement generic formatting (which may or may not be what you want), e.g. for bold

Good point. Although if someone really thinks it's useful to implement a trait from a crate called "ansi_rgb" in such a way that calling a function called "foreground" makes the text sprout wings and fly, more power to them. I tend to take the approach of if they really think they need to juggle with chainsaws then either they'll benefit from the lesson or they're a professional chainsaw juggler. The worst that can happen is they end up writing a monstrous application (still better than many I've written!) and their life as a programmer becomes very difficult, and they learn from it.

Another thing: what are your thoughts on merging the Background and Foreground traits into a single one (something like Colorize or Colorable)? There's no types that only support one of the two, and it would cut back on importing

I like the idea. Let me chew on it for a little bit.

The main drawback I can think of is name clashing, e.g. if a crate user already has a bg associated function and only want to use this crate's fg function.

That wouldn't be a problem that Universal Function Call Syntax couldn't take care of.

[hanmertens]

So I've added the 3/4-bit color types and other stuff again except the *_by_ref methods. I made epilogue a provided method, but noticed you'd just done the same. I'll rebase it to be conflictless when addressing the next feedback/when master is stable.

Regarding the generic formatting: using the current approach it's possible to make a more full-featured ansi_fmt crate with just a single fmt function, to which you can pass things such as Bold, BgRed and the like (the Canvas would then be part of the argument, not the trait). Just a thought, not currently in scope of course.

[matthew-a-thomas]

I made epilogue a provided method, but noticed you'd just done the same. I'll rebase it to be conflictless when addressing the next feedback/when master is stable

Sorry about moving things under your feet. I fixed the conflict.

using the current approach it's possible to make a more full-featured ansi_fmt crate with just a single fmt function

Certainly an interesting idea.

I merged what you have so far. If you want to create another PR for me to look at regarding combining the Foreground and Background traits then please feel free to.

Thanks so much!

[hanmertens]

Sorry about moving things under your feet. I fixed the conflict.

No problem, thanks for merging. I'll have a look at combining Foreground/Background and adding 8-bit colors so the crate has all the ANSI color escapes in the coming days.