Add mechanism to notify other clients of config changes over DBus
ClosedPublic
Actions

Authored by davidedmundson on May 22 2018, 9:14 AM.

Details

Reviewers

broulik
dfaure

Commits

R237:8579ec54838b: Add mechanism to notify other clients of config changes over DBus

Summary

Intention is not to create a registry like system, but to replace
KDElibs4Support::KGlobalSettings and to replace other system settingss
-> some app communication in a more generic way.

writeEntry gains an additional flag Notify which if set, will notify
clients of what has actually changed when we sync.

Rationale to put this into KConfig was so that we could have everything
batched and sychronised to the file sync and to get the fine detailed
exposure of what has actually changed which we don't get with a file
watcher.

Default behaviour remains identical without any broadcast messages.

As it is a new dependency it is purely optional and anything referencing
DBus is not in the public API. Our deployment on platforms without DBus
tend to be standalone applications anyway.

Test Plan

Attached unit test

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.

davidedmundson created this revision.May 22 2018, 9:14 AM

Restricted Application added a project: Frameworks. · View Herald TranscriptMay 22 2018, 9:14 AM

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript

davidedmundson requested review of this revision.May 22 2018, 9:14 AM

zzag added a subscriber: zzag.May 22 2018, 1:41 PM

zzag added inline comments.

src/core/kconfig.cpp
526	#else Q_UNUSED(changes); Q_UNUSED(path); #endif
src/core/kconfigwatcher.cpp
83	Won't this detach?

Review comments

broulik accepted this revision.Jun 6 2018, 12:06 PM

broulik added a subscriber: broulik.

broulik added inline comments.

src/core/kconfigwatcher.cpp
83	no, `parts` is `const`

This revision is now accepted and ready to land.Jun 6 2018, 12:06 PM

davidedmundson marked 2 inline comments as done.Jun 6 2018, 12:09 PM

davidedmundson added inline comments.

src/core/kconfigwatcher.cpp
83	zzag was right then I changed it, but I hadn't clicked the "done" checkbox after fixing.

zzag added inline comments.Jun 6 2018, 3:23 PM

src/core/kconfigwatcher.cpp
62	I think you forgot qAsConst. ;-) for (const QString &path : qAsConst(watchedPaths)) {

davidedmundson added dependencies: D13033: Expose getter method for KConfig::addConfigSources, D13032: Fix cleanup of kconfigtest kdeglobals.Jun 7 2018, 12:24 PM

@dfaure are you ok with this?

dfaure requested changes to this revision.Sep 8 2018, 8:58 AM

dfaure added inline comments.

autotests/kconfigtest.cpp
1798	This is normally done using QSKIP. Better than this, which will "fail to fail" in release mode (at least on the Q_ASSERT(false) line).
1805	typo: mimics The fact that it's not actually another process, means that this test would pass with a simple emit as well ;) A proper test would use QProcess to run a helper executable which would make changes to a config file.
src/core/kconfig.cpp
460	Why latin1? Can't groups use utf8?
465	I wonder if we should skip doing this work when DBUS support is disabled, for performance reasons. More #if, but more speed... in the corner-case scenario though. So I'm not sure ;)
src/core/kconfigbase.h
61	I see no reason why this wouldn't work on BSD :-) I would change this to "(requires DBus support)".
62	Let's go for 51 :)
src/core/kconfigdata.cpp
304	missing break;
src/core/kconfigwatcher.cpp
2	License header missing
21	(I'm curious, why not a raw pointer? You clean up the hash when the watcher is destroyed anyway; probably doesn't matter either way though)
22	You could also do this the C++11 way with a static object inside the create() method, which is the only place where it's used.
29	There should probably be a mutex around this whole method implementation, in case two threads create a watcher for the same KSharedConfig (which is itself threadsafe). Alternatively, the static can be a QThreadStorage, like I did in ksharedconfig.cpp.
src/core/kconfigwatcher.h
35	51
61	const ref

This revision now requires changes to proceed.Sep 8 2018, 8:58 AM

Review comments

Harbormaster completed remote builds in B2806: Diff 41547.Sep 13 2018, 1:21 PM

davidedmundson marked an inline comment as done.Sep 13 2018, 1:23 PM

davidedmundson added inline comments.

autotests/kconfigtest.cpp
1805	Fixed the typo Any test would still pass if you had explicit code that cheated to make tests pass! I want to add "--notify" support in kwriteconfig5. That currently lacks any tests, I can do that which would cover proving that this is sending it properly, whilst keeping this more advanced test still readable.
src/core/kconfig.cpp
465	We still have the loop regardless. It doesn't make sense for apps to use notify if they're running in a standalone environment. So practically I think we'd only save an if statement on a flag.
src/core/kconfigbase.h
62	Sure, I'll change this when I merge. Typing @since like this is the only strategy that hasn't caused headcaches with the fastpaced frameworks cycle. It's easier to grep for than a number.
src/core/kconfigwatcher.cpp
21	I'm returning QSharedPointers to the caller.

dfaure accepted this revision.Sep 14 2018, 7:35 AM

dfaure added inline comments.

src/core/kconfigwatcher.cpp
73	const QString & and additionalConfigSources should be put into a local const var first, to avoid a detach.

This revision is now accepted and ready to land.Sep 14 2018, 7:35 AM

Closed by commit R237:8579ec54838b: Add mechanism to notify other clients of config changes over DBus (authored by davidedmundson). · Explain WhyOct 10 2018, 1:52 PM

This revision was automatically updated to reflect the committed changes.

davidedmundson marked an inline comment as done.

This change broke the build of KConfig on Qt 5.9 (casting issue in QTest)

Thanks, resolved.

thomasfischer mentioned this in D19320: Assign documentation to correct enum value (corrects commit 8579ec54).Feb 25 2019, 8:52 PM

thomasfischer mentioned this in R237:3bfe329b7136: KConfig: Assign documentation to correct enum value.Feb 25 2019, 9:23 PM

Revision Contents
Changeset List

		Path
M		CMakeLists.txt (7 lines)
M		autotests/kconfigtest.h (1 line)
M		autotests/kconfigtest.cpp (79 lines)
M		src/core/CMakeLists.txt (10 lines)
A	M	src/core/config-kconfig.h.cmake (1 line)
M		src/core/kconfig.cpp (55 lines)
M		src/core/kconfig_p.h (2 lines)
M		src/core/kconfigbase.h (14 lines)
M		src/core/kconfigdata.h (7 lines)
M		src/core/kconfigdata.cpp (7 lines)
A	M	src/core/kconfigwatcher.h (68 lines)
A	M	src/core/kconfigwatcher.cpp (107 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	34629	2b2891f		May 22 2018, 9:14 AM	★	★
Diff 2	35212	2b2891f	Review comments	May 30 2018, 6:50 PM	★	★
Diff 3	41547	78aa013	Review comments	Sep 13 2018, 1:21 PM	★	★
Diff 4	43304	8e56083	R237:8579ec54838b7188ed016f7adb4a69bbf2e39712	Oct 10 2018, 1:51 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
f1ee644f06f6	60fe4f152c67	78aa01326a80	David Edmundson	Add mechanism to notify other clients of config changes over DBus (Show More…)	Sep 10 2018, 3:30 PM

Status	Author	Revision
Closed	davidedmundson	D13034 Add mechanism to notify other clients of config changes over DBus
Closed	davidedmundson	D13033 Expose getter method for KConfig::addConfigSources
Closed	davidedmundson	D13032 Fix cleanup of kconfigtest kdeglobals