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

Strip ANSI codes from page based on config - fixes #79 #86

Merged
merged 6 commits into from
Sep 4, 2020
Merged

Strip ANSI codes from page based on config - fixes #79 #86

merged 6 commits into from
Sep 4, 2020

Conversation

sumpygump
Copy link
Contributor

Adds new boolean config option ansi in [a-general] to switch whether ANSI codes in page content will be rendered to terminal.

If ansi is set to false or color is set to false, any ANSI codes will be stripped from fetched content.
Fixes #79

Here is a summary of results for the various values of the two options:

config: color config: ansi Result
true true colorize headings & links; translate ANSI
true false colorize headings & links; strip ANSI
false true no color; strip ANSI
false false no color; strip ANSI

Notice how it is not possible to have ansi color codes true and color false at the same time. If the color config option is false, it overrides the value of ansi.

Feature to only allow ansi codes in preformatted blocks is part of another issue (#59)

Copy link
Owner

@makew0rld makew0rld left a comment

Choose a reason for hiding this comment

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

Thanks, this is helpful! I have a few issues.

  • What is the source for that regex? You should put it as a comment in the code
  • Define the regex as a package/global variable like ansiRegex or ansiRE
  • Optionally strip ANSI in the RenderANSI function as well
  • Add the default value of a-general.ansi to config/config.go, like all config options

You can add //nolint:lll right before the regex variable to make linting pass, I don't mind in this case.

@sumpygump
Copy link
Contributor Author

Thanks for the tips. I made your suggested changes.

@makew0rld
Copy link
Owner

makew0rld commented Sep 3, 2020

The ansiRegex variable should be the output of regexp.MustCompile, not a string. Since the stripANSI function would just become one line, you can replace it with just using ansiRegex.Replace....

Also I'd appreciate if you pointed to the source directly:

// Source: https://github.com/acarl005/stripansi/blob/2749a054d4758fb0126d83aae590d7c9e8982d77/stripansi.go

On another level, I wonder if there's any point in stripping non-color codes only sometimes. The way things are done currently, those codes will be displayed on the screen when ANSI is enabled, but stripped when it's disabled. It might make sense to only strip color codes, since the move and other codes don't actually affect the terminal display anyway, thanks to cview.

A regex like `\x1b\[[0-9;]*m` (note it's a backtick string) should be able to just get the color codes. That's taken from here, which I linked in #79. I'm not sure why the stripansi regex is so long compared to these other ones. I've seen this longer version in other places like here, but there seems to be no complaints about the shorter ones on superuser.com.

@sumpygump
Copy link
Contributor Author

Okay, that makes sense. I was thinking that the goal would be to strip all ansi codes, worrying that things like clear screen and move would potentially mess up the rendering of the page. I'll switch it to the simpler regex.

@makew0rld makew0rld merged commit 7b27bd0 into makew0rld:master Sep 4, 2020
@sumpygump sumpygump deleted the feature/config-ansi branch September 4, 2020 21:38
makew0rld added a commit that referenced this pull request Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strip ANSI codes from page
2 participants