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

Making default color scheme compatible for windows terminal displays #222

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

robinbernon
Copy link
Contributor

Fixing default color scheme to be compatible with windows terminal displays.

@robinbernon
Copy link
Contributor Author

@pksunkara thoughts?

@pksunkara
Copy link
Collaborator

Looks okay to me, but I am wondering which windows terminals doesnt support these chars?

@pksunkara
Copy link
Collaborator

I am thinking that this should not be gated on OS but on term application's unicode support.

@robinbernon
Copy link
Contributor Author

These Unicode characters are just not supported on any windows terminals right now. Perhaps a request to support them could be filed somewhere but for now I think the changes will have to be made here.

@robinbernon
Copy link
Contributor Author

robinbernon commented Oct 24, 2022

I am thinking that this should not be gated on OS but on term application's unicode support.

I'm guessing this is just a windows problem. Guessing all Unix based systems should be able to support the same list of Unicode chars? (Or at lease should all include the ones here?)

@pksunkara
Copy link
Collaborator

Please use https://docs.rs/console/latest/console/struct.TermFeatures.html#method.wants_emoji instead of gating on OS. And please make sure to test it manually on your terminal.

@robinbernon
Copy link
Contributor Author

Please use https://docs.rs/console/latest/console/struct.TermFeatures.html#method.wants_emoji instead of gating on OS. And please make sure to test it manually on your terminal.

@pksunkara I've done as asked. It's decided by .wants_emoji() now :)

#[cfg(feature = "fuzzy-select")]
fuzzy_match_highlight_style: Style::new().for_stderr().bold(),
inline_selections: true,
if Term::stdout().features().wants_emoji() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can store this in a variable and use it in emoji lines instead of this big if condition block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry do you mind clarifying what you mean here? I dont understand. Do you want a variable for the term features?

Copy link
Collaborator

Choose a reason for hiding this comment

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

let has_emoji = Term::stderr().features().wants_emoji();

// later
ColorfulTheme {
  error_prefix: style((if has_emoji { "✘" } else { "×" }).to_string()).for_stderr().red(),
}

@Gordon01
Copy link
Contributor

Gordon01 commented Sep 4, 2023

Hey, @robinbernon can you make the changes, please? It would be valuable for Windows users.

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.

3 participants