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

Support COLORTERM for TrueColor ASCII #235

Closed
atluft opened this issue Oct 6, 2020 · 13 comments
Closed

Support COLORTERM for TrueColor ASCII #235

atluft opened this issue Oct 6, 2020 · 13 comments
Assignees

Comments

@atluft
Copy link
Contributor

atluft commented Oct 6, 2020

After working on the swift logo I was thinking it might be useful to be able to specify true colors for a better result:

image

Terminals that don't support COLORTERM environment variable would look like this:
image

@o2sh
Copy link
Owner

o2sh commented Oct 6, 2020

This is a very good idea @atluft
For the final implementation, you should probably modify the define_languages! macro to have a second Vec! for TrueColor. By Doing that, you will be able to get rid of the offset of 10. Also, It's preferable to read env::var("COLORTERM") only once and pass it along.

@atluft
Copy link
Contributor Author

atluft commented Oct 7, 2020

Will do, new to Rust programming and having your advice helps. All comments are welcome!

@spenserblack
Copy link
Collaborator

@atluft Just curious: did you test how the colored crate renders TrueColor on a terminal that doesn't support it? I would assume that it would pick the closest available color if it can't render the TrueColor.

@spenserblack
Copy link
Collaborator

Nevermind, I checked the source code and it appears that colored doesn't check if COLORTERM is set or not.

I think the best way would probably be to have the define_languages! macro call another macro, like define_colors!, and also define a struct for colors, like

struct Colors {
    default_colors: Vec<Color>,
    true_colors: Option<Vec<Color>>,
}

define_colors! can take 2 patterns:

  • vec![ $( color:expr ),+ ], the current usage with no true colors, would expand to
    Colors {
        default_colors: vec![ $( $color ,)+ ],
        true_colors: None,
    }
  • $color_struct:expr, which would take a definition of the Colors struct, and wouldn't need any special expansion rules.

Does that make sense? 😺

@o2sh
Copy link
Owner

o2sh commented Oct 7, 2020

did you test how the colored crate renders TrueColor on a terminal that doesn't support it

Just tested it on rxvt, it prints it in plain black:

rxvt

@spenserblack
Copy link
Collaborator

spenserblack commented Oct 7, 2020

Thanks, @o2sh!
Besides the above suggestion of adjusting the macro, we could also possibly map over defined colors and, if they're TrueColor and COLORTERM isn't set to an expected value, calculate the closest color ourselves using Euclidean distance (doesn't have to be Euclidean distance, but it's the easiest way to get closest color IMO).

Basically

match color {
    Color::TrueColor if colorterm_not_set => closest_color_euclidean(color),
    _ => color,
}

@spenserblack
Copy link
Collaborator

I made colored-rs/colored#90 for this. If it doesn't get merged, feel free to copy the code into this project 😃

@o2sh
Copy link
Owner

o2sh commented Oct 7, 2020

I made colored-rs/colored#90 for this.

I agree, it would be preferable to have this behavior - fallback to closest Color when TrueColor isn't supported - natively handled by colored

Still, impressive stuff @spenserblack

@atluft
Copy link
Contributor Author

atluft commented Oct 7, 2020

I definitely am not rusty enough for this one :-)

I will noodle around with @o2sh suggestion above another day, it looks like a good direction.

In what I tried today, I had a hard time getting expr tokens to accept a comma suffix followed by a string literal for the languages like "c++". I tried layering the parser so there were several rules for different formats, but it seemed like I could never get all the languages to match a rule or when I did I got error: variable 'x' is still repeating at this depth which I have no idea how to resolve.

End of my day here, have a good day where ever you are!

@spenserblack
Copy link
Collaborator

I got error: variable 'x' is still repeating at this depth

That means there's an un-expanded repeating macro pattern somewhere. Here is a minimal reproduction that hopefully clears it up (Rust's compiler errors for macros aren't the best 🙂 ).

I definitely am not rusty enough for this one :-)

No worries, everyone starts as a beginner 😃 👍

@atluft
Copy link
Contributor Author

atluft commented Oct 8, 2020

Thank you @spenserblack, that was a good example. I was thinking the other way around that there weren't enough matches, your example helps clearly see that the still repeating aspect is very important.

@atluft
Copy link
Contributor Author

atluft commented Oct 8, 2020

Not as clean as @o2sh posted and the new macro define_colors is definitely not an easy read. Most of the non-obvious coding was to skirt some error that I couldn't figure out. Either type not in agreement in the macro or rules not matching in the input data.

I begin to wonder, why isn't the color processing in the ascii file?

{255,0,255} .....

@o2sh
Copy link
Owner

o2sh commented Oct 8, 2020

I begin to wonder, why isn't the color processing in the ascii file?

It makes it easier to manage colors - all the coloring is defined in one file (language.rs) - and allows for run-time customization via -c, --ascii-colors <ascii-colors>

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