use console api instead of ansi codes on windows #6203

mwrock · 2019-02-23T02:32:57Z

Versions of Windows prior to Windows 10 and server 2016 do not support ANSI color control codes and render them as "gibberish" test. This means that on many "not so legacy" windows environments, Habitat output looks very unsightly.

Server 2012 R2:

The ubiquitous way to do color in a windows terminal has been to leverage the SetConsoleMode API.

The termcolor crate leverages that API on Windows providing cross platform terminal color support for all of the versions of Windows that Habitat supports.

This PR refactors our common::UI struct to use this crate and discards the term and ansi_term crates. The term crate is no longer supported and so this has the added benefit of moving us off of that crate. This also replaces Blue with Magenta in our UIWriter.end function because blue is virtually invisible with the default windows console backgrounds.

Here is the same output above with the changes in this PR:

Signed-off-by: mwrock matt@mattwrock.com

thesentinels · 2019-02-23T02:32:59Z

Thanks for the pull request! Here is what will happen next:

Your PR will be reviewed by the maintainers
If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

Signed-off-by: mwrock <matt@mattwrock.com>

mwrock · 2019-02-23T03:09:36Z

fixes #5922 and #2122

stevendanna · 2019-02-24T18:33:48Z

components/sup/src/ctl_gateway/mod.rs

@@ -185,6 +169,17 @@ impl io::Write for CtlRequest {
        // be nice to find a cleaner way to model this, but the fact
        // that CtlRequest is sending output to two destinations with
        // different formatting requirements complicates things a bit.
+        //
+        // TODO (MW): For `han sup run` scenarios, it would be nice to


baumanj · 2019-02-25T17:03:37Z

Looks like you need to locally update to nightly rusfmt based on the CI failure.

Signed-off-by: mwrock <matt@mattwrock.com>

baumanj

I haven't been through every line, but since I'm suggesting some potentially significant changes, I thought it best to get the conversation going.

baumanj · 2019-02-25T17:06:46Z

components/common/src/ui.rs

-            Status::Verified => ('✓', "Verified".into(), Colour::Green),
-            Status::Verifying => ('☛', "Verifying".into(), Colour::Green),
-            Status::Custom(c, ref s) => (c, s.to_string(), Colour::Green),
+            Status::Applying => ('↑', "Applying".into(), Color::Green),


As long as we're making a change here, what about changing the code from using color in a purely descriptive way (i.e., "green") to a semantic one (e.g., "success")? That way we can set up these mappings in one place and provide a mechanism for re-mapping the specific color choices by the user if the scheme we chose doesn't work due to their terminal defaults or visual impairment.

I'd prefer to leave that for another PR.

I'm ok with that, but can we file an issue and take it on as a direct follow-up? I'd like to take advantage of this code being in our headspace now.

I would love to see this as a feature.

baumanj · 2019-02-25T17:09:11Z

components/common/src/ui.rs

@@ -130,18 +132,18 @@ pub trait UIReader {
    fn prompt_yes_no(&mut self, question: &str, default: Option<bool>) -> Result<bool>;
 }

+pub trait ColorPrinter {
+    fn print(&mut self, buf: &[u8], color: Option<Color>, bold: bool) -> io::Result<()>;


Instead of None, what about termcolor::NoColor?

Instead of a bool for bold, can we use an enum that would make the semantics clearer from the caller? So instead of calls like:

self.out().print( format!("{} {}\n", symbol, message).as_bytes(), Some(Color::Yellow), true,

we can have

self.out().print( format!("{} {}\n", symbol, message).as_bytes(), Some(Color::Yellow), Weight::Bold,

Even better may be replacing both color and bold parameters with a termcolor::ColorSpec which can carry all that information and providing some convenience functions for returning instances configured with our common desired values. That would make the above something like:

self.out().print( format!("{} {}\n", symbol, message).as_bytes(), TextStyle::yellow_bold(),

Do you mean termcolor::NoColor vs an Option?

Yeah a weight enum seems like it would be more clear. I had thought about just passing a ColorSpec but it felt like that made alot of the calling code unnecesarily verbose.

I think making the core interface totally general (i.e., ColorSpec) and building convenience functions into the trait on top of that to make our common usage ergonomic would be good. Most importantly, I don't want the callers to pass parameters values like false and None, when we can include more evocative text like bold and yellow (or, down the road a semantic identifier like info or warning)

yeah we could add functions like fn create_no_color_spec() -> ColorSpec and fn create_color_spec(color: Color, weight: Weight) -> ColorSpec

baumanj · 2019-02-25T19:58:52Z

components/common/src/ui.rs

@@ -130,18 +132,18 @@ pub trait UIReader {
    fn prompt_yes_no(&mut self, question: &str, default: Option<bool>) -> Result<bool>;
 }

+pub trait ColorPrinter {
+    fn print(&mut self, buf: &[u8], color: Option<Color>, bold: bool) -> io::Result<()>;
+}


It's a common idiom in rust to provide output functions with a -ln suffixed variant. Since we do that several places here, why not provide a println as well?

Suggested change

}

fn println(&mut self, buf: &[u8], color: Option<Color>, bold: bool) -> io::Result<()> {

self.print(buf, color, bold)

.and_then(|_| self.print(&[b'\n'], None, false))

}

}

yeah that makes total sense.

baumanj · 2019-02-25T20:14:47Z

components/common/src/ui.rs

@@ -130,18 +132,18 @@ pub trait UIReader {
    fn prompt_yes_no(&mut self, question: &str, default: Option<bool>) -> Result<bool>;
 }

+pub trait ColorPrinter {


Why define our own trait when we could use termcolor::WriteColor?

Since our streams will implement Write as well, we can use the standard write and writeln macros.

Having a one liner color printing call seemed much simpler. I ended up adding this trait for the sake of ctrl-gateway/src/mod.rs. Not sure how you would handle its needs and implement WriteColor.

I'll defer to your judgement for what gives better ergonomics in the end. The nice thing about building upon termcolor::WriteColor is that we could keep a single code path for all our printing and just enable/disable colors with ColorChoice.

If we find code like this

buffer.set_color(ColorSpec::new().set_fg(Some(Color::Green)))?; writeln!(&mut buffer, "green text!")?; bufwtr.print(&buffer)?;

to be too verbose, there's no reason we can't build helpers on top of it like:

our_helper.print("green text!", Color::Green, Weight::Normal)?;

or

our_helper.set_color(Green).print("green text!")?;

while still retaining the logic-unifying goodness of termcolor::WriteColor in the underlying implementation.

Yeah my first draft used WriteColor but that breaks down in the ctrl-gateway when sending colorized messages through the ConsoleLine protocol where the actual coloring and underlying console API calls need to happen on the gateway client.

Can you show me a little more specifically where your approach ran into trouble?

I was hoping that the termcolor::Buffer would be serializable but its not :( Maybe you know of a way to send a Buffer through prost but I think it has to implement Serializable

I'm thinking of this more and I think I see how to do this with WriteColor. I'll play with it.

Sounds great. Let me know if you want input on anything.

Signed-off-by: mwrock <matt@mattwrock.com>

baumanj

This is awesome!

Obvious fix; these changes are the result of automation not creative thinking.

mwrock requested review from baumanj, chefsalim, christophermaier, eeyun, fnichol and jamesc as code owners February 23, 2019 02:32

mwrock requested a review from raskchanky as a code owner February 23, 2019 02:32

use console api instead of ansi codes on windows

df0bc2f

Signed-off-by: mwrock <matt@mattwrock.com>

mwrock force-pushed the termcolor branch from a30a205 to df0bc2f Compare February 23, 2019 03:00

mwrock added A-supervisor labels Feb 23, 2019

stevendanna reviewed Feb 24, 2019

View reviewed changes

mwrock added 5 commits February 25, 2019 10:45

run rustfmt

1fe19f7

Signed-off-by: mwrock <matt@mattwrock.com>

run rustfmt with feeling

7c0c6ec

Signed-off-by: mwrock <matt@mattwrock.com>

fix some test uses

0a87d9d

Signed-off-by: mwrock <matt@mattwrock.com>

fix more test uses

cf06344

Signed-off-by: mwrock <matt@mattwrock.com>

fix typo in comment

2e312b8

Signed-off-by: mwrock <matt@mattwrock.com>

baumanj suggested changes Feb 25, 2019

View reviewed changes

mwrock added 4 commits February 26, 2019 16:42

leverage WriteColor trait instead of ColorPrinter

f9123b3

Signed-off-by: mwrock <matt@mattwrock.com>

use ColorSpec for print

4b9ea17

Signed-off-by: mwrock <matt@mattwrock.com>

add println

10411d8

Signed-off-by: mwrock <matt@mattwrock.com>

remove BadWeight and use write_all

25e4ee2

Signed-off-by: mwrock <matt@mattwrock.com>

baumanj approved these changes Feb 27, 2019

View reviewed changes

mwrock merged commit 575762b into master Feb 28, 2019

mwrock deleted the termcolor branch February 28, 2019 19:09

chef-ci added a commit that referenced this pull request Feb 28, 2019

Update CHANGELOG.md with details from pull request #6203

2d8559f

Obvious fix; these changes are the result of automation not creative thinking.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use console api instead of ansi codes on windows #6203

use console api instead of ansi codes on windows #6203

mwrock commented Feb 23, 2019

thesentinels commented Feb 23, 2019

mwrock commented Feb 23, 2019

stevendanna Feb 24, 2019

baumanj commented Feb 25, 2019

baumanj left a comment

baumanj Feb 25, 2019

mwrock Feb 25, 2019

baumanj Feb 25, 2019

christophermaier Feb 26, 2019

baumanj Feb 25, 2019

baumanj Feb 25, 2019

mwrock Feb 25, 2019

mwrock Feb 25, 2019

baumanj Feb 25, 2019

mwrock Feb 25, 2019

baumanj Feb 25, 2019

mwrock Feb 25, 2019

baumanj Feb 25, 2019

mwrock Feb 25, 2019

baumanj Feb 25, 2019

mwrock Feb 25, 2019

baumanj Feb 25, 2019

mwrock Feb 25, 2019

mwrock Feb 25, 2019

baumanj Feb 25, 2019

baumanj left a comment

-}
+    fn println(&mut self, buf: &[u8], color: Option<Color>, bold: bool) -> io::Result<()> {
+        self.print(buf, color, bold)
+            .and_then(|_| self.print(&[b'\n'], None, false))
+    }
+}

use console api instead of ansi codes on windows #6203

use console api instead of ansi codes on windows #6203

Conversation

mwrock commented Feb 23, 2019

thesentinels commented Feb 23, 2019

mwrock commented Feb 23, 2019

Choose a reason for hiding this comment

baumanj commented Feb 25, 2019

baumanj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baumanj left a comment

Choose a reason for hiding this comment