Add logging #331

psyomn · 2021-04-24T19:41:51Z

Description of Change(s)

Adds a very primitive and simple way to log stuff. Currently I see we don't use a lot of qDebug
and friends, but maybe we can leverage around this in the future, if we need to debug some
sort of weird behavior.

Feel free to request any sort of changes -- I'd like to keep the style and thoughts as close to the rest
of the repo, and I feel I may have overlooked something.

Thanks!

Fixes Issue(s)

N/A

check logs exist on:

Windows
Mac
Linux

cameronwhite · 2021-04-24T21:09:07Z

I think the additions look good so far! My main comment would be that we might want some logging in parts of the codebase that don't have a Qt dependency (e.g. looking for existing usage of std::cerr) so perhaps some of this should be split out and moved into a lower level library like Util?
There also is a boost logging library, but that's probably overkill for what we need :)

psyomn · 2021-04-24T21:15:10Z

Ah good point! I'll check that out, and poke you again for a check, thanks!

luzpaz · 2023-12-05T16:14:46Z

Any traction here ?

psyomn · 2023-12-06T04:25:19Z

Considering holidays are coming, it might be a nice time to resume my work on here. Possibly end of month, I'll give another jab at this.

…

On Tue, Dec 05, 2023 at 08:14:56AM -0800, luzpaz wrote: Any traction here ? -- Reply to this email directly or view it on GitHub: #331 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

cameronwhite

Looks great overall so far! Just left a few miscellaneous comments

source/app/powertabeditor.cpp

cameronwhite · 2023-12-31T03:57:02Z

source/app/powertabeditor.cpp

@@ -260,14 +261,14 @@ void PowerTabEditor::openFile(QString filename)
    int validationResult = myDocumentManager->findDocument(path);
    if (validationResult > -1)
    {
-        qDebug() << "File: " << filename << " is already open";
+        Log::d("file: {} is already open", filename.toStdString());


One possible extension would be adding format support for QString (e.g. https://www.cppstories.com/2022/custom-stdformat-cpp20/#single-values) to avoid the toStdString() calls

This could go in an <app/log.h> wrapper which adds Qt-specific stuff that doesn't belong in the core util library

So I tried this out but I'm still getting compilation errors. I'm assuming that I should include this (app/log.h) after including util/log.h:

template <> struct std::formatter<QString> : std::formatter<std::string> { auto format(const QString& qstr, std::format_context& ctx) const { return std::formatter<std::string>(qstr.toStdString(), ctx); } };

This gives me a compilation error; I'm going to go over this this weekend if I have the time.

Hmm, maybe it needs to be in the fmt namespace rather than std to work with the fmt library?

I'd also be fine having app/log.h include util/log.h so that code in the UI libraries just needs to include one header to get access to logging plus the Qt format conversions

So I just realized that I basically forgot to rename the guard of the previous app/log.h in util/log.h. This resulted into a few hours of insanity of why do these two headers flip each other off or on (haha).

Got this to work!

source/app/powertabeditor.cpp

source/util/CMakeLists.txt

test/CMakeLists.txt

cameronwhite · 2023-12-31T04:13:24Z

source/util/log.h

+#ifndef APP_LOG_H
+#define APP_LOG_H
+
+#include <chrono>


To avoid pulling in a bunch of includes in the header, one thought is that the backend function could just take the final formatted string (e.g. call vformat() beforehand), and then it can just go in the .cpp file since it doesn't need to be templated. Then most of the related members like the mutex don't need to be accessible in the header

Fixed this too! I have to AFK for a bit, but once I'm back, I'll make sure that this works on windows too and tick that.

Partially inspired by rfc5424, along with a few other idioms I picked up along the way. This should allow one to tweak the log level by setting a minimum log level in the `Log' namespace. Right now I've set the log level to debug in the main entry point.

Users should be able to copy the logs from the infodialog box, along with version information for slightly easier troubleshooting.

I'm leaving this commit here instead of squashing, so that in the future, we could technically just reverse this commit.

This implements a very simple circular buffer that will take the last N lines in a log file, and keep those instead. Right now the maximum N is hardcoded, but the code is written in a fashion that we can revisit some of this, and add toggles where we see fit (for example in a settings dialog or so). I tested out the functionality this way: $ yes some-log-line | nl | head -5000 > log.txt And then running PTE2 with a max log size of 1000, I noticed the logfile like so: $ head -10 log.txt 4001 some-log-line 4002 some-log-line 4003 some-log-line 4004 some-log-line 4005 some-log-line 4006 some-log-line 4007 some-log-line 4008 some-log-line 4009 some-log-line 4010 some-log-line and the bottom: $ tail -10 log.txt 2023-12-30T14:38:32Z: [debug]: finding translations for locale: 2023-12-30T14:38:32Z: [debug]: locale: en-US 2023-12-30T14:38:32Z: [debug]: locale: en 2023-12-30T14:38:32Z: [debug]: locale: en-Latn-US 2023-12-30T14:38:32Z: [debug]: - checking: /home/psyomn/.local/share/powertab/powertabeditor/translations 2023-12-30T14:38:32Z: [debug]: - checking: /usr/local/share/powertab/powertabeditor/translations 2023-12-30T14:38:32Z: [debug]: - checking: /usr/share/powertab/powertabeditor/translations 2023-12-30T14:38:32Z: [debug]: - checking: /home/psyomn/programming/cc/fork/powertabeditor/build/bin/data/translations 2023-12-30T14:38:32Z: [debug]: - checking: /usr/share/qt/translations 2023-12-30T14:38:32Z: [debug]: loaded qt base translations from /usr/share/qt/translations/qtbase_en.qm and last checks: $ nl log.txt | head -1 1 4001 some-log-line $ nl log.txt | tail -1 1011 2023-12-30T14:38:32Z: [debug]: loaded qt base translations from /usr/share/qt/translations/qtbase_en.qm

This adds a custom format wrapper for QString objects, so that they can be logged directly.

psyomn · 2024-01-08T01:51:15Z

Windows: so I'm getting radio silence from logs on windows, and I don't get it. I'm guessing stdout/stderr don't emit like in sane, normal, programming environments too, so I really can't gauge where exactly the fail is happening. I've read here and there that on Windows GUI programs can actually mute these unless you > them to files (but that didn't work either). I'll revisit this next weekend and see what's up.

Linux works fine.

cameronwhite · 2024-01-08T02:55:59Z

Yeah I think that's probably the expected behaviour and is kinda tricky to change: e.g. https://stackoverflow.com/questions/3360548/console-output-in-a-qt-gui-app

IIRC, with qDebug() the behaviour was the same, but the logs did show up in the Visual Studio debugger. I think it might be using a Windows API like OutputDebugString()

psyomn · 2024-01-08T03:06:35Z

Ah, I was merely complaining that `cout` was not available as a fallback; but that doesn't allow me to figure out why the log file is not being created currently (I checked the paths, and APPDATA in theory should've had this file). This will be something I'll take a look at next weekend.

…

On Sun, Jan 07, 2024 at 06:56:10PM -0800, Cameron White wrote: Yeah I think that's probably the expected behaviour and is kinda tricky to change: e.g. https://stackoverflow.com/questions/3360548/console-output-in-a-qt-gui-app IIRC, with `qDebug()` the behaviour was the same, but the logs did show up in the Visual Studio debugger. I think it might be using a Windows API like `OutputDebugString()` -- Reply to this email directly or view it on GitHub: #331 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

psyomn force-pushed the feature/add-logging branch from 16f68c1 to 9d69185 Compare December 15, 2023 06:01

psyomn force-pushed the feature/add-logging branch 7 times, most recently from 162ab39 to 3961ac8 Compare December 30, 2023 20:02

cameronwhite reviewed Dec 31, 2023

View reviewed changes

psyomn force-pushed the feature/add-logging branch 3 times, most recently from 4942db1 to 387f58b Compare January 5, 2024 03:32

Add simple logging utility

ff34267

Partially inspired by rfc5424, along with a few other idioms I picked up along the way. This should allow one to tweak the log level by setting a minimum log level in the `Log' namespace. Right now I've set the log level to debug in the main entry point.

psyomn force-pushed the feature/add-logging branch from 387f58b to abfb9ad Compare January 5, 2024 03:36

psyomn added 12 commits January 7, 2024 01:20

Add logging path to Paths namespace

d7d9ba2

Replace std::cerr with Log::e in score/...

933c675

Replace std::cerr with Log::e in formats/...

e436cb5

Replace std::cerr with Log::e in midioutputdevice.cpp

bd117d8

Replace qDebug with Log::d in app/...

5a33e41

Replace qDebug with Log::d in build/main.cpp

ab56bc3

Show logs in infodialog's edit box

26cf888

Users should be able to copy the logs from the infodialog box, along with version information for slightly easier troubleshooting.

Add fmt for Linux CI/CD build

34f0088

Add fmt for macOS CI/CD build

e72a569

Add fmt for Windows CI/CD build

64307ad

Add fmtlib dependency mention in README.md

3ce38cb

Mention C++20 compiler requirement in README.md

2ca68d1

psyomn added 2 commits January 7, 2024 01:20

Add fmtlib (>=8.0.0) in appropriate CMake files

5ba2a3d

Replace use of std::format with fmt:format in logger

f5da24a

I'm leaving this commit here instead of squashing, so that in the future, we could technically just reverse this commit.

psyomn force-pushed the feature/add-logging branch 2 times, most recently from 12318dd to e11f0b4 Compare January 7, 2024 22:06

psyomn added 4 commits January 7, 2024 17:06

Add format wrapper for QString

aabde63

This adds a custom format wrapper for QString objects, so that they can be logged directly.

Use implicit QString formatter in logger

5bb4293

Hide logging includes by limiting template use

e11f0b4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging #331

Add logging #331

psyomn commented Apr 24, 2021 •

edited

Loading

cameronwhite commented Apr 24, 2021

psyomn commented Apr 24, 2021

luzpaz commented Dec 5, 2023

psyomn commented Dec 6, 2023 via email

cameronwhite left a comment

cameronwhite Dec 31, 2023

psyomn Jan 5, 2024

cameronwhite Jan 5, 2024

psyomn Jan 7, 2024

cameronwhite Jan 7, 2024

cameronwhite Dec 31, 2023

psyomn Jan 7, 2024

psyomn commented Jan 8, 2024

cameronwhite commented Jan 8, 2024

psyomn commented Jan 8, 2024 via email

Add logging #331

Are you sure you want to change the base?

Add logging #331

Conversation

psyomn commented Apr 24, 2021 • edited Loading

Description of Change(s)

Fixes Issue(s)

cameronwhite commented Apr 24, 2021

psyomn commented Apr 24, 2021

luzpaz commented Dec 5, 2023

psyomn commented Dec 6, 2023 via email

cameronwhite left a comment

Choose a reason for hiding this comment

cameronwhite Dec 31, 2023

Choose a reason for hiding this comment

psyomn Jan 5, 2024

Choose a reason for hiding this comment

cameronwhite Jan 5, 2024

Choose a reason for hiding this comment

psyomn Jan 7, 2024

Choose a reason for hiding this comment

cameronwhite Jan 7, 2024

Choose a reason for hiding this comment

cameronwhite Dec 31, 2023

Choose a reason for hiding this comment

psyomn Jan 7, 2024

Choose a reason for hiding this comment

psyomn commented Jan 8, 2024

cameronwhite commented Jan 8, 2024

psyomn commented Jan 8, 2024 via email

psyomn commented Apr 24, 2021 •

edited

Loading