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

Filter out the escape sequences #3

Closed
yxnan opened this issue Oct 25, 2022 · 9 comments
Closed

Filter out the escape sequences #3

yxnan opened this issue Oct 25, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@yxnan
Copy link

yxnan commented Oct 25, 2022

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

p-e-w commented Oct 26, 2022

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

yxnan commented Oct 26, 2022

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!

@yxnan yxnan closed this as not planned Won't fix, can't repro, duplicate, stale Oct 26, 2022
@0unknwn
Copy link

0unknwn commented Oct 26, 2022

Neofetch has the same issue for me.

@p-e-w
Copy link
Owner

p-e-w commented Oct 27, 2022

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: dylanaraps/neofetch#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 p-e-w reopened this Oct 27, 2022
@p-e-w p-e-w closed this as completed in 7373a4f Oct 27, 2022
@p-e-w
Copy link
Owner

p-e-w commented Oct 27, 2022

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.

@p-e-w p-e-w added the enhancement New feature or request label Oct 27, 2022
@0unknwn
Copy link

0unknwn commented Oct 27, 2022

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

p-e-w commented Oct 27, 2022

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

0unknwn commented Oct 27, 2022

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

@yxnan
Copy link
Author

yxnan commented Oct 27, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants