Merge pull request #236 from daleglass-overte/optimize_settings

Optimize settings system

interface/src/Application.cpp

@@ -1933,29 +1933,14 @@ Application::Application(
 
     // Make sure we don't time out during slow operations at startup
     updateHeartbeat();
-    QTimer* settingsTimer = new QTimer();
-    moveToNewNamedThread(settingsTimer, "Settings Thread", [this, settingsTimer]{
-        // This needs to run on the settings thread, so we need to pass the `settingsTimer` as the
-        // receiver object, otherwise it will run on the application thread and trigger a warning
-        // about trying to kill the timer on the main thread.
-        connect(qApp, &Application::beforeAboutToQuit, settingsTimer, [this, settingsTimer]{
-            // Disconnect the signal from the save settings
-            QObject::disconnect(settingsTimer, &QTimer::timeout, this, &Application::saveSettings);
-            // Stop the settings timer
-            settingsTimer->stop();
-            // Delete it (this will trigger the thread destruction
-            settingsTimer->deleteLater();
-            // Mark the settings thread as finished, so we know we can safely save in the main application
-            // shutdown code
-            _settingsGuard.trigger();
-        });
 
-        int SAVE_SETTINGS_INTERVAL = 10 * MSECS_PER_SECOND; // Let's save every seconds for now
+
-        settingsTimer->setSingleShot(false);
+    QTimer* settingsTimer = new QTimer();
-        settingsTimer->setInterval(SAVE_SETTINGS_INTERVAL); // 10s, Qt::CoarseTimer acceptable
+    int SAVE_SETTINGS_INTERVAL = 10 * MSECS_PER_SECOND; // Let's save every seconds for now
-        QObject::connect(settingsTimer, &QTimer::timeout, this, &Application::saveSettings);
+    settingsTimer->setSingleShot(false);
-        settingsTimer->start();
+    settingsTimer->setInterval(SAVE_SETTINGS_INTERVAL); // 10s, Qt::CoarseTimer acceptable
-    }, QThread::LowestPriority);
+    QObject::connect(settingsTimer, &QTimer::timeout, this, &Application::saveSettings);
+    settingsTimer->start();
 
     if (Menu::getInstance()->isOptionChecked(MenuOption::FirstPersonLookAt)) {
         getMyAvatar()->setBoomLength(MyAvatar::ZOOM_MIN);  // So that camera doesn't auto-switch to third person.
@@ -2845,13 +2830,6 @@ void Application::cleanupBeforeQuit() {
     locationUpdateTimer.stop();
     identityPacketTimer.stop();
     pingTimer.stop();
-
-    // Wait for the settings thread to shut down, and save the settings one last time when it's safe
-    if (_settingsGuard.wait()) {
-        // save state
-        saveSettings();
-    }
-
     _window->saveGeometry();
 
     // Destroy third party processes after scripts have finished using them.

interface/src/Application.h

@@ -5,6 +5,7 @@
 //  Created by Andrzej Kapolka on 5/10/13.
 //  Copyright 2013 High Fidelity, Inc.
 //  Copyright 2020 Vircadia contributors.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
@@ -481,7 +482,7 @@ public slots:
     void setIsInterstitialMode(bool interstitialMode);
 
     void updateVerboseLogging();
-    
+
     void setCachebustRequire();
 
     void changeViewAsNeeded(float boomLength);
@@ -719,8 +720,6 @@ private:
 
     bool _notifiedPacketVersionMismatchThisDomain;
 
-    ConditionalGuard _settingsGuard;
-
     GLCanvas* _glWidget{ nullptr };
 
     typedef bool (Application::* AcceptURLMethod)(const QString &);

interface/src/scripting/SettingsScriptingInterface.cpp

@@ -4,6 +4,7 @@
 //
 //  Created by Brad Hefta-Gaub on 2/25/14.
 //  Copyright 2014 High Fidelity, Inc.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html

interface/src/scripting/SettingsScriptingInterface.h

@@ -4,6 +4,7 @@
 //
 //  Created by Brad Hefta-Gaub on 3/22/14.
 //  Copyright 2014 High Fidelity, Inc.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
@@ -37,7 +38,7 @@ public slots:
      * @function Settings.getValue
      * @param {string} key - The name of the setting.
      * @param {string|number|boolean|object} [defaultValue=""] - The value to return if the setting doesn't exist.
-     * @returns {string|number|boolean|object} The value stored in the named setting if it exists, otherwise the 
+     * @returns {string|number|boolean|object} The value stored in the named setting if it exists, otherwise the
      *     <code>defaultValue</code>.
      * @example <caption>Retrieve non-existent setting values.</caption>
      * var value1 = Settings.getValue("Script Example/Nonexistent Key");
@@ -50,11 +51,11 @@ public slots:
     QVariant getValue(const QString& setting, const QVariant& defaultValue);
 
     /*@jsdoc
-     * Stores a value in a named setting. If the setting already exists, its value is overwritten. If the value is 
+     * Stores a value in a named setting. If the setting already exists, its value is overwritten. If the value is
      * <code>null</code> or <code>undefined</code>, the setting is deleted.
      * @function Settings.setValue
      * @param {string} key - The name of the setting. Be sure to use a unique name if creating a new setting.
-     * @param {string|number|boolean|object|undefined} value - The value to store in the setting. If <code>null</code> or 
+     * @param {string|number|boolean|object|undefined} value - The value to store in the setting. If <code>null</code> or
      *     <code>undefined</code> is specified, the setting is deleted.
      * @example <caption>Store and retrieve an object value.</caption>
      * Settings.setValue("Script Example/My Key", { x: 0, y: 10, z: 0 });

libraries/shared/src/SettingHandle.cpp

@@ -4,6 +4,7 @@
 //
 //  Created by AndrewMeadows 2015.10.05
 //  Copyright 2015 High Fidelity, Inc.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
@@ -15,6 +16,7 @@
 #include <math.h>
 
 
+Q_LOGGING_CATEGORY(settings_handle, "settings.handle")
 
 const QString Settings::firstRun { "firstRun" };
 
@@ -34,13 +36,11 @@ void Settings::remove(const QString& key) {
     _manager->remove(key);
 }
 
-QStringList Settings::childGroups() const {
+// QStringList Settings::childGroups() const {
-    return _manager->childGroups();
+// }
-}
 
-QStringList Settings::childKeys() const {
+// QStringList Settings::childKeys() const {
-    return _manager->childKeys();
+// }
-}
 
 QStringList Settings::allKeys() const {
     return _manager->allKeys();
@@ -52,37 +52,56 @@ bool Settings::contains(const QString& key) const {
 }
 
 int Settings::beginReadArray(const QString& prefix) {
-    return _manager->beginReadArray(prefix);
+    _groups.push(Group(prefix));
+    _groupPrefix = getGroupPrefix();
+    int size = _manager->value(_groupPrefix + "/size", -1).toInt();
+    _groups.top().setSize(size);
+    return size;
 }
 
 void Settings::beginWriteArray(const QString& prefix, int size) {
-    _manager->beginWriteArray(prefix, size);
+    _groups.push(Group(prefix));
+    _groupPrefix = getGroupPrefix();
+    _manager->setValue(_groupPrefix + "/size", size);
+
+    _groups.top().setSize(size);
+    _groups.top().setIndex(0);
+
+    _groupPrefix = getGroupPrefix();
 }
 
 void Settings::endArray() {
-    _manager->endArray();
+    if (!_groups.empty()) {
-}
+        _groups.pop();
-
+        _groupPrefix = getGroupPrefix();
-void Settings::setArrayIndex(int i) {
-    _manager->setArrayIndex(i);
-}
-
-void Settings::beginGroup(const QString& prefix) {
-    _manager->beginGroup(prefix);
-}
-
-void Settings::endGroup() {
-    _manager->endGroup();
-}
-
-void Settings::setValue(const QString& name, const QVariant& value) {
-    if (_manager->value(name) != value) {
-        _manager->setValue(name, value);
     }
 }
 
+void Settings::setArrayIndex(int i) {
+    if (!_groups.empty()) {
+        _groups.top().setIndex(i);
+        _groupPrefix = getGroupPrefix();
+    }
+}
+
+void Settings::beginGroup(const QString& prefix) {
+    _groups.push(Group(prefix));
+    _groupPrefix = getGroupPrefix();
+}
+
+void Settings::endGroup() {
+    _groups.pop();
+    _groupPrefix = getGroupPrefix();
+}
+
+void Settings::setValue(const QString& name, const QVariant& value) {
+    QString fullPath = getPath(name);
+    _manager->setValue(fullPath, value);
+}
+
 QVariant Settings::value(const QString& name, const QVariant& defaultValue) const {
-    return _manager->value(name, defaultValue);
+    QString fullPath = getPath(name);
+    return _manager->value(fullPath, defaultValue);
 }
 
 
@@ -153,3 +172,31 @@ void Settings::getQuatValueIfValid(const QString& name, glm::quat& quatValue) {
     endGroup();
 }
 
+QString Settings::getGroupPrefix() const {
+    QString ret;
+
+    for(Group g : _groups) {
+        if (!ret.isEmpty()) {
+            ret.append("/");
+        }
+
+        ret.append(g.name());
+
+        if ( g.isArray() ) {
+            // QSettings indexes arrays from 1
+            ret.append(QString("/%1").arg(g.index() + 1));
+        }
+    }
+
+    return ret;
+}
+
+QString Settings::getPath(const QString &key) const {
+    QString ret = _groupPrefix;
+    if (!ret.isEmpty() ) {
+        ret.append("/");
+    }
+
+    ret.append(key);
+    return ret;
+}

libraries/shared/src/SettingHandle.h

@@ -4,6 +4,7 @@
 //
 //  Created by Clement on 1/18/15.
 //  Copyright 2015 High Fidelity, Inc.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
@@ -20,6 +21,7 @@
 #include <QtCore/QReadWriteLock>
 #include <QtCore/QSharedPointer>
 #include <QtCore/QDebug>
+#include <QLoggingCategory>
 
 #include <glm/glm.hpp>
 #include <glm/gtc/quaternion.hpp>
@@ -27,17 +29,65 @@
 #include "SettingInterface.h"
 
 
-// TODO: remove
+Q_DECLARE_LOGGING_CATEGORY(settings_handle)
+
+/**
+ * @brief QSettings analog
+ *
+ * This class emulates the interface of QSettings, and forwards all reads and writes to the global Setting::Manager.
+ *
+ * It should be used in the same manner as QSettings -- created only wherever it happens to be needed.
+ * It's not thread safe, and each thread should have their own.
+ *
+ * Unlike QSettings, destruction doesn't cause the config to be saved, instead Config::Manager is in
+ * charge of taking care of that.
+ *
+ * @note childGroups and childKeys are unimplemented because nothing in the code needs them so far.
+ */
 class Settings {
 public:
+
+    class Group {
+        public:
+
+        Group(const QString &groupName = QString()) {
+            _name = groupName;
+        }
+
+        QString name() const { return _name; }
+
+        bool isArray() const { return _arraySize != -1; }
+
+        void setIndex(int i) {
+            if ( _arraySize < i+1) {
+                _arraySize = i+1;
+            }
+
+            _arrayIndex = i;
+        }
+
+        int index() const { return _arrayIndex; }
+        int size() const { return _arraySize; }
+        void setSize(int sz) { _arraySize = sz; }
+
+        private:
+
+        QString _name;
+        int _arrayIndex{0};
+        int _arraySize{-1};
+    };
+
     static const QString firstRun;
     Settings();
 
     QString fileName() const;
 
     void remove(const QString& key);
-    QStringList childGroups() const;
+
-    QStringList childKeys() const;
+    // These are not currently being used
+    // QStringList childGroups() const;
+    // QStringList childKeys() const;
+
     QStringList allKeys() const;
     bool contains(const QString& key) const;
     int    beginReadArray(const QString & prefix);
@@ -61,33 +111,108 @@ public:
     void getQuatValueIfValid(const QString& name, glm::quat& quatValue);
 
 private:
+    QString getGroupPrefix() const;
+    QString getPath(const QString &value) const;
+
     QSharedPointer<Setting::Manager> _manager;
+    QStack<Group> _groups;
+    QString _groupPrefix;
 };
 
 namespace Setting {
+
+    /**
+     * @brief Handle to a setting of type T.
+     *
+     * This creates an object that manipulates a setting in the settings system. Changes will be written
+     * to the configuration file at some point controlled by Setting::Manager.
+     *
+     * @tparam T Type of the setting.
+     */
     template <typename T>
     class Handle : public Interface {
     public:
+
+        /**
+         * @brief Construct handle to a setting
+         *
+         * @param key The key corresponding to the setting in the settings file. Eg, 'shadowsEnabled'
+         */
         Handle(const QString& key) : Interface(key) {}
+
+        /**
+         * @brief Construct a handle to a setting
+         *
+         * @param path Path to the key corresponding to the setting in the settings file. Eg, QStringList() << group << key
+         */
         Handle(const QStringList& path) : Interface(path.join("/")) {}
 
+        /**
+         * @brief Construct handle to a setting with a default value
+         *
+         * @param key The key corresponding to the setting in the settings file. Eg, 'shadowsEnabled'
+         * @param defaultValue Default value for this setting
+         */
         Handle(const QString& key, const T& defaultValue) : Interface(key), _defaultValue(defaultValue) {}
+
+        /**
+         * @brief Construct a handle to a setting with a default value
+         *
+         * @param path Path to the key corresponding to the setting in the settings file. Eg, QStringList() << group << key
+         * @param defaultValue Default value for this setting
+         */
         Handle(const QStringList& path, const T& defaultValue) : Handle(path.join("/"), defaultValue) {}
 
+        /**
+         * @brief Construct a handle to a deprecated setting
+         *
+         * If used, a warning will written to the log.
+         *
+         * @param key The key corresponding to the setting in the settings file. Eg, 'shadowsEnabled'
+         * @return Handle The handle object
+         */
         static Handle Deprecated(const QString& key) {
             Handle handle = Handle(key);
             handle.deprecate();
             return handle;
         }
+
+        /**
+         * @brief Construct a handle to a deprecated setting
+         *
+         * If used, a warning will written to the log.
+         *
+         * @param path Path to the key corresponding to the setting in the settings file. Eg, QStringList() << group << key
+         * @return Handle The handle object
+         */
         static Handle Deprecated(const QStringList& path) {
             return Deprecated(path.join("/"));
         }
 
+        /**
+         * @brief Construct a handle to a deprecated setting with a default value
+         *
+         * If used, a warning will written to the log.
+         *
+         * @param key The key corresponding to the setting in the settings file. Eg, 'shadowsEnabled'
+         * @param defaultValue Default value for this setting
+         * @return Handle The handle object
+         */
         static Handle Deprecated(const QString& key, const T& defaultValue) {
             Handle handle = Handle(key, defaultValue);
             handle.deprecate();
             return handle;
         }
+
+        /**
+         * @brief Construct a handle to a deprecated setting with a default value
+         *
+         * If used, a warning will written to the log.
+         *
+         * @param path Path to the key corresponding to the setting in the settings file. Eg, QStringList() << group << key
+         * @param defaultValue Default value for this setting
+         * @return Handle The handle object
+         */
         static Handle Deprecated(const QStringList& path, const T& defaultValue) {
             return Deprecated(path.join("/"), defaultValue);
         }
@@ -96,32 +221,66 @@ namespace Setting {
             deinit();
         }
 
-        // Returns setting value, returns its default value if not found
+        /**
+         * @brief Returns the value of the setting, or the default value if not found
+         *
+         * @return T Value of the associated setting
+         */
         T get() const {
             return get(_defaultValue);
         }
 
-        // Returns setting value, returns other if not found
+        /**
+         * @brief Returns the value of the setting, or 'other' if not found.
+         *
+         * @param other Value to return if the setting is not set
+         * @return T  Value of the associated setting
+         */
         T get(const T& other) const {
             maybeInit();
             return (_isSet) ? _value : other;
         }
 
+        /**
+         * @brief Returns whether the setting is set to a value
+         *
+         * @return true The setting has a value
+         * @return false The setting has no value
+         */
         bool isSet() const {
             maybeInit();
             return _isSet;
         }
 
+        /**
+         * @brief Returns the default value for this setting
+         *
+         * @return const T& Default value for this setting
+         */
         const T& getDefault() const {
             return _defaultValue;
         }
 
+        /**
+         * @brief Sets the value to the default
+         *
+         */
         void reset() {
             set(_defaultValue);
         }
 
+        /**
+         * @brief Set the setting to the specified value.
+         *
+         * The value will be stored in the configuration file.
+         *
+         * @param value Value to set the setting to.
+         */
         void set(const T& value) {
             maybeInit();
+
+            // qCDebug(settings_handle) << "Setting" << this->getKey() << "to" << value;
+
             if ((!_isSet && (value != _defaultValue)) || _value != value) {
                 _value = value;
                 _isSet = true;
@@ -132,6 +291,12 @@ namespace Setting {
             }
         }
 
+        /**
+         * @brief Remove the value from the setting
+         *
+         * This returns the setting to an unset state. If read, it will be read as the default value.
+         *
+         */
         void remove() {
             maybeInit();
             if (_isSet) {
@@ -148,7 +313,7 @@ namespace Setting {
         void deprecate() {
             if (_isSet) {
                 if (get() != getDefault()) {
-                    qInfo().nospace() << "[DEPRECATION NOTICE] " << _key << "(" << get() << ") has been deprecated, and has no effect";
+                    qCInfo(settings_handle).nospace() << "[DEPRECATION NOTICE] " << _key << "(" << get() << ") has been deprecated, and has no effect";
                 } else {
                     remove();
                 }

libraries/shared/src/SettingHelpers.cpp

@@ -4,6 +4,7 @@
 //
 //  Created by Clement on 9/13/16.
 //  Copyright 2016 High Fidelity, Inc.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html

libraries/shared/src/SettingInterface.cpp

@@ -4,6 +4,7 @@
 //
 //  Created by Clement on 2/2/15.
 //  Copyright 2015 High Fidelity, Inc.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
@@ -23,23 +24,15 @@
 #include "SharedUtil.h"
 #include "ThreadHelpers.h"
 
+Q_LOGGING_CATEGORY(settings_interface, "settings.interface")
+
 namespace Setting {
     // This should only run as a post-routine in the QCoreApplication destructor
     void cleanupSettingsSaveThread() {
         auto globalManager = DependencyManager::get<Manager>();
         Q_ASSERT(qApp && globalManager);
 
-        // Grab the settings thread to shut it down
+        globalManager->terminateThread();
-        QThread* settingsManagerThread = globalManager->thread();
-
-        // Quit the settings manager thread and wait for it so we
-        // don't get concurrent accesses when we save all settings below
-        settingsManagerThread->quit();
-        settingsManagerThread->wait();
-
-        // [IMPORTANT] Save all settings when the QApplication goes down
-        globalManager->saveAll();
-
         qCDebug(shared) << "Settings thread stopped.";
     }
 
@@ -48,37 +41,6 @@ namespace Setting {
         auto globalManager = DependencyManager::get<Manager>();
         Q_ASSERT(qApp && globalManager);
 
-        // Let's set up the settings private instance on its own thread
-        QThread* thread = new QThread(qApp);
-        Q_CHECK_PTR(thread);
-        thread->setObjectName("Settings Thread");
-
-        // Setup setting periodical save timer
-        QObject::connect(thread, &QThread::started, globalManager.data(), [globalManager] {
-            setThreadName("Settings Save Thread");
-            globalManager->startTimer();
-        });
-        QObject::connect(thread, &QThread::finished, globalManager.data(), &Manager::stopTimer);
-
-        // Setup manager threading affinity
-        // This makes the timer fire on the settings thread so we don't block the main
-        // thread with a lot of file I/O.
-        // We bring back the manager to the main thread when the QApplication goes down
-        globalManager->moveToThread(thread);
-        QObject::connect(thread, &QThread::finished, globalManager.data(), [] {
-            auto globalManager = DependencyManager::get<Manager>();
-            Q_ASSERT(qApp && globalManager);
-
-            // Move manager back to the main thread (has to be done on owning thread)
-            globalManager->moveToThread(qApp->thread());
-        });
-
-        // Start the settings save thread
-        thread->start();
-        qCDebug(shared) << "Settings thread started.";
-
-        // Register cleanupSettingsSaveThread to run inside QCoreApplication's destructor.
-        // This will cleanup the settings thread and save all settings before shut down.
         qAddPostRoutine(cleanupSettingsSaveThread);
     }
 
@@ -115,7 +77,7 @@ namespace Setting {
             // in an assignment-client - the QSettings backing we use for this means persistence of these
             // settings from an AC (when there can be multiple terminating at same time on one machine)
             // is currently not supported
-            qWarning() << "Setting::Interface::init() for key" << _key << "- Manager not yet created." << 
+            qCWarning(settings_interface) << "Setting::Interface::init() for key" << _key << "- Manager not yet created." <<
                 "Settings persistence disabled.";
         } else {
             _manager = DependencyManager::get<Manager>();
@@ -125,11 +87,12 @@ namespace Setting {
                 manager->registerHandle(this);
                 _isInitialized = true;
             } else {
-                qWarning() << "Settings interface used after manager destroyed";
+                qCWarning(settings_interface) << "Settings interface used after manager destroyed";
             }
-        
+
             // Load value from disk
             load();
+            //qCDebug(settings_interface) << "Setting" << this->getKey() << "initialized to" << getVariant();
         }
     }
 
@@ -144,20 +107,21 @@ namespace Setting {
         }
     }
 
-    
+
     void Interface::maybeInit() const {
         if (!_isInitialized) {
+            //qCDebug(settings_interface) << "Initializing setting" << this->getKey();
             const_cast<Interface*>(this)->init();
         }
     }
-    
+
     void Interface::save() {
         auto manager = _manager.lock();
         if (manager) {
             manager->saveSetting(this);
         }
     }
-    
+
     void Interface::load() {
         auto manager = _manager.lock();
         if (manager) {

libraries/shared/src/SettingInterface.h

@@ -4,6 +4,7 @@
 //
 //  Created by Clement on 2/2/15.
 //  Copyright 2015 High Fidelity, Inc.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
@@ -16,6 +17,9 @@
 #include <QtCore/QWeakPointer>
 #include <QtCore/QString>
 #include <QtCore/QVariant>
+#include <QLoggingCategory>
+
+Q_DECLARE_LOGGING_CATEGORY(settings_interface)
 
 namespace Setting {
     class Manager;
@@ -25,11 +29,11 @@ namespace Setting {
     class Interface {
     public:
         const QString& getKey() const { return _key; }
-        bool isSet() const { return _isSet; } 
+        bool isSet() const { return _isSet; }
 
         virtual void setVariant(const QVariant& variant) = 0;
         virtual QVariant getVariant() = 0;
-        
+
     protected:
         Interface(const QString& key) : _key(key) {}
         virtual ~Interface() = default;
@@ -37,7 +41,7 @@ namespace Setting {
         void init();
         void maybeInit() const;
         void deinit();
-        
+
         void save();
         void load();
 
@@ -46,7 +50,7 @@ namespace Setting {
 
     private:
         mutable bool _isInitialized = false;
-        
+
         friend class Manager;
         mutable QWeakPointer<Manager> _manager;
     };

libraries/shared/src/SettingManager.cpp

@@ -4,6 +4,7 @@
 //
 //  Created by Clement on 2/2/15.
 //  Copyright 2015 High Fidelity, Inc.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
@@ -17,13 +18,89 @@
 
 #include "SettingInterface.h"
 
+Q_LOGGING_CATEGORY(settings_manager, "settings.manager")
+Q_LOGGING_CATEGORY(settings_writer, "settings.manager.writer")
+
 namespace Setting {
 
+
+    void WriteWorker::start() {
+        // QSettings seems to have some issues with moving to a new thread.
+        // Make sure the object is only created once the thread is already up and running.
+        init();
+    }
+
+    void WriteWorker::setValue(const QString key, const QVariant value) {
+       //qCDebug(settings_writer) << "Setting config " << key << "to" << value;
+
+        init();
+
+        if (!_qSettings->contains(key) || _qSettings->value(key) != value) {
+            _qSettings->setValue(key, value);
+        }
+    }
+
+    void WriteWorker::removeKey(const QString key) {
+        init();
+        _qSettings->remove(key);
+    }
+
+    void WriteWorker::sync() {
+        //qCDebug(settings_writer) << "Forcing settings sync";
+        init();
+        _qSettings->sync();
+    }
+
+    void WriteWorker::threadFinished() {
+        qCDebug(settings_writer) << "Settings write worker syncing and terminating";
+        sync();
+        this->deleteLater();
+    }
+
+    void WriteWorker::terminate() {
+        qCDebug(settings_writer) << "Settings write worker being asked to terminate. Syncing and terminating.";
+        sync();
+        this->deleteLater();
+        QThread::currentThread()->exit(0);
+    }
+
+    Manager::Manager(QObject *parent) {
+        WriteWorker *worker = new WriteWorker();
+
+        // We operate purely from memory, and forward all changes to a thread that has writing the
+        // settings as its only job.
+
+        qCDebug(settings_manager) << "Initializing settings write thread";
+
+        _workerThread.setObjectName("Settings Writer");
+        worker->moveToThread(&_workerThread);
+        // connect(&_workerThread, &QThread::started, worker, &WriteWorker::start, Qt::QueuedConnection);
+
+        // All normal connections are queued, so that we're sure they happen asynchronously.
+        connect(&_workerThread, &QThread::finished, worker, &WriteWorker::threadFinished, Qt::QueuedConnection);
+        connect(this, &Manager::valueChanged, worker, &WriteWorker::setValue, Qt::QueuedConnection);
+        connect(this, &Manager::keyRemoved, worker, &WriteWorker::removeKey, Qt::QueuedConnection);
+        connect(this, &Manager::syncRequested, worker, &WriteWorker::sync, Qt::QueuedConnection);
+
+        // This one is blocking because we want to wait until it's actually processed.
+        connect(this, &Manager::terminationRequested, worker, &WriteWorker::terminate, Qt::BlockingQueuedConnection);
+
+
+        _workerThread.start();
+
+        // Load all current settings
+        QSettings settings;
+        _fileName = settings.fileName();
+
+        for(QString key : settings.allKeys()) {
+            //qCDebug(settings_manager) << "Loaded key" << key << "with value" << settings.value(key);
+            _settings[key] = settings.value(key);
+        }
+    }
+
+
     Manager::~Manager() {
-        // Cleanup timer
+
-        stopTimer();
-        delete _saveTimer;
-        _saveTimer = nullptr;
     }
 
     // Custom deleter does nothing, because we need to shutdown later than the dependency manager
@@ -33,7 +110,7 @@ namespace Setting {
         const QString& key = handle->getKey();
         withWriteLock([&] {
             if (_handles.contains(key)) {
-                qWarning() << "Setting::Manager::registerHandle(): Key registered more than once, overriding: " << key;
+                qCWarning(settings_manager) << "Setting::Manager::registerHandle(): Key registered more than once, overriding: " << key;
             }
             _handles.insert(key, handle);
         });
@@ -47,13 +124,10 @@ namespace Setting {
 
     void Manager::loadSetting(Interface* handle) {
         const auto& key = handle->getKey();
+
         withWriteLock([&] {
-            QVariant loadedValue;
+            QVariant loadedValue = _settings[key];
-            if (_pendingChanges.contains(key) && _pendingChanges[key] != UNSET_VALUE) {
+
-                loadedValue = _pendingChanges[key];
-            } else {
-                loadedValue = _qSettings.value(key);
-            }
             if (loadedValue.isValid()) {
                 handle->setVariant(loadedValue);
             }
@@ -63,144 +137,76 @@ namespace Setting {
 
     void Manager::saveSetting(Interface* handle) {
         const auto& key = handle->getKey();
-        QVariant handleValue = UNSET_VALUE;
+
         if (handle->isSet()) {
-            handleValue = handle->getVariant();
+            QVariant handleValue = handle->getVariant();
+
+            withWriteLock([&] {
+                _settings[key] = handleValue;
+            });
+
+            emit valueChanged(key, handleValue);
+        } else {
+            withWriteLock([&] {
+                _settings.remove(key);
+            });
+
+            emit keyRemoved(key);
         }
 
-        withWriteLock([&] {
-            _pendingChanges[key] = handleValue;
-        });
     }
 
-    static const int SAVE_INTERVAL_MSEC = 5 * 1000; // 5 sec
+    void Manager::forceSave() {
-    void Manager::startTimer() {
+        emit syncRequested();
-        if (!_saveTimer) {
-            _saveTimer = new QTimer(this);
-            Q_CHECK_PTR(_saveTimer);
-            _saveTimer->setSingleShot(true); // We will restart it once settings are saved.
-            _saveTimer->setInterval(SAVE_INTERVAL_MSEC); // 5s, Qt::CoarseTimer acceptable
-            connect(_saveTimer, SIGNAL(timeout()), this, SLOT(saveAll()));
-        }
-        _saveTimer->start();
     }
 
-    void Manager::stopTimer() {
+    void Manager::terminateThread() {
-        if (_saveTimer) {
+        qCDebug(settings_manager) << "Terminating settings writer thread";
-            _saveTimer->stop();
-        }
-    }
 
-    void Manager::saveAll() {
+        emit terminationRequested(); // This blocks
-        withWriteLock([&] {
-            bool forceSync = false;
-            for (auto key : _pendingChanges.keys()) {
-                auto newValue = _pendingChanges[key];
-                auto savedValue = _qSettings.value(key, UNSET_VALUE);
-                if (newValue == savedValue) {
-                    continue;
-                }
-                forceSync = true;
-                if (newValue == UNSET_VALUE || !newValue.isValid()) {
-                    _qSettings.remove(key);
-                } else {
-                    _qSettings.setValue(key, newValue);
-                }
-            }
-            _pendingChanges.clear();
 
-            if (forceSync) {
+        _workerThread.exit();
-                _qSettings.sync();
+        _workerThread.wait(THREAD_TERMINATION_TIMEOUT);
-            }
+        qCDebug(settings_manager) << "Settings writer terminated";
-        });
-
-        // Restart timer
-        if (_saveTimer) {
-            _saveTimer->start();
-        }
     }
 
     QString Manager::fileName() const {
         return resultWithReadLock<QString>([&] {
-            return _qSettings.fileName();
+            return _fileName;
         });
     }
 
     void Manager::remove(const QString &key) {
         withWriteLock([&] {
-            _qSettings.remove(key);
+            _settings.remove(key);
         });
-    }
 
-    QStringList Manager::childGroups() const {
+        emit keyRemoved(key);
-        return resultWithReadLock<QStringList>([&] {
-            return _qSettings.childGroups();
-        });
-    }
-
-    QStringList Manager::childKeys() const {
-        return resultWithReadLock<QStringList>([&] {
-            return _qSettings.childKeys();
-        });
     }
 
     QStringList Manager::allKeys() const {
         return resultWithReadLock<QStringList>([&] {
-            return _qSettings.allKeys();
+            return _settings.keys();
         });
     }
 
     bool Manager::contains(const QString &key) const {
         return resultWithReadLock<bool>([&] {
-            return _qSettings.contains(key);
+            return _settings.contains(key);
-        });
-    }
-
-    int Manager::beginReadArray(const QString &prefix) {
-        return resultWithReadLock<int>([&] {
-            return _qSettings.beginReadArray(prefix);
-        });
-    }
-
-    void Manager::beginGroup(const QString &prefix) {
-        withWriteLock([&] {
-            _qSettings.beginGroup(prefix);
-        });
-    }
-
-    void Manager::beginWriteArray(const QString &prefix, int size) {
-        withWriteLock([&] {
-            _qSettings.beginWriteArray(prefix, size);
-        });
-    }
-
-    void Manager::endArray() {
-        withWriteLock([&] {
-            _qSettings.endArray();
-        });
-    }
-
-    void Manager::endGroup() {
-        withWriteLock([&] {
-            _qSettings.endGroup();
-        });
-    }
-
-    void Manager::setArrayIndex(int i) {
-        withWriteLock([&] {
-            _qSettings.setArrayIndex(i);
         });
     }
 
     void Manager::setValue(const QString &key, const QVariant &value) {
         withWriteLock([&] {
-            _qSettings.setValue(key, value);
+            _settings[key] = value;
         });
+
+        emit valueChanged(key, value);
     }
 
     QVariant Manager::value(const QString &key, const QVariant &defaultValue) const {
         return resultWithReadLock<QVariant>([&] {
-            return _qSettings.value(key, defaultValue);
+            return _settings.value(key, defaultValue);
         });
     }
 }

libraries/shared/src/SettingManager.h

@@ -4,6 +4,7 @@
 //
 //  Created by Clement on 2/2/15.
 //  Copyright 2015 High Fidelity, Inc.
+//  Copyright 2022 Overte e.V.
 //
 //  Distributed under the Apache License, Version 2.0.
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
@@ -17,35 +18,169 @@
 #include <QtCore/QTimer>
 #include <QtCore/QUuid>
 
+#include <QSettings>
+#include <QThread>
+#include <QLoggingCategory>
+
 #include "DependencyManager.h"
 #include "shared/ReadWriteLockable.h"
 
+Q_DECLARE_LOGGING_CATEGORY(settings_manager)
+Q_DECLARE_LOGGING_CATEGORY(settings_writer)
+
+// This is for the testing system.
+class SettingsTests;
+class SettingsTestsWorker;
+
+
+
 namespace Setting {
     class Interface;
 
+    /**
+     * @brief Settings write worker
+     *
+     * This class is used by Setting::Manager to write settings to permanent storage
+     * without blocking anything else. It receives setting updates, and writes them
+     * to disk whenever convenient.
+     *
+     * All communication to this class must be done over queued connections.
+     *
+     * This class is purely an implementation detail and shouldn't be used outside of Setting::Manager.
+     *
+     */
+    class WriteWorker : public QObject {
+        Q_OBJECT
+
+        public slots:
+
+        /**
+         * @brief Initialize anything that needs initializing, called on thread start.
+         *
+         */
+        void start();
+
+        /**
+         * @brief Sets a configuration value
+         *
+         * @param key Configuration key
+         * @param value Configuration value
+         */
+        void setValue(const QString key, const QVariant value);
+
+        /**
+         * @brief Remove a value from the configuration
+         *
+         * @param key Key to remove
+         */
+        void removeKey(const QString key);
+
+        /**
+         * @brief Force writing the config to disk
+         *
+         */
+        void sync();
+
+        /**
+         * @brief Called when the thread is terminating
+         *
+         */
+        void threadFinished();
+
+        /**
+         * @brief Thread is being asked to finish work and quit
+         *
+         */
+        void terminate();
+
+        private:
+
+        void init() {
+            if (!_qSettings) {
+                _qSettings = new QSettings();
+            }
+        }
+
+        QSettings* _qSettings = nullptr;
+    };
+
+    /**
+     * @brief Settings manager
+     *
+     * This class is the main implementation of the settings system, and the container
+     * of the current configuration.
+     *
+     * Most users should either use the Setting::Handle or the Settings classes instead,
+     * both of which talk to the single global instance of this class.
+     *
+     * The class is thread-safe, and delegates config writing to a separate thread. It
+     * is safe to change settings as often as it might be needed.
+     *
+     */
     class Manager : public QObject, public ReadWriteLockable, public Dependency {
         Q_OBJECT
 
     public:
+        Manager(QObject *parent = nullptr);
+
         void customDeleter() override;
 
-        // thread-safe proxies into QSettings
+        /**
+         * @brief Returns the filename where the config file will be written
+         *
+         * @return QString Path to the config file
+         */
         QString fileName() const;
+
+        /**
+         * @brief Remove a configuration key
+         *
+         * @param key Key to remove
+         */
         void remove(const QString &key);
-        QStringList childGroups() const;
+
-        QStringList childKeys() const;
+        /**
+         * @brief Lists all keys in the configuration
+         *
+         * @return QStringList List of keys
+         */
         QStringList allKeys() const;
+
+        /**
+         * @brief Returns whether a key is part of the configuration
+         *
+         * @param key Key to look for
+         * @return true Key is in the configuration
+         * @return false Key isn't in the configuration
+         */
         bool contains(const QString &key) const;
-        int beginReadArray(const QString &prefix);
+
-        void beginGroup(const QString &prefix);
+        /**
-        void beginWriteArray(const QString &prefix, int size = -1);
+         * @brief Set a setting to a value
-        void endArray();
+         *
-        void endGroup();
+         * @param key Setting to set
-        void setArrayIndex(int i);
+         * @param value Value
+         */
         void setValue(const QString &key, const QVariant &value);
+
+        /**
+         * @brief Returns the value of a setting
+         *
+         * @param key Setting to look for
+         * @param defaultValue Default value to return, if the setting has no value
+         * @return QVariant Current value of the setting, of defaultValue.
+         */
         QVariant value(const QString &key, const QVariant &defaultValue = QVariant()) const;
 
     protected:
+        /**
+         * @brief How long to wait for writer thread termination
+         *
+         * We probably want a timeout here since we don't want to block shutdown indefinitely in case of
+         * any weirdness.
+         */
+        const int THREAD_TERMINATION_TIMEOUT = 2000;
+
         ~Manager();
         void registerHandle(Interface* handle);
         void removeHandle(const QString& key);
@@ -53,24 +188,41 @@ namespace Setting {
         void loadSetting(Interface* handle);
         void saveSetting(Interface* handle);
 
-    private slots:
-        void startTimer();
-        void stopTimer();
 
-        void saveAll();
+        /**
+         * @brief Force saving the config to disk.
+         *
+         * Normally unnecessary to use. Asynchronous.
+         */
+        void forceSave();
+
+        /**
+         * @brief Write config to disk and terminate the writer thread
+         *
+         * This is part of the shutdown process.
+         */
+        void terminateThread();
+
+    signals:
+        void valueChanged(const QString key, QVariant value);
+        void keyRemoved(const QString key);
+        void syncRequested();
+        void terminationRequested();
 
     private:
         QHash<QString, Interface*> _handles;
-        QPointer<QTimer> _saveTimer = nullptr;
-        const QVariant UNSET_VALUE { QUuid::createUuid() };
-        QHash<QString, QVariant> _pendingChanges;
 
         friend class Interface;
+        friend class ::SettingsTests;
+        friend class ::SettingsTestsWorker;
+
         friend void cleanupSettingsSaveThread();
         friend void setupSettingsSaveThread();
 
 
-        QSettings _qSettings;
+        QHash<QString, QVariant> _settings;
+        QString _fileName;
+        QThread _workerThread;
     };
 }
 

tests/shared/CMakeLists.txt

@@ -1,6 +1,9 @@
 
 # Declare dependencies
 macro (setup_testcase_dependencies)
+  setup_memory_debugger()
+  setup_thread_debugger()
+
 
   # link in the shared libraries
   link_hifi_libraries(shared test-utils)

tests/shared/src/SettingsTests.cpp

@@ -0,0 +1,226 @@
+//
+//  Created by Dale Glass on 2022/10/22
+//  Copyright 2022 Overte e.V.
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+
+#include "SettingsTests.h"
+
+#include <QtTest/QtTest>
+#include <SettingHandle.h>
+#include <SettingManager.h>
+#include <DependencyManager.h>
+#include <QDebug>
+#include <QCoreApplication>
+#include "SettingInterface.h"
+#include "SettingHandle.h"
+
+
+
+QTEST_MAIN(SettingsTests)
+
+void SettingsTestsWorker::saveSettings() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    QThread *thread = QThread::currentThread();
+
+    while(! thread->isInterruptionRequested() ) {
+        //qDebug() << "Thread is saving config";
+        sm->forceSave();
+
+        // Not having any wait here for some reason locks up the benchmark.
+        // Logging a message also does the trick.
+        //
+        // This looks like a bug somewhere and needs investigating.
+        thread->yieldCurrentThread();
+    }
+
+    thread->exit(0);
+}
+
+void SettingsTests::initTestCase() {
+    QCoreApplication::setOrganizationName("OverteTest");
+
+    DependencyManager::set<Setting::Manager>();
+
+    Setting::init();
+}
+
+void SettingsTests::cleanupTestCase() {
+  //  Setting::cleanupSettingsSaveThread();
+}
+
+void SettingsTests::loadSettings() {
+    Settings s;
+    qDebug() << "Loaded" << s.fileName();
+}
+
+void SettingsTests::saveSettings() {
+    Settings s;
+    s.setValue("TestValue", "Hello");
+
+    auto sm = DependencyManager::get<Setting::Manager>();
+    sm->setValue("TestValueSM", "Hello");
+
+    // There seems to be a bug here, data gets lost without this call here.
+    sm->forceSave();
+    qDebug() << "Wrote" << s.fileName();
+}
+
+void SettingsTests::testSettings() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    Settings s;
+
+    s.setValue("settingsTest", 1);
+    QVERIFY(sm->value("settingsTest") == 1);
+
+    QVERIFY(!sm->contains("nonExistingKey"));
+    QVERIFY(sm->value("nonExistingKey") == QVariant());
+
+}
+
+void SettingsTests::testGroups() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    Settings s;
+
+    s.setValue("valueNotInGroupBefore", 1);
+    s.beginGroup("testGroup");
+    s.setValue("valueInGroup", 2);
+    s.endGroup();
+
+    s.beginGroup("testGroupFirst");
+    s.beginGroup("testGroupSecond");
+    s.setValue("valueInGroup", 44);
+    s.endGroup();
+    s.endGroup();
+
+    s.setValue("valueNotInGroupAfter", 3);
+
+    QVERIFY(sm->value("valueNotInGroupBefore") == 1);
+    QVERIFY(sm->value("testGroup/valueInGroup") == 2);
+    QVERIFY(sm->value("testGroupFirst/testGroupSecond/valueInGroup") == 44);
+    QVERIFY(sm->value("valueNotInGroupAfter") == 3);
+}
+
+void SettingsTests::testArray() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    Settings s;
+
+    s.beginWriteArray("testArray", 2);
+    s.setValue("A", 1);
+    s.setValue("B", 2);
+    s.setArrayIndex(1);
+    s.setValue("A", 11);
+    s.setValue("B", 22);
+    s.endArray();
+
+    s.setValue("valueNotInArray", 6);
+
+    QVERIFY(sm->value("testArray/size") == 2);
+    QVERIFY(sm->value("testArray/1/A") == 1);
+    QVERIFY(sm->value("testArray/1/B") == 2);
+    QVERIFY(sm->value("testArray/2/A") == 11);
+    QVERIFY(sm->value("testArray/2/B") == 22);
+    QVERIFY(sm->value("valueNotInArray") == 6);
+}
+
+void SettingsTests::testArrayInGroup() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    Settings s;
+
+    s.beginGroup("groupWithArray");
+    s.beginWriteArray("arrayInGroup", 2);
+    s.setValue("X", 10);
+    s.setArrayIndex(1);
+    s.setValue("X", 20);
+    s.endArray();
+    s.endGroup();
+
+    s.setValue("valueNotInArrayOrGroup", 8);
+
+    QVERIFY(sm->value("groupWithArray/arrayInGroup/size") == 2);
+    QVERIFY(sm->value("groupWithArray/arrayInGroup/1/X") == 10);
+    QVERIFY(sm->value("groupWithArray/arrayInGroup/2/X") == 20);
+    QVERIFY(sm->value("valueNotInArrayOrGroup") == 8);
+}
+
+void SettingsTests::testHandleUnused() {
+
+    {
+        Setting::Handle<int> testHandle("unused_handle", -1);
+    }
+}
+
+void SettingsTests::testHandle() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    Setting::Handle<int> testHandle("integer_value", -1);
+
+    QVERIFY(!testHandle.isSet());
+    QVERIFY(testHandle.get() == -1);
+    QVERIFY(testHandle.get(-5) == -5);
+    QVERIFY(testHandle.getDefault() == -1);
+
+    testHandle.set(42);
+    QVERIFY(testHandle.get() == 42);
+    QVERIFY(testHandle.isSet());
+    QVERIFY(sm->value("integer_value") == 42);
+
+    testHandle.reset();
+    QVERIFY(testHandle.get() == -1);
+    QVERIFY(testHandle.isSet());
+    QVERIFY(sm->value("integer_value") == -1);
+
+    testHandle.remove();
+    QVERIFY(!testHandle.isSet());
+}
+
+
+void SettingsTests::benchmarkSetValue() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    int i = 0;
+
+    QBENCHMARK {
+        sm->setValue("BenchmarkSetValue", ++i);
+    }
+
+}
+
+
+void SettingsTests::benchmarkSaveSettings() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    int i = 0;
+
+    QBENCHMARK {
+        sm->setValue("BenchmarkSave", ++i);
+        sm->forceSave();
+    }
+
+}
+
+
+void SettingsTests::benchmarkSetValueConcurrent() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    int i = 0;
+
+    _settingsThread = new QThread(qApp);
+    _testWorker = new SettingsTestsWorker();
+
+    _settingsThread->setObjectName("Save thread");
+    _testWorker->moveToThread(_settingsThread);
+
+    QObject::connect(_settingsThread, &QThread::started, _testWorker, &SettingsTestsWorker::saveSettings, Qt::QueuedConnection );
+
+    _settingsThread->start();
+    QBENCHMARK {
+        sm->setValue("BenchmarkSetValueConcurrent", ++i);
+    }
+
+    _settingsThread->requestInterruption();
+    _settingsThread->wait();
+
+    delete _testWorker;
+    delete _settingsThread;
+}
+

tests/shared/src/SettingsTests.h

@@ -0,0 +1,53 @@
+//
+//  Created by Dale Glass 2022/10/22
+//  Copyright 2022 Overte e.V.
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+#ifndef overte_SettingsTests_h
+#define overte_SettingsTests_h
+
+#include <QtCore/QObject>
+
+
+
+
+class SettingsTestsWorker : public QObject {
+    Q_OBJECT
+
+public slots:
+    void saveSettings();
+};
+
+class SettingsTests : public QObject {
+    Q_OBJECT
+
+private slots:
+    void initTestCase();
+    void loadSettings();
+    void saveSettings();
+
+    void testSettings();
+    void testGroups();
+    void testArray();
+    void testArrayInGroup();
+
+    void testHandleUnused();
+    void testHandle();
+
+
+    void benchmarkSetValue();
+    void benchmarkSaveSettings();
+    void benchmarkSetValueConcurrent();
+
+    void cleanupTestCase();
+
+private:
+    QThread *_settingsThread = nullptr;
+    SettingsTestsWorker *_testWorker = nullptr;
+
+};
+
+#endif // overte_SettingsTests_h