From 19dbb17fb9a0bae714e23a31deedfd3d60069425 Mon Sep 17 00:00:00 2001 From: RickyRister <42636155+RickyRister@users.noreply.github.com> Date: Tue, 5 May 2026 12:03:01 -0700 Subject: [PATCH] [SettingsManager] Properly handle multithreaded access again (#6844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [SettingsManager] Properly handle multithreaded access again * Add comment * Add batch write function --------- Co-authored-by: Lukas Brübach --- .../client/settings/card_counter_settings.cpp | 4 ++- .../card/database/card_database.h | 5 +++ .../libcockatrice/card/set/card_set.h | 9 +++++ .../interface_card_set_priority_controller.h | 8 +++++ .../noop_card_set_priority_controller.h | 3 ++ .../database/card_set/card_sets_model.cpp | 17 ++++++---- .../settings/card_database_settings.cpp | 14 ++++++++ .../settings/card_database_settings.h | 2 ++ .../settings/settings_manager.cpp | 34 +++++++++++++++++-- .../libcockatrice/settings/settings_manager.h | 4 ++- 10 files changed, 88 insertions(+), 12 deletions(-) diff --git a/cockatrice/src/client/settings/card_counter_settings.cpp b/cockatrice/src/client/settings/card_counter_settings.cpp index 71ce4cfc6..399365c99 100644 --- a/cockatrice/src/client/settings/card_counter_settings.cpp +++ b/cockatrice/src/client/settings/card_counter_settings.cpp @@ -11,6 +11,8 @@ CardCounterSettings::CardCounterSettings(const QString &settingsPath, QObject *p void CardCounterSettings::setColor(int counterId, const QColor &color) { + QSettings settings = getSettings(); + QString key = QString("cards/counters/%1/color").arg(counterId); if (settings.value(key).value() == color) @@ -36,7 +38,7 @@ QColor CardCounterSettings::color(int counterId) const defaultColor = QColor::fromHsv(h, s, v); } - return settings.value(QString("cards/counters/%1/color").arg(counterId), defaultColor).value(); + return getSettings().value(QString("cards/counters/%1/color").arg(counterId), defaultColor).value(); } QString CardCounterSettings::displayName(int counterId) const diff --git a/libcockatrice_card/libcockatrice/card/database/card_database.h b/libcockatrice_card/libcockatrice/card/database/card_database.h index fc6958525..b1af3a2c0 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database.h +++ b/libcockatrice_card/libcockatrice/card/database/card_database.h @@ -130,6 +130,11 @@ public: /** @brief Notifies listeners that enabled sets changed. */ void notifyEnabledSetsChanged(); + ICardSetPriorityController *getPriorityController() + { + return setPriorityController; + }; + public slots: /** * @brief Adds a card to the database. diff --git a/libcockatrice_card/libcockatrice/card/set/card_set.h b/libcockatrice_card/libcockatrice/card/set/card_set.h index fe4b66522..2cedb597e 100644 --- a/libcockatrice_card/libcockatrice/card/set/card_set.h +++ b/libcockatrice_card/libcockatrice/card/set/card_set.h @@ -181,6 +181,15 @@ public: */ void loadSetOptions(); + void setSortKeyInMemory(unsigned int _sortKey) + { + sortKey = _sortKey; + } + void setEnabledInMemory(bool _enabled) + { + enabled = _enabled; + } + /// @return The sort key assigned to this set. [[nodiscard]] int getSortKey() const { diff --git a/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h b/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h index b8fbbc74a..333190015 100644 --- a/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h +++ b/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h @@ -6,6 +6,12 @@ class ICardSetPriorityController { public: + struct SetSaveData { + QString shortName; + unsigned int sortKey; + bool enabled; + }; + virtual ~ICardSetPriorityController() = default; virtual void setSortKey(QString shortName, unsigned int sortKey) = 0; @@ -15,6 +21,8 @@ public: virtual unsigned int getSortKey(QString shortName) const = 0; virtual bool isEnabled(QString shortName) const = 0; virtual bool isKnown(QString shortName) const = 0; + + virtual void saveSets(const QVector &data) = 0; }; #endif // COCKATRICE_INTERFACE_CARD_SET_PRIORITY_CONTROLLER_H diff --git a/libcockatrice_interfaces/libcockatrice/interfaces/noop_card_set_priority_controller.h b/libcockatrice_interfaces/libcockatrice/interfaces/noop_card_set_priority_controller.h index e5027648c..16fc4c19a 100644 --- a/libcockatrice_interfaces/libcockatrice/interfaces/noop_card_set_priority_controller.h +++ b/libcockatrice_interfaces/libcockatrice/interfaces/noop_card_set_priority_controller.h @@ -28,6 +28,9 @@ public: { return true; } + + void saveSets(const QVector & /* data */) override { + }; }; #endif // COCKATRICE_NOOP_CARD_SET_PRIORITY_CONTROLLER_H diff --git a/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp b/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp index f6dc4f9cf..c0164ad75 100644 --- a/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp +++ b/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp @@ -228,16 +228,19 @@ void SetsModel::sort(int column, Qt::SortOrder order) void SetsModel::save(CardDatabase *db) { - // order - for (int i = 0; i < sets.size(); i++) - sets[i]->setSortKey(static_cast(i + 1)); + QVector saveData; + saveData.reserve(sets.size()); - // enabled sets - for (const CardSetPtr &set : sets) - set->setEnabled(enabledSets.contains(set)); + for (int i = 0; i < sets.size(); ++i) { + const unsigned int sortKey = static_cast(i + 1); + const bool enabled = enabledSets.contains(sets[i]); + sets[i]->setSortKeyInMemory(sortKey); + sets[i]->setEnabledInMemory(enabled); + saveData.append({sets[i]->getShortName(), sortKey, enabled}); + } + db->getPriorityController()->saveSets(saveData); sets.sortByKey(); - db->notifyEnabledSetsChanged(); } diff --git a/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp b/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp index 26a91a4dd..4887afd2f 100644 --- a/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp @@ -34,3 +34,17 @@ bool CardDatabaseSettings::isKnown(QString shortName) const { return getValue("isknown", "sets", std::move(shortName)).toBool(); } + +void CardDatabaseSettings::saveSets(const QVector &data) +{ + batchWrite([&](QSettings &s) { + s.beginGroup("sets"); + for (const auto &entry : data) { + s.beginGroup(entry.shortName); + s.setValue("sortkey", entry.sortKey); + s.setValue("enabled", entry.enabled); + s.endGroup(); + } + s.endGroup(); + }); +} \ No newline at end of file diff --git a/libcockatrice_settings/libcockatrice/settings/card_database_settings.h b/libcockatrice_settings/libcockatrice/settings/card_database_settings.h index bb946ea80..5dea1b552 100644 --- a/libcockatrice_settings/libcockatrice/settings/card_database_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/card_database_settings.h @@ -26,6 +26,8 @@ public: bool isEnabled(QString shortName) const override; bool isKnown(QString shortName) const override; + void saveSets(const QVector &data) override; + private: explicit CardDatabaseSettings(const QString &settingPath, QObject *parent = nullptr); }; diff --git a/libcockatrice_settings/libcockatrice/settings/settings_manager.cpp b/libcockatrice_settings/libcockatrice/settings/settings_manager.cpp index bb7d7791b..b66edd630 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_manager.cpp +++ b/libcockatrice_settings/libcockatrice/settings/settings_manager.cpp @@ -4,13 +4,20 @@ SettingsManager::SettingsManager(const QString &_settingPath, const QString &_defaultGroup, const QString &_defaultSubGroup, QObject *parent) - : QObject(parent), defaultGroup(_defaultGroup), defaultSubGroup(_defaultSubGroup), - settings(_settingPath, QSettings::IniFormat) + : QObject(parent), settingPath(_settingPath), defaultGroup(_defaultGroup), defaultSubGroup(_defaultSubGroup) { } +QSettings SettingsManager::getSettings() const +{ + // Do not store the QSettings instance in a field, as that is not threadsafe (see #6747) + return QSettings(settingPath, QSettings::IniFormat); +} + void SettingsManager::setValue(const QVariant &value, const QString &name) { + auto settings = getSettings(); + if (!defaultGroup.isEmpty()) { settings.beginGroup(defaultGroup); } @@ -35,6 +42,8 @@ void SettingsManager::setValue(const QVariant &value, const QString &group, const QString &subGroup) { + auto settings = getSettings(); + if (!group.isEmpty()) { settings.beginGroup(group); } @@ -56,6 +65,8 @@ void SettingsManager::setValue(const QVariant &value, void SettingsManager::deleteValue(const QString &name) { + auto settings = getSettings(); + if (!defaultGroup.isEmpty()) { settings.beginGroup(defaultGroup); } @@ -77,6 +88,8 @@ void SettingsManager::deleteValue(const QString &name) void SettingsManager::deleteValue(const QString &name, const QString &group, const QString &subGroup) { + auto settings = getSettings(); + if (!group.isEmpty()) { settings.beginGroup(group); } @@ -98,6 +111,8 @@ void SettingsManager::deleteValue(const QString &name, const QString &group, con QVariant SettingsManager::getValue(const QString &name) const { + auto settings = getSettings(); + if (!defaultGroup.isEmpty()) { settings.beginGroup(defaultGroup); } @@ -121,6 +136,8 @@ QVariant SettingsManager::getValue(const QString &name) const QVariant SettingsManager::getValue(const QString &name, const QString &group, const QString &subGroup) const { + auto settings = getSettings(); + if (!group.isEmpty()) { settings.beginGroup(group); } @@ -142,10 +159,21 @@ QVariant SettingsManager::getValue(const QString &name, const QString &group, co return value; } +void SettingsManager::batchWrite(std::function batchWriteFunction) +{ + auto settings = getSettings(); + settings.setAtomicSyncRequired(false); + batchWriteFunction(settings); + settings.sync(); // single flush + settings.setAtomicSyncRequired(true); +} + /** * Calls sync on the underlying QSettings object */ void SettingsManager::sync() { + auto settings = getSettings(); + settings.sync(); -} \ No newline at end of file +} diff --git a/libcockatrice_settings/libcockatrice/settings/settings_manager.h b/libcockatrice_settings/libcockatrice/settings/settings_manager.h index 6e8fe7fdb..a02bafcac 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_manager.h +++ b/libcockatrice_settings/libcockatrice/settings/settings_manager.h @@ -23,14 +23,16 @@ public: QVariant getValue(const QString &name) const; QVariant getValue(const QString &name, const QString &group, const QString &subGroup = QString()) const; + void batchWrite(std::function batchWriteFunction); void sync(); protected: + QString settingPath; QString defaultGroup; QString defaultSubGroup; - mutable QSettings settings; + QSettings getSettings() const; void setValue(const QVariant &value, const QString &name);