Less frequent writes of settings.json to sd/flash

src/krux/pages/settings_page.py

@@ -25,7 +25,7 @@
 from ..settings import (
     CategorySetting,
     NumberSetting,
-    Store,
+    store,
     SD_PATH,
     FLASH_PATH,
     SETTINGS_FILENAME,
@@ -230,14 +230,29 @@ def _settings_exit_check(self):
             try:
                 # Check for SD hot-plug
                 with SDHandler():
-                    Store.save_settings()
+                    if store.save_settings():
+                        self._display_centered_text(
+                            t("Changes persisted to SD card!"),
+                            duration=SD_MSG_TIME,
+                        )
+            except OSError:
+                self._display_centered_text(
+                    t("SD card not detected.")
+                    + "\n\n"
+                    + t("Changes will last until shutdown."),
+                    duration=SD_MSG_TIME,
+                )
+        else:
+            self.ctx.display.clear()
+            try:
+                if store.save_settings():
                     self._display_centered_text(
-                        t("Changes persisted to SD card!"),
+                        t("Changes persisted to Flash!"),
                         duration=SD_MSG_TIME,
                     )
-            except OSError:
+            except:
                 self._display_centered_text(
-                    t("SD card not detected.")
+                    t("Unexpected error saving to Flash.")
                     + "\n\n"
                     + t("Changes will last until shutdown."),
                     duration=SD_MSG_TIME,
@@ -370,7 +385,7 @@ def category_setting(self, settings_namespace, setting):
                 # Restore previous theme
                 setting.__set__(settings_namespace, starting_category)
                 theme.update()
-                Store.save_settings()
+                store.save_settings()
 
         return MENU_CONTINUE
 

src/krux/settings.py

@@ -75,6 +75,8 @@ def __get__(self, obj, objtype=None):
         return store.get(obj.namespace, self.attr, self.default_value)
 
     def __set__(self, obj, value):
+        if self.attr == "location":
+            store.update_file_location(value)
         store.set(obj.namespace, self.attr, value)
 
 
@@ -101,6 +103,7 @@ class Store:
     def __init__(self):
         self.settings = {}
         self.file_location = "/" + FLASH_PATH + "/"
+        self.dirty = False
 
         # Check for the correct settings persist location
         try:
@@ -144,39 +147,54 @@ def get(self, namespace, setting_name, default_value):
         return s[setting_name]
 
     def set(self, namespace, setting_name, setting_value):
-        """Stores a setting value under the given namespace. We don't use SDHandler
-        here because set is called too many times every time the user changes a setting
-        and SDHandler remount causes a small delay
+        """Stores a setting value under the given namespace if new/changed.
+        Does NOT automatically save settings to flash or sd!
         """
         s = self.settings
         for level in namespace.split("."):
             s[level] = s.get(level, {})
             s = s[level]
         old_value = s.get(setting_name, None)
-        s[setting_name] = setting_value
-
-        # if is a change in settings persist location, delete file from old location,
-        # and later it will save on the new location
-        if setting_name == "location" and old_value:
-            # update the file location
-            self.file_location = "/" + setting_value + "/"
+        if old_value != setting_value:
+            s[setting_name] = setting_value
+            self.dirty = True
+
+    def update_file_location(self, location):
+        """Assumes settings.persist.location will be changed to location:
+        tries to delete current persistent settings file
+        then updates file_location attribute
+        """
+        if "/" + location + "/" != self.file_location:
             try:
-                # remove old SETTINGS_FILENAME
-                os.remove("/" + old_value + "/" + SETTINGS_FILENAME)
+                if os.stat(self.file_location + SETTINGS_FILENAME):
+                    os.remove(self.file_location + SETTINGS_FILENAME)
             except:
                 pass
+            self.file_location = "/" + location + "/"
 
-        Store.save_settings()
-
-    @staticmethod
-    def save_settings():
+    def save_settings(self):
         """Helper to persist SETTINGS_FILENAME where user selected"""
-        try:
-            # save the new SETTINGS_FILENAME
-            with open(store.file_location + SETTINGS_FILENAME, "w") as f:
-                f.write(json.dumps(store.settings))
-        except:
-            pass
+        persisted = False
+
+        if self.dirty:
+            settings_filename = self.file_location + SETTINGS_FILENAME
+            new_contents = json.dumps(self.settings)
+            try:
+                with open(settings_filename, "r") as f:
+                    old_contents = f.read()
+            except:
+                old_contents = None
+
+            if new_contents != old_contents:
+                try:
+                    with open(settings_filename, "w") as f:
+                        f.write(new_contents)
+                    persisted = True
+                except:
+                    pass
+            self.dirty = False
+
+        return persisted
 
 
 # Initialize singleton

tests/test_settings.py

@@ -66,6 +66,9 @@ def test_store_set(mocker, m5stickv):
         ("ns1.ns2", "setting", "call1_value2", 2, "call3_value2"),
         ("ns1.ns2.ns3", "setting", "call1_value3", 3, "call3_value3"),
     ]
+
+    assert s.dirty == False
+
     for case in cases:
         s.set(case[0], case[1], case[2])
         assert s.get(case[0], case[1], "default") == case[2]
@@ -76,6 +79,73 @@ def test_store_set(mocker, m5stickv):
         s.set(case[0], case[1], case[4])
         assert s.get(case[0], case[1], "default") == case[4]
 
+    assert s.dirty == True
+
+
+def test_store_update_file_location(mocker):
+    ms, mr = mocker.Mock(), mocker.Mock()
+    mocker.patch("os.stat", ms)
+    mocker.patch("os.remove", mr)
+    from krux.settings import Store, SD_PATH, FLASH_PATH, SETTINGS_FILENAME
+
+    s = Store()
+
+    # default is /flash/
+    assert s.file_location == "/" + FLASH_PATH + "/"
+
+    # from flash to sd removes /flash/settings.json
+    s.update_file_location(SD_PATH)
+    assert s.file_location == "/" + SD_PATH + "/"
+    ms.assert_called_once_with("/" + FLASH_PATH + "/" + SETTINGS_FILENAME)
+    mr.assert_called_once_with("/" + FLASH_PATH + "/" + SETTINGS_FILENAME)
+    ms.reset_mock()
+    mr.reset_mock()
+
+    # from sd to flash removes /sd/settings.json
+    s.update_file_location(FLASH_PATH)
+    assert s.file_location == "/" + FLASH_PATH + "/"
+    ms.assert_called_once_with("/" + SD_PATH + "/" + SETTINGS_FILENAME)
+    mr.assert_called_once_with("/" + SD_PATH + "/" + SETTINGS_FILENAME)
+    ms.reset_mock()
+    mr.reset_mock()
+
+    # updating with existing file_location does nothing
+    s.update_file_location(FLASH_PATH)
+    assert s.file_location == "/" + FLASH_PATH + "/"
+    ms.assert_not_called()
+    mr.assert_not_called()
+
+
+def test_store_save_settings(mocker):
+    mo = mocker.mock_open()
+    mocker.patch("builtins.open", mo)
+    from krux.settings import Store, SETTINGS_FILENAME
+
+    s = Store()
+    filename = s.file_location + SETTINGS_FILENAME
+
+    # new setting change: is dirty, save_settings() persists, file written
+    s.set("name.space", "setting", "custom_value")
+    assert s.dirty == True
+    assert s.save_settings() == True
+    mo.assert_called_with(filename, "w")
+
+    # no setting change: not dirty, save_settings() doesn't persist, file not even read
+    mo.reset_mock()
+    assert s.dirty == False
+    assert s.save_settings() == False
+    mo.assert_not_called()
+
+    # settings changed to original: dirty but save_settings() doesn't persist, file read -- not written
+    mo = mocker.mock_open(read_data='{"name": {"space": {"setting": "custom_value"}}}')
+    mocker.patch("builtins.open", mo)
+    s.set("name.space", "setting", "new_custom_value")
+    s.set("name.space", "setting", "custom_value")
+    assert s.dirty == True
+    assert s.save_settings() == False
+    mo.assert_called_once()
+    assert s.dirty == False
+
 
 def test_setting(mocker, m5stickv):
     mo = mocker.mock_open()