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

Colored test output #603

Open
phil-opp opened this issue May 7, 2019 · 22 comments
Open

Colored test output #603

phil-opp opened this issue May 7, 2019 · 22 comments

Comments

@phil-opp
Copy link
Owner

phil-opp commented May 7, 2019

See #591 (comment)

@matthew-a-thomas

This comment has been minimized.

@matthew-a-thomas

This comment has been minimized.

@matthew-a-thomas

This comment has been minimized.

@phil-opp
Copy link
Owner Author

@matthew-a-thomas Looks great, thanks for creating this!

I think this is a bit too complex for adding it to the blog, but how about including it as an external crate? This also has the advantage that people can use it for their own projects without needing to copy the whole code. If you want, we can create the crate under the rust-osdev organization, or you can just create your own repository. What do you think?

Regarding your code:

  • I like the flexibility that the extended_format function gives, but it introduces quite a bit boilerplate and uses a uncommon interface. I think it would be worth to add a few convenience functions for common colors, such as your red_string function above. Perhaps just simple red, blue, green, yellow, etc. functions that allow colored printing with minimal boilerplate.

  • Your Commands::print function could be generalized to all types that implement Display, not just strings. Also, we probably also want print_debug or print_hex methods for using the fmt::Debug or fmt::LowerHex traits.

  • Expanding on the previous point, maybe a different design would work better and allow to keep using the normal formatting traits instead of needing to call print methods. I was thinking of something like this:

    serial_print!("{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}",
        Foreground(Rgb::new(0, 0, 255)), "W",
        Foreground(Rgb::new(0, 255, 0)), "o",
        Foreground(Rgb::new(0, 255, 255)), "o",
        Foreground(Rgb::new(255, 0, 0)), "h"
        Foreground(Rgb::new(255, 0, 255)), "o"
        Foreground(Rgb::new(255, 255, 0))), "o"
        Foreground(Rgb::new(255, 255, 255)), "!"
        Reset()
    );

    The Foreground type would be a struct that implements Display by writing "\x1B[38;2;{}m", ExtendedFormatRgb::from(rgb) and Reset would a struct that implements Display by writing "\x1B[0m". This way, the individual string/number components can still use the formatting traits like {}, {:?}, {:x}, etc.

  • As another alternative, I could imagine a design like this:

    struct Colored<T> {
        foreground: Foreground,
        background: Background,
        inner: T,
    }
    
    impl<T> fmt::Display for Colored<T> where T: fmt::Display {
        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
            write!(self.f, "\x1B[38;2;{}m\x1B[48;2;{}m{}\x1B[0m",
            self.foreground, self.background, self.inner)
          }
    }
    
    impl<T> fmt::Debug for Colored<T> where T: fmt::Debug {
        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
            write!(self.f, "\x1B[38;2;{}m\x1B[48;2;{}m{:?}\x1B[0m",
            self.foreground, self.background, self.inner)
          }
    }
    
    // […] equivalent implementations for the other formatting traits
    
    println!("{}{:?}", Colored::red("hello"), Colored::green(Foo("world")));

    The Colored struct wraps an arbitrary type and implements all formatting traits that the wrapped type implements, prepending the formatting commands and appending the reset command. This way, the type is very easy to use, but it might issue more commands than needed. So maybe it's a good idea to add this type on top of other abstractions, so that the user can decide whether they want to use the simple and easy way or the most performant way.

These are just some ideas from the top of my head, so feel free to dismiss them if you don't like them.

@matthew-a-thomas
Copy link

matthew-a-thomas commented Jul 15, 2019

@phil-opp Awesome feedback. Perfect for a beginner Rustacean like me—thanks!

I see your point about the uncommon interface, and the lack of support for fmt::Debug et al.

I like your idea of decorating things that already implement the formatting traits.

What do you think about this?

use colored::*;

fn main() {
    println!("{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}",
        "Red".fg(red()),
        "Orange".fg(orange()),
        "Yellow".fg(yellow()),
        "Yellow green".fg(yellow_green()),
        "Green".fg(green()),
        "Green cyan".fg(green_cyan()),
        "Cyan".fg(cyan()),
        "Cyan blue".fg(white()).bg(cyan_blue()),
        "Blue".fg(white()).bg(blue()),
        "Blue magenta".fg(white()).bg(blue_magenta()),
        "Magenta".fg(magenta()),
        "Magenta pink".fg(magenta_pink()),
        "Custom color".fg(Rgb::new(123, 231, 111)).bg(Rgb::new(10, 100, 20))
    );
}

Colorful output

Source is here: https://github.com/matthew-a-thomas/colored-rs

To me this feels more expressive without being verbose, but I don't know how idiomatic it is. There are still some wasted characters like a redundant "\x1B[0m" string when you use both foreground and background.

Taking a step back to survey for a moment, I see that we're reinventing the wheel.

External crates already exist, although none that I see will work in a no_std environment. Do you still think it would be valuable to create a crate? If so I think it would be neat to have the crate live under rust-osdev.

@phil-opp
Copy link
Owner Author

What do you think about this?

Looks good!

To me this feels more expressive without being verbose, but I don't know how idiomatic it is. There are still some wasted characters like a redundant "\x1B[0m" string when you use both foreground and background.

If it's still correct (i.e. no invalid control sequences), I think that's fine for now.

Taking a step back to survey for a moment, I see that we're reinventing the wheel.

I looked through a few existing crates. Your approach seems most similar to https://github.com/mackwic/colored. However, that crate (and most others) depend on the standard library, so I think it's still worth to create a simple no_std crate. Also, I like that your code works with all formatting traits and isn't hardcoded to &str/String.

If so I think it would be neat to have the crate live under rust-osdev.

Cool! I sent you an invite to the organization. You should then be able to move your repository if you like or create a new one. You will still have admin permissions on the repository of course.


Note that we probably need a different name for the crate in order to publish it to crates.io, given that colored is already taken. Do you have any ideas?

@matthew-a-thomas
Copy link

matthew-a-thomas commented Jul 16, 2019

I sent you an invite to the organization. You should then be able to move your repository if you like or create a new one.

Awesome, thanks! I'll work on that next chance I get.

Note that we probably need a different name for the crate in order to publish it to crates.io, given that colored is already taken. Do you have any ideas?

Hmm. I'm brainstorming. It will have these advantages over other ANSI color crates I'm seeing:

  • Works in no_std
  • Very simple (just colors)
  • Colors all the formatting traits
  • Supports all RGB colors

Maybe its name can reflect these things.

I don't think it should be named something like ansi or color (even if those names were free) because this does only a couple of ANSI control strings, neither does it do much at all with color (only RGB). But then again we don't want some ridiculous name like no_std_compliant_simple_ansi_terminal_rgb_colors.

Here are some names that aren't listed in crates.io:

I guess American English is okay? I naturally write "color" not "colour", and there's a 7.6:1 ratio of "color" to "colour" on crates.io.

I think I like "rgb_ansi" (edit: I like "ansi_rgb" better) but I'm not dead set on it. What are your preferences, @phil-opp?

@phil-opp
Copy link
Owner Author

I prefer American English too. I don't have any strong preferences about the name, but I think ansi in the name makes sense to differentiate it from other color crates (e.g. for RGB-HSV conversion or for image manipulation). I agree that ansi_color is too similar to ansi_colours.

@matthew-a-thomas
Copy link

matthew-a-thomas commented Jul 16, 2019

@phil-opp I've never moved a repository in GitHub before, nor have I done anything with Teams. I think it's showing: I renamed my repo to ansi_rgb and transferred ownership to rust-osdev, then created the ansi_rgb team, then noticed that I should have done those two things in the reverse order because now I don't have admin privileges on the repo to give it to the team 🤦‍♂ Could you add the repo to the team for me?

@phil-opp
Copy link
Owner Author

No worries! I added the repository to the team, giving it write access. You personally have full admin access now, so feel free to change the repository and permission settings however you like.

I think we should push an initial release to crates.io soon to reserve the name.

@phil-opp
Copy link
Owner Author

Also, we should include the project in https://github.com/rust-osdev/about. Feel free to add yourself to the members list too.

@matthew-a-thomas
Copy link

@phil-opp Boom! https://crates.io/crates/ansi_rgb

Will update documentation, package better, etc later

@phil-opp
Copy link
Owner Author

Awesome! I try to include it on the blog soon.

@matthew-a-thomas
Copy link

matthew-a-thomas commented Jul 17, 2019

Awesome! I try to include it on the blog soon.

@phil-opp The crate works so it'd be okay for people to use, but there are a few things I'd like to tidy up. Would it be okay to wait until 0.2.0 is released (targeting next Monday)? In the meantime feel free to add issues for other things I've missed

@phil-opp
Copy link
Owner Author

Sure, this can wait until you feel that it's ready for public consumption. At the moment, I'm quite short on time anyway, so no need to hurry. Thanks for all your work!

@matthew-a-thomas
Copy link

@phil-opp 0.2.0 came faster than I expected: https://crates.io/crates/ansi_rgb/0.2.0 🎉

@phil-opp
Copy link
Owner Author

Awesome!

@64
Copy link

64 commented Jul 23, 2019

FWIW I have a basic ANSI escape sequence parser in my kernel which processes the output of println and determines the correct color to write to the framebuffer. Mostly it is just a simple way of controlling text color without needing any color setting APIs in the kernel. It could be a good fit for this crate if there’s a demand?

@matthew-a-thomas
Copy link

@64 Consuming ANSI escape sequences definitely sounds useful. However, consumption of ANSI escape sequences is a separate concern than producing them, and I suspect very little code would be common between the two. Is there a compelling reason to bundle both concerns up into the same crate?

If we decide the reason we want to begin consuming escape sequences is because this crate produces them, I foresee the following pressures being applied:

  • When we produce another escape sequence, people will expect consumption to work, too
  • When we consume another escape sequence, people will expect to be able to produce it, too

Those pressures applied over time will tend to push scope boundaries, so at some point we'd limit the crate's scope sufficiently to prevent that from happening "too much". So why not limit the scope now?

What do you think: what are some advantages of expanding the functionality of this crate versus just having two crates?

@64
Copy link

64 commented Jul 23, 2019

There’s only so many escape sequences you can actually implement with VGA text mode, at least, so I don’t see the ‘pressure over time’ being a particular concern.

I guess there’s not much reason to have it together other than the fact that it’s a nice logical unit to have all color handling in one crate. I’m generally not a fan of micro crates but it’s down to personal preference at the end of the day; I won’t strongly object. It’s just a little bit extra maintenance overhead for everyone involved when there’s a new crate.

@matthew-a-thomas
Copy link

There’s only so many escape sequences you can actually implement with VGA text mode, at least, so I don’t see the ‘pressure over time’ being a particular concern.

@64 I'm not trying to be difficult, but this suggests to me another reason to hesitate in putting them together: ansi_rgb is intended to work in all environments (that have a terminal); your proposal presumably is for environments having a framebuffer, VGA text mode, or something like that: those are often not the same environment.

I feel like I'm thinking too subjectively about this. What concrete information would help us make this decision? For example, if we knew what percent of people who produce RGB escape sequences also need to interpret them then we wouldn't have to make a subjective decision.

But I see your point about producing and consuming being logically connected.

I would be interested in seeing your code. If you haven't already I'm sure it'd be possible to abstract it out so that the concept of e.g. a framebuffer isn't necessary in the crate. For example, there could just be a function in the crate that tokenizes a string, and elsewhere (outside the crate where the framebuffer concept is necessary) the tokens could be translated into commands against the framebuffer.

@64
Copy link

64 commented Jul 24, 2019

Well, everyone who consumes them is also going to need to produce them (otherwise what’s the point). As for the number of people who actually want to consume them, it’s probably quite low but I’d wager that more people would use it if it was available because it’s an easy way to control color through println alone.

At the moment it just produces bytes which control color in the VGA text mode, i.e the upper byte at each position. But as you say it could be abstracted into something more general then converted into that representation later, depending on video mode.

The code itself is pretty rough and ready with magic numbers scattered around the place because I ported it from a C version I wrote a while back and have been meaning to refactor it properly for a while (and moving it into a crate others could use is a good chance for that).

https://github.com/64/solstice/blob/master/src/drivers/vga/ransid.rs

IDK, maybe it’s unnecessary to make a crate for it, it’s just hard to gauge demand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants