[SettingsManager] Properly handle multithreaded access again (#6844)

* [SettingsManager] Properly handle multithreaded access again

* Add comment

* Add batch write function

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

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<QColor>() == 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<QColor>();
+    return getSettings().value(QString("cards/counters/%1/color").arg(counterId), defaultColor).value<QColor>();
 }
 
 QString CardCounterSettings::displayName(int counterId) const

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.

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
     {

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<SetSaveData> &data) = 0;
 };
 
 #endif // COCKATRICE_INTERFACE_CARD_SET_PRIORITY_CONTROLLER_H

libcockatrice_interfaces/libcockatrice/interfaces/noop_card_set_priority_controller.h

@@ -28,6 +28,9 @@ public:
     {
         return true;
     }
+
+    void saveSets(const QVector<SetSaveData> & /* data */) override {
+    };
 };
 
 #endif // COCKATRICE_NOOP_CARD_SET_PRIORITY_CONTROLLER_H

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
+    QVector<ICardSetPriorityController::SetSaveData> saveData;
-    for (int i = 0; i < sets.size(); i++)
+    saveData.reserve(sets.size());
-        sets[i]->setSortKey(static_cast<unsigned int>(i + 1));
 
-    // enabled sets
+    for (int i = 0; i < sets.size(); ++i) {
-    for (const CardSetPtr &set : sets)
+        const unsigned int sortKey = static_cast<unsigned int>(i + 1);
-        set->setEnabled(enabledSets.contains(set));
+        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();
 }
 

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<ICardSetPriorityController::SetSaveData> &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();
+    });
+}

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<SetSaveData> &data) override;
+
 private:
     explicit CardDatabaseSettings(const QString &settingPath, QObject *parent = nullptr);
 };

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),
+    : QObject(parent), settingPath(_settingPath), defaultGroup(_defaultGroup), defaultSubGroup(_defaultSubGroup)
-      settings(_settingPath, QSettings::IniFormat)
 {
 }
 
+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<void(QSettings &)> 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();
 }

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<void(QSettings &)> batchWriteFunction);
 
     void sync();
 
 protected:
+    QString settingPath;
     QString defaultGroup;
     QString defaultSubGroup;
 
-    mutable QSettings settings;
+    QSettings getSettings() const;
 
     void setValue(const QVariant &value, const QString &name);