Conversation
|
This is a pretty large wad of new code, so I'd like some context on how you arrived at this being a good design that fits in with the current design of the library/API -- I understand the high-level goal of being accessible from a |
src/utils.rs
Outdated
| ColorGradient::new(channels[0], channels[1], channels[2]) | ||
| } | ||
|
|
||
| pub(crate) fn interpolate(&self, other: &ColorGradient, t: f32) -> ColorGradient { |
There was a problem hiding this comment.
What is t? Use Self instead of repeating the type name.
There was a problem hiding this comment.
t is a common variable name in interpolation animation. From 0.0 to 1.0, 0.5 being a 50/50 blend of 2 colors. Out of my head, I don't know how else I can rename the variable besides x
|
|
||
| macro_rules! impl_fmt { | ||
| ($name:ident) => { | ||
| ($name:ident,$format_char:expr) => { |
There was a problem hiding this comment.
Why is this necessary? Could it be extracted into a separate commit/PR?
There was a problem hiding this comment.
Well, that's probably the part I like the least about my PR, but I didn't manage otherwise.
Basically, to apply a gradient on a given string, I need to apply a color after each character.
So I needed to get the properly formatted StyledObject<D>.value.
Before the PR, the object was being directly printed into the formatter buffer via fmt::$name::fmt(&self.val, f)?;.
Now I get back a String from format!(_) macro, which require a way to format that data as an argument to delegate printing format to the same implementation (Display, Debug...).
If there is a way to get back the formatted string without such a hack, I would prefer it.
|
Note: I'll break down the commit into logical units once the PR looks good. Otherwise, it is wasted time. |
It copies the existing API, where |
|
Changes done and comments answered, now waiting for your input @djc 😄 |
|
When writing color detection for windows, it appeared that CMD/powershell were not respecting the TERM/COLORTERM variables. Other windows terms may, though. |
|
|
||
| static STDOUT_COLORS: Lazy<AtomicBool> = | ||
| Lazy::new(|| AtomicBool::new(default_colors_enabled(&Term::stdout()))); | ||
| static STDOUT_TRUE_COLORS: Lazy<AtomicBool> = |
There was a problem hiding this comment.
Instead of adding these, should we change STDOUT_COLORS/STDERR_COLORS to use a tri-value AtomicU8 to encode an enum ColorSupport { None, Some, True }? (Probably without changing any existing public API.)
There was a problem hiding this comment.
It would work and allow to easily support more terminal color ranges.
Unlikely to happen, as I've seen only one terminal with like 512. Probably pointless at this stage.
src/utils.rs
Outdated
| } | ||
| fmt::$name::fmt(&self.val, f)?; | ||
| // Get the underlying value | ||
| let mut buf = format!($format_char, &self.val); |
There was a problem hiding this comment.
What are the efficiency implications here? This looks like it would do a lot of extra allocations.
There was a problem hiding this comment.
Sadly there are extra allocations. This is related to this comment about the additional macro argument.
As good measure, I tweaked the code to have the allocation cost only for gradients, as it isn't needed for normal colors.
|
Code updated, comments answered once again. Waiting for more input on the new code/comments @djc 😄 unfinished comments conversation: Oh my god I sound like an AI with such formatting. |
|
Heads up @djc you seem busy, is there someone else who can finish the review of the PR ? |
|
I don't think so. I had not forgotten but my son is sick so a little more time constrained than usual. |
|
Hey there, any updates ? |
|
Slowly catching up with my backlog. |
use Self for ColorGradient functions
|
Ah, I see some conflicts appeared. Should I dust off the PR, using a fixed size array instead ? |
Sorry for the slow response -- it's been very busy these past few months. Yes, I would be happy to rereview if you figure out a way to deal with the |
|
I tried to wrangle with the const constraint but the code always ends up in a mess. I'll simply have my own version of console for the purpose of my application. |
|
Sorry about that. 😞 |
2 participants
As said on this issue, I said I would propose a PR.
Notable elements and changes
The API is fully backwards compatible, as
consolewas a transitive dependency and I needed my projectAdd supports for multi-step gradients. If the terminal doesn't support "true colors", it will have no effect.
utils functions to check/force true color support
From a
dotted_string, you can use[on_]gradient(_HEX_COLOR)+For example:
".magenta.gradient_E50000_FF8D00_FFEEOO_028121_004CFF_770088"This will use a gradient on true colors terminals and fall back on the magenta color for other terminals.
From a
StyledObject<D>perspective, use.[on_]gradient(GradientColor)to add a color to the gradient.This PR doesn't bring additional tests, as I want first to be sure the API is final
The gradient support is heavily inspired by Tinterm