From 4c258b95682e49ad15f9bca28f705e2c8a85e824 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 4 Aug 2016 10:16:08 -0700 Subject: [PATCH 1/4] fix bug which caused getValue for a non-existent key to return the integer 2 --- libraries/shared/src/SettingManager.cpp | 7 +++---- libraries/shared/src/SettingManager.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp index abb8525b03..2ccee513da 100644 --- a/libraries/shared/src/SettingManager.cpp +++ b/libraries/shared/src/SettingManager.cpp @@ -70,11 +70,10 @@ namespace Setting { QVariant handleValue = UNSET_VALUE; if (handle->isSet()) { handleValue = handle->getVariant(); + withWriteLock([&] { + _pendingChanges[key] = handleValue; + }); } - - withWriteLock([&] { - _pendingChanges[key] = handleValue; - }); } static const int SAVE_INTERVAL_MSEC = 5 * 1000; // 5 sec diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h index 1f309c966f..ffdd4ba42a 100644 --- a/libraries/shared/src/SettingManager.h +++ b/libraries/shared/src/SettingManager.h @@ -46,7 +46,7 @@ namespace Setting { private: QHash _handles; QPointer _saveTimer = nullptr; - const QVariant UNSET_VALUE { QUuid::createUuid().variant() }; + const QVariant UNSET_VALUE { QUuid::createUuid() }; QHash _pendingChanges; friend class Interface; From 45c21ca52391d7590737519b02abfc47a404b30a Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 4 Aug 2016 13:37:08 -0700 Subject: [PATCH 2/4] make UNSET_VALUE still work while still returning and empty string value for undefined keys --- libraries/shared/src/SettingHandle.cpp | 6 +++++- libraries/shared/src/SettingManager.cpp | 9 +++++---- libraries/shared/src/SettingManager.h | 1 + 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/libraries/shared/src/SettingHandle.cpp b/libraries/shared/src/SettingHandle.cpp index 13f9ea48ce..ae0f54104e 100644 --- a/libraries/shared/src/SettingHandle.cpp +++ b/libraries/shared/src/SettingHandle.cpp @@ -89,7 +89,11 @@ void Settings::setValue(const QString& name, const QVariant& value) { } QVariant Settings::value(const QString& name, const QVariant& defaultValue) const { - return _manager->value(name, defaultValue); + QVariant result = _manager->value(name, defaultValue); + if (result == _manager->unsetValue()) { + return QVariant(QString()); + } + return result; } diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp index 2ccee513da..a42e62c1c8 100644 --- a/libraries/shared/src/SettingManager.cpp +++ b/libraries/shared/src/SettingManager.cpp @@ -53,7 +53,7 @@ namespace Setting { const auto& key = handle->getKey(); withWriteLock([&] { QVariant loadedValue; - if (_pendingChanges.contains(key)) { + if (_pendingChanges.contains(key) && _pendingChanges[key] != UNSET_VALUE) { loadedValue = _pendingChanges[key]; } else { loadedValue = value(key); @@ -70,10 +70,11 @@ namespace Setting { QVariant handleValue = UNSET_VALUE; if (handle->isSet()) { handleValue = handle->getVariant(); - withWriteLock([&] { - _pendingChanges[key] = handleValue; - }); } + + withWriteLock([&] { + _pendingChanges[key] = handleValue; + }); } static const int SAVE_INTERVAL_MSEC = 5 * 1000; // 5 sec diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h index ffdd4ba42a..836c522342 100644 --- a/libraries/shared/src/SettingManager.h +++ b/libraries/shared/src/SettingManager.h @@ -28,6 +28,7 @@ namespace Setting { public: void customDeleter() override; + QVariant unsetValue() { return UNSET_VALUE; } protected: ~Manager(); From 981a8d76facb774a1264c0861ba5456b0acd3446 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 4 Aug 2016 13:47:43 -0700 Subject: [PATCH 3/4] code review --- libraries/shared/src/SettingHandle.cpp | 2 +- libraries/shared/src/SettingManager.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/shared/src/SettingHandle.cpp b/libraries/shared/src/SettingHandle.cpp index ae0f54104e..71427d1554 100644 --- a/libraries/shared/src/SettingHandle.cpp +++ b/libraries/shared/src/SettingHandle.cpp @@ -91,7 +91,7 @@ void Settings::setValue(const QString& name, const QVariant& value) { QVariant Settings::value(const QString& name, const QVariant& defaultValue) const { QVariant result = _manager->value(name, defaultValue); if (result == _manager->unsetValue()) { - return QVariant(QString()); + return defaultValue; } return result; } diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h index 836c522342..e785c5f147 100644 --- a/libraries/shared/src/SettingManager.h +++ b/libraries/shared/src/SettingManager.h @@ -28,7 +28,7 @@ namespace Setting { public: void customDeleter() override; - QVariant unsetValue() { return UNSET_VALUE; } + const QVariant& unsetValue() { return UNSET_VALUE; } protected: ~Manager(); From a2441ca84c4b66bb66639c6ccf33599efe2890b3 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 4 Aug 2016 14:09:12 -0700 Subject: [PATCH 4/4] back out some uneeded chagnes --- libraries/shared/src/SettingHandle.cpp | 6 +----- libraries/shared/src/SettingManager.h | 1 - 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/libraries/shared/src/SettingHandle.cpp b/libraries/shared/src/SettingHandle.cpp index 71427d1554..13f9ea48ce 100644 --- a/libraries/shared/src/SettingHandle.cpp +++ b/libraries/shared/src/SettingHandle.cpp @@ -89,11 +89,7 @@ void Settings::setValue(const QString& name, const QVariant& value) { } QVariant Settings::value(const QString& name, const QVariant& defaultValue) const { - QVariant result = _manager->value(name, defaultValue); - if (result == _manager->unsetValue()) { - return defaultValue; - } - return result; + return _manager->value(name, defaultValue); } diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h index e785c5f147..ffdd4ba42a 100644 --- a/libraries/shared/src/SettingManager.h +++ b/libraries/shared/src/SettingManager.h @@ -28,7 +28,6 @@ namespace Setting { public: void customDeleter() override; - const QVariant& unsetValue() { return UNSET_VALUE; } protected: ~Manager();