Add `gnu_legacy` feature

alexkunde commented 1 year ago

Hi, this crate is used in the rust drop-in replacement for gnu coreutils called uucoreutils. The original GNU util ls is still using some extended modes that are yet not supported by this crate.

two-digit style codes
reset prefix for any string

To continue using this crate, it would really help to improve on those two topics. This PR will add both functionalities.

nu-ansi-term still needs to officially release the feature, so this is a draft for now.

After the merge, this GNU test will turn green:

OLD

FAIL: tests/ls/color-term
=========================

--- exp 2023-05-16 15:56:30.337121900 +0200
+++ out 2023-05-16 15:56:30.102644900 +0200
@@ -1,4 +1,4 @@
-^[[0m^[[01;32mexe^[[0m$
-^[[0m^[[01;32mexe^[[0m$
+^[[1;32mexe^[[0m$
+^[[1;32mexe^[[0m$
 exe$
 exe$
FAIL tests/ls/color-term.sh (exit status: 1)

After both PRs

SUCCESS: tests/ls/color-term
=========================
@@ -1,4 +1,4 @@
^[[0m^[[01;32mexe^[[0m$
^[[0m^[[01;32mexe^[[0m$
 exe$
 exe$
SUCCESS tests/ls/color-term.sh (exit status: 0)

Thanks for your help!

sharkdp commented 1 year ago

Thank you for working on this! Did you see #62? I recently worked on similar GNU ls compatibility tests, but I had some trouble with getting it to work on all platforms. Maybe we should focus on getting #62 merged in one way or another first?

alexkunde commented 1 year ago

Hi @sharkdp , I think it's all going down the same way. I will have a look at #62 on how we might combine this.

alexkunde commented 1 year ago

@sharkdp I updated my coreutils branch with this version and it indeed fixed the gnu tests. https://github.com/uutils/coreutils/pull/4867#issuecomment-1582351106 -> we still need a real nu-ansi-term release to succeed

tavianator commented 1 year ago

I think it's a little strange to hardcode two-digit CSI codes. I don't think GNU ls does that, it just prints what's in $LS_COLORS verbatim.

alexkunde commented 1 year ago

Thanks for the heads up, you are correct in a sense that ls only uses what LS_COLORS provides. On the other hand if LS_COLORS is not set, it will use an internal default, that is double digit code.

From what I understand LSCOLORS crate is using terminal libraries to paint the parsed numbers back into terminal native color codes. Therefore in my opinion this works as intended, two-digit code in, two digit code out. There is no direct parse, but two individual sides with different goals. The gnu_legacy mode is currently the try to provide the closest to this pipe-through.

alexkunde commented 1 year ago

@sharkdp I did another round of tests against my implementation and while it will fix two gnu drop in tests, something seems off when I do more extensive testing. I will have another look on behaviour. Sorry letting you wait so much.

tavianator commented 1 year ago

Here's an interesting GNU behaviour:

tavianator@graphene $ LS_COLORS= ls --color /usr/bin/ls | xxd
00000000: 1b5b 306d 1b5b 3031 3b33 326d 2f75 7372  .[0m.[01;32m/usr
00000010: 2f62 696e 2f6c 731b 5b30 6d0a            /bin/ls.[0m.
tavianator@graphene $ LS_COLORS="no=01;37" ls --color /usr/bin/ls | xxd
00000000: 1b5b 306d 1b5b 3031 3b33 376d 1b5b 6d1b  .[0m.[01;37m.[m.
00000010: 5b30 313b 3332 6d2f 7573 722f 6269 6e2f  [01;32m/usr/bin/
00000020: 6c73 1b5b 306d 0a                        ls.[0m.

alexkunde commented 1 year ago

Hi @tavianator , can you please elaborate on what you find interesting and how it relates to this PR?

tavianator commented 1 year ago

If no is set, then GNU ls doesn't just print \033[0m before the path, it prints \033[0m\033[${no}m\033[m, which might be another source of differences vs. the GNU tests

alexkunde commented 1 year ago

@sharkdp I revisited the strange behaviour as mentioned above. ls when coloring, will only reset the very first colored item in a list, not every item. The gnu tests don't really show this, or my sample was too small.

I added a new function called .to_nu_ansi_term_style_with_reset() which will explictly call reset before printing the colored string. The alternative would be to extend the style object of LSCOLORS, which would lead to breaking changes (if ..Default::default() is not used to fill it.

Changes

added: feature gnu_legacy
fix: made features mutually exclusive, as they should be for different terminals
fix: updated CI/CD to reflect the changes and added additional matrix dimension for per feature tests
~~added: new function .to_nu_ansi_style_with_reset() for nu-ansi-term and gnu_legacy features~~ (use struct function from nu-ansi-term directly
added: crossterm feature for binary
added: crossterm tests
added: gnu_legacy tests
added: compile errors, when no feature is selected or nu-ansi-term and gnu_legacy are used together
added: example for crossterm to readme.MD

To-Do

wait for official release of nu-ansi-term, the current one has the change we need, but is broken

GNU tests

Please see https://github.com/uutils/coreutils/pull/4867 for the current status.

tavianator commented 1 year ago

ls when coloring, will only reset the very first colored item in a list, not every item.

Ah I think I might know why: if ls starts with non-default formatting then plain files will inherit it, but it will reset before colored files to make sure they get the right style.

$ printf '\033[3mITALICS\n'; ls file dir
ITALICS
file dir

file will inherit the italics style (with default LS_COLORS, anyway), but it will be reset before dir.

Instead of .to_nu_ansi_term_style_with_reset(), maybe a separate .reset_style() method would work?

alexkunde commented 1 year ago

@tavianator yes exactly. Seems like I can simply use .reset_before_style() from the original struct, so no extra function is needed.

sharkdp commented 1 year ago

Sorry letting you wait so much.

There is absolutely no hurry, certainly not from my side. Thank you very much for working on this. I will wait until this is in a merge-able state before I review it.

sylvestre commented 1 year ago

@alexkunde do you have an eta for completed this? thanks

sylvestre commented 1 year ago

@sharkdp it looks good to me (besides the conflict), maybe this could be reviewed now ? thanks (i would like to use this PR for https://github.com/uutils/coreutils thanks

sharkdp commented 1 year ago

I'm happy to include this but it would be great if someone could put in the work to resolve the conflicts and maybe also to review this.

sylvestre commented 1 year ago

i rebased it here: https://github.com/sharkdp/lscolors/pull/75

sharkdp / lscolors