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

Gravity update: Handle color escape sequences #3191

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

catch-error
Copy link

What does this PR aim to accomplish?:

Currently when running gravity update, the color escape sequences used by the shell script are printed literally on the web gui:

  [�[1;32m✓�[0m] DNS resolution is available

  [i] Neutrino emissions detected�[0m...
  [�[1;32m✓�[0m] Pulling blocklist source list into range
  ...

This should be the matching real colors instead.

How does this PR accomplish the above?:

The patch checks each line fragment for the existance of a pattern that represents an (ANSI) color code escape sequence and substitutes it with a fitting HTML code.

To preserve color coding also when removing line(s) because of OVER escape sequences, the code is changed from using .text() to .html().

Link documentation PRs if any are needed to support this PR:

N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

@yubiuser
Copy link
Member

yubiuser commented Jan 7, 2025

Interesting, what browser do you use?
I don't see colors, but the escape sequences are printed as they should as symbols.

@rdwebdesign
Copy link
Member

rdwebdesign commented Jan 7, 2025

I just checked and also don't see color codes.
I think they should be stripped before data is sent to the browser. Actually this is how v5 works.

@catch-error
Copy link
Author

I tried it on Chrome & Safari on MacOS, Chrome & Firefox and Windows & Linux. Some subtle differences how the actual escape character is printed, but the rest looks pretty much the same on all systems. I have been working with 6.0 code a lot lately, so maybe screwed something up on my DEV VM.

@rdwebdesign
Copy link
Member

rdwebdesign commented Jan 8, 2025

Did you change /opt/pihole/COL_TABLE file?

I was able to reproduce your issue only when I edit this file to force the use of colors even when the command is not executed from a terminal.

Output with modified COL_TABLE:
image

Output with original COL_TABLE:
image

@catch-error
Copy link
Author

No. But I have the WEBCALL always set for pihole. This dates back quite a bit and I simply forgot about it. When it was introduced, it had been a convenient way to always enable colors in a code independent way (with the side effect of also having them in the web interface). And since this is still in the 6.0 code, it just carried over.

Is there a technical reason for not wanting gravity output colors in the web interface? Iirc WEBCALL has been introduced specifically for piholeDebug.sh (and is still in pihole code today, although the debug option has been removed from the 6.0 web interface), but never extended to cover also the gravity output.

@rdwebdesign
Copy link
Member

Iirc WEBCALL has been introduced specifically for piholeDebug.sh

Actually, WEBCALL was introduced in 2017, to be used by /var/www/html/scripts/php/debug.php (the -w flag, line 55).
It was introduced specifically to replace Carriage Return (\r) with newline (\n) character before send the text to the web interface, not to allow colors.

At that time, the debug log in the web interface didn't had colors, because using them resulted in a broken output (like you are seeing).

Later (in 2022), I changed the COL_TABLE code to allow color in the web interface, but this only worked because I also created a PHP function to translate ANSI color codes into HTML+CSS.

This was created to fix the Debug Logs uploaded via web interface. Before that, the debug logs generated via web interface didn't had colors and it was harder for us to identify certain issues.

Now there is no web interface debug log in v6, so there is no need for this flag anymore. We just forgot to remove the unused code from COL_TABLE. Any terminal with the capacity to show colors will always use the colors without this flag.

Gravity Update was never showed in colors in v5 web interface (there was never a PHP function to translate them).

@catch-error
Copy link
Author

I wrote my own PHP code for that, though never submitted it. Still I‘d like to see colors also being included in the web interface - it just helps.

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.

3 participants