Allow for optional colors #611

spenserblack · 2022-03-14T14:38:33Z

This was originally going to be a quick and painless change using an OptionalColor type and .into(), but it quickly turned into way more changes than expected 😅 Hopefully nothing has broken.

The last commit is TEMP because it seems like strum is naively using None instead of Option::None in its macro definition, breaking that commit. spenserblack@5142c5f is OK to review, though. I'll make a PR to strum, but, assuming spenserblack@5142c5f is good, ~~it's ready to be merged if we don't want to wait~~.

Edit: Nevermind, tests fail on that commit.

Edit2: Force-pushed, 0ef1c64 should be OK.

Edit3: Peternator7/strum#214 merged, on next release this whole PR should be ready to review.

Edit4: No need for a new strum release. I realized that Fg is a more descriptive variant than None.

o2sh · 2022-03-17T15:38:50Z

Is this really a good idea to alter the logo's original colour palette? IMO, this may help a small portion of users on light theme terminals in terms of readability but they will lose in terms of aesthetics.

spenserblack · 2022-03-17T18:49:37Z

Well the main reason was to allow the logo's "white" color to just be the text color. I agree that this is a massive change, but we've gotten a few notes that onefetch doesn't look good on light terminals, and one user pointed out that

neofetch has no problem with this

(Source: #33 (comment))

It appears that Neofetch doesn't really use the white color. When the default color is white, or the selected color with --ascii_colors is white, it actually leaves the text uncolored, instead using the default foreground color.
https://github.com/dylanaraps/neofetch/blob/ccd5d9f52609bbdcd5d8fa78c4fdb0f12954125f/neofetch#L4725-L4728

I think we have 2 options:

Make a stance that onefetch is preferred on dark terminal themes
Alter the logo colors

spenserblack · 2022-03-17T18:57:13Z

CI is failing from clippy test unrelated to this PR

Replaces references to `colored::Color` with `Option<colored::Color>`. Related to #33

Should improve support for light terminal themes by letting the theme pick the default text color. Note that white is not always mapped to a dark color on light themes. `Token` tests adjusted to not assume white color is the default. Addresses #33

spenserblack · 2022-03-17T19:14:27Z

Or I suppose we could just make the Fg variant available (but currently unused), and just fix existing logos as light terminal users point them out.

o2sh · 2022-03-18T00:15:14Z

src/info/langs/language.rs

+        pub enum OptionalColor {
+            $( $color, )*
+            TrueColor{ r: u8, g: u8, b: u8 },
+            Fg,


What about mapping White to None? Like that we don't need to touch the language definitions

Yeah, that would work (and be more similar to neofetch). Perhaps White and Fg can be effective aliases of each other?

o2sh · 2022-03-18T00:26:03Z

src/info/langs/language.rs

+macro_rules! define_optional_color {
+    ($( $color:ident ),*) => {
+        #[derive(Debug)]
+        pub enum OptionalColor {


I'm not sur about the name, what about AsciiColor instead ?

Yeah, this was back when there was a None variant. AsciiColor sounds good to me.

o2sh · 2022-03-18T00:53:02Z

You're right, I guess I was just reluctant to add more complexity to the code that would only be beneficial to a small fraction of users. But, what you did is actually pretty clean and as you pointed out neofetch does exactly that.

spenserblack · 2022-03-18T19:21:11Z

I was just reluctant to add more complexity to the code that would only be beneficial to a small fraction of users

That makes sense. I wouldn't be opposed to delaying any decision on this PR until we get a good idea of how many users really need this (I'm of the opinion that a good light theme would reverse black and white TBH, but it looks like some popular ones don't do that).

By making White ignored as you suggested (#611 (comment)), some complexity could probably be reduced. Perhaps ui::ascii_art::add_colored_segment could use a wrapper type that implements Colorize and ignores White.

owo-colors also looks like it has an interesting way to colorize that could probably greatly reduce complexity, as opposed to needing to rely on colored's (limited) Color enum. I think Default would do what we need. We can make a const called White that aliases Default to reduce diff size and prevent confusion with new contributors.

o2sh · 2022-03-18T20:50:15Z

owo-colors also looks like it has an interesting way to colorize that could probably greatly reduce complexity, as opposed to needing to rely on colored's (limited) Color enum. I think Default would do what we need. We can make a const called White that aliases Default to reduce diff size and prevent confusion with new contributors.

This is exactly what we need. It will reduce code clutter by removing the need of using Option everywhere.
Let's do it 🚀

spenserblack · 2022-03-19T02:55:50Z

Let's do it 🚀

Sounds good! I'll leave this PR open for now until the switch to owo-colors is merged. It's unlikely we'll reference commits from this specific PR, but better safe than sorry 🙂

spenserblack marked this pull request as ready for review March 17, 2022 18:57

spenserblack added 3 commits March 17, 2022 15:07

Define OptionalColor type

9f1f8c4

Use optional colors everywhere

d185342

Replaces references to `colored::Color` with `Option<colored::Color>`. Related to #33

Replace White with Fg color

c9460b7

Should improve support for light terminal themes by letting the theme pick the default text color. Note that white is not always mapped to a dark color on light themes. `Token` tests adjusted to not assume white color is the default. Addresses #33

o2sh reviewed Mar 18, 2022

View reviewed changes

spenserblack mentioned this pull request Mar 22, 2022

Use Default color instead of White #625

Merged

1 task

o2sh closed this in 7b89eff Mar 22, 2022

spenserblack deleted the feature/none-color branch March 22, 2022 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for optional colors #611

Allow for optional colors #611

spenserblack commented Mar 14, 2022 •

edited

Loading

o2sh commented Mar 17, 2022 •

edited

Loading

spenserblack commented Mar 17, 2022

spenserblack commented Mar 17, 2022

spenserblack commented Mar 17, 2022

o2sh Mar 18, 2022 •

edited

Loading

spenserblack Mar 18, 2022

o2sh Mar 18, 2022 •

edited

Loading

spenserblack Mar 18, 2022

o2sh commented Mar 18, 2022 •

edited

Loading

spenserblack commented Mar 18, 2022

o2sh commented Mar 18, 2022 •

edited

Loading

spenserblack commented Mar 19, 2022

Allow for optional colors #611

Allow for optional colors #611

Conversation

spenserblack commented Mar 14, 2022 • edited Loading

o2sh commented Mar 17, 2022 • edited Loading

spenserblack commented Mar 17, 2022

spenserblack commented Mar 17, 2022

spenserblack commented Mar 17, 2022

o2sh Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

spenserblack Mar 18, 2022

Choose a reason for hiding this comment

o2sh Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

spenserblack Mar 18, 2022

Choose a reason for hiding this comment

o2sh commented Mar 18, 2022 • edited Loading

spenserblack commented Mar 18, 2022

o2sh commented Mar 18, 2022 • edited Loading

spenserblack commented Mar 19, 2022

spenserblack commented Mar 14, 2022 •

edited

Loading

o2sh commented Mar 17, 2022 •

edited

Loading

o2sh Mar 18, 2022 •

edited

Loading

o2sh Mar 18, 2022 •

edited

Loading

o2sh commented Mar 18, 2022 •

edited

Loading

o2sh commented Mar 18, 2022 •

edited

Loading