D13034: Add mechanism to notify other clients of config changes over DBus

[David Edmundson]

davidedmundson marked an inline comment as done.
davidedmundson added inline comments.

INLINE COMMENTS

> dfaure wrote in kconfigtest.cpp:1800
> 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.

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.

> dfaure wrote in kconfig.cpp: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 ;)

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.

> dfaure wrote in kconfigbase.h:62
> Let's go for 51 :)

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.

> dfaure wrote in kconfigwatcher.cpp:20
> (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)

I'm returning QSharedPointers to the caller.

REPOSITORY
  R237 KConfig

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

To: davidedmundson, broulik, dfaure
Cc: dfaure, broulik, zzag, kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180913/73f69eaf/attachment.html>