D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Christophe Giboudeaux]

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 depends on ECM >= 5.61, it has no reason to keep on installing 
it in KDE_INSTALL_CONFDIR

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks, kossebau
Cc: cgiboudeaux, rikmills, kossebau, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Rik Mills]

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, #frameworks, kossebau
Cc: rikmills, kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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
  https://phabricator.kde.org/D22143?vs=64789=64808

REVISION DETAIL
  https://phabricator.kde.org/D22143

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  kactivities-stats.categories
  src/CMakeLists.txt
  src/common/database/Database.cpp
  src/resultmodel.cpp
  src/resultset.cpp
  src/resultwatcher.cpp

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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
  CMakeLists.txt
  autotests/CMakeLists.txt
  kactivities-stats.categories
  src/CMakeLists.txt
  src/common/database/Database.cpp
  src/resultmodel.cpp
  src/resultset.cpp
  src/resultwatcher.cpp

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Ivan Čukić]

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 kactivities-stats-logsettings.h
> +IDENTIFIER KACTIVITIE_STATS_LOG
> +CATEGORY_NAME kf5.kactivity.stat)

KACTIVITIE -> KACTIVITIE**S **

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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 FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  kactivities-stats.categories
  src/CMakeLists.txt
  src/common/database/Database.cpp
  src/resultmodel.cpp
  src/resultset.cpp
  src/resultwatcher.cpp

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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
  https://phabricator.kde.org/D22143?vs=62695=64784

BRANCH
  arcpatch-D22143

REVISION DETAIL
  https://phabricator.kde.org/D22143

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  kactivities-stats.categories
  src/CMakeLists.txt
  src/common/database/Database.cpp
  src/resultmodel.cpp
  src/resultset.cpp
  src/resultwatcher.cpp

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Ivan Čukić]

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

> CMakeLists.txt:22
> +   # Generated by macro ecm_qt_declare_logging_category in src/CMakeLists.txt
> +   ${CMAKE_BINARY_DIR}/src/kactivities-stat-logsettings.cpp
> +

plural `stats`

> kactivities-stats.categories:1
> +org.kde.kactivities.stats KActivities Stats DEFAULT_SEVERITY [WARNING] 
> IDENTIFIER [KACTIVITY_STAT_LOG]

You mixed singulars and plurals - go for plurals everywhere:

`s/KACTIVITY_STAT_LOG/KACTIVITIES_STATS_LOG/g`

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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 where this file is 
coming from.

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

meven added a comment.


  In D22143#502476 , @meven wrote:
  
  > 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 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.
  > >
  > >
  > > The current implementation of the macro is an internal detail. It is not 
part of the API contract.
  >
  >
  > I am quoting the macro documentation :
  >
  > > A header file, , will be generated along with a corresponding 
source file, ...
  >
  > https://api.kde.org/ecm/module/ECMQtDeclareLoggingCategory.html
  >
  > So here, It seems to me this generated cpp file is part of the macro 
documented behavior.
  
  
  With this and my added comment on line 21 of autotests/CMakeLists.txt 
  What do you think @kossebau ?

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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
  https://phabricator.kde.org/D22143

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  kactivities-stats.categories
  src/CMakeLists.txt
  src/common/database/Database.cpp
  src/resultmodel.cpp
  src/resultset.cpp
  src/resultwatcher.cpp

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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 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.
  >
  >
  > The current implementation of the macro is an internal detail. It is not 
part of the API contract.
  
  
  I am quoting the macro documentation :
  
  > A header file, , will be generated along with a corresponding 
source file, ...
  
  https://api.kde.org/ecm/module/ECMQtDeclareLoggingCategory.html
  
  So here, It seems to me this generated cpp file is part of the macro 
documented behavior.
  
  > Binding one's code to current internal implementation has two 
disadvantages: it makes it harder for the macro developers to enhance the 
macro, because they would break your code, Or they do not know someone is 
relying on internal details, and your code breaks one day.
  
  Fair point, in general, not relying on internal details
  
  > I just tried to give one in my previous comment: regenerating the 
respective logging source files in the autotest dir again, by calling the macro 
there once more.
  
  I might do just do that, thanks.

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

[Friedrich W. H. Kossebau]

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 and is indirectly documented 
through the ecm_qt_declare_logging_category macro.
  
  
  The current implementation of the macro is an internal detail. It is not part 
of the API contract.
  Binding one's code to current internal implementation has two disadvantages: 
it makes it harder for the macro developers to enhance the macro, because they 
would break your code, Or they do not know someone is relying on internal 
details, and your code breaks one day.
  
  > I don't think this is an important issue.
  > 
  > For what it is worth, the same technique is used in other repos :
  
  IMHO all cases which are bad code :)
  
  > This does not justify it but makes the points that other KDE devs did not 
see an issue with it.
  
  All us KDE devs have different backgrounds, so not surprising there are 
different opinions :)
  
  > I would gladly use an alternative if presented with one.
  
  I just tried to give one in my previous comment: regenerating the respective 
logging source files in the autotest dir again, by calling the macro there once 
more.
  Or extend the documentation of the macro, to specify the cpp name. Then its 
part of the API contract officially :)

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

[Méven Car]

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 don't think this is an important issue.
  
  For what it is worth, the same technique is used in other repos :
  
  kdebugsettings/autotests/CMakeLists.txt
  22:${CMAKE_BINARY_DIR}/src/kdebugsettings_debug.cpp
  
  plasma-thunderbolt/autotests/kded/CMakeLists.txt
  5:${CMAKE_BINARY_DIR}/src/kded
  11:${CMAKE_BINARY_DIR}/src/kded/kded_bolt_debug.cpp
  
  pulseaudio-qt/autotests/CMakeLists.txt
  8:${CMAKE_BINARY_DIR}/src/debug.cpp
  
  This does not justify it but makes the points that other KDE devs did not see 
an issue with it.
  
  I would gladly use an alternative if presented with one.

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

[Friedrich W. H. Kossebau]

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 go for 
regenerating the respective logging source files in the autotest dir then again 
instead.

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 Čukić]

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 Čukić]

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

[Méven Car]

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
  https://phabricator.kde.org/D22143

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  kactivities-stats.categories
  src/CMakeLists.txt
  src/common/database/Database.cpp
  src/resultmodel.cpp
  src/resultset.cpp
  src/resultwatcher.cpp

To: meven, ivan, #frameworks
Cc: kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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

[Méven Car]

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 just lack a viable alternative. I had a hard time to uncover this solution 
> in the first place.

ping @kossebau

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

[Méven Car]

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 add its 
> content here by instead adding this to the list:
> 
>   ${KActivitiesStats_LOG_SRCS}
> 
> That variable will also make it easier to track where this file is actually 
> coming from, instead having to guess it.

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 just lack a viable alternative. I had a hard time to uncover this solution in 
the first place.

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks
Cc: kossebau, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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 
> commented above:
> 
>   ecm_qt_declare_logging_category(KActivitiesStats_LOG_SRCS
>   HEADER kactivities-stat-logsettings.h
>   IDENTIFIER KACTIVITY_STAT_LOG
>   CATEGORY_NAME kf5.kactivity.stat)
>   list(APPEND KActivitiesStats_LIB_SRCS ${KActivitiesStats_LOG_SRCS})

Do you have a suggestion how to share the variable between the two CMake files ?
I tried moving the ecm_qt_declare_logging_category to the top level CMake but 
then I don't know how to share them with the children dirs.
I read about PARENT_SCOPE but the problem still stands scope-wise.
Or should I convert the add_subdirectory to include ones ? This would need 
other changes.

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks
Cc: kossebau, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Friedrich W. H. Kossebau]

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 carries the source files generated by the macro, and add its 
content here by instead adding this to the list:

  ${KActivitiesStats_LOG_SRCS}

That variable will also make it easier to track where this file is actually 
coming from, instead having to guess it.

> CMakeLists.txt:19
>  
> +ecm_qt_declare_logging_category(KActivitiesStats_LIB_SRCS
> +HEADER kactivities-stat-logsettings.h

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 
commented above:

  ecm_qt_declare_logging_category(KActivitiesStats_LOG_SRCS
  HEADER kactivities-stat-logsettings.h
  IDENTIFIER KACTIVITY_STAT_LOG
  CATEGORY_NAME kf5.kactivity.stat)
  list(APPEND KActivitiesStats_LIB_SRCS ${KActivitiesStats_LOG_SRCS})

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks
Cc: kossebau, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

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
  add-proper-logging

REVISION DETAIL
  https://phabricator.kde.org/D22143

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  kactivities-stats.categories
  src/CMakeLists.txt
  src/common/database/Database.cpp
  src/resultmodel.cpp
  src/resultset.cpp
  src/resultwatcher.cpp

To: meven, ivan, #frameworks
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns