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

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kconfigtest.cpp:1793
> +#if !KCONFIG_USE_DBUS
> +        QEXPECT_FAIL("", "KConfig notification requires DBus", QTest::Continue)
> +        Q_ASSERT(false);

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).

> kconfigtest.cpp:1800
> +
> +    //mimcs a config in another process, which is watching for events
> +    auto remoteConfig = KSharedConfig::openConfig(TEST_SUBDIR "kconfigtest");

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.

> kconfig.cpp:460
> +                    if (e.bNotify) {
> +                        notifyGroupsGlobal[QString::fromLatin1(it.key().mGroup)] << it.key().mKey;
> +                    }

Why latin1? Can't groups use utf8?

> kconfig.cpp:465
> +                    if (e.bNotify) {
> +                        notifyGroupsLocal[QString::fromLatin1(it.key().mGroup)] << it.key().mKey;
> +                    }

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 ;)

> kconfigbase.h:61
> +        /**<
> +         * Notify remote KConfigWatchers of changes (Linux only)
> +         * @since 5.CONFIGWATCHERVERSION

I see no reason why this wouldn't work on BSD :-)

I would change this to "(requires DBus support)".

> kconfigbase.h:62
> +         * Notify remote KConfigWatchers of changes (Linux only)
> +         * @since 5.CONFIGWATCHERVERSION
> +         */

Let's go for 51 :)

> kconfigdata.cpp:304
> +        case EntryNotify:
> +            it->bNotify = bf;
>          default:

missing break;

> kconfigwatcher.cpp:1
> +#include "kconfigwatcher.h"
> +

License header missing

> kconfigwatcher.cpp:20
> +
> +typedef QHash<KSharedConfig*, QWeakPointer<KConfigWatcher>> ConfigWatcherMap;
> +Q_GLOBAL_STATIC(ConfigWatcherMap, s_globalWatcherList)

(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)

> kconfigwatcher.cpp:21
> +typedef QHash<KSharedConfig*, QWeakPointer<KConfigWatcher>> ConfigWatcherMap;
> +Q_GLOBAL_STATIC(ConfigWatcherMap, s_globalWatcherList)
> +

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.

> kconfigwatcher.cpp:28
> +
> +    if (!s_globalWatcherList->contains(c)) {
> +        watcher = KConfigWatcher::Ptr(new KConfigWatcher(config));

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.

> kconfigwatcher.h:34
> + * Notifies when another client has updated this config file with the Notify flag set.
> + * @since 5.CONFIGWATCHERVERSION
> + */

51

> kconfigwatcher.h:60
> +private Q_SLOTS:
> +    void onConfigChangeNotification(QHash<QString, QByteArrayList> changes);
> +

const ref

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/20180908/b6b56e65/attachment-0001.html>