-
Notifications
You must be signed in to change notification settings - Fork 20
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
Adding a new colour scheme to parse $LS_COLORS #2
base: master
Are you sure you want to change the base?
Conversation
The colour scheme still needs some work
os.stat.st_nlink can be used to determine if a file is a hard link. I don't know if we get access to the file path in the color scheme though. Probably other special file types can be polled with the same method. |
@Nelyah, pinging you about this. It's something many people've wanted so it'd be great if you could fix this up/or rationalize why it can't be fixed up so we can merge this : ) |
Thanks for the reminder, I had indeed left it all a bit behind. I'm working on it right now, and I will probably give a more detailed update later (hopefully tonight). :) |
Special files should be added as well in the future.
I pushed another commit, which should properly parse the I added as well device and socket colouring, however it seems to be more than just doing a As of now, the colour scheme recognises:
I specifically added a rule to prevent the directory from being coloured since they (mostly) are exectuable. Also, one issue I've been running into (but which seems irrelevant to this issue), is ranger not having the |
Briefly testing this seemed to work. About the plugin, what's your reasoning for having that code in a plugin? (I'm just thinking about simplicity of distribution it'd be easier if there was only one file. Otherwise we have to completely rethink the colorschemes repo ; ) |
Setting the colorscheme back to top level of the git repo.
I originally followed the documentation which was suggesting to define new context tags from the However, I put the code from the plugin file to the colorscheme one and it seems to work. I pushed the last version with the colorscheme file at the top level of the git repo so you wouldn't have to reorganise it :) edit: Also added a rule to no colour links as executables (same as with |
ls_colors.py
Outdated
import ranger.gui.widgets.browsercolumn | ||
from os import getenv | ||
|
||
ls_colors = getenv('LS_COLORS').split(':') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I unset LS_COLORS this throws an exception that's uncaught. (getenv('LS_COLORS')
is None, so getenv('LS_COLORS').split(':')
raises an AttributeError.)
I think you want to check if getenv('LS_COLORS')
is None
instead of ls_colors
. Or catch the exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide a default to getenv
? Otherwise you can use os.environ.get('LS_COLORS', default value)
. Imo that looks like the proper way to solve this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to look it up myself, getenv('LS_COLORS', default='')
looks good. (NB: in two places in the file)
What I didn't realise earlier. It seems the uncaught exception makes ranger fall back to the default colors. Once I add the default
value or exception handling in class base(ColorScheme):
and when LS_COLORS
is not set, then ranger becomes black&white.
I don't think this is what we want here (ls --color
falls back to default colors when LS_COLORS
is not set).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if we want the default LS_COLORS
, we should either let ranger
automatically fall back to its default scheme, or we should manually read the default dircolors
file or run the dircolors
command without arguments to get the default colouring. Also, are we sure that is what ranger
really does? (falling back to default LS_COLORS
) This could simply be a matter of catching the exception.
I've had trouble however finding the system wide config file that dircolors
automatically reads. The man page (man dir_colors
) points to either ~/.dircolors
(user-defined) or /etc/DIR_COLORS
(system-wide). I, however, do not have the system-wide one and yet dircolors
is still able to read the colours from somewhere.
Have you had any luck finding whence it gets the colours?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what's the best to do. I guess given the name 'ls_colors' it should be as close as reasonable to "what ls does".
ranger gets its default colors from https://github.com/ranger/ranger/blob/master/ranger/colorschemes/default.py which happens to be the same as (my?) ls defaults when LS_COLORS is unset.
I also don't see where my dircolors
gets its defaults from.
Given we don't seem to see where dircolors
gets its config from (and one would need to check user file, system file, fallback) I think calling dircolors
if LS_COLORS
is unset is a good handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit to handle the case where LS_COLORS
isn't set.
Scenario cases:
LS_COLORS
is defined and parsedLS_COLORS
isn't defined,dircolors
is called and we use its output to define theLS_COLORS
for the colorschemeLS_COLORS
isn't defined and thedircolors
command can't be found. We use a black & white colorscheme.
the 3) is probably not what the user want, but it might be a good idea to let them know something is wrong if neither a LS_COLORS
env variable or a dircolors
command can be found while using the ls_colors.py
scheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ranger probably "falls back", actually doesn't change to ls_colors.py, because that throws an error.
The proper behavior would be to do what ls does. The default.py
colorscheme doesn't completely match what ls --color
does without LS_COLORS
set.
I meant to use whatever actually is the default value for LS_COLORS
as the default but calling dircolors every time is probably fine too.
Do you really need to duplicate the try/except block for the getenv?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need to duplicate the try/except block for the getenv?
Can you elaborate? I don't see duplication here. Or should this thread be resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refers to an old version. the old file I still have from September has two calls getenv('LS_COLORS')
. The current version in this PR does not have that duplication.
In this case, fall back to the default result of the `dircolors` command. If the command can't be found, uses a black & white scheme (empty LS_COLORS)
I removed the try/except block inside the class. This would probably allow for better interaction with potential plugins. |
Maybe keeping it in the class is better yeah. That way themes based on this one can override the behavior. And I agree about the name. It's not necessary but it's nicer when people start extending your colorscheme. |
I haven't had much time these past few days, just to let you know I have not forgotten it. I'll try to get on to it this week-end! |
Also fixed an issue where the '.' char wouldn't be taken into account when considering extensions.
I (finally) added the code correctly, and called the super for the Colorscheme class (compatible python2). As mentionned in the commit, I also fixed an issue that would make files highlighted if they ended with the extension (but without the |
Awesome. I'll be taking a look at getting this installed locally soon. Also eagerly awaiting this being added to master. |
I just want to let you know that I'll go for an extended (~1 year) trip around the world and since I like to travel light I won't have any computer (except a smartphone), which means I won't be able to patch should any issue arise. I'm leaving on the 28th of October, and on the case where we haven't resolved this PR by then someone will have to take it up from there (I could add that someone as a contributor to my repo). |
Currently it works great. Just one nitpick: the tags are no longer in a distinct color. Is that intentional? |
ls_colors.py
Outdated
self.tup_ls_colors += [('directory', key[1])] | ||
|
||
self.progress_bar_color = 1 | ||
print(self.ls_colors_keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print(self.ls_colors_keys) |
I presume this line was here for debug purposes only?
I have found 3 more problems:
|
I guess @Nelyah has left for their trip. I'll try to see if I can find time to pick up the slack. I very much want to see this landed. |
Actually I don't remember exactly in which directory the second issue was, but I tried several folders in /dev (it was somewhere there) and it looked great. |
@toonn how are we doing? :) |
This got a bit neglected, I'll review and merge soon™. |
@toonn Anything we can do to help? :) |
Someone in the irc channel actually said they wanted to do an exa colorscheme, which would be a superset of this. |
|
Hi, just bumping this as well. This would be a very nice plugin to have :) |
I tried this color scheme on my system and got strange blinking colors. I'm using https://github.com/trapd00r/LS_COLORS. My terminal is xterm. Output of
Output of
Output of
|
Oof that's terrible. Could you minimize your LS_COLORS? Pretty hard to figure out what might be causing this since it's rather long. |
Could it be related to this comment? |
Here are some specific ones. Python files:
Executable files:
|
to_delete.append(i) | ||
|
||
# remove style attrattributes from the array | ||
attribute_list[:] = [val for i, val in enumerate(attribute_list) if i in to_delete] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that every i
is added to to_delete
unconditionally, this is essentially clearing the entire list. Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does seem incorrect, yes. Though it is copying the entire list if I'm not mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're correct. It is copying. I saw to_delete
and read that last condition as not in
. I'm guessing that the intention was to filter out any of the style attributes that were matched in the loop above it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's not right either. Another method is making decisions based on those attributes.
I just realized that attribute 5 is blink, which makes me wonder why my terminal isn't blinking outside of ranger. Maybe I should be using different color definitions? |
After some digging, I found that these color definitions are xterm control sequences, which are supported by dircolors. If your terminal is capable of 256 colors, the following can apply. From https://www.xfree86.org/current/ctlseqs.html:
According to that, the blink attribute is being used as a delimiter between the foreground/background code and the color value. This is obvious from: Lines 144 to 149 in de43255
Although, it appears that even when 256 colors are not in use, the blink attribute is interpreted as another bold:
Now, in this color scheme, when parsing the attributes, blink is interpreted as-is, but should probably be ignored. Lines 103 to 104 in de43255
@Nelyah It's clear from looking at the code that you're already aware of everything I've said above about xterm control sequences. Are there any other implications to simply ignoring the blink attribute on lines 103-104? Removing them gets rid of the blinking, but it's also causing crashes that I'm unable to see. Does anyone know how to use ranger's logging within a color scheme? Using a standard logger overwrites the interface with messages, and it's difficult to use. |
Sorry for the reply that was long in coming @jmcantrell. I will try to give this a closer look in the coming days. As for logging, you can always debug the code and output errors to a file. On a separate terminal window you could use |
I ended up rewriting pretty much completely a previous version of this PR: That version fix a number of issues:
To change the base colorscheme, rename the file to Also included is some testing code, available when running the file by itself ( Screenshots: |
@toonn What do you think of the above? @benoit-pierre Do you think it would make sense to open a separate PR with your implementation? |
A separate PR seems suitable. Don't like having the filename of the theme being part of configuration though. |
@toonn Any suggestions? Personally, I wouldn't be attached to any particular naming scheme. |
Either have it be in |
Just a ping as this PR could be useful for several people and the discussion seems to be near the end (if I understand correctly ;-) |
March 2023... still eagerly waiting... |
@jfmoulin, have you tried out the colorscheme in this PR? If you have been using it and haven't noticed issues for a while then we could go ahead and merge this. |
Hi @toonn , |
@jfmoulin, I don't think renaming the file does anything. The class name inside the file is the important bit. But I'm not sure what you think renaming anything will do? You're supposed to set your Ranger colorscheme to |
@toonn Originally based on: https://github.com/ranger/colorschemes/raw/a250fe866200940eb06d877a274333a2a54c34f3/ls_colors.py Usage: copy this file to Anyways, setting |
I've tried one more thing: I ran the test in ls_colors.py, the just started ranger uses the proper coloring to display the faked files. So far, so good. However, when I jump to a directory which is not the test dir, the entries are not colored (within the same instance of ranger)! |
@toonn, things seem to be working after removing some scripts which were manipulating the LS_COLORS on the fly. |
Still praying for this to land 🙏 Thanks for the thread bumps :D |
It seems to support code https://en.wikipedia.org/wiki/ANSI_escape_code#8-bit I've got a I'd like more terminals to support I don't think the older version of this |
Following the discussion from ranger/ranger#169 (comment)
Plugin and colour scheme to parse the
$LS_COLORS
The colour scheme still needs some work. It works for the extensions (such things as
*.txt
), but doesn't work for special files (denoteddi
,ln
, ...)