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

Highlight files like LS_COLORS #680

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

QBobWatson
Copy link

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.

This new highlighter 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.
@danielshahaf
Copy link
Member

Thanks!

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

Who wrote files-highlighter.zsh and its documentation?

@danielshahaf danielshahaf added this to the 0.8.0 milestone Feb 4, 2020
@QBobWatson
Copy link
Author

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
Copy link
Member

Thanks :-) Am swamped so will review later.

@rvalieris
Copy link

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
Copy link
Author

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
Copy link

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

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

@QBobWatson
Copy link
Author

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

@danielshahaf
Copy link
Member

danielshahaf commented Feb 14, 2020 via email

@rvalieris
Copy link

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
Copy link
Author

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
Copy link
Member

danielshahaf commented Feb 14, 2020 via email

@rvalieris
Copy link

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

@QBobWatson
Copy link
Author

I deleted unnecessary emulate -L zsh commands.

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

@danielshahaf
Copy link
Member

danielshahaf commented Feb 14, 2020 via email

@rvalieris
Copy link

rvalieris commented Feb 17, 2020

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
Copy link
Author

QBobWatson commented Feb 17, 2020

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
Copy link
Author

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

@rvalieris
Copy link

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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.

term

@danielshahaf
Copy link
Member

danielshahaf commented Feb 18, 2020 via email

@QBobWatson
Copy link
Author

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
Copy link
Member

danielshahaf commented Feb 18, 2020 via email

@danielshahaf danielshahaf mentioned this pull request Feb 18, 2020
@QBobWatson
Copy link
Author

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
Copy link
Member

Cross-referencing #605.

Copy link
Member

@danielshahaf danielshahaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QBobWatson Reviewed. Sorry for the delay.

I'm afraid I found multiple blockers to merging.

I don't know if you're used to this granularity of code review, but it's really nothing out of the ordinary.

I have not tested yet, due to the amount of blockers.

filename using `ZSH_HIGHLIGHT_FILE_PATTERNS` instead. This array has the form
`(glob1 style1 glob2 style2 glob3 style3 ...)`, where the globs are arbitrary
glob patterns, and the styles are color specifications. For instance, if have
`setopt extended_glob` and you write
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a missing word in this sentence, but more importantly, how can the behaviour possibly depend on the user's value of the EXTENDED_GLOB option if ${zsyh_user_options[extendedglob]} is not consulted?


BUFFER=$': file.tar file.tgz file.webm'

local LS_COLORS='rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=00:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arc=01;31:*.arj=01;31:*.taz=01;31:*.lha=01;31:*.lz4=01;31:*.lzh=01;31:*.lzma=01;31:*.tlz=01;31:*.txz=01;31:*.tzo=01;31:*.t7z=01;31:*.zip=01;31:*.z=01;31:*.Z=01;31:*.dz=01;31:*.gz=01;31:*.lrz=01;31:*.lz=01;31:*.lzo=01;31:*.xz=01;31:*.zst=01;31:*.tzst=01;31:*.bz2=01;31:*.bz=01;31:*.tbz=01;31:*.tbz2=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.war=01;31:*.ear=01;31:*.sar=01;31:*.rar=01;31:*.alz=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.cab=01;31:*.wim=01;31:*.swm=01;31:*.dwm=01;31:*.esd=01;31:*.jpg=01;35:*.jpeg=01;35:*.mjpg=01;35:*.mjpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.webm=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=00;36:*.au=00;36:*.flac=00;36:*.m4a=00;36:*.mid=00;36:*.midi=00;36:*.mka=00;36:*.mp3=00;36:*.mpc=00;36:*.ogg=00;36:*.ra=00;36:*.wav=00;36:*.oga=00;36:*.opus=00;36:*.spx=00;36:*.xspf=00;36:'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you minimize the value to the smallest thing that's required for the test? Both out of general test writing principles, and because there are copyright concerns in copying such a value wholesale. (Each entry by itself is uncopyrightable, but the collection as a whole might nevertheless be copyrightable.)

Ditto in the other tests.


BUFFER=$': file.tar file.tgz Makefile #file.bak# file.PgP file.flv'

ZSH_HIGHLIGHT_FILE_PATTERNS=('Makefile' 'fg=#F0BE45,bold' 'SConstruct' 'fg=#F0BE45,bold' 'CMakeLists.txt' 'fg=#F0BE45,bold' 'BUILD' 'fg=#F0BE45,bold' 'README*' 'fg=#F0BE45,bold' '(#i)*.png' 'fg=#B50769' '(#i)*.jpeg' 'fg=#B50769' '(#i)*.jpg' 'fg=#B50769' '(#i)*.gif' 'fg=#B50769' '(#i)*.bmp' 'fg=#B50769' '(#i)*.tiff' 'fg=#B50769' '(#i)*.tif' 'fg=#B50769' '(#i)*.ppm' 'fg=#B50769' '(#i)*.pgm' 'fg=#B50769' '(#i)*.pbm' 'fg=#B50769' '(#i)*.pnm' 'fg=#B50769' '(#i)*.webp' 'fg=#B50769' '(#i)*.heic' 'fg=#B50769' '(#i)*.raw' 'fg=#B50769' '(#i)*.arw' 'fg=#B50769' '(#i)*.svg' 'fg=#B50769' '(#i)*.stl' 'fg=#B50769' '(#i)*.eps' 'fg=#B50769' '(#i)*.dvi' 'fg=#B50769' '(#i)*.ps' 'fg=#B50769' '(#i)*.cbr' 'fg=#B50769' '(#i)*.jpf' 'fg=#B50769' '(#i)*.cbz' 'fg=#B50769' '(#i)*.xpm' 'fg=#B50769' '(#i)*.ico' 'fg=#B50769' '(#i)*.cr2' 'fg=#B50769' '(#i)*.orf' 'fg=#B50769' '(#i)*.nef' 'fg=#B50769' '(#i)*.avi' 'fg=#D33682' '(#i)*.flv' 'fg=#D33682' '(#i)*.m2v' 'fg=#D33682' '(#i)*.m4v' 'fg=#D33682' '(#i)*.mkv' 'fg=#D33682' '(#i)*.mov' 'fg=#D33682' '(#i)*.mp4' 'fg=#D33682' '(#i)*.mpeg' 'fg=#D33682' '(#i)*.mpg' 'fg=#D33682' '(#i)*.ogm' 'fg=#D33682' '(#i)*.ogv' 'fg=#D33682' '(#i)*.vob' 'fg=#D33682' '(#i)*.wmv' 'fg=#D33682' '(#i)*.webm' 'fg=#D33682' '(#i)*.m2ts' 'fg=#D33682' '(#i)*.aac' 'fg=#F1559C' '(#i)*.m4a' 'fg=#F1559C' '(#i)*.mp3' 'fg=#F1559C' '(#i)*.ogg' 'fg=#F1559C' '(#i)*.wma' 'fg=#F1559C' '(#i)*.mka' 'fg=#F1559C' '(#i)*.opus' 'fg=#F1559C' '(#i)*.alac' 'fg=#F1559C' '(#i)*.ape' 'fg=#F1559C' '(#i)*.flac' 'fg=#F1559C' '(#i)*.wav' 'fg=#F1559C' '(#i)*.asc' 'fg=#6A7F00' '(#i)*.enc' 'fg=#6A7F00' '(#i)*.gpg' 'fg=#6A7F00' '(#i)*.pgp' 'fg=#6A7F00' '(#i)*.sig' 'fg=#6A7F00' '(#i)*.signature' 'fg=#6A7F00' '(#i)*.pfx' 'fg=#6A7F00' '(#i)*.p12' 'fg=#6A7F00' '(#i)*.djvu' 'fg=#878AE0' '(#i)*.doc' 'fg=#878AE0' '(#i)*.docx' 'fg=#878AE0' '(#i)*.dvi' 'fg=#878AE0' '(#i)*.eml' 'fg=#878AE0' '(#i)*.eps' 'fg=#878AE0' '(#i)*.fotd' 'fg=#878AE0' '(#i)*.odp' 'fg=#878AE0' '(#i)*.odt' 'fg=#878AE0' '(#i)*.pdf' 'fg=#878AE0' '(#i)*.ppt' 'fg=#878AE0' '(#i)*.pptx' 'fg=#878AE0' '(#i)*.rtf' 'fg=#878AE0' '(#i)*.xls' 'fg=#878AE0' '(#i)*.xlsx' 'fg=#878AE0' '(#i)*.zip' 'fg=#657B83,bold' '(#i)*.tar' 'fg=#657B83,bold' '(#i)*.Z' 'fg=#657B83,bold' '(#i)*.z' 'fg=#657B83,bold' '(#i)*.gz' 'fg=#657B83,bold' '(#i)*.bz2' 'fg=#657B83,bold' '(#i)*.a' 'fg=#657B83,bold' '(#i)*.ar' 'fg=#657B83,bold' '(#i)*.7z' 'fg=#657B83,bold' '(#i)*.iso' 'fg=#657B83,bold' '(#i)*.dmg' 'fg=#657B83,bold' '(#i)*.tc' 'fg=#657B83,bold' '(#i)*.rar' 'fg=#657B83,bold' '(#i)*.par' 'fg=#657B83,bold' '(#i)*.tgz' 'fg=#657B83,bold' '(#i)*.xz' 'fg=#657B83,bold' '(#i)*.txz' 'fg=#657B83,bold' '(#i)*.lzma' 'fg=#657B83,bold' '(#i)*.deb' 'fg=#657B83,bold' '(#i)*.rpm' 'fg=#657B83,bold' '(#i)*.zst' 'fg=#657B83,bold' '*~' 'fg=#586E75' '\#*\#' 'fg=#586E75' '(#i)*.tmp' 'fg=#586E75' '(#i)*.swp' 'fg=#586E75' '(#i)*.swo' 'fg=#586E75' '(#i)*.swn' 'fg=#586E75' '(#i)*.bak' 'fg=#586E75' '(#i)*.bk' 'fg=#586E75' '(#i)*.class' 'fg=#5158A9' '(#i)*.elc' 'fg=#5158A9' '(#i)*.o' 'fg=#5158A9' '(#i)*.pyc' 'fg=#5158A9')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line breaks, please, for readability in situ and for readability of future diffs. (This affects other tests too.)

You can get them easily by running print -rC2 -- ${(qq)ZSH_HIGHLIGHT_FILE_PATTERNS}.


ZSH_HIGHLIGHT_FILE_TYPES=('cd' 'fg=#EA6630' 'su' 'fg=#878AE0,bold' 'ex' 'fg=#BDD040' 'bd' 'fg=#AC3000' 'ln' 'fg=#B58900' 'tw' 'fg=#268BD2,bold,underline' 'or' 'fg=#FC5246,bold' 'fi' 'fg=#93A1A1' 'lp' 'fg=#657B83' 'sg' 'fg=#878AE0' 'di' 'fg=#268BD2,bold' 'ow' 'fg=#DC322F,underline' 'pi' 'fg=#D33682' 'so' 'fg=#D33682,bold')

touch file.fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list of commands is duplicated between the two tests. Shouldn't there be comments cross-referencing the two identical copies, to ensure they remain identical?

Comment on lines +10 to +15
If you are happy with your `LS_COLORS`, simply add the following line to your
`.zshrc` after sourcing `zsh-syntax-highlighting.zsh`:

```zsh
zsh_highlight_files_extract_ls_colors
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please adjust this to also cater to people who don't already have LS_COLORS set?

word=$words[1]
words=("${(@)words:1}")
len=$#buf
buf="${buf/#[^$word[1]]#}" # strip whitespace
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? Why should this do what the comment claims it would? ${word} may include whitespace.

Comment on lines +184 to +186
l) [[ -e "$word_subst" ]] \
&& col=$ZSH_HIGHLIGHT_FILE_TYPES[ln] \
|| col=$ZSH_HIGHLIGHT_FILE_TYPES[or];;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if $word is a symlink whose target is a broken symlink?

* `su` - files that have the suid bit set
* `sg` - files that have the sgid bit set
* `ow` - files that are world-writable
* `tw` - files that are world-writable and sticky
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this say files or directories?

Comment on lines +208 to +212
for key val in "${(@)ZSH_HIGHLIGHT_FILE_PATTERNS}"; do
if [[ $basename = $~key ]]; then
col=$val
break
fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document that ordering is significant and first match wins.

if (( end > start + $#basename )); then
# There is a path component
if [[ $ZSH_HIGHLIGHT_FILE_TYPES[lp] = "same" ]]; then
region_highlight+=("$start $end $col")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update for memo= support in master: 810c2dc.

@danielshahaf danielshahaf removed this from the 0.8.0 milestone Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants