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 more descriptive Qt message handler #1945

Closed
wants to merge 1 commit into from

Conversation

Wallacoloo
Copy link
Member

This adds a timestamp and label to each message sent to the console through Qt. For example, instead of logging

QPen::setWidth: Setting a pen width with a negative value is not defined

lmms instead logs

[23:37:06:462][Warning] QPen::setWidth: Setting a pen width with a negative value is not defined

One advantage of this is that it's easy to spot what messages are just debug info vs which ones are actually problems. The bigger advantage is that since it's difficult to find the source of warnings emitted internally from Qt (e.g. the above example - you can't just grep the LMMS source for the warning message), this lets you set a breakpoint in the logging function in order to get a backtrace at the point the error/warning occurs and find which bit of lmms code is responsible.

@tresf
Copy link
Member

tresf commented Apr 11, 2015

@Wallacoloo this looks very nice!

Not knowing much about this, does this capture all console messages from LMMS?

@Wallacoloo
Copy link
Member Author

This will only capture messages logged with qDebug(), qWarning(), etc. So it won't hook uses of printf, which is used interchangeably throughout the code base.

This was a function I found I needed while tracking down another bug, so I thought I'd see if it might be useful in the main repository. If having the message tags/timestamped applied only to some of the messages isn't desirable, I can remove that altogether. As mentioned above, the real value of this is the ability to insert breakpoints for tracing errors that come directly from the Qt shared objects / dlls, as those are very difficult to hook otherwise.

@tresf
Copy link
Member

tresf commented Apr 11, 2015

Yeah, and that was my exact concern... If we aren't making proper use of qDebug, qWarning today, this may have little value. I believe @badosu added some support for capturing startup errors, so I'll ping him for input. :)

@badosu
Copy link
Contributor

badosu commented Apr 11, 2015

@Wallacoloo I see that you're logging to the CLI, can you see a case where we can use this handler to show up messages to the user via GUI?

@Wallacoloo
Copy link
Member Author

@badosu Even most of the warning messages aren't relevant to the user. It does allow us to capture some "critical"/fatal errors in a way that might allow us to present a dialog to the user explaining the error before exiting (and maybe saving a backup of their project), but there are better ways to do that (e.g. SIGSEGV handlers, etc).

I'm going to close this, because it just doesn't seem very useful without near-100% consistent usage of qt's logging functions.

@Wallacoloo Wallacoloo closed this Apr 11, 2015
@badosu
Copy link
Contributor

badosu commented Apr 11, 2015

@Wallacoloo I don't understand why you're closing it, if you find any issues you can still improve it.

@Wallacoloo
Copy link
Member Author

Well I wanted to avoid clutter in the pull request list. But we should probably implement consistent logging throughout the code, so I guess we can discuss that here. Is it o.k. to just go through the code base and replace all printf()s with qDebug() and qWarning(), etc?

@Wallacoloo Wallacoloo reopened this Apr 11, 2015
@softrabbit
Copy link
Member

Could this be extended into something that handles both informational UI messages (e.g. the Song loading messages) and Qt messages?

@tresf
Copy link
Member

tresf commented Apr 12, 2015

^----- +1 :)

@Wallacoloo
Copy link
Member Author

It could, if we used some sort of tagging mechanism (e.g.
Logger::log(LogTag tag, QString msg)). Might help to decouple parts of the
core from the UI. It would at least remove the signal/slot usage that is
somewhat controversial over on the lmms splashscreen PR too.

I wonder if it's worth implementing something basic or just finding an
already existing log framework.
On Apr 12, 2015 6:08 AM, "Raine M. Ekman" notifications@github.com wrote:

Could this be extended into something that handles both informational UI
messages (e.g. the Song loading messages) and Qt messages?


Reply to this email directly or view it on GitHub
#1945 (comment).

@Wallacoloo
Copy link
Member Author

@softrabbit #1982 (pending approval) provides a Messenger class that can be used for routing things like song loading messages.

@tresf
Copy link
Member

tresf commented Apr 28, 2015

@Wallacoloo just ran across this comment which I find to be a bit relevant to this conversation in terms of pros/cons of various ways to echo messages...

#1108 (comment)

@Wallacoloo
Copy link
Member Author

Closing this. We can re-open it or start another conversation elsewhere if anyone ever wants to take on the task of enforcing a consistent logging style in LMMS.

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.

4 participants