Skip to content

Commit 3cd7bf1

Browse files
ThomasFarstrikeThomasFarstrike
authored
Merge pull request #125 from bitcoin3us/fix/radio-require-one-selection
SettingActivity: keep exactly one radio option selected
2 parents 4b971a5 + f1f6faa commit 3cd7bf1

4 files changed

Lines changed: 187 additions & 3 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ Frameworks:
1818
- AppManager: support .mpk/.zip files with compression and a redundant top-level directory
1919
- AppManager: export 'mpos' global to apps for convenience
2020
- LightsManager: allow changing number of LEDs after initialization
21+
- SettingActivity: add `allow_deselect` option (default False) to radiobuttons
2122
- SharedPreferences: don't print potentially sensitive values on serial port
2223
- WebServer: add basic 'View Screen' functionality to view the device's display remotely
2324

‎internal_filesystem/builtin/apps/com.micropythonos.settings/assets/settings.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,11 @@ def getIntent(self):
9797
{"title": "Timezone", "key": "timezone", "ui": "dropdown", "ui_options": [(tz, tz) for tz in TimeZone.get_timezones()], "changed_callback": lambda *args: TimeZone.refresh_timezone_preference()},
9898
{"title": "Number Format", "key": "number_format", "ui": "dropdown", "ui_options": NumberFormat.get_format_options(), "changed_callback": lambda *args: NumberFormat.refresh_preference(), "default_value": "comma_dot"},
9999
# Advanced settings, alphabetically:
100-
{"title": "Auto Start App", "key": "auto_start_app", "ui": "radiobuttons", "ui_options": [(app.name, app.fullname) for app in AppManager.get_app_list()]},
100+
# `allow_deselect=True` because saving an empty string here is
101+
# meaningful: it means "boot straight to the launcher, don't
102+
# autostart anything". The user needs to be able to reach that
103+
# state by tapping the currently-selected app to un-select it.
104+
{"title": "Auto Start App", "key": "auto_start_app", "ui": "radiobuttons", "ui_options": [(app.name, app.fullname) for app in AppManager.get_app_list()], "allow_deselect": True},
101105
{"title": "Check IMU Calibration", "key": "check_imu_calibration", "ui": "activity", "activity_class": CheckIMUCalibrationActivity},
102106
{"title": "Calibrate IMU", "key": "calibrate_imu", "ui": "activity", "activity_class": CalibrateIMUActivity},
103107
# Expert settings, alphabetically

‎internal_filesystem/lib/mpos/ui/setting_activity.py‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ def onCreate(self):
5454
self.radio_container.set_height(lv.SIZE_CONTENT)
5555
self.radio_container.set_flex_flow(lv.FLEX_FLOW.COLUMN)
5656
self.radio_container.add_event_cb(self.radio_event_handler, lv.EVENT.VALUE_CHANGED, None)
57+
# `allow_deselect` is an opt-in for settings where "nothing
58+
# selected" is a legitimate value (e.g. Auto Start App: empty
59+
# means "don't autostart anything"). Default is False so the
60+
# normal "exactly one option always selected" radio-group
61+
# convention holds for the typical case.
62+
self._radio_allow_deselect = bool(setting.get("allow_deselect", False))
5763
# Create radio buttons and check the right one
5864
self.active_radio_index = -1 # none
5965
for i, (option_text, option_value) in enumerate(ui_options):
@@ -146,9 +152,26 @@ def radio_event_handler(self, event):
146152
current_checkbox_index = target_obj.get_index()
147153
print(f"current_checkbox_index: {current_checkbox_index}")
148154
if not checked:
155+
# Radio-button convention: clicking the already-selected option
156+
# must NOT un-select it. Exactly one option is always selected
157+
# once the user has made a choice. Without this guard, a user
158+
# could land on Settings, tap the current wallet type, and save
159+
# an empty wallet_type — leading to the welcome screen coming
160+
# back even though they meant to keep the config intact.
161+
#
162+
# Opt-out: a setting definition can pass `allow_deselect=True`
163+
# for groups where "nothing selected" is a legitimate value.
164+
# Example: the OS Settings "Auto Start App" picker — an empty
165+
# value means "boot straight to the launcher", which is a
166+
# first-class option users need to be able to select by tapping
167+
# the currently-active app.
149168
if self.active_radio_index == current_checkbox_index:
150-
print(f"unchecking {current_checkbox_index}")
151-
self.active_radio_index = -1 # nothing checked
169+
if getattr(self, '_radio_allow_deselect', False):
170+
print(f"radio: un-check of active option {current_checkbox_index} (allow_deselect=True)")
171+
self.active_radio_index = -1
172+
else:
173+
print(f"radio: ignoring un-check of active option {current_checkbox_index} (radios require exactly one)")
174+
target_obj.add_state(lv.STATE.CHECKED)
152175
return
153176
else:
154177
if self.active_radio_index >= 0: # is there something to uncheck?

‎tests/test_graphical_setting_activity_radio.py‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
"""
2+
Graphical test for SettingActivity radio-button invariant.
3+
4+
Clicking the already-selected radio option previously let the user
5+
un-select it and save an empty string for the setting. This broke the
6+
radio-group convention (exactly one option selected once the user has
7+
made a choice) and let apps with required radio settings — Lightning
8+
Piggy's wallet_type being the motivating case — silently lose their
9+
configuration on a stray tap.
10+
11+
The fix: when the user clicks the currently-active option, re-add the
12+
CHECKED state so the selection sticks. Changing the pick by clicking a
13+
different option works exactly as before.
14+
15+
Usage:
16+
Desktop: ./tests/unittest.sh tests/test_graphical_setting_activity_radio.py
17+
Device: ./tests/unittest.sh tests/test_graphical_setting_activity_radio.py --ondevice
18+
"""
19+
20+
import unittest
21+
import lvgl as lv
22+
23+
from mpos.ui.setting_activity import SettingActivity
24+
from mpos import wait_for_render
25+
26+
27+
class _FakeEvent:
28+
"""Minimal stand-in for an LVGL event — SettingActivity.radio_event_handler
29+
only calls event.get_target_obj()."""
30+
def __init__(self, target):
31+
self._target = target
32+
def get_target_obj(self):
33+
return self._target
34+
35+
36+
class _RadioHandlerFixture:
37+
"""Holds the same attributes SettingActivity.radio_event_handler reads on
38+
`self`. Avoids instantiating the full Activity (which needs a running
39+
AppManager) just to exercise one callback."""
40+
def __init__(self, container, active_index, allow_deselect=False):
41+
self.radio_container = container
42+
self.active_radio_index = active_index
43+
self._radio_allow_deselect = allow_deselect
44+
45+
46+
class TestSettingActivityRadioInvariant(unittest.TestCase):
47+
"""Verify the radio-group invariant: exactly one option stays selected
48+
after the user has made a pick."""
49+
50+
def setUp(self):
51+
self.screen = lv.obj()
52+
self.screen.set_size(320, 240)
53+
lv.screen_load(self.screen)
54+
55+
self.container = lv.obj(self.screen)
56+
self.container.set_flex_flow(lv.FLEX_FLOW.COLUMN)
57+
58+
self.cb0 = lv.checkbox(self.container)
59+
self.cb0.set_text("Option 0")
60+
self.cb1 = lv.checkbox(self.container)
61+
self.cb1.set_text("Option 1")
62+
63+
wait_for_render(2)
64+
65+
def tearDown(self):
66+
lv.screen_load(lv.obj())
67+
wait_for_render(2)
68+
69+
def _invoke(self, target, active_index, allow_deselect=False):
70+
"""Run radio_event_handler as it would run in production — we bind the
71+
unbound function to a fixture that exposes the attributes the handler
72+
reads on `self`."""
73+
fixture = _RadioHandlerFixture(
74+
self.container, active_index, allow_deselect=allow_deselect)
75+
SettingActivity.radio_event_handler(fixture, _FakeEvent(target))
76+
return fixture
77+
78+
def test_click_active_option_keeps_it_selected(self):
79+
"""Clicking the already-selected option must NOT un-select it.
80+
81+
Before the fix: fixture.active_radio_index flips to -1 and the
82+
checkbox stays un-checked, so save_setting would persist "".
83+
After the fix: the checkbox is re-checked and active_radio_index
84+
stays pointing at the same option.
85+
"""
86+
self.cb0.add_state(lv.STATE.CHECKED)
87+
# Simulate LVGL's "checkbox toggle on click" behaviour: STATE.CHECKED
88+
# has just been removed and VALUE_CHANGED is about to fire.
89+
self.cb0.remove_state(lv.STATE.CHECKED)
90+
self.assertFalse(bool(self.cb0.get_state() & lv.STATE.CHECKED))
91+
92+
fixture = self._invoke(self.cb0, active_index=0)
93+
94+
self.assertTrue(
95+
bool(self.cb0.get_state() & lv.STATE.CHECKED),
96+
"active option must be re-checked when user tries to un-select it",
97+
)
98+
self.assertEqual(
99+
fixture.active_radio_index, 0,
100+
"active_radio_index must stay pointing at the active option",
101+
)
102+
103+
def test_click_different_option_switches_selection(self):
104+
"""Clicking a *different* option still switches normally: the new one
105+
becomes checked, the old one loses its checked state, and
106+
active_radio_index moves."""
107+
self.cb0.add_state(lv.STATE.CHECKED)
108+
# LVGL toggled cb1 on, firing VALUE_CHANGED with cb1 as the target.
109+
self.cb1.add_state(lv.STATE.CHECKED)
110+
111+
fixture = self._invoke(self.cb1, active_index=0)
112+
113+
self.assertFalse(
114+
bool(self.cb0.get_state() & lv.STATE.CHECKED),
115+
"previously-active option must lose its CHECKED state",
116+
)
117+
self.assertTrue(
118+
bool(self.cb1.get_state() & lv.STATE.CHECKED),
119+
"newly-clicked option must stay CHECKED",
120+
)
121+
self.assertEqual(fixture.active_radio_index, 1)
122+
123+
def test_first_selection_from_empty_state(self):
124+
"""Before any choice is made, active_radio_index = -1. Clicking any
125+
option selects it normally."""
126+
# Nothing checked yet; user clicks cb1.
127+
self.cb1.add_state(lv.STATE.CHECKED)
128+
129+
fixture = self._invoke(self.cb1, active_index=-1)
130+
131+
self.assertTrue(bool(self.cb1.get_state() & lv.STATE.CHECKED))
132+
self.assertEqual(fixture.active_radio_index, 1)
133+
134+
def test_allow_deselect_opt_in_lets_user_un_select_active(self):
135+
"""With `allow_deselect=True` (set by settings whose empty-string
136+
value is a legitimate choice — e.g. OS Settings "Auto Start App"
137+
where empty means "don't autostart anything"), clicking the
138+
currently-active option DOES un-select it and active_radio_index
139+
flips to -1. The invariant from the other tests is opt-out for
140+
this class of setting."""
141+
self.cb0.add_state(lv.STATE.CHECKED)
142+
# User taps the already-selected option → LVGL strips CHECKED and
143+
# fires VALUE_CHANGED.
144+
self.cb0.remove_state(lv.STATE.CHECKED)
145+
self.assertFalse(bool(self.cb0.get_state() & lv.STATE.CHECKED))
146+
147+
fixture = self._invoke(self.cb0, active_index=0, allow_deselect=True)
148+
149+
self.assertFalse(
150+
bool(self.cb0.get_state() & lv.STATE.CHECKED),
151+
"with allow_deselect=True the active option must stay un-checked",
152+
)
153+
self.assertEqual(
154+
fixture.active_radio_index, -1,
155+
"active_radio_index must flip to -1 so save_setting persists ''",
156+
)

0 commit comments

Comments
 (0)