Highlight files like LS_COLORS

[QBobWatson]

This PR adds a new highlighter that highlights names of existing files on the command line, in the style of LS_COLORS. It can pull its configuration directly from LS_COLORS, or can be configured separately.

No existing code is touched.

[danielshahaf]

Thanks!

We're in the pre-0.7.0 freeze; milestoning 0.8.0.

Who wrote files-highlighter.zsh and its documentation?

[QBobWatson]

I wrote all the code. I just pushed a commit with your copyright boilerplate included. I also fixed the indentation to be consistent with your codebase.

[danielshahaf]

Thanks :-) Am swamped so will review later.

[rvalieris]

I tried running this but zsh_highlight_files_extract_ls_colors doesn't seems to work. _zsh_highlight_highlighter_files_ansi_to_zle is looping on my LS_COLORS but its not matching anyhing. my LS_COLORS is set with dircolors -b

[QBobWatson]

I tried running this but zsh_highlight_files_extract_ls_colors doesn't seems to work. _zsh_highlight_highlighter_files_ansi_to_zle is looping on my LS_COLORS but its not matching anyhing. my LS_COLORS is set with dircolors -b

Can you attach your LS_COLORS? I'll see if I can get it working.

[rvalieris]

Can you attach your LS_COLORS? I'll see if I can get it working.

sure, https://pastebin.com/raw/G67RnpRX

[QBobWatson]

I fixed a couple of corner cases. Your LS_COLORS produces the expected output here. Does it work for you?

[danielshahaf]

Consider adding a test case? (See highlighters//test-/ and tests/README.md)

[rvalieris]

same results, but I figured it out, _zsh_highlight_highlighter_files_ansi_to_zle needs to setopt extended_glob, otherwise $#match is always zero.

variables set: https://pastebin.com/raw/eHiqxwGw

now its working. compared to LS_COLORS, directories, symlinks, and executables are highlighting. however .png/.jpg/.gz/.tar files are not.

what is weird is that .tgz is highlighting, .tar is not, but both have the same color settings.

[QBobWatson]

I fixed a couple more places where empty patterns weren't being expanded correctly. I also added emulate -L zsh; setopt extended_glob to all functions.

I'll work on a test case.

[danielshahaf]

emulate -L zsh is already done up in _zsh_highlight, and it applies to callee functions as well, so I'm not sure why you need to do it in the highlighter as well. In any case, even if you do need to do it in the highlighter, doing it in the entry points only should suffice.

[rvalieris]

@danielshahaf , I think its because the function zsh_highlight_files_extract_ls_colors is executed on my .zshrc after the plugin was loaded.

[QBobWatson]

I deleted unnecessary emulate -L zsh commands.

I added four test cases: two with my setup, and two with @rvalieris

[danielshahaf]

Thanks. I'm still swamped/backlogged though, I'm afraid :(

[rvalieris]

I figured out why some files were not highlighting. zsh_highlight_files_extract_ls_colors has a list of things that are parsed as FILE_TYPES and the rest is passed on to FILE_PATTERNS however this list is not exhaustive. try this:

unset ZSH_HIGHLIGHT_FILE_TYPES
unset ZSH_HIGHLIGHT_FILE_PATTERNS
typeset -gA ZSH_HIGHLIGHT_FILE_TYPES
typeset -ga ZSH_HIGHLIGHT_FILE_PATTERNS
# none of these are file patterns
export LS_COLORS='no=0:fi=0:rs=0:mi=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:'
echo $ZSH_HIGHLIGHT_FILE_PATTERNS
st fg=7,bg=4 rs no mh mi ca fg=0,bg=1

then when _zsh_highlight_highlighter_files_paint tries to loop over FILE_PATTERNS it will not work because of these extraneous values.

so where the full list of things that can appear in LS_COLORS ? I couldn't find much documentation on this but man 5 dir_colors could help.

another thing, only the file part(basename) of the path is being highlighted the rest is white, this is inconsistent with the ls output that highlights the entire path with the color of the file. maybe we could have a user option to choose which way the user wants ?

[QBobWatson]

Hi @rvalieris,

In your example, try print -l "${(@)ZSH_HIGHLIGHT_FILE_PATTERNS}". I get

st
fg=7,bg=4
rs

no

mh

mi

ca
fg=0,bg=1

The logic in zsh_highlight_files_extract_ls_colors puts the keys di|fi|ln|pi|so|bd|cd|or|ex|su|sg|ow|tw in ZSH_HIGHLIGHT_FILE_TYPES; the rest go into _PATTERNS. The blank lines show up because the color code "0" gets translated into the empty string, which is then ignored in _zsh_highlight_highlighter_files_paint. (One could debate whether this is the best thing to do, but I think it's reasonable.)

I was never able to find reliable documentation on LS_COLORS, so I went straight to the source.

As for the dirname vs base name: the key lp in ZSH_HIGHLIGHT_FILE_TYPES defines what color the dirname is highlighted in. I agree that the user should be able to choose whether to highlight the whole path or just the basename; I'll add that.

Edit: probably zsh_highlight_files_extract_ls_colors should suppress the keys that are valid in LS_COLORS but not ZSH_HIGHLIGHT_FILE_TYPES: namely, lc|rc|ec|rs|no|mi|do|st|ca|mh|cl. I'll do that too.

[QBobWatson]

Hi @rvalieris -- I made both changes. What do you think?

[rvalieris]

In your example, try print -l "${(@)ZSH_HIGHLIGHT_FILE_PATTERNS}". I get The blank lines show up because the color code "0" gets translated into the empty string,

doh ! you had updated this before but I didn't pulled the changes, nevermind.

yes, I think suppressing those keys makes more sense because they aren't patterns after all.

I assume lp isn't used by dircolors ? anyway setting it to same works as I expected !

its looking pretty nice now ! thanks for all the work @QBobWatson !

one extra thing I will add is look into https://github.com/trapd00r/LS_COLORS, this is probably the most complex LS_COLORS people are gonna use, I tested it a bit and looks good but theres so much stuff that I can't be sure.

[danielshahaf]

I was never able to find reliable documentation on LS_COLORS, so I went straight to the source.

Please be mindful of copyright issues. coreutils is GPL'd and z-sy-h is BSD'd. In a nutshell, that means we can't copy code from coreutils, except in specific, limited cases (e.g., uncopyrightability, cleanroom, etc). This includes implementation, documentation, and even example values of the envvar.

I agree that the user should be able to choose whether to highlight the whole path or just the basename; I'll add that.

Options are bad™. I recommend not to add an option unless there's an actual (not hypothetical) need for it. Personally, I'd expect the whole path to be highlighted, not just the basename. I understand the PR as of yesterday highlighted only the basename, though; what's the rationale for that? (honest question)

[danielshahaf]

By the way, sorry to hijack the thread, but would either of you happen to have some spare cycles to help clear up the last blocker to the 0.7.0 release? It's an edge case issue, but it blocks getting several others merged. If so, please respond on the relevant issue/PR, not here. Thanks!

[QBobWatson]

I assume lp isn't used by dircolors ? anyway setting it to same works as I expected !

That's correct -- I got it from exa.

one extra thing I will add is look into https://github.com/trapd00r/LS_COLORS, this is probably the most complex LS_COLORS people are gonna use, I tested it a bit and looks good but theres so much stuff that I can't be sure.

Well it's long, but I don't know that it's more complex than yours. I also tried it and it looks okay.

[QBobWatson]

Please be mindful of copyright issues. coreutils is GPL'd and z-sy-h is BSD'd. In a nutshell, that means we can't copy code from coreutils, except in specific, limited cases (e.g., uncopyrightability, cleanroom, etc). This includes implementation, documentation, and even example values of the envvar.

Surely we can refer to the source to see what the keys do? The implementation in files-highlighter.zsh of course has nothing to do with the C code in coreutils.

Options are bad™.

Clearly you are not an Emacs user :)

I recommend not to add an option unless there's an actual (not hypothetical) need for it. Personally, I'd expect the whole path to be highlighted, not just the basename. I understand the PR as of yesterday highlighted only the basename, though; what's the rationale for that? (honest question)

This is how exa does it -- it prints path components with a color specified by the lp key, then the basename with the color used to highlight that kind of file. I think it looks cool, so I kept it.

[danielshahaf]

Please be mindful of copyright issues. coreutils is GPL'd and z-sy-h is BSD'd. In a nutshell, that means we can't copy code from coreutils, except in specific, limited cases (e.g., uncopyrightability, cleanroom, etc). This includes implementation, documentation, and even example values of the envvar.

Surely we can refer to the source to see what the keys do?

Yes, we can refer to the source to obtain information such as "the format code xy is used to highlight fifos". However, if the source has a table or a commented enum declaration with the same information, we can't copy that table or enum declaration wholesale.

The implementation in files-highlighter.zsh of course has nothing to do with the C code in coreutils.

By this do you mean "the highlighter isn't written in the same programming language as coreutils" or "the highlighter was implemented without reference to the coreutils implementation"? The former would be a problem (porting/translating doesn't absolve copyright concerns; that's why J. K. Rowling gets royalties from sales of Harry Potter translations); the latter would be good news.

I recommend not to add an option unless there's an actual (not hypothetical) need for it. Personally, I'd expect the whole path to be highlighted, not just the basename. I understand the PR as of yesterday highlighted only the basename, though; what's the rationale for that? (honest question)

This is how exa does it -- it prints path components with a color specified by the lp key, then the basename with the color used to highlight that kind of file. I think it looks cool, so I kept it.

I see. Well, speaking as a maintainer, it's your call. Speaking as a user, I'll reserve judgement on what behaviour I personally prefer until I've had a chance to try them both.

[QBobWatson]

By this do you mean "the highlighter isn't written in the same programming language as coreutils" or "the highlighter was implemented without reference to the coreutils implementation"? The former would be a problem (porting/translating doesn't absolve copyright concerns; that's why J. K. Rowling gets royalties from sales of Harry Potter translations); the latter would be good news.

Ok. All I did was look at ls.c to see what the keys mean.

Anything else I need to do to the PR?

[danielshahaf]

Ok. All I did was look at ls.c to see what the keys mean.

That should be fine; thanks for confirming.

Anything else I need to do to the PR?

I don't see anything obviously wrong in the diff: there's the highlighter, docs, and tests. That's not a detailed review, though.

How would one test this PR? Run «eval $(dircolors)» then load z-sy-h with this PR enabled?

[QBobWatson]

I don't see anything obviously wrong in the diff: there's the highlighter, docs, and tests. That's not a detailed review, though. How would one test this PR? Run «eval $(dircolors)» then load z-sy-h with this PR enabled?

Yes, that's the most straightforward way. You have to run zsh_highlight_files_extract_ls_colors after eval $(dircolors). You can also set ZSH_HIGHLIGHT_FILE_TYPES and ZSH_HIGHLIGHT_FILE_PATTERNS directly, which is what I do.

[danielshahaf]

Cross-referencing #605.