move to enum, use clearer var names

Author: Cvolton

loader/include/Geode/loader/ModSettingsManager.hpp

@@ -16,8 +16,8 @@ namespace geode {
         friend class ::geode::Mod;
 
         void markRestartRequired();
-        void markDirty();
-        void unmarkDirty();
+        void queueSave();
+        void saveFinished();
 
     public:
         static ModSettingsManager* from(Mod* mod);
@@ -70,6 +70,6 @@ namespace geode {
          */
         void addDependant(Mod* mod);
 
-        bool dirty() const;
+        bool shouldSave() const;
     };
 }

loader/src/loader/ModImpl.cpp

@@ -167,15 +167,17 @@ VersionInfo Mod::Impl::getVersion() const {
 matjson::Value& Mod::Impl::getSaveContainer() {
     // saved value taken, so we need to save it every time
     // since we dont know what the caller will do with the container
-    m_savedTaken = true;
+    m_saveRequestState = SaveRequestState::SaveUntilExit;
 
     return m_saved;
 }
 
 matjson::Value& Mod::Impl::getSaveContainerTemp() {
     // saved value dirty - caller promises to get rid
     // of its ref before next save
-    m_savedDirty = true;
+    if(m_saveRequestState == SaveRequestState::Clean) {
+        m_saveRequestState = SaveRequestState::SaveOnce;
+    }
 
     return m_saved;
 }
@@ -232,11 +234,11 @@ Result<> Mod::Impl::loadData() {
             }
         } else {
             // this used to early return but skipping saved values is not great behavior here imo
-            m_settings->markDirty();
+            m_settings->queueSave();
             log::warn("Unable to load settings: {}", json.unwrapErr());
         }
     } else {
-        m_settings->markDirty();
+        m_settings->queueSave();
     }
 
     // Saved values
@@ -266,7 +268,7 @@ Result<> Mod::Impl::saveData() {
     }
 
     // ModSettingsManager keeps track of the whole savedata
-    if (m_settings->dirty()) {
+    if (m_settings->shouldSave()) {
         log::debug("Saving settings for mod {}", m_metadata.getID());
 
         matjson::Value json = m_settings->save();
@@ -278,22 +280,24 @@ Result<> Mod::Impl::saveData() {
         if (!res) {
             log::error("Unable to save settings: {}", res.unwrapErr());
         } else {
-            m_settings->unmarkDirty();
+            m_settings->saveFinished();
         }
     } else {
         // duplicated line to retain old expectations of saveData being called after json dump but before file write
         ModStateEvent(ModEventType::DataSaved, std::move(m_self)).send();
     }
 
-    if (m_savedTaken || m_savedDirty) {
+    if (m_saveRequestState != SaveRequestState::Clean) {
         log::debug("Saving values for mod {}", m_metadata.getID());
 
         auto res2 = utils::file::writeStringSafe(m_saveDirPath / "saved.json", m_saved.dump());
         if (!res2) {
             log::error("Unable to save values: {}", res2.unwrapErr());
         }
 
-        m_savedDirty = false;
+        if(m_saveRequestState == SaveRequestState::SaveOnce) {
+            m_saveRequestState = SaveRequestState::Clean;
+        }
     }
 
     return Ok();

loader/src/loader/ModImpl.hpp

@@ -8,6 +8,12 @@
 #include <Geode/loader/Signal.hpp>
 
 namespace geode {
+    enum class SaveRequestState {
+        Clean,
+        SaveOnce,
+        SaveUntilExit,
+    };
+
     class Mod::Impl {
     public:
         Mod* m_self;
@@ -53,12 +59,7 @@ namespace geode {
          * Whether the saved values need to be saved to disk one time
          * (container dirty)
          */
-        bool m_savedDirty = false;
-        /**
-         * Whether the saved values need to be always re-saved until game exit
-         * (container taken by a mod)
-         */
-        bool m_savedTaken = false;
+        SaveRequestState m_saveRequestState = SaveRequestState::Clean;
         /**
          * Setting values. This is behind unique_ptr for interior mutability
          */

loader/src/loader/ModSettingsManager.cpp

@@ -188,8 +188,7 @@ class ModSettingsManager::Impl final {
     // update this by calling saveSettingValueToSave
     matjson::Value savedata;
     bool restartRequired = false;
-    bool dirty = false;
-    bool taken = false;
+    SaveRequestState saveRequestState = SaveRequestState::Clean;
 
     bool loadSettingValueFromSave(std::string const& key) {
         if (this->savedata.contains(key) && this->settings.contains(key)) {
@@ -209,7 +208,9 @@ class ModSettingsManager::Impl final {
         else {
             if (!this->savedata.contains(key)) {
                 log::error("Unable to load setting '{}' for mod {} (not found in savedata)", key, this->modID);
-                dirty = true;
+                if(saveRequestState == SaveRequestState::Clean) {
+                    saveRequestState = SaveRequestState::SaveOnce;
+                }
             }
             return false;
         }
@@ -288,16 +289,20 @@ void ModSettingsManager::markRestartRequired() {
     m_impl->restartRequired = true;
 }
 
-void ModSettingsManager::markDirty() {
-    m_impl->dirty = true;
+void ModSettingsManager::queueSave() {
+    if(m_impl->saveRequestState == SaveRequestState::Clean) {
+        m_impl->saveRequestState = SaveRequestState::SaveOnce;
+    }
 }
 
-void ModSettingsManager::unmarkDirty() {
-    m_impl->dirty = false;
+void ModSettingsManager::saveFinished() {
+    if(m_impl->saveRequestState == SaveRequestState::SaveOnce) {
+        m_impl->saveRequestState = SaveRequestState::Clean;
+    }
 }
 
-bool ModSettingsManager::dirty() const {
-    return m_impl->dirty || m_impl->taken;
+bool ModSettingsManager::shouldSave() const {
+    return m_impl->saveRequestState != SaveRequestState::Clean;
 }
 
 Result<> ModSettingsManager::registerCustomSettingType(std::string_view type, SettingGenerator generator) {
@@ -334,7 +339,7 @@ Result<> ModSettingsManager::load(matjson::Value const& json) {
         for (auto const& [key, _] : m_impl->settings) {
             if (!json.contains(key)) {
                 log::error("Unable to load setting '{}' for mod {} (not found in savedata)", key, m_impl->modID);
-                m_impl->dirty = true;
+                this->queueSave();
                 break;
             }
         }
@@ -351,7 +356,7 @@ matjson::Value ModSettingsManager::save() {
 }
 
 matjson::Value& ModSettingsManager::getSaveData() {
-    m_impl->taken = true;
+    m_impl->saveRequestState = SaveRequestState::SaveUntilExit;
 
     return m_impl->savedata;
 }

loader/src/loader/SettingV3.cpp

@@ -579,7 +579,7 @@ void SettingV3::markChanged() {
         if (m_impl->requiresRestart) {
             manager->markRestartRequired();
         }
-        manager->markDirty();
+        manager->queueSave();
     }
     SettingChangedEventV3(this->getModID(), this->getKey()).send(shared_from_this());
 }

loader/src/ui/mods/settings/ModSettingsPopup.cpp

@@ -71,7 +71,7 @@ void ModSettingsPopup::updateState(SettingNode* invoker) {
 
     // frame delay for debounce (avoids repeating save for every changed setting)
     Loader::get()->queueInMainThread([mod = m_mod, manager] {
-        if (manager->dirty()) {
+        if (manager->shouldSave()) {
             (void) mod->saveData();
         }
     });