Add a logging category for logs warnings
ClosedPublic

Authored by meven on Jun 23 2019, 10:28 PM.

Details

Summary

Disable logs unless the logging category kf5.kconfig.core is enabled

Test Plan

ctest

Diff Detail

Repository
R237 KConfig
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Jun 23 2019, 10:28 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 23 2019, 10:28 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Jun 23 2019, 10:28 PM
apol added a subscriber: apol.Jun 23 2019, 10:35 PM
apol added inline comments.
src/core/kdesktopfile.h
27 ↗(On Diff #60545)

Use ecm_qt_declare_logging_category from cmake.

This should never be in header files too, or it would spill on applications otherwise.

See for example how it's done in this same repo on KCONF_UPDATE_LOG.

meven updated this revision to Diff 60546.EditedJun 23 2019, 11:07 PM

Use ecm_qt_declare_logging_category to setup logging

meven updated this revision to Diff 60547.Jun 23 2019, 11:08 PM

Removed blank line

Thank you very much again @apol for your guidance.

I am not sure my naming scheme is ideal.

apol added inline comments.Jun 24 2019, 9:01 AM
src/core/CMakeLists.txt
19

This is for KF5ConfigCore, I'd call it kf5.kconfig.core.

meven updated this revision to Diff 60558.Jun 24 2019, 9:09 AM

Use KCONF_CORE_LOG as identifier and kf5.kconfig.core as category_name

meven edited the summary of this revision. (Show Details)Jun 24 2019, 9:10 AM
meven marked an inline comment as done.
apol added a comment.Jun 24 2019, 9:20 AM

The patch looks good to me.

I see that you're just doing KDesktopFile, but there's many others available, can you port the rest too?

meven updated this revision to Diff 60610.Jun 24 2019, 6:10 PM

Add logging category for all kconfig core warnings message

This would be nice to have.

meven edited the test plan for this revision. (Show Details)Jul 17 2019, 10:00 AM
meven updated this revision to Diff 61897.Jul 17 2019, 10:08 AM

Add a logging categorie, add context to logs

meven added a reviewer: apol.Tue, Jul 30, 7:04 AM
cgiboudeaux added inline comments.
CMakeLists.txt
106

This line is not useful

src/core/CMakeLists.txt
17

the framework is called kconfig, the public headers are called kconfig*.h, there's no reason to truncate the framework name.

Also, why 'deskop' in the name ? this is for kconfigcore. kconfigcore_debug.h sounds better

meven updated this revision to Diff 62903.Thu, Aug 1, 3:14 PM

Rename kconf_desktop_debug.h to more appropriate kconfig_core_log_settings.h, do not run an unnecessary install

meven marked 2 inline comments as done.Thu, Aug 1, 3:15 PM

I have opted for the file name kconfig_core_log_settings.h for the log settings header file.

ok, please change the remaining kconf/KCONF to kconfig/KCONFIG and it should be fine.

meven updated this revision to Diff 63729.Wed, Aug 14, 2:43 PM

Renamed kconf to kconfig, KCONF to KCONFIG, re-add the mistakenly removed categories file

meven added a comment.Wed, Aug 14, 2:45 PM

ok, please change the remaining kconf/KCONF to kconfig/KCONFIG and it should be fine.

Should be good now.
Thank you for your feedback.

This revision is now accepted and ready to land.Wed, Aug 14, 3:12 PM
This revision was automatically updated to reflect the committed changes.