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

Special case a few colors for Windows #36178

Merged
merged 1 commit into from
Sep 3, 2016
Merged

Conversation

sophiajt
Copy link
Contributor

As brought up on this thread the colors used in error messages on Windows can be difficult to read because of the lack of bold.

This PR makes a few changes to improve readability, namely:

  • Rather than using BRIGHT_BLUE, on Windows we now use BRIGHT_CYAN, which is easier to read on black when you do not have bold
  • We used BRIGHT_YELLOW rather than YELLOW, for the same reason
  • Titles will be BRIGHT_WHITE now, to give the illusion of being bold

Some examples:

warning

error

r? @nikomatsakis

cc @retep998

try!(self.start_attr(term::Attr::ForegroundColor(term::color::BRIGHT_CYAN)));
} else {
try!(self.start_attr(term::Attr::ForegroundColor(term::color::BRIGHT_BLUE)));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

let fg_color = if cfg!(windows) {
    term::color::BRIGHT_CYAN
} else {
    term::color::BRIGHT_BLUE
};
try!(self.start_attr(term::Attr::ForegroundColor(fg_color)));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even

try!(self.start_attr(term::Attr::ForegroundColor(
    if cfg!(windows) {
        term::color::BRIGHT_CYAN
    } else {
        term::color::BRIGHT_BLUE
    }
)));

But these seem like pretty minor details?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But these seem like pretty minor details?

Yep, hence the "Nit" ;)

@sophiajt
Copy link
Contributor Author

sophiajt commented Sep 1, 2016

For comparison, here's regular cyan:

cyan_secondary2

@sophiajt
Copy link
Contributor Author

sophiajt commented Sep 2, 2016

Chatted with Niko on IRC, and sounds like we're good. r=him

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 2, 2016

📌 Commit 1b04762 has been approved by nikomatsakis

@sophiajt
Copy link
Contributor Author

sophiajt commented Sep 2, 2016

@bors rollup

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 2, 2016
…ikomatsakis

Special case a few colors for Windows

As brought up on [this thread](rust-lang#33240 (comment)) the colors used in error messages on Windows can be difficult to read because of the lack of bold.

This PR makes a few changes to improve readability, namely:
* Rather than using BRIGHT_BLUE, on Windows we now use BRIGHT_CYAN, which is easier to read on black when you do not have bold
* We used BRIGHT_YELLOW rather than YELLOW, for the same reason
* Titles will be BRIGHT_WHITE now, to give the illusion of being bold

Some examples:

![warning](https://cloud.githubusercontent.com/assets/547158/18148466/9aa9bbe2-6f8e-11e6-927f-d0eec53cac32.PNG)

![error](https://cloud.githubusercontent.com/assets/547158/18148488/ba9fb186-6f8e-11e6-8d8e-e93d569f61de.PNG)

r? @nikomatsakis

cc @retep998
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 2, 2016
…ikomatsakis

Special case a few colors for Windows

As brought up on [this thread](rust-lang#33240 (comment)) the colors used in error messages on Windows can be difficult to read because of the lack of bold.

This PR makes a few changes to improve readability, namely:
* Rather than using BRIGHT_BLUE, on Windows we now use BRIGHT_CYAN, which is easier to read on black when you do not have bold
* We used BRIGHT_YELLOW rather than YELLOW, for the same reason
* Titles will be BRIGHT_WHITE now, to give the illusion of being bold

Some examples:

![warning](https://cloud.githubusercontent.com/assets/547158/18148466/9aa9bbe2-6f8e-11e6-927f-d0eec53cac32.PNG)

![error](https://cloud.githubusercontent.com/assets/547158/18148488/ba9fb186-6f8e-11e6-8d8e-e93d569f61de.PNG)

r? @nikomatsakis

cc @retep998
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 2, 2016
…ikomatsakis

Special case a few colors for Windows

As brought up on [this thread](rust-lang#33240 (comment)) the colors used in error messages on Windows can be difficult to read because of the lack of bold.

This PR makes a few changes to improve readability, namely:
* Rather than using BRIGHT_BLUE, on Windows we now use BRIGHT_CYAN, which is easier to read on black when you do not have bold
* We used BRIGHT_YELLOW rather than YELLOW, for the same reason
* Titles will be BRIGHT_WHITE now, to give the illusion of being bold

Some examples:

![warning](https://cloud.githubusercontent.com/assets/547158/18148466/9aa9bbe2-6f8e-11e6-927f-d0eec53cac32.PNG)

![error](https://cloud.githubusercontent.com/assets/547158/18148488/ba9fb186-6f8e-11e6-8d8e-e93d569f61de.PNG)

r? @nikomatsakis

cc @retep998
bors added a commit that referenced this pull request Sep 3, 2016
Rollup of 12 pull requests

- Successful merges: #35754, #35793, #36099, #36160, #36171, #36178, #36180, #36190, #36198, #36205, #36210, #36223
- Failed merges:
@bors bors merged commit 1b04762 into rust-lang:master Sep 3, 2016
@@ -896,6 +900,9 @@ impl Destination {
}
Style::OldSchoolNoteText | Style::HeaderMsg => {
try!(self.start_attr(term::Attr::Bold));
if cfg!(windows) {
try!(self.start_attr(term::Attr::ForegroundColor(term::color::BRIGHT_WHITE)));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... This is annoying for me since I use console with white background on Windows. These messages are completely invisible there. (Windows uses black background by default... but I'm used to seeing white background...) I guess a solution would be to detect the console setting in libterm (probably via GetConsoleScreenBufferInfo), and only apply these Windows-specific cases when the background is black...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want a white background, couldn't you just invert the brightness of all the colors so the background is "black" but "black" actually means white, and "bright_white" actually means black?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fair. I didn't know I can do that.

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

Successfully merging this pull request may close these issues.

6 participants