lsd-rs / lsd

The next gen ls command
Apache License 2.0
13.12k stars 427 forks source link

Don't convert from 16 to 256 colors in `LS_COLORS` #609

Open Un1q32 opened 2 years ago

Un1q32 commented 2 years ago

Expected behavior

If applicable, add the output of the classic ls command (\ls -la) in order to show the buggy file/directory.

lsd should respect my terminal color scheme, as it does in 0.20 and 0.19

Actual behavior

If the application panics run the command with the trace (RUST_BACKTRACE=1 lsd ...). In case of graphical errors, add a screenshot if possible.

lsd does not respect my terminal color scheme

Screenshot (9)

zwpaper commented 2 years ago

we change the color lib to a new one for the theme feature, maybe there is some color difference between the 2 libs.

please unset the LS_COLORS and let's make sure it is not working or there is a color difference.

Un1q32 commented 2 years ago

The issue is still present when I unset LS_COLORS

Screenshot (11)

zwpaper commented 2 years ago

nothing changed for both 0.20.1 and 0.21.0?

it seems like the difference between the 2 default colors.

how are the colors present if the LS_COLOR you mentioned is set?

Un1q32 commented 2 years ago

what I originally thought was the issue (the new version didnt obey my terminal color scheme) was incorrect, what seems to be happening is that 0.21.0 is using the darker versions of the terminal colors rather than the lighter ones that the old version uses

Screenshot (13)

zwpaper commented 2 years ago

my point is that even you unset the LS_COLORS, the old version still shows the color you expected, so maybe this is not related to LS_COLORS, it is caused by the color lib changes.

some minor differences happened when we are changing the lib, maybe this is some cases like that.

maybe we need to figure out why the LS_COLORS not working, but it seems really complicated...

meain commented 2 years ago

@zwpaper One possibility that I can think of is https://github.com/Peltoche/lsd/blob/1a63ccced0203f2131ad26682da849b16aa66a5d/src/color.rs#L246. But from what I understand this is correct as per https://github.com/crossterm-rs/crossterm/blob/db956267f80744b1d8ab08dcd7d92ee21b7a60f2/src/style/types/color.rs#L126 .

I am NOT able to reproduce the difference between 19 and 21 though.

zwpaper commented 2 years ago

yes, this also recalls me of this red difference when implementing the theme feature.

I was also confused about this issue as the LS_COLORS env seems not working, I tried the env but nothing changed in my terminal, but I am not good at LS_COLORS

meain commented 2 years ago

LS_COLORS works as expected. The reason why you did not see a change is probably because this is close to the default.

Un1q32 commented 2 years ago

classic ls has the colors that 0.20.1 uses

meain commented 2 years ago

I have a feeling that this may be linked to https://github.com/Peltoche/lsd/issues/248

meain commented 2 years ago

I think I was able to track down what is going on.

Taking the example of di=1;34, in lscolors, 34 is mapped to lscolors::Colors::Blue (https://github.com/sharkdp/lscolors/blob/c6e191b91f637058ba932a4f7b065f2e19d8a1ba/src/style.rs#L245) which was initially a ansi_term only lib which also mapped ansi_term::Color::Blue to 34(https://github.com/ogham/rust-ansi-term/blob/ff7eba98d55ad609c7fcc8c7bb0859b37c7545cc/src/style.rs#L273).

The crossterm integration came later and for crossterm crossterm::Color::DarkBlue , which we map to(https://github.com/Peltoche/lsd/blob/1a63ccced0203f2131ad26682da849b16aa66a5d/src/color.rs#L249) is using 4(https://github.com/crossterm-rs/crossterm/blob/db956267f80744b1d8ab08dcd7d92ee21b7a60f2/src/style/types/color.rs#L130) which matches with what neofetch(https://github.com/dylanaraps/neofetch/blob/ccd5d9f52609bbdcd5d8fa78c4fdb0f12954125f/neofetch#L646) shows. Even lscolors maps to crossterm::Color::Blue (https://github.com/sharkdp/lscolors/blob/bc95124f23d2f1c14616e6a97f5ae9e6ab786697/src/style.rs#L61) which is still not 34.

Let me do some more research, but if this is the case we will have to solve this upstream in lscolors as well.

meain commented 2 years ago

Sorry for the delay, but I did some more digging on this. The 34 comes from ansi-term using just 8 colors and it is equivalent to 4 in crossterm which expects 256 color support.

ref: https://gist.github.com/fnky/458719343aabd01cfb17a3a4f7296797


With that said, I have a feeling the the latest lsd is already doing the right thing. @OldWorldOrdr What does gnu ls give you for color with export LS_COLORS="di01;34" set. \ls --color=always? 2022-02-08-20-34-438402


EDIT: I missed you comment that classic ls used the old colors. Now this is starting to get confusing.

meain commented 2 years ago

@zwpaper Could you set export LS_COLORS="di=01;31" and see if /tmp/one/lsd-0.20.1-x86_64-unknown-linux-gnu/lsd && \ls --color=always && lsd gives you the same colors. It won't be the same shade of red as me, but at least they should be same between them.

What I don't get is how the old one has different colors in the issue screenshot

2022-02-08-20-42-413944

meain commented 2 years ago

@OldWorldOrdr Could you try building lsd after applying this patch and see if it uses the old colors.

diff --git a/src/color.rs b/src/color.rs
index b5636bf..fedad32 100644
--- a/src/color.rs
+++ b/src/color.rs
@@ -243,12 +243,12 @@ fn to_content_style(ls: &lscolors::Style) -> ContentStyle {
         },
         lscolors::style::Color::Fixed(n) => Color::AnsiValue(*n),
         lscolors::style::Color::Black => Color::Black,
-        lscolors::style::Color::Red => Color::DarkRed,
-        lscolors::style::Color::Green => Color::DarkGreen,
-        lscolors::style::Color::Yellow => Color::DarkYellow,
-        lscolors::style::Color::Blue => Color::DarkBlue,
-        lscolors::style::Color::Magenta => Color::DarkMagenta,
-        lscolors::style::Color::Cyan => Color::DarkCyan,
+        lscolors::style::Color::Red => Color::Red,
+        lscolors::style::Color::Green => Color::Green,
+        lscolors::style::Color::Yellow => Color::Yellow,
+        lscolors::style::Color::Blue => Color::Blue,
+        lscolors::style::Color::Magenta => Color::Magenta,
+        lscolors::style::Color::Cyan => Color::Cyan,
         lscolors::style::Color::White => Color::White,
     };
     let mut style = ContentStyle {

If you are not scared of running a random binary that a stranger on the internet gave you, here (md5: 0b1e358c79fc4b159bf3161de985b784) is a built version.

Un1q32 commented 2 years ago

@OldWorldOrdr Could you try building lsd after applying this patch and see if it uses the old colors.

diff --git a/src/color.rs b/src/color.rs
index b5636bf..fedad32 100644
--- a/src/color.rs
+++ b/src/color.rs
@@ -243,12 +243,12 @@ fn to_content_style(ls: &lscolors::Style) -> ContentStyle {
         },
         lscolors::style::Color::Fixed(n) => Color::AnsiValue(*n),
         lscolors::style::Color::Black => Color::Black,
-        lscolors::style::Color::Red => Color::DarkRed,
-        lscolors::style::Color::Green => Color::DarkGreen,
-        lscolors::style::Color::Yellow => Color::DarkYellow,
-        lscolors::style::Color::Blue => Color::DarkBlue,
-        lscolors::style::Color::Magenta => Color::DarkMagenta,
-        lscolors::style::Color::Cyan => Color::DarkCyan,
+        lscolors::style::Color::Red => Color::Red,
+        lscolors::style::Color::Green => Color::Green,
+        lscolors::style::Color::Yellow => Color::Yellow,
+        lscolors::style::Color::Blue => Color::Blue,
+        lscolors::style::Color::Magenta => Color::Magenta,
+        lscolors::style::Color::Cyan => Color::Cyan,
         lscolors::style::Color::White => Color::White,
     };
     let mut style = ContentStyle {

If you are not scared of running a random binary that a stranger on the internet gave you, here (md5: 0b1e358c79fc4b159bf3161de985b784) is a built version.

this worked perfectly.

zwpaper commented 2 years ago

it is all the same color in my case 🤣, both dark and light

image

image

meain commented 2 years ago

@zwpaper Yeah, same for me. In fact, if I switch it using the diff I mentioned it gives me different colors compared to gnu ls. What I have no clue now is how the old has different colors for OP.

@OldWorldOrdr If you pipe lsd to less (or redirect output to a file) with color always, you will get the actual ansi escape sequences. Could you paste that here. lsd --color=always -d /tmp. It will look something like /tmp.

Un1q32 commented 2 years ago

@meain

0.20.1:

file
folder
script.sh

0.21.0:

file
folder
script.sh

0.21.0 with patch:

file
folder
script.sh
zwpaper commented 2 years ago

hi @OldWorldOrdr, could you also paste the GNU ls piped contents with \ls --color=always

Un1q32 commented 2 years ago

@zwpaper

file
folder
script.sh
meain commented 2 years ago

@OldWorldOrdr I'm not sure how you configure Window terminal, but could you paste the color configuration.

Ideally from what I understand both 1;34 and 38;5;4 1 should give you the same color. echo '\x1b[1;34m16 color blue\x1b[0m | \x1b[38;5;4m\x1b[1m256 color dark \x1b[0m | \x1b[38;5;12m\x1b[1m256 color light\x1b[0m'

2022-02-09-19-12-023944 .

Un1q32 commented 2 years ago

@meain Screenshot (28)

meain commented 2 years ago

It just hit me. A lot of terminal emulators enable "Use bright text for bold colors" option. With this enabled, if something is bold, it automatically makes it bright. This I believe does not work on 256 color pallet. So, in your case, the old version of lsd was trying to show directories in bold, but with dark colors (because of di=1;34 .. 1 = bold 34 = blue), but Windows terminal automatically makes it bright and that is how to end up with the bright variants of the colors in the old version of lsd. Try disabling this option if this is available and see if the colors in both versions match.

beedaddy commented 2 years ago

I hope this is the right place for this comment (since it's not Windows but Arch Linux with GNOME 41.3): I, too, stumbled over the issue, that the colors for ls are different to the colors of lsd (version 0.21.0) as illustrated here: Bildschirmfoto von 2022-02-11 09-30-29 $LS_COLORS is not set. If there's anything I can do/test, please let me know.

meain commented 2 years ago

@beedaddy See my previous message here.

You said you are using gnome. Just from that, if you are using gnome-terminal then this checkbox at the bottom is the one that I am talking about in my previous message. See if disabling this will make both of the colors the same. image

beedaddy commented 2 years ago

Thanks for reminding me about this option. Disabling this option results in the same (?) color: a dark blue. Bildschirmfoto von 2022-02-11 12-12-20 So the problem is that lsd irgnores the (enabled) option?

meain commented 2 years ago

Thanks @beedaddy for verifying the issue. The issue here is that older version of lsd relied on 8/16 colors which would change colors based on if this option is enabled or not. The newer version of lsd uses 256 color pallete which does not get affected by this setting.

IMO the "right" color is the dark variant. The ideal fix here is to not do a translation from 8/16 color to 256 automatically, but from what I understand crossterm does not currently have support for using 8/16 color pallete.

Un1q32 commented 1 year ago

its been nearly a year and lsd still uses the wrong colors for me. Heres an up to date screenshot Screenshot (6)

Un1q32 commented 1 year ago

is there a way to force it to use bright colors instead of dark bold colors?

Un1q32 commented 1 year ago

Screenshot (8) refuses to make text bold

meain commented 1 year ago

is there a way to force it to use bright colors instead of dark bold colors?

You will have to use 256 colors LS_COLORS to get bright ones. For bright blue, it would be something like di=38;5;12. In general, you can use 38;5;<x> where x is the color. You can list of all the colors using the following script:

for code in {0..255}; do
    echo -n "[38;05;${code}m$(printf %03d $code) "
    [ $((${code} % 16)) -eq 15 ] && echo
done
Un1q32 commented 1 year ago

@meain ur code didn't work so i fixed it for u working code:

for code in {0..255}; do
  printf "\e[38;05;${code}m$(printf %03d $code)\n"
  [ $((${code} % 16)) -eq 15 ] && echo
done
magus424 commented 6 months ago

I've been running into this same issue; whatever code/library/etc is mapping \e[1;34m to \e[38;5;4m\e[1m seems to be the issue; the behavior between those two sets of codes is not necessarily identical, so this conversion introduces unexpected behavior.

This is why classic ls works correctly with \e[1;34m as it isn't trying to "upgrade" it to 256 colors where the behavior can be different.

If whatever is converting it to 256 just stopped doing that, it would probably fix everything.