diff --git a/docs/adr/0019-ui-modules-self-gate-preconditions.md b/docs/adr/0019-ui-modules-self-gate-preconditions.md new file mode 100644 index 000000000..88eea91f4 --- /dev/null +++ b/docs/adr/0019-ui-modules-self-gate-preconditions.md @@ -0,0 +1,69 @@ +# UI modules self-gate their preconditions (enter-then-back-out), not a menu hard-block + +A screen that depends on a runtime precondition is still opened normally from the +menu; the **module itself** checks the precondition and, when it is unmet, +renders an explanatory notice in place of its normal UI and makes its inputs +inert. The user reads the notice and **backs out** (LEFT/Cancel) on their own. We +do **not** block navigation at the menu layer. + +The motivating case is manual **Set Time/Date** (`UITimeEntry`, chaining to +`UIDateEntry`). Entry is interpreted in the observer's **local timezone**, which +we only derive from a **location fix** — `callbacks.set_time` / `set_datetime` +read `shared_state.location().timezone`. Without a fix the entered time would be +localised against a bogus UTC zone (the bug behind [ADR-0018](./0018-civil-datetime-stored-utc-aware.md)). +So the screen requires a location. This ADR fixes *where* that requirement is +enforced: inside the screen, not at the menu item that opens it. + +## The navigation mechanic that makes a hard-block possible + +`UITextMenu.key_right` (`ui/text_menu.py`) dispatches a menu item's `"callback"` +**before** its `"class"`, and the callback's return value propagates straight out +of `key_right`: a callback that returns without pushing anything simply cancels +the navigation. A `"class"` item is pushed unconditionally; its optional +`pre_callback` runs first but its return value is **discarded**, so a +`pre_callback` cannot refuse the push. Therefore a plain `"callback"` is the +*only* lever that can decline to open a screen — and this ADR chooses not to pull +it. + +## Considered options + +- **Menu-layer hard block (a gate callback).** The first cut of this change + (PR #512) routed Set Time/Date through `callbacks.enter_time_entry`, which + checked `location().lock` and, when unlocked, showed a 2-second "Set location + first" popup and returned **without** opening the screen. Rejected: it hides + the feature — the user never sees the screen and never learns what it needs — + and it splits the precondition across two layers, forcing the menu callback to + know an internal fact about the screen (that it needs a location's zone) that + already lives in the screen's own callbacks. +- **Self-gating module (chosen).** The precondition lives in exactly one place — + the module that depends on it. The feature stays discoverable; the on-screen + message teaches the user what to do; the menu stays a dumb router that always + opens the screen. +- **Disable / grey out the menu item.** Rejected: the menu has no "disabled item" + affordance, and a greyed item is just a quieter hard-block with the same + discoverability loss and no room to explain *why* or *what to do next*. + +## Consequences + +- **The check is live** (re-read every frame via `_location_locked`), so the + entry boxes appear the instant a fix locks while the user is sitting on the + screen — no need to back out and re-enter. +- **Every module owning the precondition self-gates, even when it is currently + unreachable while unmet.** `UIDateEntry` is only reached by confirming the + (already gated) time screen, so it cannot be entered without a fix today; it + still carries the same guard so it stays correct if it is ever surfaced + directly (e.g. relocated in the menu tree). +- **The exit callback must be suppressed when gated.** A module fires its + `custom_callback` (`set_time` / `set_datetime`) from `inactive()` on the way + out; a gated screen early-returns there so it never runs the callback against + the bogus UTC zone the gate exists to prevent. +- **Rendering is shared, the precondition is not.** A reusable + `UIModule.draw_gate_message()` draws the centred notice + a Cancel hint; the + precondition predicate stays local to each screen, so the base owns the generic + "how to show a gate" and each module owns "what it needs." This mirrors the + self-contained-in-the-UI stance of [ADR-0005](./0005-focus-hfd-self-contained-in-ui.md). +- **Trade-off accepted:** a hard-block would save one wasted keypress (the user + opens a screen they cannot use yet). We judge discoverability and single-owner + precondition logic worth that keypress. +- Companion glossary: [`docs/ax/ui/CONTEXT.md`](../ax/ui/CONTEXT.md) (Self-gating + module). diff --git a/docs/ax/ui/CONTEXT.md b/docs/ax/ui/CONTEXT.md index edb04fbbc..d117c075e 100644 --- a/docs/ax/ui/CONTEXT.md +++ b/docs/ax/ui/CONTEXT.md @@ -58,6 +58,10 @@ _Avoid_: display, screen driver (in code-arg context). `active`/`inactive` fire when a module reaches / leaves the top of the stack; `update` is the per-frame redraw a module overrides; `screen_update` draws the title bar and finalises the frame. _Avoid_: on_show / on_hide, render (use `update`/`screen_update`). +**Self-gating module**: +A module that enforces its own runtime **precondition** rather than relying on the menu to block entry. It always opens; when the precondition is unmet it draws a "set X first" notice (via `UIModule.draw_gate_message`) instead of its normal UI, keeps its key handlers and exit callback inert, and lets the user **back out** with LEFT/Cancel. The precondition check is live (re-read each `update`). Example: `UITimeEntry`/`UIDateEntry` gate on a location fix. See [ADR 0019](../../adr/0019-ui-modules-self-gate-preconditions.md). +_Avoid_: gated menu item, hard block, disabled screen (the menu never refuses to open a self-gating module). + ### Navigation **MenuManager**: diff --git a/python/PiFinder/ui/base.py b/python/PiFinder/ui/base.py index 812c2bfab..da51395a6 100644 --- a/python/PiFinder/ui/base.py +++ b/python/PiFinder/ui/base.py @@ -271,6 +271,35 @@ def clear_screen(self): fill=self.colors.get(0), ) + def draw_gate_message(self, message: str) -> None: + """Render a full-screen "precondition not met" notice into ``self.screen``. + + A module that gates itself on a runtime precondition (e.g. a location + fix) draws this from ``update`` in place of its normal UI and returns + early; the user reads it and backs out with LEFT/Cancel rather than + being blocked from opening the screen at all (see ADR 0019). Newlines + in ``message`` split it into centred lines; a Cancel hint is pinned to + the bottom-left, mirroring the entry screens' legends. + """ + self.clear_screen() + font = self.fonts.bold + lines = message.split("\n") + line_h = font.height + 2 + top = self.display_class.titlebar_height + block_h = line_h * len(lines) + y = top + max(4, (self.display_class.resY - top - block_h) // 2) + for line in lines: + text_w = font.font.getbbox(line)[2] + x = max(0, (self.display_class.resX - text_w) // 2) + self.draw.text((x, y), line, font=font.font, fill=self.colors.get(255)) + y += line_h + self.draw.text( + (10, self.display_class.resY - self.fonts.base.height - 2), + _(" Cancel"), + font=self.fonts.base.font, + fill=self.colors.get(255), + ) + def message(self, message, timeout: float = 2, size=None): """ Creates a box with text in the center of the screen. diff --git a/python/PiFinder/ui/dateentry.py b/python/PiFinder/ui/dateentry.py index 39f7f325c..00d662277 100644 --- a/python/PiFinder/ui/dateentry.py +++ b/python/PiFinder/ui/dateentry.py @@ -123,6 +123,17 @@ def draw_date_boxes(self): fill=self.red, ) + def _location_locked(self) -> bool: + """True when a location fix exists, so the date has a real local zone. + + ``set_datetime`` localises the entered date+time in the observer's zone, + which we only derive from a location fix. This screen is reached only + through the (equally gated) time screen today, but it self-gates on the + same precondition so it stays correct if it is ever surfaced directly + (see ADR 0019). The check is live, mirroring UITimeEntry. + """ + return bool(self.shared_state and self.shared_state.location().lock) + def draw_local_date_note(self): note_y = self.text_y + self.box_height + 10 self.draw.text( @@ -183,6 +194,8 @@ def validate_box(self, box_index, value): return False def key_number(self, number): + if not self._location_locked(): + return # gated: no location fix, so ignore digit entry current = self.boxes[self.current_box] new_value = current + str(number) @@ -197,6 +210,8 @@ def key_number(self, number): def key_minus(self): """Delete last digit in current box or move to previous box if empty.""" + if not self._location_locked(): + return # gated: nothing to edit until a location fix exists if self.boxes[self.current_box]: self.boxes[self.current_box] = self.boxes[self.current_box][:-1] else: @@ -204,6 +219,8 @@ def key_minus(self): def key_right(self): """Confirm if all boxes filled, otherwise cycle to next box.""" + if not self._location_locked(): + return False # gated: don't confirm the date if all(self.boxes) and self.current_box == 2: self._confirmed = True self.remove_from_stack() @@ -221,18 +238,22 @@ def key_left(self) -> bool: def inactive(self): """Called when the module is no longer active.""" + if not self._location_locked(): + return # gated: never fire set_datetime against a bogus (UTC) zone if self._confirmed and self.custom_callback: date_str = f"{self.boxes[0]}-{self.boxes[1]}-{self.boxes[2]}" self.custom_callback(self, date_str) def update(self, force=False): - self.draw.rectangle((0, 0, self.width, self.height), fill=self.black) - - self.draw_date_boxes() - - note_y = self.draw_local_date_note() - separator_y = self.draw_separator(note_y + 15) - self.draw_legend(separator_y) + if not self._location_locked(): + # Self-gate on a location fix, mirroring UITimeEntry (see ADR 0019). + self.draw_gate_message(_("Set location\nfirst")) + else: + self.draw.rectangle((0, 0, self.width, self.height), fill=self.black) + self.draw_date_boxes() + note_y = self.draw_local_date_note() + separator_y = self.draw_separator(note_y + 15) + self.draw_legend(separator_y) if self.shared_state: self.shared_state.set_screen(self.screen) diff --git a/python/PiFinder/ui/menu_structure.py b/python/PiFinder/ui/menu_structure.py index ad2a73d59..149b68ec2 100644 --- a/python/PiFinder/ui/menu_structure.py +++ b/python/PiFinder/ui/menu_structure.py @@ -1168,6 +1168,9 @@ def _(key: str) -> Any: }, { "name": _("Set Time/Date"), + # UITimeEntry self-gates on a location fix (it needs + # the observer's zone); it shows a "set location + # first" notice and the user backs out. See ADR 0019. "class": UITimeEntry, "custom_callback": callbacks.set_time, }, diff --git a/python/PiFinder/ui/timeentry.py b/python/PiFinder/ui/timeentry.py index ef54f059f..ca53651d5 100644 --- a/python/PiFinder/ui/timeentry.py +++ b/python/PiFinder/ui/timeentry.py @@ -108,16 +108,30 @@ def draw_time_boxes(self): fill=self.red, ) + def _location_locked(self) -> bool: + """True when a location fix exists, so manual time has a real local zone. + + Manual entry is interpreted in the observer's local timezone, which we + only derive from a location fix; without one ``set_time`` would silently + fall back to UTC. This screen self-gates on it (see ADR 0019): the user + may open it, but the entry boxes and the ``set_time`` callback stay + inert -- and a "set location first" notice shows -- until a fix arrives. + The check is live, so the boxes appear the moment a fix locks while the + screen is open, without needing to back out and re-enter. + """ + return bool(self.shared_state and self.shared_state.location().lock) + def draw_local_time_note(self): - # Add a note about local time + # Add a note about local time. The time is entered in the observer's + # local zone (this screen self-gates on a location fix -- see + # _location_locked / update -- so a zone is always known when the boxes + # are shown), so a fixed label suffices and we avoid overrunning the + # screen with a long IANA timezone name. Mirrors UIDateEntry's + # "Enter Local Date". note_y = self.text_y + self.box_height + 10 - if self.shared_state: - note_str = "Enter " + self.shared_state.location().timezone + " Time" - else: - note_str = "Enter Local Time" self.draw.text( (10, note_y), - _(note_str), + _("Enter Local Time"), font=self.fonts.base.font, fill=self.red, ) @@ -169,6 +183,8 @@ def validate_box(self, box_index, value): return False def key_number(self, number): + if not self._location_locked(): + return # gated: no location fix, so ignore digit entry current = self.boxes[self.current_box] new_value = current + str(number) @@ -185,6 +201,8 @@ def key_number(self, number): def key_minus(self): """Delete last digit in current box or move to previous box if empty""" + if not self._location_locked(): + return # gated: nothing to edit until a location fix exists if self.boxes[self.current_box]: # Delete the last digit self.boxes[self.current_box] = self.boxes[self.current_box][:-1] @@ -194,6 +212,8 @@ def key_minus(self): def key_right(self): """Confirm time and chain to date entry, or cycle to next box.""" + if not self._location_locked(): + return False # gated: don't confirm or chain to date entry if all(self.boxes) and self.current_box == 2: time_str = ( f"{self.boxes[0] or '00'}:{self.boxes[1] or '00'}" @@ -223,20 +243,26 @@ def key_left(self) -> bool: def inactive(self): """Called when the module is no longer the active module""" - if self._skip_callback: + if self._skip_callback or not self._location_locked(): + # Gated (no location fix) or an explicit cancel: never fire set_time, + # which would interpret the entry against a bogus (UTC) zone. return if self.custom_callback: time_str = f"{self.boxes[0] or '00'}:{self.boxes[1] or '00'}:{self.boxes[2] or '00'}" self.custom_callback(self, time_str) def update(self, force=False): - self.draw.rectangle((0, 0, self.width, self.height), fill=self.black) - - self.draw_time_boxes() - - note_y = self.draw_local_time_note() - separator_y = self.draw_separator(note_y + 15) - self.draw_legend(separator_y) + if not self._location_locked(): + # Self-gate: entry is meaningless without a location's timezone, so + # show a "set location first" notice and let the user back out + # (see ADR 0019) instead of hard-blocking entry from the menu. + self.draw_gate_message(_("Set location\nfirst")) + else: + self.draw.rectangle((0, 0, self.width, self.height), fill=self.black) + self.draw_time_boxes() + note_y = self.draw_local_time_note() + separator_y = self.draw_separator(note_y + 15) + self.draw_legend(separator_y) if self.shared_state: self.shared_state.set_screen(self.screen) diff --git a/python/tests/test_time_date_gate.py b/python/tests/test_time_date_gate.py new file mode 100644 index 000000000..739bcbc5e --- /dev/null +++ b/python/tests/test_time_date_gate.py @@ -0,0 +1,132 @@ +"""Unit tests for the Set Time/Date self-gate (UITimeEntry / UIDateEntry). + +Manual time/date entry is interpreted in the observer's local timezone, which +only exists once a location fix is present. Rather than hard-blocking entry from +the menu, each screen self-gates (see ADR 0019): the user may open the screen, +but the entry boxes and the set_time / set_datetime callbacks stay inert -- and +a "set location first" notice is shown -- until a fix locks; the user backs out +with LEFT/Cancel. These tests drive the modules directly in the locked and +unlocked states, asserting the gate logic (key suppression, callback +suppression, live predicate). Full-screen rendering in both states is covered by +the cold/warm crash-smoke in test_ui_modules.py. +""" + +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +import PiFinder.i18n # noqa: F401 installs the _() gettext builtin +from PiFinder.displays import get_display +from PiFinder.state import Location +from PiFinder.ui.dateentry import UIDateEntry +from PiFinder.ui.timeentry import UITimeEntry + +pytestmark = pytest.mark.unit + + +def _make_shared_state(location: Location): + """A stub shared_state exposing just what the entry modules touch.""" + return SimpleNamespace( + ui_state=lambda: MagicMock(), + location=lambda: location, + local_datetime=lambda: None, # UIDateEntry pre-fills from this + set_screen=lambda *a, **k: None, + ) + + +def _build(module_cls, location: Location): + """Construct a real entry module on a headless display with a stub state.""" + display = get_display("headless") + return module_cls( + display, + None, # camera_image + _make_shared_state(location), + {}, # command_queues + MagicMock(), # config_object + MagicMock(), # catalogs + item_definition={"custom_callback": MagicMock()}, + add_to_stack=MagicMock(), + remove_from_stack=MagicMock(), + ) + + +_LOCKED = Location(lock=True, timezone="America/New_York") +_UNLOCKED = Location(lock=False) + + +# --------------------------------------------------------------------------- # +# UITimeEntry +# --------------------------------------------------------------------------- # + + +def test_time_entry_unlocked_is_inert(): + """No location lock -> digits ignored, RIGHT doesn't chain, no set_time.""" + module = _build(UITimeEntry, _UNLOCKED) + + assert module._location_locked() is False + + module.key_number(5) + assert module.boxes == ["", "", ""] # entry suppressed + + assert module.key_right() is False + module.remove_from_stack.assert_not_called() # no chain to date entry + module.add_to_stack.assert_not_called() + + module.inactive() + module.custom_callback.assert_not_called() # set_time suppressed + + +def test_time_entry_locked_accepts_entry_and_fires_callback(): + """With a lock -> digits land in the boxes and set_time fires on exit.""" + module = _build(UITimeEntry, _LOCKED) + + assert module._location_locked() is True + + module.key_number(1) + module.key_number(2) + assert module.boxes[0] == "12" + + module.inactive() + module.custom_callback.assert_called_once_with(module, "12:00:00") + + +def test_time_entry_gate_message_renders(): + """The base gate helper draws without error on a real headless display.""" + module = _build(UITimeEntry, _UNLOCKED) + module.draw_gate_message("Set location\nfirst") # must not raise + + +# --------------------------------------------------------------------------- # +# UIDateEntry (symmetric guard -- reached only via the time screen today, but +# self-gates on the same precondition for correctness if ever surfaced directly) +# --------------------------------------------------------------------------- # + + +def test_date_entry_unlocked_is_inert(): + """No location lock -> digits ignored, no set_datetime even if confirmed.""" + module = _build(UIDateEntry, _UNLOCKED) + + assert module._location_locked() is False + + module.key_number(5) + assert module.boxes == ["", "", ""] # entry suppressed + + module._confirmed = True + module.inactive() + module.custom_callback.assert_not_called() # set_datetime suppressed + + +def test_date_entry_locked_accepts_entry_and_fires_callback(): + """With a lock -> digits land and set_datetime fires when confirmed.""" + module = _build(UIDateEntry, _LOCKED) + + assert module._location_locked() is True + + for digit in (2, 0, 2, 4): + module.key_number(digit) + assert module.boxes[0] == "2024" + + module._confirmed = True + module.inactive() + module.custom_callback.assert_called_once() diff --git a/python/tests/website/test_web_remote_tools.py b/python/tests/website/test_web_remote_tools.py index 9b5d989fd..976458cfa 100644 --- a/python/tests/website/test_web_remote_tools.py +++ b/python/tests/website/test_web_remote_tools.py @@ -1,11 +1,15 @@ +import requests + import pytest from web_test_utils import ( + get_homepage_url, login_to_remote, navigate_to_root_menu, press_keys, press_keys_and_validate, ) + """ Tests for the Tools menu in PiFinder's remote control interface. @@ -26,7 +30,7 @@ Place & Time submenu (0-indexed): 0: GPS Status (UIGPSStatus screen) 1: Set Location (submenu: Enter Coords, Load Location, Save Location) - 2: Set Time/Date (UITimeEntry screen) + 2: Set Time/Date (UITimeEntry screen; self-gates on a location fix) 3: Reset Location (callback: gps_reset) 4: Reset Time/Date (callback: datetime_reset) @@ -35,6 +39,28 @@ R → enter Tools submenu (now at Status, index 0) """ + +def _force_location(driver, lat=50.0, lon=3.0, altitude=10.0): + """Establish a location fix via the web GPS endpoint. + + UITimeEntry opens regardless of location, but self-gates: without a fix it + shows a "set location first" notice with inert entry boxes (see ADR 0019). + Tests that need the live entry boxes seed a fix first. POST /gps/update + sleeps ~1s server-side to let the GPS thread apply the fix before returning. + """ + cookies = {cookie["name"]: cookie["value"] for cookie in driver.get_cookies()} + resp = requests.post( + f"{get_homepage_url()}/gps/update", + data={ + "latitudeDecimal": str(lat), + "longitudeDecimal": str(lon), + "altitude": str(altitude), + }, + cookies=cookies, + ) + assert resp.status_code in (200, 302) + + # --------------------------------------------------------------------------- # Tools menu entry # --------------------------------------------------------------------------- @@ -128,8 +154,14 @@ def test_tools_gps_status_screen(driver): @pytest.mark.web def test_tools_set_time_screen(driver): - """Tools > Place & Time > Set Time/Date opens the UITimeEntry screen.""" + """Tools > Place & Time > Set Time/Date opens the UITimeEntry screen. + + With a location fix seeded, the screen opens with live entry boxes. (The + screen self-gates on the fix -- see ADR 0019 -- but always opens; the gate + only governs the entry boxes and the set_time callback, not navigation.) + """ login_to_remote(driver) + _force_location(driver) navigate_to_root_menu(driver) # DDDRDDR = enter Place & Time at GPS Status (index 0) @@ -145,6 +177,29 @@ def test_tools_set_time_screen(driver): press_keys(driver, "ZL") # back to root +@pytest.mark.web +def test_tools_set_time_screen_opens_without_location(driver): + """Set Time/Date is never hard-blocked: it opens even with no location seeded. + + This guards the ADR-0019 change from regressing back to a menu-layer gate + that refused to open the screen without a fix. The module self-gates + internally instead (showing a "set location first" notice with inert boxes), + so ``ui_type`` is UITimeEntry regardless of whether a fix is present. + """ + login_to_remote(driver) + navigate_to_root_menu(driver) # deliberately no _force_location() + + press_keys_and_validate( + driver, + "DDDRDDRDDR", + { + "ui_type": "UITimeEntry", + }, + ) + + press_keys(driver, "ZL") # back to root + + @pytest.mark.web def test_tools_set_location_screen(driver): """Tools > Place & Time > Set Location > Load Location opens the UILocationList screen."""