From 334317b175166cd6e7371bdf9110b23318336798 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Sat, 22 Oct 2022 20:18:51 +0200
Subject: [PATCH 01/20] Test suite for settings system

Proof of concept still. Adds a test-specific function in Setting::Manager.
---
 libraries/shared/src/SettingManager.cpp | 11 +++++
 libraries/shared/src/SettingManager.h   |  1 +
 tests/shared/src/SettingsTests.cpp      | 61 +++++++++++++++++++++++++
 tests/shared/src/SettingsTests.h        | 25 ++++++++++
 4 files changed, 98 insertions(+)
 create mode 100644 tests/shared/src/SettingsTests.cpp
 create mode 100644 tests/shared/src/SettingsTests.h

diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index e5920b785e..f524e8e102 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -120,6 +120,17 @@ namespace Setting {
         }
     }
 
+    /**
+     * @brief Forces saving the current configuration
+     *
+     * @warning This function is for testing only, should only be called from the test suite.
+     */
+    void Manager::forceSave() {
+        withWriteLock([&] {
+             _qSettings.sync();
+        });
+    }
+
     QString Manager::fileName() const {
         return resultWithReadLock<QString>([&] {
             return _qSettings.fileName();
diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index 49f3bece4b..475753a277 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/libraries/shared/src/SettingManager.h
@@ -44,6 +44,7 @@ namespace Setting {
         void setArrayIndex(int i);
         void setValue(const QString &key, const QVariant &value);
         QVariant value(const QString &key, const QVariant &defaultValue = QVariant()) const;
+        void forceSave();
 
     protected:
         ~Manager();
diff --git a/tests/shared/src/SettingsTests.cpp b/tests/shared/src/SettingsTests.cpp
new file mode 100644
index 0000000000..18039a231a
--- /dev/null
+++ b/tests/shared/src/SettingsTests.cpp
@@ -0,0 +1,61 @@
+//
+//  Created by Bradley Austin Davis on 2017/11/08
+//  Copyright 2013-2017 High Fidelity, Inc.
+//
+//  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"
+
+
+QTEST_MAIN(SettingsTests)
+
+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::benchmarkSaveSettings() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    int i = 0;
+
+    QBENCHMARK {
+        sm->setValue("Benchmark", ++i);
+        sm->forceSave();
+    }
+
+}
diff --git a/tests/shared/src/SettingsTests.h b/tests/shared/src/SettingsTests.h
new file mode 100644
index 0000000000..927863adb6
--- /dev/null
+++ b/tests/shared/src/SettingsTests.h
@@ -0,0 +1,25 @@
+//
+//  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 SettingsTests : public QObject {
+    Q_OBJECT
+private slots:
+    void initTestCase();
+    void loadSettings();
+    void saveSettings();
+    void benchmarkSaveSettings();
+
+    void cleanupTestCase();
+};
+
+#endif // overte_SettingsTests_h

From c743abc3489f3efbb2bfa15eec5abb755d9e77c9 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Sat, 22 Oct 2022 23:56:09 +0200
Subject: [PATCH 02/20] Added threaded settings test.

Test setValue() while a thread is constantly trying to save the file
to disk to test the absolute worst case performance.
---
 tests/shared/src/SettingsTests.cpp | 56 ++++++++++++++++++++++++++++++
 tests/shared/src/SettingsTests.h   | 17 +++++++++
 2 files changed, 73 insertions(+)

diff --git a/tests/shared/src/SettingsTests.cpp b/tests/shared/src/SettingsTests.cpp
index 18039a231a..060a8cbceb 100644
--- a/tests/shared/src/SettingsTests.cpp
+++ b/tests/shared/src/SettingsTests.cpp
@@ -20,6 +20,24 @@
 
 QTEST_MAIN(SettingsTests)
 
+void SettingsTestThread::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");
 
@@ -49,6 +67,18 @@ void SettingsTests::saveSettings() {
     qDebug() << "Wrote" << s.fileName();
 }
 
+void SettingsTests::benchmarkSetValue() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    int i = 0;
+
+    QBENCHMARK {
+        sm->setValue("BenchmarkSetValue", ++i);
+    }
+
+    sm->forceSave();
+}
+
+
 void SettingsTests::benchmarkSaveSettings() {
     auto sm = DependencyManager::get<Setting::Manager>();
     int i = 0;
@@ -59,3 +89,29 @@ void SettingsTests::benchmarkSaveSettings() {
     }
 
 }
+
+
+void SettingsTests::benchmarkSetValueConcurrent() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    int i = 0;
+
+    _settingsThread = new QThread(qApp);
+    _settingsThreadObj = new SettingsTestThread;
+
+    _settingsThread->setObjectName("Save thread");
+    _settingsThreadObj->moveToThread(_settingsThread);
+
+    QObject::connect(_settingsThread, &QThread::started, _settingsThreadObj, &SettingsTestThread::saveSettings, Qt::QueuedConnection );
+
+    _settingsThread->start();
+    QBENCHMARK {
+        sm->setValue("BenchmarkSetValue", ++i);
+    }
+
+    _settingsThread->requestInterruption();
+    _settingsThread->wait();
+
+    delete _settingsThreadObj;
+    delete _settingsThread;
+}
+
diff --git a/tests/shared/src/SettingsTests.h b/tests/shared/src/SettingsTests.h
index 927863adb6..7de6f0df1d 100644
--- a/tests/shared/src/SettingsTests.h
+++ b/tests/shared/src/SettingsTests.h
@@ -11,15 +11,32 @@
 
 #include <QtCore/QObject>
 
+
+class SettingsTestThread : public QObject {
+    Q_OBJECT
+
+public slots:
+    void saveSettings();
+};
+
+
 class SettingsTests : public QObject {
     Q_OBJECT
 private slots:
     void initTestCase();
     void loadSettings();
     void saveSettings();
+
+    void benchmarkSetValue();
     void benchmarkSaveSettings();
+    void benchmarkSetValueConcurrent();
 
     void cleanupTestCase();
+
+private:
+    QThread *_settingsThread = nullptr;
+    SettingsTestThread *_settingsThreadObj = nullptr;
+
 };
 
 #endif // overte_SettingsTests_h

From b5d9a4dbba92600613aaf68e4a4c4093d846387e Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Sun, 23 Oct 2022 01:02:16 +0200
Subject: [PATCH 03/20] Make test class a friend

---
 libraries/shared/src/SettingManager.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index 475753a277..5cf16112a9 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/libraries/shared/src/SettingManager.h
@@ -20,6 +20,12 @@
 #include "DependencyManager.h"
 #include "shared/ReadWriteLockable.h"
 
+
+// This is for the testing system.
+class SettingsTests;
+
+
+
 namespace Setting {
     class Interface;
 
@@ -67,6 +73,8 @@ namespace Setting {
         QHash<QString, QVariant> _pendingChanges;
 
         friend class Interface;
+        friend class SettingsTests;
+
         friend void cleanupSettingsSaveThread();
         friend void setupSettingsSaveThread();
 

From d9468f33aa2d81b9765196faebbbe4c891008fa9 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Sun, 23 Oct 2022 12:07:42 +0200
Subject: [PATCH 04/20] Add ASAN/UBSAN support to shared tests

---
 tests/shared/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/shared/CMakeLists.txt b/tests/shared/CMakeLists.txt
index 48230337eb..a81764a639 100644
--- a/tests/shared/CMakeLists.txt
+++ b/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)

From 0896807ca5208808b8efa7c9ae25e825b67094e0 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Sun, 23 Oct 2022 12:08:03 +0200
Subject: [PATCH 05/20] Slight cleanup of tests, use friend class to avoid
 exposing test functions

---
 libraries/shared/src/SettingManager.cpp |  2 +-
 libraries/shared/src/SettingManager.h   |  6 ++++--
 tests/shared/src/SettingsTests.cpp      | 15 +++++++--------
 tests/shared/src/SettingsTests.h        |  8 +++++---
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index f524e8e102..26f7d0ca7b 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -127,7 +127,7 @@ namespace Setting {
      */
     void Manager::forceSave() {
         withWriteLock([&] {
-             _qSettings.sync();
+             _qSettings->sync();
         });
     }
 
diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index 5cf16112a9..0a8cd6482b 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/libraries/shared/src/SettingManager.h
@@ -23,6 +23,7 @@
 
 // This is for the testing system.
 class SettingsTests;
+class SettingsTestsWorker;
 
 
 
@@ -50,7 +51,6 @@ namespace Setting {
         void setArrayIndex(int i);
         void setValue(const QString &key, const QVariant &value);
         QVariant value(const QString &key, const QVariant &defaultValue = QVariant()) const;
-        void forceSave();
 
     protected:
         ~Manager();
@@ -59,6 +59,7 @@ namespace Setting {
 
         void loadSetting(Interface* handle);
         void saveSetting(Interface* handle);
+        void forceSave();
 
     private slots:
         void startTimer();
@@ -73,7 +74,8 @@ namespace Setting {
         QHash<QString, QVariant> _pendingChanges;
 
         friend class Interface;
-        friend class SettingsTests;
+        friend class ::SettingsTests;
+        friend class ::SettingsTestsWorker;
 
         friend void cleanupSettingsSaveThread();
         friend void setupSettingsSaveThread();
diff --git a/tests/shared/src/SettingsTests.cpp b/tests/shared/src/SettingsTests.cpp
index 060a8cbceb..4dd0fec8ae 100644
--- a/tests/shared/src/SettingsTests.cpp
+++ b/tests/shared/src/SettingsTests.cpp
@@ -20,7 +20,7 @@
 
 QTEST_MAIN(SettingsTests)
 
-void SettingsTestThread::saveSettings() {
+void SettingsTestsWorker::saveSettings() {
     auto sm = DependencyManager::get<Setting::Manager>();
     QThread *thread = QThread::currentThread();
 
@@ -75,7 +75,6 @@ void SettingsTests::benchmarkSetValue() {
         sm->setValue("BenchmarkSetValue", ++i);
     }
 
-    sm->forceSave();
 }
 
 
@@ -84,7 +83,7 @@ void SettingsTests::benchmarkSaveSettings() {
     int i = 0;
 
     QBENCHMARK {
-        sm->setValue("Benchmark", ++i);
+        sm->setValue("BenchmarkSave", ++i);
         sm->forceSave();
     }
 
@@ -96,22 +95,22 @@ void SettingsTests::benchmarkSetValueConcurrent() {
     int i = 0;
 
     _settingsThread = new QThread(qApp);
-    _settingsThreadObj = new SettingsTestThread;
+    _testWorker = new SettingsTestsWorker();
 
     _settingsThread->setObjectName("Save thread");
-    _settingsThreadObj->moveToThread(_settingsThread);
+    _testWorker->moveToThread(_settingsThread);
 
-    QObject::connect(_settingsThread, &QThread::started, _settingsThreadObj, &SettingsTestThread::saveSettings, Qt::QueuedConnection );
+    QObject::connect(_settingsThread, &QThread::started, _testWorker, &SettingsTestsWorker::saveSettings, Qt::QueuedConnection );
 
     _settingsThread->start();
     QBENCHMARK {
-        sm->setValue("BenchmarkSetValue", ++i);
+        sm->setValue("BenchmarkSetValueConcurrent", ++i);
     }
 
     _settingsThread->requestInterruption();
     _settingsThread->wait();
 
-    delete _settingsThreadObj;
+    delete _testWorker;
     delete _settingsThread;
 }
 
diff --git a/tests/shared/src/SettingsTests.h b/tests/shared/src/SettingsTests.h
index 7de6f0df1d..52198a3cd3 100644
--- a/tests/shared/src/SettingsTests.h
+++ b/tests/shared/src/SettingsTests.h
@@ -12,16 +12,18 @@
 #include <QtCore/QObject>
 
 
-class SettingsTestThread : public QObject {
+
+
+class SettingsTestsWorker : public QObject {
     Q_OBJECT
 
 public slots:
     void saveSettings();
 };
 
-
 class SettingsTests : public QObject {
     Q_OBJECT
+
 private slots:
     void initTestCase();
     void loadSettings();
@@ -35,7 +37,7 @@ private slots:
 
 private:
     QThread *_settingsThread = nullptr;
-    SettingsTestThread *_settingsThreadObj = nullptr;
+    SettingsTestsWorker *_testWorker = nullptr;
 
 };
 

From e960829112b72db9914f2c28254ac9dacfdd6409 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Sun, 23 Oct 2022 16:02:37 +0200
Subject: [PATCH 06/20] Fix test for the master branch

---
 libraries/shared/src/SettingManager.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index 26f7d0ca7b..f524e8e102 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -127,7 +127,7 @@ namespace Setting {
      */
     void Manager::forceSave() {
         withWriteLock([&] {
-             _qSettings->sync();
+             _qSettings.sync();
         });
     }
 

From 51d16c2ecf9af33d21dd7f86511ca32db6753c9d Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Sun, 23 Oct 2022 16:05:22 +0200
Subject: [PATCH 07/20] Fix copyright header

---
 tests/shared/src/SettingsTests.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/shared/src/SettingsTests.cpp b/tests/shared/src/SettingsTests.cpp
index 4dd0fec8ae..6d01516218 100644
--- a/tests/shared/src/SettingsTests.cpp
+++ b/tests/shared/src/SettingsTests.cpp
@@ -1,6 +1,6 @@
 //
-//  Created by Bradley Austin Davis on 2017/11/08
-//  Copyright 2013-2017 High Fidelity, Inc.
+//  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

From cdc15d7821285e88aeabeeb1a69a3abd69f3d29f Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Sun, 23 Oct 2022 23:25:42 +0200
Subject: [PATCH 08/20] Improve settings system, by moving all writes to a
 thread

This should complete what was started in the HiFi days but didn't quite succeed.

Setting::Manager is now thread safe, and delegates all settings writes to a thread
that nothing waits on, which should ensure that settings don't degrade performance
even on slow storage devices.

Functions that weren't thread safe were removed from Setting::Manager, and it was
reduced to a key/value store.

Functions that modify state like beginGroup were implemented in the Settings class
instead, which should be created only in the context where it's needed. It will
forward all changes to the manager.

A few QSettings functions were left unimplemented because they're not used in
the code. They may be implemented later if there's a need.
---
 libraries/shared/src/SettingHandle.cpp    |  99 ++++++++----
 libraries/shared/src/SettingHandle.h      |  57 ++++++-
 libraries/shared/src/SettingInterface.cpp |  53 +------
 libraries/shared/src/SettingManager.cpp   | 179 ++++++++--------------
 libraries/shared/src/SettingManager.h     |  98 ++++++++++--
 tests/shared/src/SettingsTests.cpp        |  75 +++++++++
 tests/shared/src/SettingsTests.h          |   6 +
 7 files changed, 360 insertions(+), 207 deletions(-)

diff --git a/libraries/shared/src/SettingHandle.cpp b/libraries/shared/src/SettingHandle.cpp
index 2bfc904c0a..25a8cca98e 100644
--- a/libraries/shared/src/SettingHandle.cpp
+++ b/libraries/shared/src/SettingHandle.cpp
@@ -34,13 +34,11 @@ void Settings::remove(const QString& key) {
     _manager->remove(key);
 }
 
-QStringList Settings::childGroups() const {
-    return _manager->childGroups();
-}
+// QStringList Settings::childGroups() const {
+// }
 
-QStringList Settings::childKeys() const {
-    return _manager->childKeys();
-}
+// QStringList Settings::childKeys() const {
+// }
 
 QStringList Settings::allKeys() const {
     return _manager->allKeys();
@@ -52,37 +50,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();
-}
-
-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);
+    if (!_groups.empty()) {
+        _groups.pop();
+        _groupPrefix = getGroupPrefix();
     }
 }
 
+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 +170,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;
+}
\ No newline at end of file
diff --git a/libraries/shared/src/SettingHandle.h b/libraries/shared/src/SettingHandle.h
index 4fce3bef96..f81c509c5b 100644
--- a/libraries/shared/src/SettingHandle.h
+++ b/libraries/shared/src/SettingHandle.h
@@ -27,17 +27,63 @@
 #include "SettingInterface.h"
 
 
-// TODO: remove
+/**
+ * @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,7 +107,12 @@ 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 {
diff --git a/libraries/shared/src/SettingInterface.cpp b/libraries/shared/src/SettingInterface.cpp
index ab5b3e380a..6c1e61e2a5 100644
--- a/libraries/shared/src/SettingInterface.cpp
+++ b/libraries/shared/src/SettingInterface.cpp
@@ -29,17 +29,7 @@ namespace Setting {
         auto globalManager = DependencyManager::get<Manager>();
         Q_ASSERT(qApp && globalManager);
 
-        // Grab the settings thread to shut it down
-        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();
-
+        globalManager->forceSave();
         qCDebug(shared) << "Settings thread stopped.";
     }
 
@@ -48,37 +38,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 +74,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." << 
+            qWarning() << "Setting::Interface::init() for key" << _key << "- Manager not yet created." <<
                 "Settings persistence disabled.";
         } else {
             _manager = DependencyManager::get<Manager>();
@@ -127,7 +86,7 @@ namespace Setting {
             } else {
                 qWarning() << "Settings interface used after manager destroyed";
             }
-        
+
             // Load value from disk
             load();
         }
@@ -144,20 +103,20 @@ namespace Setting {
         }
     }
 
-    
+
     void Interface::maybeInit() const {
         if (!_isInitialized) {
             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) {
diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index f524e8e102..d32b9b2944 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -19,11 +19,60 @@
 
 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) {
+        //qDebug() << "Setting config " << key << "to" << value;
+        init();
+        _qSettings->setValue(key, value);
+    }
+
+    void WriteWorker::removeKey(const QString &key) {
+        init();
+        _qSettings->remove(key);
+    }
+
+    void WriteWorker::sync() {
+        //qDebug() << "Forcing settings sync";
+        init();
+        _qSettings->sync();
+    }
+
+    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.
+
+        qDebug() << "Initializing settings write thread";
+
+        _workerThread.setObjectName("Settings Writer");
+        worker->moveToThread(&_workerThread);
+        connect(&_workerThread, &QThread::started, worker, &WriteWorker::start, Qt::QueuedConnection);
+        connect(&_workerThread, &QThread::finished, worker, &QObject::deleteLater, 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);
+        _workerThread.start();
+
+        // Load all current settings
+        QSettings settings;
+        _fileName = settings.fileName();
+
+        for(QString key : settings.allKeys()) {
+            qDebug() << "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
@@ -48,12 +97,8 @@ namespace Setting {
     void Manager::loadSetting(Interface* handle) {
         const auto& key = handle->getKey();
         withWriteLock([&] {
-            QVariant loadedValue;
-            if (_pendingChanges.contains(key) && _pendingChanges[key] != UNSET_VALUE) {
-                loadedValue = _pendingChanges[key];
-            } else {
-                loadedValue = _qSettings.value(key);
-            }
+            QVariant loadedValue = _settings[key];
+
             if (loadedValue.isValid()) {
                 handle->setVariant(loadedValue);
             }
@@ -69,56 +114,10 @@ namespace Setting {
         }
 
         withWriteLock([&] {
-            _pendingChanges[key] = handleValue;
+            _settings[key] = handleValue;
         });
     }
 
-    static const int SAVE_INTERVAL_MSEC = 5 * 1000; // 5 sec
-    void Manager::startTimer() {
-        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() {
-        if (_saveTimer) {
-            _saveTimer->stop();
-        }
-    }
-
-    void Manager::saveAll() {
-        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) {
-                _qSettings.sync();
-            }
-        });
-
-        // Restart timer
-        if (_saveTimer) {
-            _saveTimer->start();
-        }
-    }
 
     /**
      * @brief Forces saving the current configuration
@@ -126,92 +125,46 @@ namespace Setting {
      * @warning This function is for testing only, should only be called from the test suite.
      */
     void Manager::forceSave() {
-        withWriteLock([&] {
-             _qSettings.sync();
-        });
+        emit syncRequested();
     }
 
     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 {
-        return resultWithReadLock<QStringList>([&] {
-            return _qSettings.childGroups();
-        });
-    }
-
-    QStringList Manager::childKeys() const {
-        return resultWithReadLock<QStringList>([&] {
-            return _qSettings.childKeys();
-        });
+        emit keyRemoved(key);
     }
 
     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);
-        });
-    }
-
-    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);
+            return _settings.contains(key);
         });
     }
 
     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);
         });
     }
 }
diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index 0a8cd6482b..d0ea084ac2 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/libraries/shared/src/SettingManager.h
@@ -17,6 +17,9 @@
 #include <QtCore/QTimer>
 #include <QtCore/QUuid>
 
+#include <QSettings>
+#include <QThread>
+
 #include "DependencyManager.h"
 #include "shared/ReadWriteLockable.h"
 
@@ -30,25 +33,86 @@ 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();
+
+        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
         QString fileName() const;
         void remove(const QString &key);
-        QStringList childGroups() const;
-        QStringList childKeys() const;
         QStringList allKeys() const;
         bool contains(const QString &key) const;
-        int beginReadArray(const QString &prefix);
-        void beginGroup(const QString &prefix);
-        void beginWriteArray(const QString &prefix, int size = -1);
-        void endArray();
-        void endGroup();
-        void setArrayIndex(int i);
         void setValue(const QString &key, const QVariant &value);
         QVariant value(const QString &key, const QVariant &defaultValue = QVariant()) const;
 
@@ -61,17 +125,15 @@ namespace Setting {
         void saveSetting(Interface* handle);
         void forceSave();
 
-    private slots:
-        void startTimer();
-        void stopTimer();
-
-        void saveAll();
+    signals:
+        void valueChanged(const QString &key, const QVariant &value);
+        void keyRemoved(const QString &key);
+        void syncRequested();
 
     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;
@@ -81,7 +143,9 @@ namespace Setting {
         friend void setupSettingsSaveThread();
 
 
-        QSettings _qSettings;
+        QHash<QString, QVariant> _settings;
+        QString _fileName;
+        QThread _workerThread;
     };
 }
 
diff --git a/tests/shared/src/SettingsTests.cpp b/tests/shared/src/SettingsTests.cpp
index 6d01516218..f38ff15eaf 100644
--- a/tests/shared/src/SettingsTests.cpp
+++ b/tests/shared/src/SettingsTests.cpp
@@ -16,6 +16,8 @@
 #include <QDebug>
 #include <QCoreApplication>
 #include "SettingInterface.h"
+#include "SettingHandle.h"
+
 
 
 QTEST_MAIN(SettingsTests)
@@ -67,6 +69,79 @@ void SettingsTests::saveSettings() {
     qDebug() << "Wrote" << s.fileName();
 }
 
+void SettingsTests::testSettings() {
+    auto sm = DependencyManager::get<Setting::Manager>();
+    Settings s;
+
+    s.setValue("settingsTest", 1);
+    QVERIFY(sm->value("settingsTest") == 1);
+}
+
+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::benchmarkSetValue() {
     auto sm = DependencyManager::get<Setting::Manager>();
     int i = 0;
diff --git a/tests/shared/src/SettingsTests.h b/tests/shared/src/SettingsTests.h
index 52198a3cd3..4f4aff3e5a 100644
--- a/tests/shared/src/SettingsTests.h
+++ b/tests/shared/src/SettingsTests.h
@@ -29,6 +29,12 @@ private slots:
     void loadSettings();
     void saveSettings();
 
+    void testSettings();
+    void testGroups();
+    void testArray();
+    void testArrayInGroup();
+
+
     void benchmarkSetValue();
     void benchmarkSaveSettings();
     void benchmarkSetValueConcurrent();

From 6af76ae19fb45d5d739457d272651ef395f3ec84 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 16:58:54 +0100
Subject: [PATCH 09/20] Remove settings thread from Application.cpp

Thread is now managed in the Setting::Manager
---
 interface/src/Application.cpp | 36 +++++++----------------------------
 interface/src/Application.h   |  2 --
 2 files changed, 7 insertions(+), 31 deletions(-)

diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp
index b0e3812e7e..cee07a2df4 100644
--- a/interface/src/Application.cpp
+++ b/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);
-        settingsTimer->setInterval(SAVE_SETTINGS_INTERVAL); // 10s, Qt::CoarseTimer acceptable
-        QObject::connect(settingsTimer, &QTimer::timeout, this, &Application::saveSettings);
-        settingsTimer->start();
-    }, QThread::LowestPriority);
+
+    QTimer* settingsTimer = new QTimer();
+    int SAVE_SETTINGS_INTERVAL = 10 * MSECS_PER_SECOND; // Let's save every seconds for now
+    settingsTimer->setSingleShot(false);
+    settingsTimer->setInterval(SAVE_SETTINGS_INTERVAL); // 10s, Qt::CoarseTimer acceptable
+    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.
diff --git a/interface/src/Application.h b/interface/src/Application.h
index cd323eef08..df79d2255f 100644
--- a/interface/src/Application.h
+++ b/interface/src/Application.h
@@ -719,8 +719,6 @@ private:
 
     bool _notifiedPacketVersionMismatchThisDomain;
 
-    ConditionalGuard _settingsGuard;
-
     GLCanvas* _glWidget{ nullptr };
 
     typedef bool (Application::* AcceptURLMethod)(const QString &);

From 0e50b51a63b0ecf7c74a0f72dd3a2391d72676af Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:01:16 +0100
Subject: [PATCH 10/20] Improve documentation

---
 libraries/shared/src/SettingHandle.h  | 114 +++++++++++++++++++++++++-
 libraries/shared/src/SettingManager.h |  62 ++++++++++++++
 2 files changed, 174 insertions(+), 2 deletions(-)

diff --git a/libraries/shared/src/SettingHandle.h b/libraries/shared/src/SettingHandle.h
index f81c509c5b..0bddf5b4da 100644
--- a/libraries/shared/src/SettingHandle.h
+++ b/libraries/shared/src/SettingHandle.h
@@ -116,29 +116,99 @@ private:
 };
 
 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);
         }
@@ -147,32 +217,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;
@@ -183,6 +287,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) {
diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index d0ea084ac2..f62164db0e 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/libraries/shared/src/SettingManager.h
@@ -109,22 +109,84 @@ namespace Setting {
 
         void customDeleter() override;
 
+        /**
+         * @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);
+
+        /**
+         * @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;
+
+        /**
+         * @brief Set a setting to a value
+         *
+         * @param key Setting to set
+         * @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);
 
         void loadSetting(Interface* handle);
         void saveSetting(Interface* handle);
+
+
+        /**
+         * @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, const QVariant &value);
         void keyRemoved(const QString &key);

From 72bcd6a00865af8229c30b5e2914ecdbe3dac7b9 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:04:39 +0100
Subject: [PATCH 11/20] Add setting for Setting::Handle

---
 tests/shared/src/SettingsTests.cpp | 35 ++++++++++++++++++++++++++++++
 tests/shared/src/SettingsTests.h   |  3 +++
 2 files changed, 38 insertions(+)

diff --git a/tests/shared/src/SettingsTests.cpp b/tests/shared/src/SettingsTests.cpp
index f38ff15eaf..340d01f1d5 100644
--- a/tests/shared/src/SettingsTests.cpp
+++ b/tests/shared/src/SettingsTests.cpp
@@ -75,6 +75,10 @@ void SettingsTests::testSettings() {
 
     s.setValue("settingsTest", 1);
     QVERIFY(sm->value("settingsTest") == 1);
+
+    QVERIFY(!sm->contains("nonExistingKey"));
+    QVERIFY(sm->value("nonExistingKey") == QVariant());
+
 }
 
 void SettingsTests::testGroups() {
@@ -142,6 +146,37 @@ void SettingsTests::testArrayInGroup() {
     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;
diff --git a/tests/shared/src/SettingsTests.h b/tests/shared/src/SettingsTests.h
index 4f4aff3e5a..29b92f9377 100644
--- a/tests/shared/src/SettingsTests.h
+++ b/tests/shared/src/SettingsTests.h
@@ -34,6 +34,9 @@ private slots:
     void testArray();
     void testArrayInGroup();
 
+    void testHandleUnused();
+    void testHandle();
+
 
     void benchmarkSetValue();
     void benchmarkSaveSettings();

From b540c426c1db21a8874a524039098028b498b06f Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:06:33 +0100
Subject: [PATCH 12/20] Use logging categories

---
 interface/src/Application.h               | 2 +-
 libraries/shared/src/SettingHandle.cpp    | 1 +
 libraries/shared/src/SettingHandle.h      | 5 ++++-
 libraries/shared/src/SettingInterface.cpp | 6 ++++--
 libraries/shared/src/SettingInterface.h   | 7 +++++--
 libraries/shared/src/SettingManager.cpp   | 5 +++--
 libraries/shared/src/SettingManager.h     | 3 +++
 7 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/interface/src/Application.h b/interface/src/Application.h
index df79d2255f..ce200ce8bc 100644
--- a/interface/src/Application.h
+++ b/interface/src/Application.h
@@ -481,7 +481,7 @@ public slots:
     void setIsInterstitialMode(bool interstitialMode);
 
     void updateVerboseLogging();
-    
+
     void setCachebustRequire();
 
     void changeViewAsNeeded(float boomLength);
diff --git a/libraries/shared/src/SettingHandle.cpp b/libraries/shared/src/SettingHandle.cpp
index 25a8cca98e..f5c6fc3696 100644
--- a/libraries/shared/src/SettingHandle.cpp
+++ b/libraries/shared/src/SettingHandle.cpp
@@ -15,6 +15,7 @@
 #include <math.h>
 
 
+Q_LOGGING_CATEGORY(settings_handle, "settings.handle")
 
 const QString Settings::firstRun { "firstRun" };
 
diff --git a/libraries/shared/src/SettingHandle.h b/libraries/shared/src/SettingHandle.h
index 0bddf5b4da..7f5b544868 100644
--- a/libraries/shared/src/SettingHandle.h
+++ b/libraries/shared/src/SettingHandle.h
@@ -20,6 +20,7 @@
 #include <QtCore/QReadWriteLock>
 #include <QtCore/QSharedPointer>
 #include <QtCore/QDebug>
+#include <QLoggingCategory>
 
 #include <glm/glm.hpp>
 #include <glm/gtc/quaternion.hpp>
@@ -27,6 +28,8 @@
 #include "SettingInterface.h"
 
 
+Q_DECLARE_LOGGING_CATEGORY(settings_handle)
+
 /**
  * @brief QSettings analog
  *
@@ -309,7 +312,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();
                 }
diff --git a/libraries/shared/src/SettingInterface.cpp b/libraries/shared/src/SettingInterface.cpp
index 6c1e61e2a5..9f4c8fd60d 100644
--- a/libraries/shared/src/SettingInterface.cpp
+++ b/libraries/shared/src/SettingInterface.cpp
@@ -74,7 +74,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>();
@@ -84,11 +84,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();
         }
     }
 
@@ -106,6 +107,7 @@ namespace Setting {
 
     void Interface::maybeInit() const {
         if (!_isInitialized) {
+            //qCDebug(settings_interface) << "Initializing setting" << this->getKey();
             const_cast<Interface*>(this)->init();
         }
     }
diff --git a/libraries/shared/src/SettingInterface.h b/libraries/shared/src/SettingInterface.h
index 575641c0e7..fa30b24e08 100644
--- a/libraries/shared/src/SettingInterface.h
+++ b/libraries/shared/src/SettingInterface.h
@@ -16,6 +16,9 @@
 #include <QtCore/QWeakPointer>
 #include <QtCore/QString>
 #include <QtCore/QVariant>
+#include <QLoggingCategory>
+
+Q_DECLARE_LOGGING_CATEGORY(settings_interface)
 
 namespace Setting {
     class Manager;
@@ -37,7 +40,7 @@ namespace Setting {
         void init();
         void maybeInit() const;
         void deinit();
-        
+
         void save();
         void load();
 
@@ -46,7 +49,7 @@ namespace Setting {
 
     private:
         mutable bool _isInitialized = false;
-        
+
         friend class Manager;
         mutable QWeakPointer<Manager> _manager;
     };
diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index d32b9b2944..7bbfe96e8b 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -65,7 +65,7 @@ namespace Setting {
         _fileName = settings.fileName();
 
         for(QString key : settings.allKeys()) {
-            qDebug() << "Loaded key" << key << "with value" << settings.value(key);
+            //qCDebug(settings_manager) << "Loaded key" << key << "with value" << settings.value(key);
             _settings[key] = settings.value(key);
         }
     }
@@ -82,7 +82,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);
         });
@@ -96,6 +96,7 @@ namespace Setting {
 
     void Manager::loadSetting(Interface* handle) {
         const auto& key = handle->getKey();
+
         withWriteLock([&] {
             QVariant loadedValue = _settings[key];
 
diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index f62164db0e..2b62842028 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/libraries/shared/src/SettingManager.h
@@ -19,10 +19,13 @@
 
 #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;

From 06d7b89455d097684a006b3e8d9267d9c589085b Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:07:02 +0100
Subject: [PATCH 13/20] Improve logging, forgotten commit

---
 libraries/shared/src/SettingInterface.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libraries/shared/src/SettingInterface.cpp b/libraries/shared/src/SettingInterface.cpp
index 9f4c8fd60d..a9f7e701c7 100644
--- a/libraries/shared/src/SettingInterface.cpp
+++ b/libraries/shared/src/SettingInterface.cpp
@@ -23,13 +23,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);
 
-        globalManager->forceSave();
+        globalManager->terminateThread();
         qCDebug(shared) << "Settings thread stopped.";
     }
 

From 51e1df5e4c362ef6e7238806d690c0b74ef04dd8 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:08:08 +0100
Subject: [PATCH 14/20] Improve logging, v3

---
 libraries/shared/src/SettingManager.cpp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index 7bbfe96e8b..9bf6429ce7 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -17,6 +17,9 @@
 
 #include "SettingInterface.h"
 
+Q_LOGGING_CATEGORY(settings_manager, "settings.manager")
+Q_LOGGING_CATEGORY(settings_writer, "settings.manager.writer")
+
 namespace Setting {
 
 

From f7ab2be17373631eec5e596a2f8457d2757d9167 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:13:30 +0100
Subject: [PATCH 15/20] Unimportant changes

Signals were made non-references for debugging, but that shouldn't
actually matter since Qt copies the parameters anyway.
---
 libraries/shared/src/SettingInterface.h | 4 ++--
 libraries/shared/src/SettingManager.cpp | 7 ++++---
 libraries/shared/src/SettingManager.h   | 8 ++++----
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/libraries/shared/src/SettingInterface.h b/libraries/shared/src/SettingInterface.h
index fa30b24e08..2503fdbcdd 100644
--- a/libraries/shared/src/SettingInterface.h
+++ b/libraries/shared/src/SettingInterface.h
@@ -28,11 +28,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;
diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index 9bf6429ce7..9276a93f08 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -29,13 +29,14 @@ namespace Setting {
         init();
     }
 
-    void WriteWorker::setValue(const QString &key, const QVariant &value) {
-        //qDebug() << "Setting config " << key << "to" << value;
+    void WriteWorker::setValue(const QString key, const QVariant value) {
+       //qCDebug(settings_writer) << "Setting config " << key << "to" << value;
+
         init();
         _qSettings->setValue(key, value);
     }
 
-    void WriteWorker::removeKey(const QString &key) {
+    void WriteWorker::removeKey(const QString key) {
         init();
         _qSettings->remove(key);
     }
diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index 2b62842028..726331eea4 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/libraries/shared/src/SettingManager.h
@@ -65,14 +65,14 @@ namespace Setting {
          * @param key Configuration key
          * @param value Configuration value
          */
-        void setValue(const QString &key, const QVariant &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);
+        void removeKey(const QString key);
 
         /**
          * @brief Force writing the config to disk
@@ -191,8 +191,8 @@ namespace Setting {
         void terminateThread();
 
     signals:
-        void valueChanged(const QString &key, const QVariant &value);
-        void keyRemoved(const QString &key);
+        void valueChanged(const QString key, QVariant value);
+        void keyRemoved(const QString key);
         void syncRequested();
 
     private:

From d7226508067b210b209d04151e2f1e381bac43f9 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:14:10 +0100
Subject: [PATCH 16/20] Don't forward a change to QSetting if the setting
 didn't change

This considerably reduces the number of disk writes
---
 libraries/shared/src/SettingManager.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index 9276a93f08..296cbcab0a 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -33,7 +33,10 @@ namespace Setting {
        //qCDebug(settings_writer) << "Setting config " << key << "to" << value;
 
         init();
-        _qSettings->setValue(key, value);
+
+        if (!_qSettings->contains(key) || _qSettings->value(key) != value) {
+            _qSettings->setValue(key, value);
+        }
     }
 
     void WriteWorker::removeKey(const QString key) {

From 24d4f87341764b45231737def7843bc5f8fbe196 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:15:16 +0100
Subject: [PATCH 17/20] Improve logging system termination. Send a signal and
 wait until done.

---
 libraries/shared/src/SettingManager.cpp | 38 ++++++++++++++++++++++---
 libraries/shared/src/SettingManager.h   | 13 +++++++++
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index 296cbcab0a..823ebe7a18 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -45,26 +45,46 @@ namespace Setting {
     }
 
     void WriteWorker::sync() {
-        //qDebug() << "Forcing settings 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.
 
-        qDebug() << "Initializing settings write thread";
+        qCDebug(settings_manager) << "Initializing settings write thread";
 
         _workerThread.setObjectName("Settings Writer");
         worker->moveToThread(&_workerThread);
-        connect(&_workerThread, &QThread::started, worker, &WriteWorker::start, Qt::QueuedConnection);
-        connect(&_workerThread, &QThread::finished, worker, &QObject::deleteLater, Qt::QueuedConnection);
+        // 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
@@ -136,6 +156,16 @@ namespace Setting {
         emit syncRequested();
     }
 
+    void Manager::terminateThread() {
+        qCDebug(settings_manager) << "Terminating settings writer thread";
+
+        emit terminationRequested(); // This blocks
+
+        _workerThread.exit();
+        _workerThread.wait(THREAD_TERMINATION_TIMEOUT);
+        qCDebug(settings_manager) << "Settings writer terminated";
+    }
+
     QString Manager::fileName() const {
         return resultWithReadLock<QString>([&] {
             return _fileName;
diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index 726331eea4..4bc141a71d 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/libraries/shared/src/SettingManager.h
@@ -80,6 +80,18 @@ namespace Setting {
          */
         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() {
@@ -194,6 +206,7 @@ namespace Setting {
         void valueChanged(const QString key, QVariant value);
         void keyRemoved(const QString key);
         void syncRequested();
+        void terminationRequested();
 
     private:
         QHash<QString, Interface*> _handles;

From 20548b7b248f2578a3fd45224b555546d8449c91 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:15:50 +0100
Subject: [PATCH 18/20] Fix mysterious UUID issue

Turned out to be a remainant of previous code that stopped working
correctly due to the changes
---
 libraries/shared/src/SettingManager.cpp | 19 ++++++++++++++-----
 libraries/shared/src/SettingManager.h   |  2 --
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index 823ebe7a18..571a6ddedb 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -136,14 +136,23 @@ 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([&] {
-            _settings[key] = handleValue;
-        });
     }
 
 
diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index 4bc141a71d..74c2fb93e7 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/libraries/shared/src/SettingManager.h
@@ -210,8 +210,6 @@ namespace Setting {
 
     private:
         QHash<QString, Interface*> _handles;
-        const QVariant UNSET_VALUE { QUuid::createUuid() };
-
 
         friend class Interface;
         friend class ::SettingsTests;

From 7d3b45753c534cd832c03e44696d7ff004b6cd3b Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 17:16:14 +0100
Subject: [PATCH 19/20] Comment moved to header

---
 libraries/shared/src/SettingManager.cpp | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index 571a6ddedb..3b8539bb9a 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/libraries/shared/src/SettingManager.cpp
@@ -155,12 +155,6 @@ namespace Setting {
 
     }
 
-
-    /**
-     * @brief Forces saving the current configuration
-     *
-     * @warning This function is for testing only, should only be called from the test suite.
-     */
     void Manager::forceSave() {
         emit syncRequested();
     }

From 9a828077bdc1d720f889a6ac1e6c3a8444f28113 Mon Sep 17 00:00:00 2001
From: Dale Glass <dale@daleglass.net>
Date: Tue, 1 Nov 2022 20:49:00 +0100
Subject: [PATCH 20/20] Add Overte e.V. copyright

---
 interface/src/Application.h                            | 1 +
 interface/src/scripting/SettingsScriptingInterface.cpp | 1 +
 interface/src/scripting/SettingsScriptingInterface.h   | 7 ++++---
 libraries/shared/src/SettingHandle.cpp                 | 1 +
 libraries/shared/src/SettingHandle.h                   | 1 +
 libraries/shared/src/SettingHelpers.cpp                | 1 +
 libraries/shared/src/SettingInterface.cpp              | 1 +
 libraries/shared/src/SettingInterface.h                | 1 +
 libraries/shared/src/SettingManager.cpp                | 1 +
 libraries/shared/src/SettingManager.h                  | 1 +
 10 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/interface/src/Application.h b/interface/src/Application.h
index ce200ce8bc..222dbd9d40 100644
--- a/interface/src/Application.h
+++ b/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
diff --git a/interface/src/scripting/SettingsScriptingInterface.cpp b/interface/src/scripting/SettingsScriptingInterface.cpp
index 13eddddb1f..bc4e0b2930 100644
--- a/interface/src/scripting/SettingsScriptingInterface.cpp
+++ b/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
diff --git a/interface/src/scripting/SettingsScriptingInterface.h b/interface/src/scripting/SettingsScriptingInterface.h
index c51161b642..286a2533e9 100644
--- a/interface/src/scripting/SettingsScriptingInterface.h
+++ b/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 });
diff --git a/libraries/shared/src/SettingHandle.cpp b/libraries/shared/src/SettingHandle.cpp
index f5c6fc3696..bb607b1544 100644
--- a/libraries/shared/src/SettingHandle.cpp
+++ b/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
diff --git a/libraries/shared/src/SettingHandle.h b/libraries/shared/src/SettingHandle.h
index 7f5b544868..e474ee61de 100644
--- a/libraries/shared/src/SettingHandle.h
+++ b/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
diff --git a/libraries/shared/src/SettingHelpers.cpp b/libraries/shared/src/SettingHelpers.cpp
index d8fcc6b6de..5695472fae 100644
--- a/libraries/shared/src/SettingHelpers.cpp
+++ b/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
diff --git a/libraries/shared/src/SettingInterface.cpp b/libraries/shared/src/SettingInterface.cpp
index a9f7e701c7..af92d754c0 100644
--- a/libraries/shared/src/SettingInterface.cpp
+++ b/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
diff --git a/libraries/shared/src/SettingInterface.h b/libraries/shared/src/SettingInterface.h
index 2503fdbcdd..5ee8cc1e37 100644
--- a/libraries/shared/src/SettingInterface.h
+++ b/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
diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp
index 3b8539bb9a..7a98d01232 100644
--- a/libraries/shared/src/SettingManager.cpp
+++ b/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
diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h
index 74c2fb93e7..b2e85e4395 100644
--- a/libraries/shared/src/SettingManager.h
+++ b/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