D22143: Add proper logging using ECMQtDeclareLoggingCategory

cgiboudeaux added a comment. In D22143#526887 , @rikmills wrote: > This is installing BOTH /etc/xdg/kactivities-stats.categories AND/usr/share/qlogging-categories5/kactivities-stats.categories which is surely wrong? Indeed. As it

D22143: Add proper logging using ECMQtDeclareLoggingCategory

rikmills added a comment. This is installing BOTH /etc/xdg/kactivities-stats.categories AND/usr/share/qlogging-categories5/kactivities-stats.categories which is surely wrong? REPOSITORY R159 KActivities Statistics REVISION DETAIL https://phabricator.kde.org/D22143 To: meven, ivan,

D22143: Add proper logging using ECMQtDeclareLoggingCategory

This revision was automatically updated to reflect the committed changes. Closed by commit R159:2bb035ff67fd: Add proper logging using ECMQtDeclareLoggingCategory (authored by meven). REPOSITORY R159 KActivities Statistics CHANGES SINCE LAST UPDATE

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven updated this revision to Diff 64789. meven added a comment. Fix plural REPOSITORY R159 KActivities Statistics CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22143?vs=64787=64789 BRANCH arcpatch-D22143 REVISION DETAIL https://phabricator.kde.org/D22143 AFFECTED FILES

D22143: Add proper logging using ECMQtDeclareLoggingCategory

ivan added inline comments. INLINE COMMENTS > kactivities-stats.categories:1 > +org.kde.kactivities.stats KActivities Stats DEFAULT_SEVERITY [WARNING] > IDENTIFIER [KACTIVITY_STAT_LOG] Missed one here > CMakeLists.txt:21 > +HEADER

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven updated this revision to Diff 64787. meven added a comment. Rebase on master REPOSITORY R159 KActivities Statistics CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22143?vs=64784=64787 BRANCH arcpatch-D22143 REVISION DETAIL https://phabricator.kde.org/D22143 AFFECTED

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven updated this revision to Diff 64784. meven added a comment. kactivities-stat-logsettings.h -> kactivities-stats-logsettings.h, KACTIVITY_STAT_LOG -> KACTIVITIE_STATS_LOG REPOSITORY R159 KActivities Statistics CHANGES SINCE LAST UPDATE

D22143: Add proper logging using ECMQtDeclareLoggingCategory

ivan requested changes to this revision. ivan added a comment. This revision now requires changes to proceed. Change everything to plural. I agree with @kossebau, though I don't think it will lead to problems in future, so I could say it is bearable dirtyness. INLINE COMMENTS >

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven added a comment. @kossebau what about my last changes ? @ivan I feel this is ok to have `${CMAKE_BINARY_DIR}/src/kactivities-stat-logsettings.cpp` in the test CMakeLists.txt file with the documentation associated with ECMQtDeclareLoggingCategory and my comment, no-one can miss

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven added a comment. In D22143#502476 , @meven wrote: > In D22143#502336 , @kossebau wrote: > > > In D22143#502314 , @meven wrote: > > > > > >

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven updated this revision to Diff 62695. meven added a comment. Add comment regarding the origin of a generated cpp file REPOSITORY R159 KActivities Statistics CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22143?vs=62526=62695 BRANCH arcpatch-D22143 REVISION DETAIL

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven added a comment. In D22143#502336 , @kossebau wrote: > In D22143#502314 , @meven wrote: > > > > Relying on undocumented names of generated sources files does not get my +1. That needs

D22143: Add proper logging using ECMQtDeclareLoggingCategory

kossebau added a comment. In D22143#502314 , @meven wrote: > > Relying on undocumented names of generated sources files does not get my +1. That needs someone else to take responsibility :) > > Well this kind of file generation is common

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven added a comment. > Relying on undocumented names of generated sources files does not get my +1. That needs someone else to take responsibility :) Well this kind of file generation is common and is indirectly documented through the ecm_qt_declare_logging_category macro. I

D22143: Add proper logging using ECMQtDeclareLoggingCategory

kossebau added a comment. Nope, current solution does not satisfy my personal standards, sorry :) Relying on undocumented names of generated sources files does not get my +1. That needs someone else to take responsibility :) If transfering the name via variables s not possible, I would

D22143: Add proper logging using ECMQtDeclareLoggingCategory

ivan added a reviewer: kossebau. REPOSITORY R159 KActivities Statistics REVISION DETAIL https://phabricator.kde.org/D22143 To: meven, ivan, #frameworks, kossebau Cc: kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

ivan added a comment. If @kossebau is satisfied, go for it REPOSITORY R159 KActivities Statistics REVISION DETAIL https://phabricator.kde.org/D22143 To: meven, ivan, #frameworks Cc: kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven updated this revision to Diff 62526. meven marked 5 inline comments as done. meven added a comment. Better category name REPOSITORY R159 KActivities Statistics CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22143?vs=60786=62526 BRANCH arcpatch-D22143 REVISION DETAIL

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven added a comment. @ivan What are you thoughts about this ? REPOSITORY R159 KActivities Statistics REVISION DETAIL https://phabricator.kde.org/D22143 To: meven, ivan, #frameworks Cc: kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven added inline comments. INLINE COMMENTS > meven wrote in CMakeLists.txt:21 > While it is true, the solution here is not ideal, it is quite commonly used > in kde projects and anyone opening the CMakeLists.txt should come up pretty > quickly that this file is being generated elsewhere. > I

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven added inline comments. INLINE COMMENTS > kossebau wrote in CMakeLists.txt:21 > Instead of relying on an undocumented cpp file name generated by > ecm_qt_declare_logging_category, you rather want to have a separate SRC > variable which carries the source files generated by the macro, and

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven added inline comments. INLINE COMMENTS > kossebau wrote in CMakeLists.txt:19 > As we want to reuse the source files also with the tests, better store in a > separate var here, e.g. named `KActivitiesStats_LOG_SRCS`. > And add this manually to KActivitiesStats_LIB_SRCS and for the test as

D22143: Add proper logging using ECMQtDeclareLoggingCategory

kossebau added inline comments. INLINE COMMENTS > CMakeLists.txt:21 > > + ${CMAKE_BINARY_DIR}/src/kactivities-stat-logsettings.cpp > + Instead of relying on an undocumented cpp file name generated by ecm_qt_declare_logging_category, you rather want to have a separate SRC variable which

D22143: Add proper logging using ECMQtDeclareLoggingCategory

meven created this revision. meven added reviewers: ivan, Frameworks. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. meven requested review of this revision. TEST PLAN Build kdeinit5 kdebugsettings REPOSITORY R159 KActivities Statistics BRANCH