From 72055e0dd8c67dc4dfbe246a49457170b1d1fcde Mon Sep 17 00:00:00 2001 From: Jean Do Date: Sun, 26 Nov 2023 06:50:28 -0400 Subject: [PATCH] Less frequent writes of settings.json to sd/flash * krux.pages.settings_page accesses singleton "store" instead of class "Store" * krux.pages.settings_page explicitely calls store.save_settings() when exiting * krux.settings.Store adds attribute ".dirty" boolean to track settings changes * krux.settings.Store adds method ".update_file_location()" to delete old file * krux.settings.Store.save_settings() method writes only if ".dirty" and file would be altered * adds new tests and adjusts existing tests. --- src/krux/pages/settings_page.py | 27 ++++++++++--- src/krux/settings.py | 62 ++++++++++++++++++----------- tests/test_settings.py | 70 +++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 28 deletions(-) diff --git a/src/krux/pages/settings_page.py b/src/krux/pages/settings_page.py index f77849e25..27a302c57 100644 --- a/src/krux/pages/settings_page.py +++ b/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 diff --git a/src/krux/settings.py b/src/krux/settings.py index fe7f49200..cd25a0672 100644 --- a/src/krux/settings.py +++ b/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 diff --git a/tests/test_settings.py b/tests/test_settings.py index 39e6ef90d..160b78db3 100644 --- a/tests/test_settings.py +++ b/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()