Refactor SettingsStorage implementation

Remove redundant fragmentation of logic.

PR #17354.

src/base/bittorrent/statistics.cpp

@@ -97,7 +97,7 @@ void Statistics::save() const
         {u"AlltimeDL"_qs, (m_alltimeDL + m_sessionDL)},
         {u"AlltimeUL"_qs, (m_alltimeUL + m_sessionUL)}
     };
-    SettingsPtr settings = Profile::instance()->applicationSettings(u"qBittorrent-data"_qs);
+    std::unique_ptr<QSettings> settings = Profile::instance()->applicationSettings(u"qBittorrent-data"_qs);
     settings->setValue(u"Stats/AllStats"_qs, stats);
 
     m_lastUpdateTimer.start();
@@ -106,7 +106,7 @@ void Statistics::save() const
 
 void Statistics::load()
 {
-    const SettingsPtr s = Profile::instance()->applicationSettings(u"qBittorrent-data"_qs);
+    const std::unique_ptr<QSettings> s = Profile::instance()->applicationSettings(u"qBittorrent-data"_qs);
     const QVariantHash v = s->value(u"Stats/AllStats"_qs).toHash();
 
     m_alltimeDL = v[u"AlltimeDL"_qs].toLongLong();

src/base/profile.cpp

@@ -108,7 +108,7 @@ QString Profile::profileName() const
     return m_profileImpl->profileName();
 }
 
-SettingsPtr Profile::applicationSettings(const QString &name) const
+std::unique_ptr<QSettings> Profile::applicationSettings(const QString &name) const
 {
     return m_profileImpl->applicationSettings(name);
 }

src/base/profile.h

@@ -43,8 +43,6 @@ namespace Private
     class PathConverter;
 }
 
-using SettingsPtr = std::unique_ptr<QSettings>;
-
 enum class SpecialFolder
 {
     Cache,
@@ -62,7 +60,7 @@ public:
     static const Profile *instance();
 
     Path location(SpecialFolder folder) const;
-    SettingsPtr applicationSettings(const QString &name) const;
+    std::unique_ptr<QSettings> applicationSettings(const QString &name) const;
 
     Path rootPath() const;
     QString configurationName() const;

src/base/profile_p.cpp

@@ -113,12 +113,12 @@ Path Private::DefaultProfile::downloadLocation() const
     return Path(QStandardPaths::writableLocation(QStandardPaths::DownloadLocation));
 }
 
-SettingsPtr Private::DefaultProfile::applicationSettings(const QString &name) const
+std::unique_ptr<QSettings> Private::DefaultProfile::applicationSettings(const QString &name) const
 {
 #if defined(Q_OS_WIN) || defined(Q_OS_MACOS)
-    return SettingsPtr(new QSettings(QSettings::IniFormat, QSettings::UserScope, profileName(), name));
+    return std::unique_ptr<QSettings>(new QSettings(QSettings::IniFormat, QSettings::UserScope, profileName(), name));
 #else
-    return SettingsPtr(new QSettings(profileName(), name));
+    return std::unique_ptr<QSettings>(new QSettings(profileName(), name));
 #endif
 }
 
@@ -168,7 +168,7 @@ Path Private::CustomProfile::downloadLocation() const
     return m_downloadLocation;
 }
 
-SettingsPtr Private::CustomProfile::applicationSettings(const QString &name) const
+std::unique_ptr<QSettings> Private::CustomProfile::applicationSettings(const QString &name) const
 {
     // here we force QSettings::IniFormat format always because we need it to be portable across platforms
 #if defined(Q_OS_WIN) || defined(Q_OS_MACOS)
@@ -177,7 +177,7 @@ SettingsPtr Private::CustomProfile::applicationSettings(const QString &name) con
     const auto CONF_FILE_EXTENSION = u".conf"_qs;
 #endif
     const Path settingsFilePath = configLocation() / Path(name + CONF_FILE_EXTENSION);
-    return SettingsPtr(new QSettings(settingsFilePath.data(), QSettings::IniFormat));
+    return std::unique_ptr<QSettings>(new QSettings(settingsFilePath.data(), QSettings::IniFormat));
 }
 
 Path Private::NoConvertConverter::fromPortablePath(const Path &portablePath) const

src/base/profile_p.h

@@ -53,7 +53,7 @@ namespace Private
         virtual Path dataLocation() const = 0;
         virtual Path downloadLocation() const = 0;
 
-        virtual SettingsPtr applicationSettings(const QString &name) const = 0;
+        virtual std::unique_ptr<QSettings> applicationSettings(const QString &name) const = 0;
 
         QString configurationName() const;
 
@@ -83,7 +83,7 @@ namespace Private
         Path configLocation() const override;
         Path dataLocation() const override;
         Path downloadLocation() const override;
-        SettingsPtr applicationSettings(const QString &name) const override;
+        std::unique_ptr<QSettings> applicationSettings(const QString &name) const override;
 
     private:
         /**
@@ -107,7 +107,7 @@ namespace Private
         Path configLocation() const override;
         Path dataLocation() const override;
         Path downloadLocation() const override;
-        SettingsPtr applicationSettings(const QString &name) const override;
+        std::unique_ptr<QSettings> applicationSettings(const QString &name) const override;
 
     private:
         const Path m_rootPath;

src/base/rss/rss_autodownloader.cpp

@@ -453,7 +453,7 @@ void AutoDownloader::loadRules(const QByteArray &data)
 
 void AutoDownloader::loadRulesLegacy()
 {
-    const SettingsPtr settings = Profile::instance()->applicationSettings(u"qBittorrent-rss"_qs);
+    const std::unique_ptr<QSettings> settings = Profile::instance()->applicationSettings(u"qBittorrent-rss"_qs);
     const QVariantHash rules = settings->value(u"download_rules"_qs).toHash();
     for (const QVariant &ruleVar : rules)
     {

src/base/settingsstorage.cpp

@@ -43,39 +43,13 @@
 
 using namespace std::chrono_literals;
 
-namespace
-{
-    // Encapsulates serialization of settings in "atomic" way.
-    // write() does not leave half-written files,
-    // read() has a workaround for a case of power loss during a previous serialization
-    class TransactionalSettings
-    {
-    public:
-        explicit TransactionalSettings(const QString &name)
-            : m_name(name)
-        {
-        }
-
-        QVariantHash read() const;
-        bool write(const QVariantHash &data) const;
-
-    private:
-        // we return actual file names used by QSettings because
-        // there is no other way to get that name except
-        // actually create a QSettings object.
-        // if serialization operation was not successful we return empty string
-        Path deserialize(const QString &name, QVariantHash &data) const;
-        Path serialize(const QString &name, const QVariantHash &data) const;
-
-        const QString m_name;
-    };
-}
-
 SettingsStorage *SettingsStorage::m_instance = nullptr;
 
 SettingsStorage::SettingsStorage()
-    : m_data {TransactionalSettings(u"qBittorrent"_qs).read()}
+    : m_nativeSettingsName {u"qBittorrent"_qs}
 {
+    readNativeSettings();
+
     m_timer.setSingleShot(true);
     m_timer.setInterval(5s);
     connect(&m_timer, &QTimer::timeout, this, &SettingsStorage::save);
@@ -108,8 +82,7 @@ bool SettingsStorage::save()
     const QWriteLocker locker(&m_lock);  // guard for `m_dirty` too
     if (!m_dirty) return true;
 
-    const TransactionalSettings settings(u"qBittorrent"_qs);
-    if (!settings.write(m_data))
+    if (!writeNativeSettings())
     {
         m_timer.start();
         return false;
@@ -137,40 +110,40 @@ void SettingsStorage::storeValueImpl(const QString &key, const QVariant &value)
     }
 }
 
-void SettingsStorage::removeValue(const QString &key)
+void SettingsStorage::readNativeSettings()
 {
-    const QWriteLocker locker(&m_lock);
-#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
-    if (m_data.remove(key))
-#else
-    if (m_data.remove(key) > 0)
-#endif
+    // We return actual file names used by QSettings because
+    // there is no other way to get that name except actually create a QSettings object.
+    // If serialization operation was not successful we return empty string.
+    const auto deserialize = [](QVariantHash &data, const QString &nativeSettingsName) -> Path
     {
-        m_dirty = true;
-        m_timer.start();
-    }
-}
-
-bool SettingsStorage::hasKey(const QString &key) const
-{
-    const QReadLocker locker {&m_lock};
-    return m_data.contains(key);
-}
+        std::unique_ptr<QSettings> nativeSettings = Profile::instance()->applicationSettings(nativeSettingsName);
+        if (nativeSettings->allKeys().isEmpty())
+            return {};
+
+        // Copy everything into memory. This means even keys inserted in the file manually
+        // or that we don't touch directly in this code (eg disabled by ifdef). This ensures
+        // that they will be copied over when save our settings to disk.
+        for (const QString &key : asConst(nativeSettings->allKeys()))
+        {
+            const QVariant value = nativeSettings->value(key);
+            if (value.isValid())
+                data[key] = value;
+        }
 
-QVariantHash TransactionalSettings::read() const
-{
-    QVariantHash res;
+        return Path(nativeSettings->fileName());
+    };
 
-    const Path newPath = deserialize(m_name + u"_new", res);
+    const Path newPath = deserialize(m_data, (m_nativeSettingsName + u"_new"));
     if (!newPath.isEmpty())
-    { // "_new" file is NOT empty
+    {
+        // "_new" file is NOT empty
         // This means that the PC closed either due to power outage
         // or because the disk was full. In any case the settings weren't transferred
         // in their final position. So assume that qbittorrent_new.ini/qbittorrent_new.conf
         // contains the most recent settings.
-        LogMsg(QObject::tr("Detected unclean program exit. Using fallback file to restore settings: %1")
-                .arg(newPath.toString())
-            , Log::WARNING);
+        LogMsg(tr("Detected unclean program exit. Using fallback file to restore settings: %1")
+               .arg(newPath.toString()), Log::WARNING);
 
         QString finalPathStr = newPath.data();
         const int index = finalPathStr.lastIndexOf(u"_new", -1, Qt::CaseInsensitive);
@@ -182,26 +155,43 @@ QVariantHash TransactionalSettings::read() const
     }
     else
     {
-        deserialize(m_name, res);
+        deserialize(m_data, m_nativeSettingsName);
     }
-
-    return res;
 }
 
-bool TransactionalSettings::write(const QVariantHash &data) const
+bool SettingsStorage::writeNativeSettings() const
 {
+    std::unique_ptr<QSettings> nativeSettings = Profile::instance()->applicationSettings(m_nativeSettingsName + u"_new");
+
     // QSettings deletes the file before writing it out. This can result in problems
     // if the disk is full or a power outage occurs. Those events might occur
     // between deleting the file and recreating it. This is a safety measure.
     // Write everything to qBittorrent_new.ini/qBittorrent_new.conf and if it succeeds
     // replace qBittorrent.ini/qBittorrent.conf with it.
-    const Path newPath = serialize(m_name + u"_new", data);
-    if (newPath.isEmpty())
+    for (auto i = m_data.begin(); i != m_data.end(); ++i)
+        nativeSettings->setValue(i.key(), i.value());
+
+    nativeSettings->sync(); // Important to get error status
+
+    switch (nativeSettings->status())
     {
-        Utils::Fs::removeFile(newPath);
-        return false;
+    case QSettings::NoError:
+        break;
+    case QSettings::AccessError:
+        LogMsg(tr("An access error occurred while trying to write the configuration file."), Log::CRITICAL);
+        break;
+    case QSettings::FormatError:
+        LogMsg(tr("A format error occurred while trying to write the configuration file."), Log::CRITICAL);
+        break;
+    default:
+        LogMsg(tr("An unknown error occurred while trying to write the configuration file."), Log::CRITICAL);
+        break;
     }
 
+    if (nativeSettings->status() != QSettings::NoError)
+        return false;
+
+    const Path newPath {nativeSettings->fileName()};
     QString finalPathStr = newPath.data();
     const int index = finalPathStr.lastIndexOf(u"_new", -1, Qt::CaseInsensitive);
     finalPathStr.remove(index, 4);
@@ -211,47 +201,22 @@ bool TransactionalSettings::write(const QVariantHash &data) const
     return Utils::Fs::renameFile(newPath, finalPath);
 }
 
-Path TransactionalSettings::deserialize(const QString &name, QVariantHash &data) const
+void SettingsStorage::removeValue(const QString &key)
 {
-    SettingsPtr settings = Profile::instance()->applicationSettings(name);
-
-    if (settings->allKeys().isEmpty())
-        return {};
-
-    // Copy everything into memory. This means even keys inserted in the file manually
-    // or that we don't touch directly in this code (eg disabled by ifdef). This ensures
-    // that they will be copied over when save our settings to disk.
-    for (const QString &key : asConst(settings->allKeys()))
+    const QWriteLocker locker(&m_lock);
+#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
+    if (m_data.remove(key))
+#else
+    if (m_data.remove(key) > 0)
+#endif
     {
-        const QVariant value = settings->value(key);
-        if (value.isValid())
-            data[key] = value;
+        m_dirty = true;
+        m_timer.start();
     }
-
-    return Path(settings->fileName());
 }
 
-Path TransactionalSettings::serialize(const QString &name, const QVariantHash &data) const
+bool SettingsStorage::hasKey(const QString &key) const
 {
-    SettingsPtr settings = Profile::instance()->applicationSettings(name);
-    for (auto i = data.begin(); i != data.end(); ++i)
-        settings->setValue(i.key(), i.value());
-
-    settings->sync(); // Important to get error status
-
-    switch (settings->status())
-    {
-    case QSettings::NoError:
-        return Path(settings->fileName());
-    case QSettings::AccessError:
-        LogMsg(QObject::tr("An access error occurred while trying to write the configuration file."), Log::CRITICAL);
-        break;
-    case QSettings::FormatError:
-        LogMsg(QObject::tr("A format error occurred while trying to write the configuration file."), Log::CRITICAL);
-        break;
-    default:
-        LogMsg(QObject::tr("An unknown error occurred while trying to write the configuration file."), Log::CRITICAL);
-        break;
-    }
-    return {};
+    const QReadLocker locker {&m_lock};
+    return m_data.contains(key);
 }

src/base/settingsstorage.h

@@ -116,9 +116,12 @@ public slots:
 private:
     QVariant loadValueImpl(const QString &key, const QVariant &defaultValue = {}) const;
     void storeValueImpl(const QString &key, const QVariant &value);
+    void readNativeSettings();
+    bool writeNativeSettings() const;
 
     static SettingsStorage *m_instance;
 
+    const QString m_nativeSettingsName;
     bool m_dirty = false;
     QVariantHash m_data;
     QTimer m_timer;