p-e-w / shin

A shell in every text input on your system
GNU General Public License v3.0
276 stars 12 forks source link

Filter out the escape sequences #3

Closed yxnan closed 1 year ago

yxnan commented 1 year ago

I think it's useful to leave out the escape sequences, because usually text editor will not properly render shell color code anyways.

For example, this is the output of command tldr ls

image

p-e-w commented 1 year ago

This is a bug in your version of tldr, which you might want to report upstream. Programs are supposed to check whether stdout is a TTY before emitting escape sequences. If they don't check that, other operations such as redirecting output to a file or piping it to a pager will also fail.

Almost all programs do this correctly. Try ls or man, they won't display colors or render a pager when run from Shin. The implementation of TLDR I use, Tealdeer, also does the right thing. Every terminal handling library does this automatically. Blindly printing raw escape sequences is very bad practice.

While Shin could strip escapes from the output and it would happen to work in the simple case you posted, in other cases it would fail spectacularly. Not all escape sequences are basic SGR codes controlling formatting attributes. Cursor movement, terminal modes etc., when ignored and stripped out, will lead to corrupt output if the program mistakenly assumes it's running in a TTY.

In summary, I'm leaning towards "won't fix" here. This is without question a problem with the program you're running, and it also affects scenarios other than Shin.

yxnan commented 1 year ago

Thanks for your explaination, now I see it's the programs' duty to do the extra works.

I'll check if the issue is still persist in newest tldr-python-client and report to them. And thanks for point to the alternative project tealdeer!

0unknwn commented 1 year ago

Neofetch has the same issue for me.

p-e-w commented 1 year ago

I couldn't believe a tool as popular as Neofetch would have this problem. Don't people ever pipe Neofetch output to a file?!?

But indeed, here it is: https://github.com/dylanaraps/neofetch/issues/753

And what's even worse, they "fixed" it by adding an "--stdout" flag, instead of checking whether they're running in a TTY!

This sucks so bad. Everything is broken.

But reality is real. You've changed my mind. I will implement the workaround of removing SGR escapes in Shin.

p-e-w commented 1 year ago

Done. Kindly test whether this fixes the problem with the programs you're using.

Note that only SGR escape sequences are removed. This is deliberate. If a program emits other kinds of escape sequences outside of a TTY, there are likely bigger issues than escapes littering the output, and I'd rather not mask those.

0unknwn commented 1 year ago

neofetch looks better now:

[?25l[?7l             .',;::::;,'.
         .';:cccccccccccc:;,.
      .;cccccccccccccccccccccc;.
    .:cccccccccccccccccccccccccc:.
  .;ccccccccccccc;.:dddl:.;ccccccc;.
 .:ccccccccccccc;OWMKOOXMWd;ccccccc:.
.:ccccccccccccc;KMMc;cc;xMMc:ccccccc:.
,cccccccccccccc;MMM.;cc;;WW::cccccccc,
:cccccccccccccc;MMM.;cccccccccccccccc:
:ccccccc;oxOOOo;MMM0OOk.;cccccccccccc:
cccccc:0MMKxdd:;MMMkddc.;cccccccccccc;
ccccc:XM0';cccc;MMM.;cccccccccccccccc'
ccccc;MMo;ccccc;MMW.;ccccccccccccccc;
ccccc;0MNc.ccc.xMMd:ccccccccccccccc;
cccccc;dNMWXXXWM0::cccccccccccccc:,
cccccccc;.:odl:.;cccccccccccccc:,.
:cccccccccccccccccccccccccccc:'.
.:cccccccccccccccccccccc:;,..
  '::cccccccccccccc::;,.
[41C-------------- 
[41COS: Fedora Linux 37 (Workstation Edition) x86_64 
[41CKernel: 5.19.15-301.fc37.x86_64 
[41CUptime: 21 hours, 2 mins 
[41CPackages: 6600 (rpm), 103 (flatpak) 
[41CShell: bash 5.2.2 
[41CResolution: 1920x1080 
[41CDE: GNOME 43.0 
[41CWM: Mutter 
[41CWM Theme: Adwaita 
[41CTheme: adw-gtk3 [GTK2/3] 
[?25h[?7h
p-e-w commented 1 year ago

So they are using cursor movement escape sequences too.

As explained above, I won't be removing those in Shin. Even if I did, the output would still look broken because they are using cursor movement to align the text. This is precisely why stripping out such sequences makes no sense. They are required for the output to work.

To quote from the Neofetch issue linked above (emphasis added):

Neofetch currently doesn't have an option to do this and I'm not sure if I'll add this feature since Neofetch wasn't intended to be piped into other commands.

You can try the --stdout flag described in the linked issue, but apparently people are still having problems with that. At the end of the day, if a program is exclusively designed for the terminal and refuses to cater to the possibility that people want to pipe its output elsewhere, there's only so much we can do.

0unknwn commented 1 year ago

Sounds reasonable to me. Neofetch seems to be not maintained anymore anyway.

yxnan commented 1 year ago

hyfetch is an active fork of neofetch, and the fix to detect the output type has been already merged: https://github.com/hykilpikonna/hyfetch/pull/31, so you can use that forked version instead