[Bug 41199] [patch] Enhance logging system

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=41199

--- Comment #12 from Olli Salli <ollisal at gmail.com> 2011-10-05 06:53:52 PDT ---
(In reply to comment #11)
> New version pushed (always here [1])
> 
> 
> (In reply to comment #10)
> > THOUGH: please verify it doesn't
> > put "" around the original string as I think it might do. That would be ugly.
> [...]
> > One thing in its implementation too:
> > 
> > qDebug() << qPrintable(libraryName) << qPrintable(libraryVersion) << "DEBUG:"
> > << msg;
> > 
> > the library name and version are in the ASCII subset anyway. qPrintable will be
> > a needless memory allocation and conversion, having no effect on the output. Or
> > does that somehow get rid of the QDebug behavior of putting "" around printed
> > QStrings? You can get rid of that by not making QStrings out of them in the
> > first place (also needless memory alloc and copy), but just using preprocessor
> > string concatenation like the old code did, as that yields a C string literal,
> > not a QString.
> 
> Yes, that's the only way that I found to get rid of the "" around the QStrings,
> if you have any other suggestions on how to do it I'll be happy to change it
> because it looks ugly to me too...

For the default debug behavior, you can get rid of it for the lib name and
version, and gain some performance via the way I suggested before: make
debugCallback = NULL make invokeDefaultCallback() utilize the default behavior
(instead of just setDebugCallback() having those semantics). Whether you then
do

if (debugCallback) {
  debugCallback(QLatin1String("tp-qt4"), PACKAGE_VERSION, type, msg);
} else {
  defaultDebug(type, msg);
}

or write the code of defaultDebug inline in the else branch, you can implement
prefixing the library name and version by preprocessor string concatenation:

TELEPATHY_QT4_NO_EXPORT void defaultDebug(QtMsgType type, const QString &msg)
{
  if (type == QtDebugMsg) {
     qDebug() << "tp-qt4 " PACKAGE_VERSION " DEBUG:" << qPrintable(msg);
  } else {
     qWarning() << "tp-qt4 " PACKAGE_VERSION " WARN:" << qPrintable(msg);
  }
}

Note the use of qWarning (to preserve previous behavior even when a non-default
qt message handler is installed) and how we can this way prevent constructing
QStrings and having to convert them back to const char * in the console charset
for both the library name (which, here, we know) and the version (which we,
again, know, without invokeDebugCallback passing it to us).

But doesn't the copy ctor:

51                    debug = new QDebug(&msg);
52                    (*debug) << a.msg;
47    53    

also surround a.msg with ""? I don't think the library actually currently uses
the copy ctor though with a non-empty string - but you should verify that it
works properly for non-empty strings too for completeness, so this doesn't
cause confusion in the future.

Otherwise looks good to me now. Just these improvements here and we can merge
with confidence that the behavior is also performance wise retained at the
previous level, but with the added flexibility of specifying a debug callback.

> Verified ENABLE_DEBUG_OUTPUT=OFF and tested that both runtime enable and
> disable work, but tbh I couldn't do it with the tests because I cannot enable
> the normal output... Anyway I tested it with kde-telepathy file transfer

Yeah make check redirects stderr from the test binaries. I mainly meant that
you check that it doesn't crash or burn, or even fail to compile with any
selected behavior. However the individual testcase targets, like make
check-AccountBasics, don't redirect so if you want to look at debug output you
can try those out.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.