Merge pull request #8057 from jherico/settings

Fixing issues with unclosed groups in settings persistence

libraries/networking/src/AccountManager.cpp

@@ -173,6 +173,7 @@ void AccountManager::setAuthURL(const QUrl& authURL) {
                         <<  "from previous settings file";
                 }
             }
+            settings.endGroup();
 
             if (_accountInfo.getAccessToken().token.isEmpty()) {
                 qCWarning(networking) << "Unable to load account file. No existing account settings will be loaded.";

libraries/script-engine/src/ScriptEngines.cpp

@@ -334,6 +334,7 @@ void ScriptEngines::clearScripts() {
     Settings settings;
     settings.beginWriteArray(SETTINGS_KEY);
     settings.remove("");
+    settings.endArray();
 }
 
 void ScriptEngines::saveScripts() {

libraries/shared/src/SettingHandle.cpp

@@ -18,6 +18,7 @@
 
 const QString Settings::firstRun { "firstRun" };
 
+
 Settings::Settings() : 
     _manager(DependencyManager::get<Setting::Manager>()), 
     _locker(&(_manager->getLock())) 
@@ -25,6 +26,9 @@ Settings::Settings() :
 }
 
 Settings::~Settings() {
+    if (_prefixes.size() != 0) {
+        qFatal("Unstable Settings Prefixes:  You must call endGroup for every beginGroup and endArray for every begin*Array call");
+    }
 }
 
 void Settings::remove(const QString& key) {
@@ -50,14 +54,17 @@ bool Settings::contains(const QString& key) const {
 }
 
 int Settings::beginReadArray(const QString & prefix) {
+    _prefixes.push(prefix);
     return _manager->beginReadArray(prefix);
 }
 
 void Settings::beginWriteArray(const QString& prefix, int size) {
+    _prefixes.push(prefix);
     _manager->beginWriteArray(prefix, size);
 }
 
 void Settings::endArray() {
+    _prefixes.pop();
     _manager->endArray();
 }
 
@@ -66,10 +73,12 @@ void Settings::setArrayIndex(int i) {
 }
 
 void Settings::beginGroup(const QString& prefix) {
+    _prefixes.push(prefix);
     _manager->beginGroup(prefix);
 }
 
 void Settings::endGroup() {
+    _prefixes.pop();
     _manager->endGroup();
 }
 

libraries/shared/src/SettingHandle.h

@@ -58,8 +58,10 @@ public:
     void setQuatValue(const QString& name, const glm::quat& quatValue);
     void getQuatValueIfValid(const QString& name, glm::quat& quatValue);
 
+private:
     QSharedPointer<Setting::Manager> _manager;
     QWriteLocker _locker;
+    QStack<QString> _prefixes;
 };
 
 namespace Setting {
@@ -75,15 +77,40 @@ namespace Setting {
         virtual ~Handle() { deinit(); }
         
         // Returns setting value, returns its default value if not found
-        T get() { return get(_defaultValue); }
+        T get() const { 
+            return get(_defaultValue); 
+        }
+
         // Returns setting value, returns other if not found
-        T get(const T& other) { maybeInit(); return (_isSet) ? _value : other; }
-        T getDefault() const { return _defaultValue; }
+        T get(const T& other) const { 
+            maybeInit(); 
+            return (_isSet) ? _value : other; 
+        }
+
+        const T& getDefault() const { 
+            return _defaultValue; 
+        }
         
-        void set(const T& value) { maybeInit(); _value = value; _isSet = true; }
-        void reset() { set(_defaultValue); }
-        
-        void remove() { maybeInit(); _isSet = false; }
+        void reset() { 
+            set(_defaultValue); 
+        }
+
+        void set(const T& value) { 
+            maybeInit(); 
+            if ((!_isSet && (value != _defaultValue)) || _value != value) {
+                _value = value; 
+                _isSet = true; 
+                save(); 
+            } 
+        }
+
+        void remove() { 
+            maybeInit(); 
+            if (_isSet) { 
+                _isSet = false; 
+                save(); 
+            }  
+        }
         
     protected:
         virtual void setVariant(const QVariant& variant);

libraries/shared/src/SettingInterface.cpp

@@ -98,6 +98,8 @@ namespace Setting {
                 // Register Handle
                 manager->registerHandle(this);
                 _isInitialized = true;
+            } else {
+                qWarning() << "Settings interface used after manager destroyed";
             }
         
             // Load value from disk
@@ -117,9 +119,9 @@ namespace Setting {
     }
 
     
-    void Interface::maybeInit() {
+    void Interface::maybeInit() const {
         if (!_isInitialized) {
-            init();
+            const_cast<Interface*>(this)->init();
         }
     }
     

libraries/shared/src/SettingInterface.h

@@ -39,19 +39,20 @@ namespace Setting {
         virtual ~Interface() = default;
 
         void init();
-        void maybeInit();
+        void maybeInit() const;
         void deinit();
         
         void save();
         void load();
-        
-        bool _isInitialized = false;
+
         bool _isSet = false;
         const QString _key;
+
+    private:
+        mutable bool _isInitialized = false;
         
         friend class Manager;
-
-        QWeakPointer<Manager> _manager;
+        mutable QWeakPointer<Manager> _manager;
     };
 }
 

libraries/shared/src/SettingManager.cpp

@@ -33,8 +33,8 @@ namespace Setting {
     void Manager::customDeleter() { }
 
 
-    void Manager::registerHandle(Setting::Interface* handle) {
-        QString key = handle->getKey();
+    void Manager::registerHandle(Interface* handle) {
+        const QString& key = handle->getKey();
         withWriteLock([&] {
             if (_handles.contains(key)) {
                 qWarning() << "Setting::Manager::registerHandle(): Key registered more than once, overriding: " << key;
@@ -58,7 +58,9 @@ namespace Setting {
             } else {
                 loadedValue = value(key);
             }
-            handle->setVariant(loadedValue);
+            if (loadedValue.isValid()) {
+                handle->setVariant(loadedValue);
+            }
         });
     }
 
@@ -94,6 +96,7 @@ namespace Setting {
     }
 
     void Manager::saveAll() {
+        bool forceSync = false;
         withWriteLock([&] {
             for (auto key : _pendingChanges.keys()) {
                 auto newValue = _pendingChanges[key];
@@ -101,15 +104,21 @@ namespace Setting {
                 if (newValue == savedValue) {
                     continue;
                 }
-                if (newValue == UNSET_VALUE) {
+                if (newValue == UNSET_VALUE || !newValue.isValid()) {
+                    forceSync = true;
                     remove(key);
                 } else {
+                    forceSync = true;
                     setValue(key, newValue);
                 }
             }
             _pendingChanges.clear();
         });
 
+        if (forceSync) {
+            sync();
+        }
+
         // Restart timer
         if (_saveTimer) {
             _saveTimer->start();