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

Use terminal color scheme #90

Closed
chmln opened this issue Dec 27, 2018 · 16 comments · Fixed by #262
Closed

Use terminal color scheme #90

chmln opened this issue Dec 27, 2018 · 16 comments · Fixed by #262
Labels
kind/bug Something isn't working

Comments

@chmln
Copy link

chmln commented Dec 27, 2018

Expected behavior

lsd uses colors from my terminal color scheme, being consistent with all the other command line tools.

Actual behavior

It appears lsd uses hard-coded colors, instead of the terminal color scheme. The hard coded colors don't look as nice. See screenshot below.

image

My terminal colors:
image

@chmln chmln added the kind/bug Something isn't working label Dec 27, 2018
@Peltoche
Copy link
Collaborator

Hello @chmln,

As everyone has its own color theme it will be very difficult to make everyone happy. There is a currently a PR (#84) which aim to integrate the LS_COLOR variable to lsd. I think it could solve you issue as you will be able to customize your colors as much as you want. I hope we will be able to merge it soon.

This is sound as a solution to your problem?

@chmln
Copy link
Author

chmln commented Dec 31, 2018

Hey, thanks for responding so fast!

While LS_COLORS is one solution, the easiest way to keep everyone happy is to use built-in terminal ANSI colors. This would remove the need for every user to customize via LS_COLORS to get readable colors.

Fixed colors make more sense for webpages, but not so much for terminals where the background colors can vary a lot. Powershell, for instance, has a dark blue background by default. I use dark gray. In both cases, contrast suffers. Support for built-in colors would be a huge usability win.

Its rather easy to implement proper ANSI color support. It would entail just using e.g. ansi_term::Colour::Blue instead of Color::Fixed(33) // DodgerBlue1. If you're open to the idea, I can submit a PR :)

@Peltoche
Copy link
Collaborator

I fear the will be not enough colors with the basic ANSI colors. Most of the peoples expects to find the same result than the README but there color theme would change it and it could be deceptive.

There is another possible solution: as proposed into #92, it could be a good idea to propose a configuration file with a custom theme.

@Peltoche
Copy link
Collaborator

As we will not directly implement the proposed solution I set this issue as close

@thekashifmalik
Copy link

Would you be open to using terminal color scheme behind a flag?

It really does seem like expected behavior; CLI tools usually respect terminal colors.

@Peltoche Peltoche reopened this May 21, 2019
@Peltoche
Copy link
Collaborator

It seems that a lot of people want this feature so I reopen this issue in order to find a solution.

I see three solutions:

  • Move every existing colors to ANSI colors. This will break the output for many peoples.
  • Add a flag to choose between hardcoded colors vs AINSI colors.

The first solution seems to be the most standard one, that why I would tend to choose this one but it include a big fat change for current users and that's not good...

Any suggestions?

@evrimoztamur
Copy link

I don't think that it'll be so destructive to the existing users. See how exa handles it at https://github.com/ogham/exa/blob/master/src/style/colours.rs

@thekashifmalik
Copy link

It seems that a lot of people want this feature so I reopen this issue in order to find a solution.

I see three solutions:

  • Move every existing colors to ANSI colors. This will break the output for many peoples.
  • Add a flag to choose between hardcoded colors vs AINSI colors.

The first solution seems to be the most standard one, that why I would tend to choose this one but it include a big fat change for current users and that's not good...

Any suggestions?

I like the first option too. We could also add custom colors behind a flag in the future. You are correct that it would change existing colors for users so perhaps we could bump the semver version to indicate that.

That being said, maybe color changes are not breaking functional changes.

What do you think?

@jbergstroem
Copy link

My vote would be to default to ansi similar to exa then introduce theme support (#92) for people that want to fully customize colors.

@AndydeCleyre
Copy link

Just agreeing here that exa does a very nice job of using a user's already-chosen colors:

image

@Peltoche
Copy link
Collaborator

I'm sorry I don't have much time in the next few days so I hope that someone would like to make a PR. I should be a easy one because there is not logic changes.

@thekashifmalik
Copy link

I can take a look.

@meain
Copy link
Member

meain commented May 24, 2019

@thekashifmalik That would be awesome, I can help you if you need any help with navigating the codebase.

@Kevin-Mok
Copy link

It seems to use the terminal colors for me if I just call ls first in fish.
screenshot-2019-08-06_23:52:01
Not sure why this works for me, but I am using version 0.15.1 and have tested this in urxvt and st. 🤔 So to make this automatically happen for all my terminals, I added ls > /dev/null to my config.fish file, and now it just uses the terminal colors even for my first call to lsd in a terminal.

@meain
Copy link
Member

meain commented Aug 7, 2019

@Kevin-Mok From what I can understand from here, this happens because fish sets the LS_COLORS variable when you call ls. lsd does use LS_COLORS to figure out the user colors.

@Peltoche A simple fix would be completely let lscolors take care of colors.

@erlanger
Copy link

@Peltoche A simple fix would be completely let lscolors take care of colors.

This would be the best solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants