From 6e25e3f94bd6f46aaf3c3eda9974e60661f94db4 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Tue, 16 Dec 2025 18:08:22 -0600 Subject: [PATCH 1/6] UX changes to require a selection for multiselect settings --- src/seedsigner/views/settings_views.py | 38 +++++++++++++++++++++++++ tests/screenshot_generator/generator.py | 1 + tests/test_flows_settings.py | 28 ++++++++++++++---- 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/seedsigner/views/settings_views.py b/src/seedsigner/views/settings_views.py index cbf66e08..56141c20 100644 --- a/src/seedsigner/views/settings_views.py +++ b/src/seedsigner/views/settings_views.py @@ -204,6 +204,13 @@ class SettingsEntryUpdateSelectionView(View): ) if ret_value == RET_CODE__BACK_BUTTON: + if self.settings_entry.type == SettingsConstants.TYPE__MULTISELECT: + # After the user finishes toggling multiselect options, initial_value will + # have their final selections when they hit BACK to exit. All current + # multiselect settings require at least one option to be selected. + if not initial_value: + return Destination(SettingsSelectionRequiredWarningView, view_args={"attr_name": self.settings_entry.attr_name}) + if self.blocking_view: return Destination(self.blocking_view, clear_history=True) return settings_menu_view_destination @@ -261,6 +268,37 @@ class SettingsEntryUpdateSelectionView(View): +class SettingsSelectionRequiredWarningView(View): + def __init__(self, attr_name: str): + super().__init__() + self.settings_entry = SettingsDefinition.get_settings_entry(attr_name) + + + def run(self): + from seedsigner.gui.screens.screen import WarningScreen + + # TRANSLATOR_NOTE: Title of a warning dialog when configuring a setting that requires at least one option to be selected. + title = _("Selection Required") + + # TRANSLATOR_NOTE: The name of the setting being configured (e.g. "Script types") will be inserted. + text = _("At least one option must be selected for \"{}\".").format(self.settings_entry.display_name) + + # TRANSLATOR_NOTE: Text for the button that returns the user to the setting configuration screen. + button_text = _("Return to setting") + + self.run_screen( + WarningScreen, + title=title, + status_headline=None, + text=text, + button_data=[ButtonOption(button_text)], + show_back_button=False, + ) + + return Destination(SettingsEntryUpdateSelectionView, view_args=dict(attr_name=self.settings_entry.attr_name)) + + + class SettingsIngestSettingsQRView(View): def __init__(self, data: str): from seedsigner.hardware.microsd import MicroSD diff --git a/tests/screenshot_generator/generator.py b/tests/screenshot_generator/generator.py index 673086ff..69654196 100644 --- a/tests/screenshot_generator/generator.py +++ b/tests/screenshot_generator/generator.py @@ -427,6 +427,7 @@ def generate_screenshots(locale): ScreenshotConfig(settings_views.DonateView), ScreenshotConfig(settings_views.SettingsIngestSettingsQRView, dict(data=settingsqr_data_persistent), screenshot_name="SettingsIngestSettingsQRView_persistent"), ScreenshotConfig(settings_views.SettingsIngestSettingsQRView, dict(data=settingsqr_data_not_persistent), screenshot_name="SettingsIngestSettingsQRView_not_persistent"), + ScreenshotConfig(settings_views.SettingsSelectionRequiredWarningView, dict(attr_name=SettingsConstants.SETTING__SCRIPT_TYPES)), ], "Misc Error Views": [ ScreenshotConfig(NotYetImplementedView), diff --git a/tests/test_flows_settings.py b/tests/test_flows_settings.py index 75fc530b..ff73a3d7 100644 --- a/tests/test_flows_settings.py +++ b/tests/test_flows_settings.py @@ -37,17 +37,35 @@ class TestSettingsFlows(FlowTest): def test_multiselect(self): - """ Multiselect Settings options should stay in-place; requires BACK to exit. """ + """ + Multiselect Settings options should stay in-place; requires BACK to exit. If no + selections are made, route to the warning screen and return the user to the + settings entry until at least one option is selected. + """ # Which option are we testing? - settings_entry = SettingsDefinition.get_settings_entry(SettingsConstants.SETTING__XPUB_QR_FORMAT) + settings_entry = SettingsDefinition.get_settings_entry(SettingsConstants.SETTING__SIG_TYPES) + + # Enable all options to start + self.settings.set_value(settings_entry.attr_name, [option[0] for option in settings_entry.selection_options]) + + # Sanity check, we only expect two options for this setting + assert len(settings_entry.selection_options) == 2 self.run_sequence([ FlowStep(MainMenuView, button_data_selection=MainMenuView.SETTINGS), FlowStep(settings_views.SettingsMenuView, button_data_selection=settings_views.SettingsMenuView.ADVANCED), FlowStep(settings_views.SettingsMenuView, button_data_selection=ButtonOption(settings_entry.display_name)), - FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=0), # select/deselect first option - FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1), # select/deselect second option - FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1), # select/deselect second option + FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=0), # deselect first option + FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1), # deselect second option + FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1), # select second option + FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1), # deselect second option + FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=RET_CODE__BACK_BUTTON), # BACK to exit + + # Both options were deselected, should route to the warning screen + FlowStep(settings_views.SettingsSelectionRequiredWarningView), + FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=0), # select first option + + # Now we can exit FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=RET_CODE__BACK_BUTTON), # BACK to exit FlowStep(settings_views.SettingsMenuView), ]) From adaf3d1ffbfbf233790da4cd395c4dbeb29493df Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Tue, 16 Dec 2025 18:18:54 -0600 Subject: [PATCH 2/6] SettingsQR values cannot be blank --- src/seedsigner/models/settings.py | 5 +++++ tests/test_settings.py | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/src/seedsigner/models/settings.py b/src/seedsigner/models/settings.py index 19eba895..d15b1653 100644 --- a/src/seedsigner/models/settings.py +++ b/src/seedsigner/models/settings.py @@ -81,6 +81,10 @@ class Settings(Singleton): for entry in data.split()[split_index:]: abbreviated_name, value = entry.split("=") + # Empty values ("some_setting= other_setting=E") are invalid + if value == "": + raise InvalidSettingsQRData(f"{abbreviated_name} cannot be empty") + # Parse multi-value settings; integer-ize where needed if "," in value: values_updated = [] @@ -103,6 +107,7 @@ class Settings(Singleton): values = [value] else: values = value + for v in values: if v not in [opt[0] for opt in settings_entry.selection_options]: if settings_entry.attr_name == SettingsConstants.SETTING__PERSISTENT_SETTINGS and v == SettingsConstants.OPTION__ENABLED: diff --git a/tests/test_settings.py b/tests/test_settings.py index 8cc391f9..f896dcaf 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -108,6 +108,14 @@ class TestSettings(BaseTest): assert "passphrase" in str(e.value) + def test_settingsqr_fails_empty_values(self): + """ SettingsQR parser should fail if a setting is empty """ + settingsqr_data = "settings::v1 persistent=D sigs= camera=180" + with pytest.raises(InvalidSettingsQRData) as e: + Settings.parse_settingsqr(settingsqr_data) + assert "sigs" in str(e.value) + + def test_settingsqr_parses_line_break_separators(self): """ SettingsQR parser should read line breaks as acceptable separators """ settingsqr_data = "settings::v1\nname=Foo\nsigs=ss,ms\nscripts=nat,nes,tr\npassphrase=E\n" From 40e9fe76d639a955917d16277c0cbba624c08401 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Tue, 16 Dec 2025 18:45:18 -0600 Subject: [PATCH 3/6] Ensure persistent settings load valid multiselects --- src/seedsigner/models/settings.py | 3 ++ tests/test_settings.py | 48 ++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/seedsigner/models/settings.py b/src/seedsigner/models/settings.py index d15b1653..c2bf933c 100644 --- a/src/seedsigner/models/settings.py +++ b/src/seedsigner/models/settings.py @@ -161,6 +161,9 @@ class Settings(Singleton): if type(new_settings[entry.attr_name]) == str: # Break comma-separated SettingsQR input into List new_settings[entry.attr_name] = new_settings[entry.attr_name].split(",") + elif new_settings[entry.attr_name] is None: + # Multiselect cannot be None; load defaults to avoid issues + new_settings[entry.attr_name] = entry.default_value for key, value in new_settings.items(): self.set_value(key, value) diff --git a/tests/test_settings.py b/tests/test_settings.py index f896dcaf..2d548d1b 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -1,7 +1,8 @@ +import json import pytest from base import BaseTest from seedsigner.models.settings import InvalidSettingsQRData, Settings -from seedsigner.models.settings_definition import SettingsConstants, SettingsDefinition +from seedsigner.models.settings_definition import SettingsConstants, SettingsDefinition, SettingsEntry @@ -35,6 +36,51 @@ class TestSettings(BaseTest): assert settings.get_value(settings_entry.attr_name) == settings_entry.default_value + def test_load_persistent_settings(self): + """ Settings should load previously saved persistent settings from disk, if any + exist. Empty multiselect settings should load defaults. """ + # Initial Settings will start with defaults + settings = Settings.get_instance() + + # Enable persistent settings and make another change + settings.set_value(SettingsConstants.SETTING__PERSISTENT_SETTINGS, SettingsConstants.OPTION__ENABLED) + + assert settings.get_value(SettingsConstants.SETTING__QR_DENSITY) != SettingsConstants.DENSITY__HIGH + settings.set_value(SettingsConstants.SETTING__QR_DENSITY, SettingsConstants.DENSITY__HIGH) + + # Hold on to the settings.json content + settings_json = None + with open(Settings.SETTINGS_FILENAME) as settings_file: + settings_json = json.loads(settings_file.read()) + + # Now wipe out the Settings singleton + BaseTest.reset_settings() + + # This also deletes settings.json, so recreate it + with open(Settings.SETTINGS_FILENAME, "w") as settings_file: + settings_file.write(json.dumps(settings_json)) + + # Now instantiate the Settings singleton again; it should load from disk + settings = Settings.get_instance() + assert settings.get_value(SettingsConstants.SETTING__QR_DENSITY) == SettingsConstants.DENSITY__HIGH + + # Wipe out the Settings singleton again + BaseTest.reset_settings() + + # Alter the settings.json to have an empty multiselect setting + settings_entry = SettingsDefinition.get_settings_entry(SettingsConstants.SETTING__SIG_TYPES) + settings_json[settings_entry.attr_name] = None + with open(Settings.SETTINGS_FILENAME, "w") as settings_file: + settings_file.write(json.dumps(settings_json)) + + print(json.dumps(settings_json, indent=4)) + + # Re-instantiate and verify that the multiselect setting has loaded its defaults + settings = Settings.get_instance() + sig_types = settings.get_value(settings_entry.attr_name) + assert sig_types == settings_entry.default_value + + def test_parse_settingsqr_data(self): """ SettingsQR parser should successfully parse a valid settingsqr input string and From f639d3df54fe57bafde1cb2c1599c89bf4256af9 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Tue, 16 Dec 2025 19:04:21 -0600 Subject: [PATCH 4/6] Minor cleanup --- tests/test_settings.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index 2d548d1b..487bf586 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -2,7 +2,7 @@ import json import pytest from base import BaseTest from seedsigner.models.settings import InvalidSettingsQRData, Settings -from seedsigner.models.settings_definition import SettingsConstants, SettingsDefinition, SettingsEntry +from seedsigner.models.settings_definition import SettingsConstants, SettingsDefinition @@ -52,14 +52,14 @@ class TestSettings(BaseTest): settings_json = None with open(Settings.SETTINGS_FILENAME) as settings_file: settings_json = json.loads(settings_file.read()) - + # Now wipe out the Settings singleton BaseTest.reset_settings() # This also deletes settings.json, so recreate it with open(Settings.SETTINGS_FILENAME, "w") as settings_file: settings_file.write(json.dumps(settings_json)) - + # Now instantiate the Settings singleton again; it should load from disk settings = Settings.get_instance() assert settings.get_value(SettingsConstants.SETTING__QR_DENSITY) == SettingsConstants.DENSITY__HIGH @@ -72,9 +72,7 @@ class TestSettings(BaseTest): settings_json[settings_entry.attr_name] = None with open(Settings.SETTINGS_FILENAME, "w") as settings_file: settings_file.write(json.dumps(settings_json)) - - print(json.dumps(settings_json, indent=4)) - + # Re-instantiate and verify that the multiselect setting has loaded its defaults settings = Settings.get_instance() sig_types = settings.get_value(settings_entry.attr_name) From dd9b3ecea615cf5e557fcd5df56c85e9976bed07 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Mon, 22 Dec 2025 18:44:03 -0600 Subject: [PATCH 5/6] Better bulletproofing --- src/seedsigner/models/settings.py | 10 ++++--- tests/test_settings.py | 43 ++++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/src/seedsigner/models/settings.py b/src/seedsigner/models/settings.py index c2bf933c..14741e67 100644 --- a/src/seedsigner/models/settings.py +++ b/src/seedsigner/models/settings.py @@ -159,10 +159,12 @@ class Settings(Singleton): # Clean the incoming data, if necessary if entry.type == SettingsConstants.TYPE__MULTISELECT: if type(new_settings[entry.attr_name]) == str: - # Break comma-separated SettingsQR input into List - new_settings[entry.attr_name] = new_settings[entry.attr_name].split(",") - elif new_settings[entry.attr_name] is None: - # Multiselect cannot be None; load defaults to avoid issues + # Break comma-separated multiselect options into List; avoid empty + # values. + new_settings[entry.attr_name] = [value for value in new_settings[entry.attr_name].split(",") if value.strip()] + + if not new_settings[entry.attr_name]: + # Multiselect cannot be empty; load defaults to avoid issues new_settings[entry.attr_name] = entry.default_value for key, value in new_settings.items(): diff --git a/tests/test_settings.py b/tests/test_settings.py index 487bf586..044f8433 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -38,7 +38,7 @@ class TestSettings(BaseTest): def test_load_persistent_settings(self): """ Settings should load previously saved persistent settings from disk, if any - exist. Empty multiselect settings should load defaults. """ + exist. """ # Initial Settings will start with defaults settings = Settings.get_instance() @@ -62,21 +62,40 @@ class TestSettings(BaseTest): # Now instantiate the Settings singleton again; it should load from disk settings = Settings.get_instance() + + # Persistent setting change should have survived assert settings.get_value(SettingsConstants.SETTING__QR_DENSITY) == SettingsConstants.DENSITY__HIGH - - # Wipe out the Settings singleton again - BaseTest.reset_settings() - # Alter the settings.json to have an empty multiselect setting - settings_entry = SettingsDefinition.get_settings_entry(SettingsConstants.SETTING__SIG_TYPES) - settings_json[settings_entry.attr_name] = None - with open(Settings.SETTINGS_FILENAME, "w") as settings_file: - settings_file.write(json.dumps(settings_json)) - # Re-instantiate and verify that the multiselect setting has loaded its defaults + def test_load_empty_multiselect_settings(self): + """ Empty multiselect settings should load defaults. """ + # Initial Settings will start with defaults settings = Settings.get_instance() - sig_types = settings.get_value(settings_entry.attr_name) - assert sig_types == settings_entry.default_value + + # Enable persistent settings to write settings.json to disk + settings.set_value(SettingsConstants.SETTING__PERSISTENT_SETTINGS, SettingsConstants.OPTION__ENABLED) + + # Hold on to the settings.json content + settings_dict = None + with open(Settings.SETTINGS_FILENAME) as settings_file: + settings_dict = json.loads(settings_file.read()) + + def _verify_defaults_loaded(attr_name: str): + # Verify that the multiselect setting has loaded its defaults + settings = Settings.get_instance() + cur_setting_value = settings.get_value(attr_name) + assert cur_setting_value == SettingsDefinition.get_settings_entry(attr_name).default_value + + # Alter the settings to test against various empty values + for empty_value in ["", ",", None]: + settings_dict[SettingsConstants.SETTING__SIG_TYPES] = empty_value + settings.update(settings_dict) + _verify_defaults_loaded(SettingsConstants.SETTING__SIG_TYPES) + + # One last test: remove the multiselect setting entirely + del settings_dict[SettingsConstants.SETTING__SIG_TYPES] + settings.update(settings_dict) + _verify_defaults_loaded(SettingsConstants.SETTING__SIG_TYPES) def test_parse_settingsqr_data(self): From a8e68142e86b45da3623469111f1e0bf4a53f297 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Tue, 23 Dec 2025 06:54:18 -0600 Subject: [PATCH 6/6] Minor test improvement --- tests/test_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index 044f8433..f29cd7fb 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -87,7 +87,7 @@ class TestSettings(BaseTest): assert cur_setting_value == SettingsDefinition.get_settings_entry(attr_name).default_value # Alter the settings to test against various empty values - for empty_value in ["", ",", None]: + for empty_value in ["", ",", [], None]: settings_dict[SettingsConstants.SETTING__SIG_TYPES] = empty_value settings.update(settings_dict) _verify_defaults_loaded(SettingsConstants.SETTING__SIG_TYPES)