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

Add option to use additional colors #13

Closed
wants to merge 1 commit into from

Conversation

ichttt
Copy link
Contributor

@ichttt ichttt commented Mar 16, 2020

This allows to mimic log4j's HighlightConverter more accurate.
By default, it still only highlights errror and warning, info/debug coloring need to be enabled by changing the log4j xml to include a true parameter.
This is useful for forge, as it currently uses log4j's highlight converter (reasoning being the additional colors), which obviously ignored missing ansi support and therefor looks ugly on non-ansi consoles.

@@ -62,17 +62,23 @@
private static final String ANSI_RESET = "\u001B[m";
private static final String ANSI_ERROR = "\u001B[31;1m"; // Bold Red
private static final String ANSI_WARN = "\u001B[33;1m"; // Bold Yellow
private static final String ANSI_INFO = "\u001B[32m"; // Blue

Choose a reason for hiding this comment

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

Considering that TCA is used by some Minecraft server modifications, then turning INFO level blue by default is not a great idea imho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First off, coloring below log level warn is disabled by default, you need to set a flag to enable it. Also, this is designed to mimic the log4j highlighter, so I have choosen the same colors as log4j

Choose a reason for hiding this comment

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

I'm aware that one needs to set a flag to enable it.

@stephan-gh
Copy link
Member

Thanks for your PR!

Originally this was just intended as a simple alternative to Log4j's more complex (but also powerful) %highlight. I think I would rather not make it as powerful.

In general Log4j's HighlightConverter has the noConsoleNoAnsi option. That should work in most cases (disable colors unless running in a proper terminal), but it's a bit more strict. TCA enables colors also for IntelliJ (with some really ugly hacks!), which doesn't report a full console but at least allows console colors.
At least for Forge this could be easily avoided because there is control about the start parameters for IntelliJ. So you could add a system property that disables noConsoleNoAnsi just for IntelliJ and should then have only colors when you really want them.

I think that might be a better solution than editing the HighlightErrorConverter to act exactly like %hilghlight (the name %highlightError gets a bit misleading with this change...).

%highlightError was a pretty bad hack I did back then because I was unable to make Log4j's highlighter aware of the terminal state in TCA. At this point I would rather remove it entirely. That would be quite a bit of work given the amount of projects it is used in.

So, is there any way we can make this work with noConsoleNoAnsi for Forge?

@ichttt
Copy link
Contributor Author

ichttt commented Mar 17, 2020

noConsoleNoAnsi does not work too well, as there are consoles that do not support ansi. So when you use e.g. the windows cmd has a java console, but does not support ansi codes. TCA correctly prints Advanced terminal features are not available in this environment, but l4j uses ansi codes as it found a console...
I still don't see a way to make l4j aware of TCA's state, especially as l4j is initialized very, very early...
And I know highlightError is not an appropiate name, but I didn't want to introduce a breaking change...

@stephan-gh
Copy link
Member

Closing in favor of #16.

@stephan-gh stephan-gh closed this May 7, 2021
@ichttt ichttt deleted the patch-1 branch May 8, 2021 13:06
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.

None yet

3 participants