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
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
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=45221
REVISION DETAIL
https://phabricator.kde.org/D7380
AFFECTED FILES
CMakeLists.txt
src/CMakeLists.txt
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)
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*.
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.
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
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
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,
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
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=18344
REVISION DETAIL
https://phabricator.kde.org/D7380
AFFECTED FILES
CMakeLists.txt
src/CMakeLists.txt
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
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
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
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
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
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:
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
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=18325
REVISION DETAIL
https://phabricator.kde.org/D7380
AFFECTED FILES
src/kcompactdisc.cpp
src/kcompactdisc_p.cpp
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,
20 matches
Mail list logo
