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..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 @@ -481,7 +482,7 @@ public slots: void setIsInterstitialMode(bool interstitialMode); void updateVerboseLogging(); - + void setCachebustRequire(); void changeViewAsNeeded(float boomLength); @@ -719,8 +720,6 @@ private: bool _notifiedPacketVersionMismatchThisDomain; - ConditionalGuard _settingsGuard; - GLCanvas* _glWidget{ nullptr }; typedef bool (Application::* AcceptURLMethod)(const QString &); 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 * defaultValue. * @example Retrieve non-existent setting values. * 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 * null or undefined, 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 null or + * @param {string|number|boolean|object|undefined} value - The value to store in the setting. If null or * undefined is specified, the setting is deleted. * @example Store and retrieve an object value. * 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 2bfc904c0a..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 @@ -15,6 +16,7 @@ #include +Q_LOGGING_CATEGORY(settings_handle, "settings.handle") const QString Settings::firstRun { "firstRun" }; @@ -34,13 +36,11 @@ void Settings::remove(const QString& key) { _manager->remove(key); } -QStringList Settings::childGroups() const { - 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 +52,56 @@ bool Settings::contains(const QString& key) const { } int Settings::beginReadArray(const QString& prefix) { - return _manager->beginReadArray(prefix); + _groups.push(Group(prefix)); + _groupPrefix = getGroupPrefix(); + int size = _manager->value(_groupPrefix + "/size", -1).toInt(); + _groups.top().setSize(size); + return size; } void Settings::beginWriteArray(const QString& prefix, int size) { - _manager->beginWriteArray(prefix, size); + _groups.push(Group(prefix)); + _groupPrefix = getGroupPrefix(); + _manager->setValue(_groupPrefix + "/size", size); + + _groups.top().setSize(size); + _groups.top().setIndex(0); + + _groupPrefix = getGroupPrefix(); } void Settings::endArray() { - _manager->endArray(); -} - -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 +172,31 @@ void Settings::getQuatValueIfValid(const QString& name, glm::quat& quatValue) { endGroup(); } +QString Settings::getGroupPrefix() const { + QString ret; + + for(Group g : _groups) { + if (!ret.isEmpty()) { + ret.append("/"); + } + + ret.append(g.name()); + + if ( g.isArray() ) { + // QSettings indexes arrays from 1 + ret.append(QString("/%1").arg(g.index() + 1)); + } + } + + return ret; +} + +QString Settings::getPath(const QString &key) const { + QString ret = _groupPrefix; + if (!ret.isEmpty() ) { + ret.append("/"); + } + + ret.append(key); + return ret; +} \ No newline at end of file diff --git a/libraries/shared/src/SettingHandle.h b/libraries/shared/src/SettingHandle.h index 4fce3bef96..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 @@ -20,6 +21,7 @@ #include #include #include +#include #include #include @@ -27,17 +29,65 @@ #include "SettingInterface.h" -// TODO: remove +Q_DECLARE_LOGGING_CATEGORY(settings_handle) + +/** + * @brief QSettings analog + * + * This class emulates the interface of QSettings, and forwards all reads and writes to the global Setting::Manager. + * + * It should be used in the same manner as QSettings -- created only wherever it happens to be needed. + * It's not thread safe, and each thread should have their own. + * + * Unlike QSettings, destruction doesn't cause the config to be saved, instead Config::Manager is in + * charge of taking care of that. + * + * @note childGroups and childKeys are unimplemented because nothing in the code needs them so far. + */ class Settings { public: + + class Group { + public: + + Group(const QString &groupName = QString()) { + _name = groupName; + } + + QString name() const { return _name; } + + bool isArray() const { return _arraySize != -1; } + + void setIndex(int i) { + if ( _arraySize < i+1) { + _arraySize = i+1; + } + + _arrayIndex = i; + } + + int index() const { return _arrayIndex; } + int size() const { return _arraySize; } + void setSize(int sz) { _arraySize = sz; } + + private: + + QString _name; + int _arrayIndex{0}; + int _arraySize{-1}; + }; + static const QString firstRun; Settings(); QString fileName() const; void remove(const QString& key); - QStringList childGroups() const; - QStringList childKeys() const; + + // These are not currently being used + // QStringList childGroups() const; + // QStringList childKeys() const; + QStringList allKeys() const; bool contains(const QString& key) const; int beginReadArray(const QString & prefix); @@ -61,33 +111,108 @@ public: void getQuatValueIfValid(const QString& name, glm::quat& quatValue); private: + QString getGroupPrefix() const; + QString getPath(const QString &value) const; + QSharedPointer _manager; + QStack _groups; + QString _groupPrefix; }; namespace Setting { + + /** + * @brief Handle to a setting of type T. + * + * This creates an object that manipulates a setting in the settings system. Changes will be written + * to the configuration file at some point controlled by Setting::Manager. + * + * @tparam T Type of the setting. + */ template class Handle : public Interface { public: + + /** + * @brief Construct handle to a setting + * + * @param key The key corresponding to the setting in the settings file. Eg, 'shadowsEnabled' + */ Handle(const QString& key) : Interface(key) {} + + /** + * @brief Construct a handle to a setting + * + * @param path Path to the key corresponding to the setting in the settings file. Eg, QStringList() << group << key + */ Handle(const QStringList& path) : Interface(path.join("/")) {} + /** + * @brief Construct handle to a setting with a default value + * + * @param key The key corresponding to the setting in the settings file. Eg, 'shadowsEnabled' + * @param defaultValue Default value for this setting + */ Handle(const QString& key, const T& defaultValue) : Interface(key), _defaultValue(defaultValue) {} + + /** + * @brief Construct a handle to a setting with a default value + * + * @param path Path to the key corresponding to the setting in the settings file. Eg, QStringList() << group << key + * @param defaultValue Default value for this setting + */ Handle(const QStringList& path, const T& defaultValue) : Handle(path.join("/"), defaultValue) {} + /** + * @brief Construct a handle to a deprecated setting + * + * If used, a warning will written to the log. + * + * @param key The key corresponding to the setting in the settings file. Eg, 'shadowsEnabled' + * @return Handle The handle object + */ static Handle Deprecated(const QString& key) { Handle handle = Handle(key); handle.deprecate(); return handle; } + + /** + * @brief Construct a handle to a deprecated setting + * + * If used, a warning will written to the log. + * + * @param path Path to the key corresponding to the setting in the settings file. Eg, QStringList() << group << key + * @return Handle The handle object + */ static Handle Deprecated(const QStringList& path) { return Deprecated(path.join("/")); } + /** + * @brief Construct a handle to a deprecated setting with a default value + * + * If used, a warning will written to the log. + * + * @param key The key corresponding to the setting in the settings file. Eg, 'shadowsEnabled' + * @param defaultValue Default value for this setting + * @return Handle The handle object + */ static Handle Deprecated(const QString& key, const T& defaultValue) { Handle handle = Handle(key, defaultValue); handle.deprecate(); return handle; } + + /** + * @brief Construct a handle to a deprecated setting with a default value + * + * If used, a warning will written to the log. + * + * @param path Path to the key corresponding to the setting in the settings file. Eg, QStringList() << group << key + * @param defaultValue Default value for this setting + * @return Handle The handle object + */ static Handle Deprecated(const QStringList& path, const T& defaultValue) { return Deprecated(path.join("/"), defaultValue); } @@ -96,32 +221,66 @@ namespace Setting { deinit(); } - // Returns setting value, returns its default value if not found + /** + * @brief Returns the value of the setting, or the default value if not found + * + * @return T Value of the associated setting + */ T get() const { return get(_defaultValue); } - // Returns setting value, returns other if not found + /** + * @brief Returns the value of the setting, or 'other' if not found. + * + * @param other Value to return if the setting is not set + * @return T Value of the associated setting + */ T get(const T& other) const { maybeInit(); return (_isSet) ? _value : other; } + /** + * @brief Returns whether the setting is set to a value + * + * @return true The setting has a value + * @return false The setting has no value + */ bool isSet() const { maybeInit(); return _isSet; } + /** + * @brief Returns the default value for this setting + * + * @return const T& Default value for this setting + */ const T& getDefault() const { return _defaultValue; } + /** + * @brief Sets the value to the default + * + */ void reset() { set(_defaultValue); } + /** + * @brief Set the setting to the specified value. + * + * The value will be stored in the configuration file. + * + * @param value Value to set the setting to. + */ void set(const T& value) { maybeInit(); + + // qCDebug(settings_handle) << "Setting" << this->getKey() << "to" << value; + if ((!_isSet && (value != _defaultValue)) || _value != value) { _value = value; _isSet = true; @@ -132,6 +291,12 @@ namespace Setting { } } + /** + * @brief Remove the value from the setting + * + * This returns the setting to an unset state. If read, it will be read as the default value. + * + */ void remove() { maybeInit(); if (_isSet) { @@ -148,7 +313,7 @@ namespace Setting { void deprecate() { if (_isSet) { if (get() != getDefault()) { - qInfo().nospace() << "[DEPRECATION NOTICE] " << _key << "(" << get() << ") has been deprecated, and has no effect"; + qCInfo(settings_handle).nospace() << "[DEPRECATION NOTICE] " << _key << "(" << get() << ") has been deprecated, and has no effect"; } else { remove(); } 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 ab5b3e380a..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 @@ -23,23 +24,15 @@ #include "SharedUtil.h" #include "ThreadHelpers.h" +Q_LOGGING_CATEGORY(settings_interface, "settings.interface") + namespace Setting { // This should only run as a post-routine in the QCoreApplication destructor void cleanupSettingsSaveThread() { auto globalManager = DependencyManager::get(); 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->terminateThread(); qCDebug(shared) << "Settings thread stopped."; } @@ -48,37 +41,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 +77,7 @@ namespace Setting { // in an assignment-client - the QSettings backing we use for this means persistence of these // settings from an AC (when there can be multiple terminating at same time on one machine) // is currently not supported - qWarning() << "Setting::Interface::init() for key" << _key << "- Manager not yet created." << + qCWarning(settings_interface) << "Setting::Interface::init() for key" << _key << "- Manager not yet created." << "Settings persistence disabled."; } else { _manager = DependencyManager::get(); @@ -125,11 +87,12 @@ namespace Setting { manager->registerHandle(this); _isInitialized = true; } else { - qWarning() << "Settings interface used after manager destroyed"; + qCWarning(settings_interface) << "Settings interface used after manager destroyed"; } - + // Load value from disk load(); + //qCDebug(settings_interface) << "Setting" << this->getKey() << "initialized to" << getVariant(); } } @@ -144,20 +107,21 @@ namespace Setting { } } - + void Interface::maybeInit() const { if (!_isInitialized) { + //qCDebug(settings_interface) << "Initializing setting" << this->getKey(); const_cast(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/SettingInterface.h b/libraries/shared/src/SettingInterface.h index 575641c0e7..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 @@ -16,6 +17,9 @@ #include #include #include +#include + +Q_DECLARE_LOGGING_CATEGORY(settings_interface) namespace Setting { class Manager; @@ -25,11 +29,11 @@ namespace Setting { class Interface { public: const QString& getKey() const { return _key; } - bool isSet() const { return _isSet; } + bool isSet() const { return _isSet; } virtual void setVariant(const QVariant& variant) = 0; virtual QVariant getVariant() = 0; - + protected: Interface(const QString& key) : _key(key) {} virtual ~Interface() = default; @@ -37,7 +41,7 @@ namespace Setting { void init(); void maybeInit() const; void deinit(); - + void save(); void load(); @@ -46,7 +50,7 @@ namespace Setting { private: mutable bool _isInitialized = false; - + friend class Manager; mutable QWeakPointer _manager; }; diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp index e5920b785e..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 @@ -17,13 +18,89 @@ #include "SettingInterface.h" +Q_LOGGING_CATEGORY(settings_manager, "settings.manager") +Q_LOGGING_CATEGORY(settings_writer, "settings.manager.writer") + namespace Setting { + + void WriteWorker::start() { + // QSettings seems to have some issues with moving to a new thread. + // Make sure the object is only created once the thread is already up and running. + init(); + } + + void WriteWorker::setValue(const QString key, const QVariant value) { + //qCDebug(settings_writer) << "Setting config " << key << "to" << value; + + init(); + + if (!_qSettings->contains(key) || _qSettings->value(key) != value) { + _qSettings->setValue(key, value); + } + } + + void WriteWorker::removeKey(const QString key) { + init(); + _qSettings->remove(key); + } + + void WriteWorker::sync() { + //qCDebug(settings_writer) << "Forcing settings sync"; + init(); + _qSettings->sync(); + } + + void WriteWorker::threadFinished() { + qCDebug(settings_writer) << "Settings write worker syncing and terminating"; + sync(); + this->deleteLater(); + } + + void WriteWorker::terminate() { + qCDebug(settings_writer) << "Settings write worker being asked to terminate. Syncing and terminating."; + sync(); + this->deleteLater(); + QThread::currentThread()->exit(0); + } + + Manager::Manager(QObject *parent) { + WriteWorker *worker = new WriteWorker(); + + // We operate purely from memory, and forward all changes to a thread that has writing the + // settings as its only job. + + qCDebug(settings_manager) << "Initializing settings write thread"; + + _workerThread.setObjectName("Settings Writer"); + worker->moveToThread(&_workerThread); + // connect(&_workerThread, &QThread::started, worker, &WriteWorker::start, Qt::QueuedConnection); + + // All normal connections are queued, so that we're sure they happen asynchronously. + connect(&_workerThread, &QThread::finished, worker, &WriteWorker::threadFinished, Qt::QueuedConnection); + connect(this, &Manager::valueChanged, worker, &WriteWorker::setValue, Qt::QueuedConnection); + connect(this, &Manager::keyRemoved, worker, &WriteWorker::removeKey, Qt::QueuedConnection); + connect(this, &Manager::syncRequested, worker, &WriteWorker::sync, Qt::QueuedConnection); + + // This one is blocking because we want to wait until it's actually processed. + connect(this, &Manager::terminationRequested, worker, &WriteWorker::terminate, Qt::BlockingQueuedConnection); + + + _workerThread.start(); + + // Load all current settings + QSettings settings; + _fileName = settings.fileName(); + + for(QString key : settings.allKeys()) { + //qCDebug(settings_manager) << "Loaded key" << key << "with value" << settings.value(key); + _settings[key] = settings.value(key); + } + } + + Manager::~Manager() { - // Cleanup timer - stopTimer(); - delete _saveTimer; - _saveTimer = nullptr; + } // Custom deleter does nothing, because we need to shutdown later than the dependency manager @@ -33,7 +110,7 @@ namespace Setting { const QString& key = handle->getKey(); withWriteLock([&] { if (_handles.contains(key)) { - qWarning() << "Setting::Manager::registerHandle(): Key registered more than once, overriding: " << key; + qCWarning(settings_manager) << "Setting::Manager::registerHandle(): Key registered more than once, overriding: " << key; } _handles.insert(key, handle); }); @@ -47,13 +124,10 @@ namespace Setting { void Manager::loadSetting(Interface* handle) { const auto& key = handle->getKey(); + withWriteLock([&] { - QVariant loadedValue; - 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); } @@ -63,144 +137,76 @@ namespace Setting { void Manager::saveSetting(Interface* handle) { const auto& key = handle->getKey(); - QVariant handleValue = UNSET_VALUE; + if (handle->isSet()) { - handleValue = handle->getVariant(); + QVariant handleValue = handle->getVariant(); + + withWriteLock([&] { + _settings[key] = handleValue; + }); + + emit valueChanged(key, handleValue); + } else { + withWriteLock([&] { + _settings.remove(key); + }); + + emit keyRemoved(key); } - withWriteLock([&] { - _pendingChanges[key] = handleValue; - }); } - static const int SAVE_INTERVAL_MSEC = 5 * 1000; // 5 sec - void Manager::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::forceSave() { + emit syncRequested(); } - void Manager::stopTimer() { - if (_saveTimer) { - _saveTimer->stop(); - } - } + void Manager::terminateThread() { + qCDebug(settings_manager) << "Terminating settings writer thread"; - 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(); + emit terminationRequested(); // This blocks - if (forceSync) { - _qSettings.sync(); - } - }); - - // Restart timer - if (_saveTimer) { - _saveTimer->start(); - } + _workerThread.exit(); + _workerThread.wait(THREAD_TERMINATION_TIMEOUT); + qCDebug(settings_manager) << "Settings writer terminated"; } 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 49f3bece4b..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 @@ -17,35 +18,169 @@ #include #include +#include +#include +#include + #include "DependencyManager.h" #include "shared/ReadWriteLockable.h" +Q_DECLARE_LOGGING_CATEGORY(settings_manager) +Q_DECLARE_LOGGING_CATEGORY(settings_writer) + +// This is for the testing system. +class SettingsTests; +class SettingsTestsWorker; + + + namespace Setting { class Interface; + /** + * @brief Settings write worker + * + * This class is used by Setting::Manager to write settings to permanent storage + * without blocking anything else. It receives setting updates, and writes them + * to disk whenever convenient. + * + * All communication to this class must be done over queued connections. + * + * This class is purely an implementation detail and shouldn't be used outside of Setting::Manager. + * + */ + class WriteWorker : public QObject { + Q_OBJECT + + public slots: + + /** + * @brief Initialize anything that needs initializing, called on thread start. + * + */ + void start(); + + /** + * @brief Sets a configuration value + * + * @param key Configuration key + * @param value Configuration value + */ + void setValue(const QString key, const QVariant value); + + /** + * @brief Remove a value from the configuration + * + * @param key Key to remove + */ + void removeKey(const QString key); + + /** + * @brief Force writing the config to disk + * + */ + void sync(); + + /** + * @brief Called when the thread is terminating + * + */ + void threadFinished(); + + /** + * @brief Thread is being asked to finish work and quit + * + */ + void terminate(); + + private: + + void init() { + if (!_qSettings) { + _qSettings = new QSettings(); + } + } + + QSettings* _qSettings = nullptr; + }; + + /** + * @brief Settings manager + * + * This class is the main implementation of the settings system, and the container + * of the current configuration. + * + * Most users should either use the Setting::Handle or the Settings classes instead, + * both of which talk to the single global instance of this class. + * + * The class is thread-safe, and delegates config writing to a separate thread. It + * is safe to change settings as often as it might be needed. + * + */ class Manager : public QObject, public ReadWriteLockable, public Dependency { Q_OBJECT public: + Manager(QObject *parent = nullptr); + void customDeleter() override; - // thread-safe proxies into QSettings + /** + * @brief Returns the filename where the config file will be written + * + * @return QString Path to the config file + */ QString fileName() const; + + /** + * @brief Remove a configuration key + * + * @param key Key to remove + */ void remove(const QString &key); - QStringList childGroups() const; - QStringList childKeys() const; + + /** + * @brief Lists all keys in the configuration + * + * @return QStringList List of keys + */ QStringList allKeys() const; + + /** + * @brief Returns whether a key is part of the configuration + * + * @param key Key to look for + * @return true Key is in the configuration + * @return false Key isn't in the configuration + */ bool contains(const QString &key) const; - int beginReadArray(const QString &prefix); - void beginGroup(const QString &prefix); - void beginWriteArray(const QString &prefix, int size = -1); - void endArray(); - void endGroup(); - void setArrayIndex(int i); + + /** + * @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); @@ -53,24 +188,41 @@ namespace Setting { void loadSetting(Interface* handle); void saveSetting(Interface* handle); - private slots: - void startTimer(); - void stopTimer(); - void saveAll(); + /** + * @brief Force saving the config to disk. + * + * Normally unnecessary to use. Asynchronous. + */ + void forceSave(); + + /** + * @brief Write config to disk and terminate the writer thread + * + * This is part of the shutdown process. + */ + void terminateThread(); + + signals: + void valueChanged(const QString key, QVariant value); + void keyRemoved(const QString key); + void syncRequested(); + void terminationRequested(); private: QHash _handles; - QPointer _saveTimer = nullptr; - const QVariant UNSET_VALUE { QUuid::createUuid() }; - QHash _pendingChanges; friend class Interface; + friend class ::SettingsTests; + friend class ::SettingsTestsWorker; + friend void cleanupSettingsSaveThread(); friend void setupSettingsSaveThread(); - QSettings _qSettings; + QHash _settings; + QString _fileName; + QThread _workerThread; }; } 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) diff --git a/tests/shared/src/SettingsTests.cpp b/tests/shared/src/SettingsTests.cpp new file mode 100644 index 0000000000..340d01f1d5 --- /dev/null +++ b/tests/shared/src/SettingsTests.cpp @@ -0,0 +1,226 @@ +// +// Created by Dale Glass on 2022/10/22 +// Copyright 2022 Overte e.V. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + + +#include "SettingsTests.h" + +#include +#include +#include +#include +#include +#include +#include "SettingInterface.h" +#include "SettingHandle.h" + + + +QTEST_MAIN(SettingsTests) + +void SettingsTestsWorker::saveSettings() { + auto sm = DependencyManager::get(); + QThread *thread = QThread::currentThread(); + + while(! thread->isInterruptionRequested() ) { + //qDebug() << "Thread is saving config"; + sm->forceSave(); + + // Not having any wait here for some reason locks up the benchmark. + // Logging a message also does the trick. + // + // This looks like a bug somewhere and needs investigating. + thread->yieldCurrentThread(); + } + + thread->exit(0); +} + +void SettingsTests::initTestCase() { + QCoreApplication::setOrganizationName("OverteTest"); + + DependencyManager::set(); + + Setting::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(); + sm->setValue("TestValueSM", "Hello"); + + // There seems to be a bug here, data gets lost without this call here. + sm->forceSave(); + qDebug() << "Wrote" << s.fileName(); +} + +void SettingsTests::testSettings() { + auto sm = DependencyManager::get(); + Settings s; + + s.setValue("settingsTest", 1); + QVERIFY(sm->value("settingsTest") == 1); + + QVERIFY(!sm->contains("nonExistingKey")); + QVERIFY(sm->value("nonExistingKey") == QVariant()); + +} + +void SettingsTests::testGroups() { + auto sm = DependencyManager::get(); + 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::testHandleUnused() { + + { + Setting::Handle testHandle("unused_handle", -1); + } +} + +void SettingsTests::testHandle() { + auto sm = DependencyManager::get(); + Setting::Handle 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(); + int i = 0; + + QBENCHMARK { + sm->setValue("BenchmarkSetValue", ++i); + } + +} + + +void SettingsTests::benchmarkSaveSettings() { + auto sm = DependencyManager::get(); + int i = 0; + + QBENCHMARK { + sm->setValue("BenchmarkSave", ++i); + sm->forceSave(); + } + +} + + +void SettingsTests::benchmarkSetValueConcurrent() { + auto sm = DependencyManager::get(); + int i = 0; + + _settingsThread = new QThread(qApp); + _testWorker = new SettingsTestsWorker(); + + _settingsThread->setObjectName("Save thread"); + _testWorker->moveToThread(_settingsThread); + + QObject::connect(_settingsThread, &QThread::started, _testWorker, &SettingsTestsWorker::saveSettings, Qt::QueuedConnection ); + + _settingsThread->start(); + QBENCHMARK { + sm->setValue("BenchmarkSetValueConcurrent", ++i); + } + + _settingsThread->requestInterruption(); + _settingsThread->wait(); + + delete _testWorker; + delete _settingsThread; +} + diff --git a/tests/shared/src/SettingsTests.h b/tests/shared/src/SettingsTests.h new file mode 100644 index 0000000000..29b92f9377 --- /dev/null +++ b/tests/shared/src/SettingsTests.h @@ -0,0 +1,53 @@ +// +// Created by Dale Glass 2022/10/22 +// Copyright 2022 Overte e.V. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +#ifndef overte_SettingsTests_h +#define overte_SettingsTests_h + +#include + + + + +class SettingsTestsWorker : public QObject { + Q_OBJECT + +public slots: + void saveSettings(); +}; + +class SettingsTests : public QObject { + Q_OBJECT + +private slots: + void initTestCase(); + void loadSettings(); + void saveSettings(); + + void testSettings(); + void testGroups(); + void testArray(); + void testArrayInGroup(); + + void testHandleUnused(); + void testHandle(); + + + void benchmarkSetValue(); + void benchmarkSaveSettings(); + void benchmarkSetValueConcurrent(); + + void cleanupTestCase(); + +private: + QThread *_settingsThread = nullptr; + SettingsTestsWorker *_testWorker = nullptr; + +}; + +#endif // overte_SettingsTests_h