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 _manager; + QStack _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(); 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(); 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(); - 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(); @@ -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(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([&] { - return _qSettings.fileName(); + return _fileName; }); } void Manager::remove(const QString &key) { withWriteLock([&] { - _qSettings.remove(key); + _settings.remove(key); }); - } - QStringList Manager::childGroups() const { - return resultWithReadLock([&] { - return _qSettings.childGroups(); - }); - } - - QStringList Manager::childKeys() const { - return resultWithReadLock([&] { - return _qSettings.childKeys(); - }); + emit keyRemoved(key); } QStringList Manager::allKeys() const { return resultWithReadLock([&] { - return _qSettings.allKeys(); + return _settings.keys(); }); } bool Manager::contains(const QString &key) const { return resultWithReadLock([&] { - return _qSettings.contains(key); - }); - } - - int Manager::beginReadArray(const QString &prefix) { - return resultWithReadLock([&] { - 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([&] { - 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 #include +#include +#include + #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 _handles; - QPointer _saveTimer = nullptr; const QVariant UNSET_VALUE { QUuid::createUuid() }; - QHash _pendingChanges; + friend class Interface; friend class ::SettingsTests; @@ -81,7 +143,9 @@ namespace Setting { friend void setupSettingsSaveThread(); - QSettings _qSettings; + QHash _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 #include #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(); + Settings s; + + s.setValue("settingsTest", 1); + QVERIFY(sm->value("settingsTest") == 1); +} + +void SettingsTests::testGroups() { + auto sm = DependencyManager::get(); + 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(); + 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(); + 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(); 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();