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 colored console output #3197

Merged
merged 2 commits into from
Jan 2, 2021
Merged

Add colored console output #3197

merged 2 commits into from
Jan 2, 2021

Conversation

poelzi
Copy link
Contributor

@poelzi poelzi commented Oct 21, 2020

TODO:

Colorize log level, thread-name and category of console output.
Supports the NO_COLOR environment for disabling color output.

Screenshot_20201021_220346

Errors, warnings are clearly visible. Controllers and debug output is also clearly distinguished.

@Holzhaus
Copy link
Member

These ANSI escape codes are only supported on Unix. Let's find out that broke QT_MESSAGE_PATTERN support and just use that instead.

@Holzhaus
Copy link
Member

These ANSI escape codes are only supported on Unix. Let's find out that broke QT_MESSAGE_PATTERN support and just use that instead.

Found the issue. Just apply this patch:

diff --git a/src/util/logging.cpp b/src/util/logging.cpp
index c967947789..047f9a4902 100644
--- a/src/util/logging.cpp
+++ b/src/util/logging.cpp
@@ -195,24 +195,9 @@ void handleMessage(
         }
     }

-    QByteArray ba;
-    ba.reserve(baSize);
-
-    ba.append(levelName);
-    ba.append(" [");
-    ba.append(threadName, threadName.size());
-    if (categoryName) {
-        ba.append("] ");
-        ba.append(categoryName, categoryName_len);
-        ba.append(": ");
-    } else {
-        ba.append("]: ");
-    }
-    ba.append(input8Bit, input8Bit.size());
-    ba.append('\n');
-
-    // Verify that the reserved size matches the actual size
-    DEBUG_ASSERT(ba.size() == baSize);
+    QString logMessage = qFormatLogMessage(type, context, input);
+    QByteArray ba = logMessage.toLocal8Bit();
+    ba.append("\n");

     if (isDebugAssert) {
         if (s_debugAssertBreak) {

Then you can set QT_MESSAGE_PATTERN however you want, e.g.:

export QT_MESSAGE_PATTERN="`echo -e "\033[32m%{time h:mm:ss.zzz}%{if-category}\033[32m %{category}:%{endif} %{if-debug}\033[34m%{function}%{endif}%{if-warning}\033[31m%{backtrace depth=3}%{endif}%{if-critical}\033[31m%{backtrace depth=3}%{endif}%{if-fatal}\033[31m%{backtrace depth=3}%{endif}\033[0m %{message}"`"

@poelzi
Copy link
Contributor Author

poelzi commented Oct 21, 2020

newer consoles of windows 10, alternative consoles, macosx support ansi colors. only old windows cmd does not support it.

@Holzhaus
Copy link
Member

newer consoles of windows 10, alternative consoles, macosx support ansi colors. only old windows cmd does not support it.

OK, but why choose a more complex and way less powerful solution than what I posted above?

@poelzi
Copy link
Contributor Author

poelzi commented Oct 22, 2020

Screenshot_20201023_010411

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Let's first merge #3204 before introducing any custom extensions.

@uklotzde uklotzde marked this pull request as draft October 23, 2020 08:22
@Be-ing Be-ing marked this pull request as ready for review December 10, 2020 17:48
@Be-ing
Copy link
Contributor

Be-ing commented Dec 10, 2020

#3204 has been merged. What is that status of this now?

@poelzi
Copy link
Contributor Author

poelzi commented Dec 13, 2020

@Be-ing I don't know. The problem wtih #3204 is, that it now pollutes the log file with ansi codes if I set there something that looks nice. I added it to my nix-shell environment because I know that I will not look at those files and only use the log file.

So, I'm somehow inclined to simply close this, otherwise we would need some ansi sequence remover before writing to the log file.

@poelzi
Copy link
Contributor Author

poelzi commented Dec 13, 2020

We could use https://code.woboq.org/qt5/qt-creator/src/libs/utils/ansiescapecodehandler.cpp.html to create the logfile version

@poelzi
Copy link
Contributor Author

poelzi commented Dec 13, 2020

@uklotzde
Copy link
Contributor

I am against adding more code to a performance critical path. Instead you could color your log file afterwards by applying regular expressions. This is nothing that needs to be added to production code where it runs for all users if only a few developers are actually using such a feature.

Only exception: Hidden behind a feature flag that is disabled by default. But then we still have to maintain the addtional code.

@Holzhaus
Copy link
Member

The proper way would be to format log file messages always with the standard format, and only use the custom pattern to the terminal output. I don't know if that is possible though.

@uklotzde
Copy link
Contributor

The proper way would be to format log file messages always with the standard format, and only use the custom pattern to the terminal output. I don't know if that is possible though.

Exactly, it should be additive and optional, not subtractive and mandatory.

@poelzi
Copy link
Contributor Author

poelzi commented Dec 13, 2020

I misread the code. the formatting is only used for the terminal

@poelzi
Copy link
Contributor Author

poelzi commented Dec 13, 2020

This is a new minimal version of it. I'm using this format now since the pull request and came to like it really much.
Looks like this:
Screenshot_20201214_000133

@poelzi
Copy link
Contributor Author

poelzi commented Dec 13, 2020

Would be cool if the scripting engine could determine which js function triggered the debug() and would fill the variable, but otherwise I'm super happy with that.

src/util/cmdlineargs.cpp Outdated Show resolved Hide resolved
@poelzi poelzi force-pushed the color-logging branch 3 times, most recently from 893ebab to 152acc5 Compare December 14, 2020 00:13
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

15:22:34.647 [Main] warning  QCheckBox(0x55fdbf655b00, name="LibraryBPMButton")  does not have a property named  "alignment"
              warning [Main] QCheckBox(0x55afcafb1fa0, name="LibraryBPMButton")  does not have a property named  "alignment"

This swaps type and thread name. This should not happen.
This also adds a time stamp which is not related to color.

Not sure about the default value --color=auto. Mixxx is no console application so it should not waste CPU time to beautify the console output. However it is now only if one starts Mixxx from a console and makes Warnings more visible.

Maybe a good trade off. What doe other think?

In the same scene we can add a bash-completion snippet. :-P

Default is --colors auto which detects if user runs from terminal and
no NO_COLOR env is set.
Argument always forces colors, never prevents them
@poelzi
Copy link
Contributor Author

poelzi commented Dec 20, 2020

@daschuer changed the layout to reassemble the normal layout

@daschuer
Copy link
Member

For the future: Please do not rebase an active PR. Else we need to review the PR from the beginning instead of only the fix.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you.

@daschuer
Copy link
Member

@uklotzde What do you think now?

@Holzhaus
Copy link
Member

I don't see anything in the diff about the log file. Generally this looks good, but the log file should always be written without colors. Is that the case? I'm short on time atm, I'll have a look asap.

@poelzi
Copy link
Contributor Author

poelzi commented Dec 23, 2020

@Holzhaus the file log is always without colors. The default is no colors and only if the stdout is detected as a tty the console output is colorized.

@poelzi poelzi requested a review from uklotzde January 2, 2021 03:25
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Works well and is helpful for debugging. Thank you! LGTM

@uklotzde uklotzde merged commit 22d0c45 into mixxxdj:main Jan 2, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Feb 23, 2021

Thanks for working on this. Mixxx's console output is easier to read now.

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.

5 participants