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
