bat icon indicating copy to clipboard operation
bat copied to clipboard

Remove code that tries to handle ANSI escape inputs

Open Enselic opened this issue 2 years ago • 3 comments

Proof of concept to aid discussion in #2185. I have not made up my mind yet if this change in sensible, but I think it helps to see it in person. Make sure to ignore whitespace changes when diffing.

IMO this problem is not a blocker to get the v0.21.0 release out since this is not a change in behaviour compared to v0.20.0.

The main use case for the old code seems to be that colored input across newlines should be handled correctly. If you try this:

echo "\x1B[33mColor\nColor \x1B[m\nPlain\n" | ./target/debug/bat

it looks like this with cat:

Skärmavbild 2022-05-10 kl  08 42 04

and the same with the latest release of bat, but with the code in this PR it looks like this instead:

Skärmavbild 2022-05-10 kl  08 43 00

since we reset colors when we encounter \n it seems. But IMHO we should just consider ANSI escape inputs as out-of-scope for bat, since syntax highlighting is broken when input contains ANSI escape characters anyway.

I have also confirmed that this change makes bat show bat-bug-test in #2185 the same way as cat.

Enselic avatar May 10 '22 06:05 Enselic

So what are the things that couldn't be done anymore when removing the special ANSI sequence handling? CC @eth-p

sharkdp avatar May 22 '22 10:05 sharkdp

Apart from the use case I described from the start, I know of no additional use of the ANSI escape handling code. And worth emphasising: the current code breaks the use case in #2185. I suppose it is possible to fix it. But is it worth it?

I would also greatly appreciate any input you might have here @eth-p.

Enselic avatar May 22 '22 10:05 Enselic

Apart from the use case I described from the start, I know of no additional use of the ANSI escape handling code.

Oh, sorry. I misread your post. I understand now.

sharkdp avatar May 22 '22 10:05 sharkdp

No objections have been raised, so I think we can go ahead and merge this. We can always change our mind later and bring back the code, if we want to.

I will wait another week or so, and then rebase and merge this unless any objections are raised.

3 months has passed since last release, and there a bunch of nice fixes waiting to be released, so it would be nice to get a release out in the coming weeks, IMHO.

CC @sharkdp @keith-hall @eth-p

Enselic avatar Aug 16 '22 20:08 Enselic