From ebbd0abc75dc8c1e9e5786e3e63d478233746dd9 Mon Sep 17 00:00:00 2001
From: Shane Synan <digitalcircuit36939@gmail.com>
Date: Wed, 22 Aug 2018 16:46:01 -0500
Subject: [PATCH] settings: Fix defaults caching, cache key exists

Add separate cache of whether or not a settings key is persisted to
disk, separate from the cache of settings key values.  Modify
localKeyExists() to make use of this cache, offering a potential
slight performance boost.

Fix caching of default values as actual settings values by always
checking if the key is persisted to disk, returning the default value
if not.

This fixes ChatMonitorSettings's "OperationMode" getting set to
"InvalidMode" by ChatMonitorFilter::_operationMode's reading of a
default "InvalidMode", then when loading settings,
ChatMonitorSettingsPage::settings["OperationMode"] would incorrectly
default to "InvalidMode" instead of "OptOut".

Test case:
1.  Start with a fresh configuration.
2.  Open Chat Monitor settings, skipping first-run wizard/etc
3.  Check that "Opt Out" is the default mode
4.  Via qDebug() prints, check that ChatMonitorFilter still sees the
    default value as InvalidMode
---
 src/common/settings.cpp | 28 +++++++++++++++++++++++-----
 src/common/settings.h   | 41 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/src/common/settings.cpp b/src/common/settings.cpp
index 6273f7c7..d742af16 100644
--- a/src/common/settings.cpp
+++ b/src/common/settings.cpp
@@ -30,6 +30,7 @@ const int VERSION = 1;              /// Settings version for backwords/forwards
 const int VERSION_MINOR_INITIAL = 1; /// Initial settings version for compatible changes
 
 QHash<QString, QVariant> Settings::settingsCache;
+QHash<QString, bool> Settings::settingsKeyPersistedCache;
 QHash<QString, SettingsChangeNotifier *> Settings::settingsChangeNotifier;
 
 #ifdef Q_OS_MAC
@@ -186,6 +187,7 @@ void Settings::setLocalValue(const QString &key, const QVariant &data)
     QString normKey = normalizedKey(group, key);
     create_qsettings;
     s.setValue(normKey, data);
+    setCacheKeyPersisted(normKey, true);
     setCacheValue(normKey, data);
     if (hasNotifier(normKey)) {
         emit notifier(normKey)->valueChanged(data);
@@ -198,19 +200,32 @@ const QVariant &Settings::localValue(const QString &key, const QVariant &def)
     QString normKey = normalizedKey(group, key);
     if (!isCached(normKey)) {
         create_qsettings;
+        // Since we're loading from settings anyways, cache whether or not the key exists on disk
+        setCacheKeyPersisted(normKey, s.contains(normKey));
+        // Cache key value
         setCacheValue(normKey, s.value(normKey, def));
     }
-    return cacheValue(normKey);
+    if (cacheKeyPersisted(normKey)) {
+        return cacheValue(normKey);
+    } else {
+        // Don't return possibly wrong cached values
+        // A key gets cached with the first default value requested and never changes afterwards
+        return def;
+    }
 }
 
 
 bool Settings::localKeyExists(const QString &key)
 {
     QString normKey = normalizedKey(group, key);
-    // Do NOT check the cache as default values get cached, too.  Otherwise loading a setting once
-    // will mark it as existing in settings, even when it only exists in cache (and not on disk).
-    create_qsettings;
-    return s.contains(normKey);
+    if (!isKeyPersistedCached(normKey)) {
+        create_qsettings;
+        // Cache whether or not key exists on disk
+        // We can't cache key value as we don't know the default
+        setCacheKeyPersisted(normKey, s.contains(normKey));
+    }
+
+    return cacheKeyPersisted(normKey);
 }
 
 
@@ -224,6 +239,9 @@ void Settings::removeLocalKey(const QString &key)
     if (isCached(normKey)) {
         settingsCache.remove(normKey);
     }
+    if (isKeyPersistedCached(normKey)) {
+        settingsKeyPersistedCache.remove(normKey);
+    }
     if (hasNotifier(normKey)) {
         emit notifier(normKey)->valueChanged({});
     }
diff --git a/src/common/settings.h b/src/common/settings.h
index 36576058..625291e8 100644
--- a/src/common/settings.h
+++ b/src/common/settings.h
@@ -141,7 +141,8 @@ private:
     }
 
 
-    static QHash<QString, QVariant> settingsCache;
+    static QHash<QString, QVariant> settingsCache;         ///< Cached settings values
+    static QHash<QString, bool> settingsKeyPersistedCache; ///< Cached settings key exists on disk
     static QHash<QString, SettingsChangeNotifier *> settingsChangeNotifier;
 
     inline QString normalizedKey(const QString &group, const QString &key)
@@ -152,6 +153,44 @@ private:
     }
 
 
+    /**
+     * Update the cache of whether or not a given settings key persists on disk
+     *
+     * @param normKey Normalized settings key ID
+     * @param exists  True if key exists, otherwise false
+     */
+    inline void setCacheKeyPersisted(const QString &normKey, bool exists)
+    {
+        settingsKeyPersistedCache[normKey] = exists;
+    }
+
+
+    /**
+     * Check if the given settings key ID persists on disk (rather than being a default value)
+     *
+     * @see Settings::localKeyExists()
+     *
+     * @param normKey Normalized settings key ID
+     * @return True if key exists and persistence has been cached, otherwise false
+     */
+    inline const bool &cacheKeyPersisted(const QString &normKey)
+    {
+        return settingsKeyPersistedCache[normKey];
+    }
+
+
+    /**
+     * Check if the persistence of the given settings key ID has been cached
+     *
+     * @param normKey Normalized settings key ID
+     * @return True if key persistence has been cached, otherwise false
+     */
+    inline bool isKeyPersistedCached(const QString &normKey)
+    {
+        return settingsKeyPersistedCache.contains(normKey);
+    }
+
+
     inline void setCacheValue(const QString &normKey, const QVariant &data)
     {
         settingsCache[normKey] = data;
-- 
2.20.1