D7380: KCD: use modern logging classes throughout
dfaure added inline comments. INLINE COMMENTS > dfaure wrote in testkcd.cpp:64 > There's proper API for setting applications defaults, without the need for > env vars. My comment is still valid: why doesn't this use QLoggingCategory::setFilterRules() instead? > rjvbb wrote in testkcd.cpp:69 > And ditto: why would you write an application that's supposed to print test > results and NOT enable the printing by default? > (I presume that tests like this would use `QPrint()` if it existed) In my view, qInfo is exactly meant for this use case, the one you call QPrint(). REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: dfaure, ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb set the repository for this revision to R349 KCompactDisc Library. REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: dfaure, ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb updated this revision to Diff 45221. rjvbb added a comment. refactored for master/head CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7380?vs=18470&id=45221 REVISION DETAIL https://phabricator.kde.org/D7380 AFFECTED FILES CMakeLists.txt src/CMakeLists.txt src/kcompactdisc.cpp src/kcompactdisc_p.cpp src/kcompactdisc_p.h src/phonon_interface.cpp src/wmlib_interface.cpp tests/testkcd.cpp To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: dfaure, ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
dfaure added a comment. It is rather hard to follow your reasoning... 1. QtInfoMsg is *between* QtDebugMsg and QtWarningMsg, so it's *not* more verbose than QtDebugMsg, it's less. This is why our default setup (in the ECM macro) is to enable "info and up" (i.e. info, warning, critical) and not debug. 2. And QLoggingCategory::setFilterRules() has *nothing* to do with QSettings... Yes QSettings is persistent, but QLoggingCategory::setFilterRules() definitely isn't. You're right about the first point though, qDebug() is associated to the "default" category of qCDebug(), so it's affected by *.debug=false, I hadn't realized that. Surely it must be the same for qInfo() then -- except that *.info=false would be a very stupid thing to do anyway, IMHO. One *wants* the output from command-line tools. REPOSITORY R342 KIO AudioCD REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: dfaure, ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb added a comment. > BTW it is *not* curious that isn't qInfo isn't "disabled by *.info=false in qtlogging.ini", it's rather expected since qInfo() is one layer below categorized logging, just like qDebug and qWarning. qtlogging.ini (and the QT_LOGGING_RULES env vars) are about qC*. I've read that before, yet I notice that qDebug() statements don't print for me if I have *.debug=false in my qtlogging.ini file. Whatever the true reason, I've taken to using qWarning() in my own tests before learning about qInfo() (I have *.warning=true set in qtlogging.ini). Regardless of that, I meant that AFAIU one typically enables info level messages when even more verbose output than debug output is required. The qC* categories seem to reflect that, IMHO. > There's proper API for setting applications defaults, without the need for env vars. I only see QLoggingCategory::setFilterRules(), is that what you're referring to? I don't think it'd be proper to use QSettings for a test application, leaving a persistent resource somewhere. REPOSITORY R342 KIO AudioCD REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: dfaure, ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
dfaure added a comment.
For information, qCInfo() is enabled by default if you use the ECM macro for
declaring the logging category, or if you pass QtInfoMsg to the Qt macro. But
then again, it seems fine with me to use qInfo() in a test program, no need for
categorized output for this.
BTW it is *not* curious that isn't qInfo isn't "disabled by *.info=false in
qtlogging.ini", it's rather expected since qInfo() is one layer below
categorized logging, just like qDebug and qWarning. qtlogging.ini (and the
QT_LOGGING_RULES env vars) are about qC*.
INLINE COMMENTS
> testkcd.cpp:64
> +if (!qEnvironmentVariableIsSet("QT_LOGGING_RULES")) {
> +qputenv("QT_LOGGING_RULES",
> QByteArrayLiteral("org.kde.libkcompactdisc=true"));
> +}
There's proper API for setting applications defaults, without the need for env
vars.
REPOSITORY
R342 KIO AudioCD
REVISION DETAIL
https://phabricator.kde.org/D7380
To: rjvbb, #frameworks, davidedmundson, ltoscano
Cc: dfaure, ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb set the repository for this revision to R342 KIO AudioCD. REPOSITORY R342 KIO AudioCD REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: dfaure, ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb updated this revision to Diff 18470. rjvbb added a comment. Using qInfo() in testkcd. Curious that qInfo() is enabled (and isn't disabled by `*.info=false` in qtlogging.ini) by default but qCInfo() isn't (as you'd expect for "info level" logging). CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7380?vs=18344&id=18470 REVISION DETAIL https://phabricator.kde.org/D7380 AFFECTED FILES CMakeLists.txt src/CMakeLists.txt src/kcompactdisc.cpp src/kcompactdisc_p.cpp src/kcompactdisc_p.h src/phonon_interface.cpp src/wmlib_interface.cpp tests/testkcd.cpp To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: dfaure, ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
dfaure added a comment. Information printed by command-line programs can be done with qInfo(), which is enabled by default, while qDebug() should be disabled by defualt. REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: dfaure, ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb set the repository for this revision to R349 KCompactDisc Library. REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb updated this revision to Diff 18344. rjvbb added a comment. Updated as discussed. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7380?vs=18325&id=18344 REVISION DETAIL https://phabricator.kde.org/D7380 AFFECTED FILES CMakeLists.txt src/CMakeLists.txt src/kcompactdisc.cpp src/kcompactdisc_p.cpp src/kcompactdisc_p.h src/phonon_interface.cpp src/wmlib_interface.cpp tests/testkcd.cpp To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb added a comment. Simply because there's no quick and convenient other way for doing output in Qt, I think. But even then, if QPrint existed you'd be using it in testkcd but most likely not in the library itself and I'd still propose to activate debug output from the library in testkcd. FWIW, the autotests I've run by hand all seem to generate output that looks suspiciously as if it's generated by the logging classes, and they don't require setting a rule. REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
ltoscano added inline comments. INLINE COMMENTS > rjvbb wrote in testkcd.cpp:66 > I know it can, and I'd agree for regular applications. This is a test > application, its users should not have to go figure out why it produces no > output and how to make it print something. > > You're right though that QT_LOGGING_RULES can already have been set, I can > avoid changing it. If the output is mandatory, why use a category rule at all, or even logging? (and I know this is a pre-existing problem, but still) REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb added a comment. Some thoughts, I'll update the patch later today. INLINE COMMENTS > ltoscano wrote in kcompactdisc_p.cpp:30 > On the other side, this is not a frameworks, but a generic library. I've seen > (and pushed) org.kde.[.] for applications. What could > it be a proper choice here? org.kde.lib.[.]? Or simply > org.kde.[.]? I see KCalCore does `ecm_qt_declare_logging_category(kcalcore_LIB_SRCS HEADER kcalcore_debug.h IDENTIFIER KCALCORE_LOG CATEGORY_NAME org.kde.pim.kcalcore)`. Note the absence of a `lib` namespace/whatever. We could thus use `org.kde.kcompactdisc` unless there's already a suitable namespace to which it could be added (multimedia, audio, ...). > ltoscano wrote in testkcd.cpp:66 > Please don't set this here. It can be controlled at runtime. I know it can, and I'd agree for regular applications. This is a test application, its users should not have to go figure out why it produces no output and how to make it print something. You're right though that QT_LOGGING_RULES can already have been set, I can avoid changing it. > ltoscano wrote in testkcd.cpp:69 > ditto And ditto: why would you write an application that's supposed to print test results and NOT enable the printing by default? (I presume that tests like this would use `QPrint()` if it existed) REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
ltoscano added inline comments. INLINE COMMENTS > davidedmundson wrote in kcompactdisc_p.cpp:30 > there's a trend to use "kf5.frameworkname.subpart" as the category. On the other side, this is not a frameworks, but a generic library. I've seen (and pushed) org.kde.[.] for applications. What could it be a proper choice here? org.kde.lib.[.]? Or simply org.kde.[.]? REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson, ltoscano Cc: ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
davidedmundson accepted this revision. davidedmundson added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > kcompactdisc_p.cpp:30 > > +Q_LOGGING_CATEGORY(KCD, "cd") > Q_LOGGING_CATEGORY(CD_PLAYLIST, "cd.playlist") there's a trend to use "kf5.frameworkname.subpart" as the category. REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks, davidedmundson Cc: davidedmundson
D7380: KCD: use modern logging classes throughout
ltoscano requested changes to this revision.
ltoscano added a comment.
This revision now requires changes to proceed.
I strongly suggest to port to ecm_qt_declare_logging_category for both the
old and the new category. This script can automated the process:
https://cgit.kde.org/kde-dev-scripts.git/tree/kf5/convert-qkdebug-to-qcdebug.sh
INLINE COMMENTS
> testkcd.cpp:66
> {
> +qputenv("QT_LOGGING_RULES", QByteArrayLiteral("cd=true"));
> QCoreApplication app(argc, argv);
Please don't set this here. It can be controlled at runtime.
> testkcd.cpp:69
>
> -qDebug() << "Testing libKF5CompactDisc";
> +tKCD.setEnabled(QtDebugMsg, true);
> +qCDebug(tKCD) << "Testing libKF5CompactDisc";
ditto
REPOSITORY
R349 KCompactDisc Library
REVISION DETAIL
https://phabricator.kde.org/D7380
To: rjvbb, #frameworks, davidedmundson, ltoscano
Cc: ltoscano, davidedmundson
D7380: KCD: use modern logging classes throughout
rjvbb set the repository for this revision to R349 KCompactDisc Library. REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 To: rjvbb, #frameworks
D7380: KCD: use modern logging classes throughout
rjvbb updated this revision to Diff 18325. rjvbb added a comment. missed 2 instances CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7380?vs=18323&id=18325 REVISION DETAIL https://phabricator.kde.org/D7380 AFFECTED FILES src/kcompactdisc.cpp src/kcompactdisc_p.cpp src/kcompactdisc_p.h src/phonon_interface.cpp src/wmlib_interface.cpp tests/testkcd.cpp To: rjvbb, #frameworks
D7380: KCD: use modern logging classes throughout
rjvbb created this revision. REVISION SUMMARY This change migrates libkcompactdisc to using modern logging for debugging purposes. It introduces a `cd` base category since a `cd.playlist` category existed already. An additional `cd.test` category is introduced in the testkcd utility, which also enables logging on all `cd` categories. TEST PLAN Works as expected. It appears to be necessary to enable debug output on the `cd.test` category explicitly, despite setting the QT_LOGGING_RULES env. var, probably because the `tKCD` instance is created before main() and thus qputenv() are called. I can probably avoid that by using the approach used in the `Q_LOGGING_CATEGORY` macro but I'm not certain that's worth the trouble? REPOSITORY R349 KCompactDisc Library REVISION DETAIL https://phabricator.kde.org/D7380 AFFECTED FILES src/kcompactdisc.cpp src/kcompactdisc_p.cpp src/kcompactdisc_p.h src/phonon_interface.cpp src/wmlib_interface.cpp tests/testkcd.cpp To: rjvbb, #frameworks
