From 3e77f4e2329b97a3c9fdb362d529ff5eb3f2c093 Mon Sep 17 00:00:00 2001 From: Krzysztof Rudnicki Date: Thu, 30 Jul 2026 16:25:45 +0200 Subject: [PATCH] Make every lock screen keyboard-operable, and add the missing installer The locker was not running at all. workout-locker.service crash-looped ~1977 times in a single boot with ModuleNotFoundError while remaining `enabled`, and has no successful start in the retained journal. A locker that cannot start is a locker that does not lock. Root cause: commit 9b420a9 removed the unit's WorkingDirectory and PYTHONPATH=/opt/screen-locker on the (correct in principle) grounds that "screen_locker is now pip-installed" -- but nothing guaranteed that install. There was no installer, /opt/screen-locker no longer exists, and the package was absent from every Python version's user site-packages. The unit lost its fallback and gained a hard dependency on a manual step nobody re-ran. install.sh fixes that as a script rather than a reminder, and *gates*: it fails if the production interpreter cannot import screen_locker, because an installer that reports success while the service still cannot start is precisely the failure above. It also re-asserts an editable gatelock from ~/utils AFTER the package install and verifies it resolves there -- pyproject pins gatelock to a git tag, so a plain install silently replaces a local editable gatelock with a frozen wheel, which was observed removing ScrollableSurface and taking diet_guard down with it. The keyboard work, all of it Tk defaults rather than local mistakes: - Focus was invisible. Tk ships highlightcolor="#000000" -- a black ring on #211D1B -- and relief="flat" removes the depth cue too, so a keyboard user had no indication of where they were. Now set in the _ui_widgets factory, so a new call site cannot omit it. - Enter did nothing. X11 Tk binds on Button and never . - tk.Text trapped Tab (inserts a literal tab, refocuses itself) and no-opped Shift-Tab; the only exits are the undocumented Ctrl+Tab / Ctrl+Shift+Tab. - The lock and retry screens started with NOTHING focused. on_focus_ready was a documented no-op arguing "buttons only, nothing to focus" -- exactly backwards: buttons are what needs focusing when there is no other window on screen and a global grab is held. - The heat-skip modal grabbed input fullscreen with no focus, no ring, no Return and no Escape. It now focuses the *declining* option, so a blind Return keeps the workout rather than silently skipping it. The manual-workout form's scroll viewport was pointer-only in three ways at once: tk.Canvas has no class key bindings, so ::tk::FocusOK rejected it as a focus stop; nothing bound wheel, PageUp/PageDown or arrows, leaving only a draggable thumb; and canvas clipping does not unmap children, so below-the-fold fields stayed in the tab ring while Tk never scrolled to follow focus -- Tab led to fields the user could neither see nor reveal. Its height was also a magic 0.7*toplevel above ~230-290px of fixed chrome, i.e. satisfied at 1080p and violated at 768p, where SUBMIT/BACK were pushed off the bottom. All of that is gone by deletion: the surface container is now a gatelock.ScrollableSurface, so the inner viewport is redundant. build_surface and _ensure_container both use it, which fixes the same symmetric-clipping problem for EVERY screen rather than one at a time; center_when_fits keeps short screens looking exactly as they did. Re-arming after a repaint hooks into clear_container(), which all ~19 repaint sites already call. "Remember to call settle after painting" is a rule that would eventually be missed, and on a lock a missed re-arm means unreachable content. The aux windows hardcoded geometry("600x400") and ("700x450") while displaying more than that -- the relaxed-day prompt's own button row requests 862px inside a 700px window, sheared at both edges. That pre-dates this work by a wide margin (Tk's default focus ring accounts for 4px of the 162px overflow). They now size to content, floored at the old values and clamped to the display. Co-Authored-By: Claude Opus 5 (1M context) --- .pre-commit-config.yaml | 2 +- install.sh | 138 +++++++++++++++++ pyproject.toml | 2 +- requirements.txt | 2 +- screen_locker/_heat_skip.py | 26 +++- screen_locker/_manual_workout_dialog.py | 57 +++---- screen_locker/_manual_workout_widgets.py | 11 ++ screen_locker/_sick_dialog.py | 8 + screen_locker/_ui_widgets.py | 97 +++++++++++- screen_locker/_window_setup.py | 170 ++++++++++++++++++--- screen_locker/tests/conftest.py | 5 + screen_locker/tests/test_verify_workout.py | 16 +- 12 files changed, 465 insertions(+), 69 deletions(-) create mode 100755 install.sh diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ea307cc..e0508e9 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -99,7 +99,7 @@ repos: - --jobs=4 additional_dependencies: - pytest - - gatelock @ git+https://github.com/kuhyx/utils@gatelock-v0.2.0#subdirectory=gatelock + - gatelock @ git+https://github.com/kuhyx/utils@gatelock-v0.3.0#subdirectory=gatelock - crdt-sync @ git+https://github.com/kuhyx/utils@crdt-sync-v0.1.0#subdirectory=crdt-sync - mcp - pydantic diff --git a/install.sh b/install.sh new file mode 100755 index 0000000..b2305b6 --- /dev/null +++ b/install.sh @@ -0,0 +1,138 @@ +#!/bin/bash +# ============================================================================ +# Screen Locker installer: workout-to-unlock gate. +# +# Usage: bash install.sh +# +# Why this file exists +# -------------------- +# It didn't, and that is exactly how the locker silently stopped working. +# +# Commit 9b420a9 (2026-06-22) removed `WorkingDirectory=/opt/screen-locker` and +# `Environment=PYTHONPATH=/opt/screen-locker` from workout-locker.service, on +# the correct-in-principle grounds that "screen_locker is now pip-installed". +# But nothing *guaranteed* that install: there was no installer, `/opt/screen- +# locker` no longer exists, and the package is absent from every Python +# version's user site-packages. So the unit lost its fallback path and gained a +# hard dependency on a manual step nobody re-ran. +# +# Result: workout-locker.service crash-looped with ModuleNotFoundError +# ~1977 times in a single boot while remaining `enabled`, and never started +# successfully. A locker that cannot start is a locker that does not lock. +# +# The fix is this script, not a reminder: run it and the install is real, +# editable (so `git pull` here reaches the running service), and verified +# against the *production* interpreter rather than a dev venv. +# +# What it does: +# 1. Checks system deps (setxkbmap, needed for the VT-disable) +# 2. pip-installs this package editable into system Python's USER +# site-packages -- the service runs /usr/bin/python3 directly, not a venv +# 3. Installs + enables the systemd user units +# 4. Verifies the production interpreter can actually import the module, +# and FAILS if it cannot (a warning here would just recreate the bug) +# ============================================================================ + +set -euo pipefail + +# Split declare/assign so the command-substitution exit code is not masked. +SCRIPT_DIR="$(dirname "$(readlink -f "$0")")" +readonly SCRIPT_DIR +readonly REPO_DIR="$SCRIPT_DIR" +readonly SYSTEMD_USER_DIR="$HOME/.config/systemd/user" +readonly PROD_PYTHON="/usr/bin/python3" +# The shared lock backend, developed in lockstep with this repo. +readonly GATELOCK_DIR="$HOME/utils/gatelock" + +echo "=== Screen Locker Installer ===" + +# 1. System dependencies ------------------------------------------------------ +echo "[1/4] Checking system dependencies..." +if ! command -v setxkbmap &>/dev/null; then + echo " installing xorg-setxkbmap (required to disable VT switching)..." + sudo pacman -S --needed --noconfirm xorg-setxkbmap +else + echo " setxkbmap present." +fi + +# 2. Install the package ------------------------------------------------------ +# --break-system-packages is required on Arch (PEP 668). Editable so a later +# `git pull` in THIS clone updates the running service with no reinstall. That +# is also why the clone must live somewhere durable (~/screen-locker), never a +# scratch directory: a non-editable snapshot would freeze at this commit and +# silently stop tracking the repo. +echo "[2/4] Installing screen_locker into user site-packages (editable)..." +"$PROD_PYTHON" -m pip install --user --break-system-packages -e "$REPO_DIR" + +# Re-assert an editable gatelock, deliberately AFTER the line above. +# +# pyproject.toml pins `gatelock @ git+https://.../@gatelock-v0.2.1`, so the +# install above resolves that pin and overwrites any local editable gatelock +# with a frozen wheel built from the tag. That silently reverts every +# unreleased change in ~/utils/gatelock and breaks the other lockers that +# depend on them -- observed: it removed `ScrollableSurface` and took +# diet_guard down with it. +# +# gatelock is a sibling in the utils monorepo and is developed in lockstep with +# the four lockers, so the local clone must win. Ordering is the fix: install +# it last. +if [[ -d "$GATELOCK_DIR" ]]; then + echo " re-asserting editable gatelock from $GATELOCK_DIR..." + "$PROD_PYTHON" -m pip install --user --break-system-packages \ + --no-deps -e "$GATELOCK_DIR" +else + echo " WARNING: $GATELOCK_DIR not found; leaving the pinned gatelock" >&2 + echo " wheel in place. Local gatelock changes will NOT be" >&2 + echo " picked up by the running service." >&2 +fi + +# 3. systemd units ------------------------------------------------------------ +echo "[3/4] Installing systemd user units..." +mkdir -p "$SYSTEMD_USER_DIR" +shopt -s nullglob +for unit in "$SCRIPT_DIR"/*.service "$SCRIPT_DIR"/*.timer; do + install -m 644 "$unit" "$SYSTEMD_USER_DIR/$(basename "$unit")" + echo " installed $(basename "$unit")" +done +shopt -u nullglob +systemctl --user daemon-reload +for timer in "$SCRIPT_DIR"/*.timer; do + [[ -e "$timer" ]] || continue + systemctl --user enable --now "$(basename "$timer")" + echo " enabled $(basename "$timer")" +done + +# 4. Verify the PRODUCTION import path --------------------------------------- +# This gates rather than warns. An installer that reports success while the +# service still cannot import the module is precisely the failure mode above, +# and a warning would be read as noise. +echo "[4/4] Verifying the production interpreter can import screen_locker..." +if ! (cd /tmp && "$PROD_PYTHON" -c "import screen_locker" 2>/dev/null); then + echo "ERROR: $PROD_PYTHON cannot import screen_locker after install." >&2 + echo " The systemd service uses this interpreter, so it would" >&2 + echo " crash-loop with ModuleNotFoundError. Refusing to report" >&2 + echo " success." >&2 + exit 1 +fi +if ! (cd /tmp && "$PROD_PYTHON" -c "import gatelock" 2>/dev/null); then + echo "ERROR: $PROD_PYTHON cannot import gatelock (the shared lock" >&2 + echo " backend). Install it editable from ~/utils/gatelock." >&2 + exit 1 +fi +if [[ -d "$GATELOCK_DIR" ]]; then + gatelock_path="$(cd /tmp && "$PROD_PYTHON" -c \ + "import gatelock; print(gatelock.__file__)")" + if [[ "$gatelock_path" != "$GATELOCK_DIR"* ]]; then + echo "ERROR: gatelock resolves to $gatelock_path, not the local" >&2 + echo " clone at $GATELOCK_DIR. The pinned wheel won, so local" >&2 + echo " gatelock changes would not reach the service." >&2 + exit 1 + fi + echo " ok: gatelock resolves to the local editable clone" +fi +echo " ok: screen_locker and gatelock both import under $PROD_PYTHON" + +echo +echo "=== Done ===" +echo "The locker is live. Check when it next fires with:" +echo " systemctl --user list-timers | grep -i workout" diff --git a/pyproject.toml b/pyproject.toml index cfd95ec..a0f850e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,7 +7,7 @@ requires-python = ">=3.10" # crdt-sync: shared CRDT merge scheme + GitHub-Contents-API sync transport, # also used by diet_guard; here for phone-workout GitHub sync (Workstream C) dependencies = [ - "gatelock @ git+https://github.com/kuhyx/utils@gatelock-v0.2.1#subdirectory=gatelock", + "gatelock @ git+https://github.com/kuhyx/utils@gatelock-v0.3.0#subdirectory=gatelock", "crdt-sync @ git+https://github.com/kuhyx/utils@crdt-sync-v0.1.0#subdirectory=crdt-sync", ] diff --git a/requirements.txt b/requirements.txt index 2e7f9bd..0e74333 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,7 +4,7 @@ bandit>=1.7.0 codespell>=2.2.0 coverage>=7.4.0 crdt-sync @ git+https://github.com/kuhyx/utils@crdt-sync-v0.1.0#subdirectory=crdt-sync -gatelock @ git+https://github.com/kuhyx/utils@gatelock-v0.2.1#subdirectory=gatelock +gatelock @ git+https://github.com/kuhyx/utils@gatelock-v0.3.0#subdirectory=gatelock mcp>=1.9 mypy>=1.8.0 pre-commit>=3.6.0 diff --git a/screen_locker/_heat_skip.py b/screen_locker/_heat_skip.py index 5b4921d..32e0408 100644 --- a/screen_locker/_heat_skip.py +++ b/screen_locker/_heat_skip.py @@ -5,7 +5,7 @@ from __future__ import annotations import logging import tkinter as tk -from gatelock import LockConfig +from gatelock import LockConfig, bind_activate, bind_cancel from screen_locker._constants import HEAT_SKIP_CITY, HEAT_SKIP_TEMP_THRESHOLD @@ -83,7 +83,7 @@ class HeatSkipMixin: result[0] = False root.destroy() - tk.Button( + skip_button = tk.Button( btn_frame, text="Skip workout", command=_on_skip, @@ -94,9 +94,11 @@ class HeatSkipMixin: padx=16, pady=8, relief="flat", - ).pack(side="left", padx=12) + **_COLORS.focus_kwargs(), + ) + skip_button.pack(side="left", padx=12) - tk.Button( + decline_button = tk.Button( btn_frame, text="No, I'll workout", command=_on_no, @@ -107,7 +109,21 @@ class HeatSkipMixin: padx=16, pady=8, relief="flat", - ).pack(side="left", padx=12) + **_COLORS.focus_kwargs(), + ) + decline_button.pack(side="left", padx=12) + + # This modal grabs input and sits fullscreen with no window decoration, + # so the keyboard is the only way out for a pointerless user. It used to + # focus_force() the root and stop there: nothing was focused, no ring was + # visible (Tk's default is black on bg), Enter did nothing because Tk + # binds only on Button, and there was no . Default focus + # goes to the *declining* option, so a blind Return keeps the workout + # rather than silently skipping it. + for button in (skip_button, decline_button): + bind_activate(button) + bind_cancel(root, _on_no) + decline_button.focus_set() root.mainloop() diff --git a/screen_locker/_manual_workout_dialog.py b/screen_locker/_manual_workout_dialog.py index 2fcf24e..6345986 100644 --- a/screen_locker/_manual_workout_dialog.py +++ b/screen_locker/_manual_workout_dialog.py @@ -64,43 +64,34 @@ class ManualWorkoutDialogMixin(ManualWorkoutFormWidgetsMixin): self._build_manual_workout_form() def _mw_scrollable_form(self) -> tk.Frame: - """Create the scrollable canvas + inner two-column form frame. + """Create the two-column form frame inside the surface's viewport. - The canvas is given a concrete size from the toplevel so the fullscreen - lock screen's centered container can't shrink-wrap to a narrow column - (leaving huge empty margins); the ```` binding then stretches - the inner form to the viewport so its two columns split the full width. - Harmless on StatusWindow, whose container already fills its window. + This used to build its *own* ``Canvas`` + ``Scrollbar`` viewport, which + had three problems that are all gone by deletion now that the surface + container is itself a :class:`~gatelock.ScrollableSurface`: + + 1. **Scrolling was pointer-only.** ``tk.Canvas`` has no class-level key + bindings, so ``::tk::FocusOK`` rejected it as a focus stop, and the + canvas bound no ````, ````/```` or arrows. + The scrollbar thumb had to be *dragged* -- inside a lock that cannot + be dismissed without submitting this form. + 2. **Focus walked off-screen.** Canvas clipping does not unmap a child, + so every below-the-fold field stayed ``winfo viewable`` and stayed in + the tab ring while Tk never scrolled to follow focus. Tab led to + fields the user could neither see nor bring into view. + 3. **The height was a magic fraction.** ``height=0.7 * toplevel`` sat + above ~230-290px of fixed chrome (title, budget line, error label, + and a 24pt button row), so the constraint was ``0.3 * H >= chrome``: + satisfied at 1080p, violated at 768p, where SUBMIT/BACK were pushed + off the bottom of a centred, unscrollable container. + + Still built on the primary surface only -- an independently scrolled + copy per monitor would show two different parts of one form. """ - # The scrolling form is built on the primary surface only: it is a - # canvas-and-scrollbar viewport sized from one toplevel's geometry, - # and mirroring an independently-scrolled copy per monitor would - # show two different parts of the same form. - outer = tk.Frame(self.container.first, bg=self._colors.bg) - outer.pack(fill="both", expand=True, pady=10) - canvas = tk.Canvas(outer, bg=self._colors.bg, highlightthickness=0) - scrollbar = tk.Scrollbar(outer, orient="vertical", command=canvas.yview) - form = tk.Frame(canvas, bg=self._colors.bg) + form = tk.Frame(self.container.first, bg=self._colors.bg) + form.pack(fill="both", expand=True, pady=10) form.grid_columnconfigure(0, weight=1, uniform="mw") form.grid_columnconfigure(1, weight=1, uniform="mw") - form.bind( - "", - lambda _e: canvas.configure(scrollregion=canvas.bbox("all")), - ) - self._mw_form_window = canvas.create_window((0, 0), window=form, anchor="nw") - canvas.bind( - "", - lambda e: canvas.itemconfig(self._mw_form_window, width=e.width), - ) - canvas.configure(yscrollcommand=scrollbar.set) - canvas.pack(side="left", fill="both", expand=True) - scrollbar.pack(side="right", fill="y") - top = self.container.winfo_toplevel() - top.update_idletasks() - canvas.configure( - width=int(top.winfo_width() * 0.9), - height=int(top.winfo_height() * 0.7), - ) return form def _build_manual_workout_form(self) -> None: diff --git a/screen_locker/_manual_workout_widgets.py b/screen_locker/_manual_workout_widgets.py index 2e726ae..bb90152 100644 --- a/screen_locker/_manual_workout_widgets.py +++ b/screen_locker/_manual_workout_widgets.py @@ -13,6 +13,8 @@ from __future__ import annotations import tkinter as tk +from gatelock import escape_text_tab_trap + from screen_locker._constants import ( MANUAL_WORKOUT_RPE_MAX, MANUAL_WORKOUT_RPE_MIN, @@ -107,6 +109,7 @@ class ManualWorkoutFormWidgetsMixin: bg=self._colors.field_bg, fg=self._colors.fg, insertbackground=self._colors.fg, + **self._colors.focus_kwargs(), ) entry.pack(fill="x", pady=4) disable_paste(entry) @@ -135,6 +138,7 @@ class ManualWorkoutFormWidgetsMixin: textvariable=var, width=4, font=(self._colors.font_family, 16), + **self._colors.focus_kwargs(), ).pack(side="left", padx=5) self._mw_grid(parent, row) return var @@ -157,9 +161,15 @@ class ManualWorkoutFormWidgetsMixin: bg=self._colors.field_bg, fg=self._colors.fg, insertbackground=self._colors.fg, + **self._colors.focus_kwargs(), ) text_widget.pack(pady=4, fill="x") disable_paste(text_widget) + # Tk makes insert a literal tab and refocus the widget, and binds + # to nothing, so an untreated Text is a keyboard dead end -- + # and the only exits (Ctrl+Tab / Ctrl+Shift+Tab) are advertised nowhere. + # On this form that means never reaching SUBMIT. + escape_text_tab_trap(text_widget) self._mw_grid(parent, cell, full=True) return text_widget @@ -183,5 +193,6 @@ class ManualWorkoutFormWidgetsMixin: textvariable=self._mw_rpe_var, width=4, font=(self._colors.font_family, 16), + **self._colors.focus_kwargs(), ).pack(side="left", padx=5) self._mw_grid(parent, row) diff --git a/screen_locker/_sick_dialog.py b/screen_locker/_sick_dialog.py index 9785821..a7d5e7f 100644 --- a/screen_locker/_sick_dialog.py +++ b/screen_locker/_sick_dialog.py @@ -6,6 +6,8 @@ import logging import tkinter as tk from typing import TYPE_CHECKING +from gatelock import escape_text_tab_trap + from screen_locker import _sick_tracker from screen_locker._constants import ( COMMITMENT_PROMPT_TIMEOUT_SECONDS, @@ -145,12 +147,18 @@ class SickDialogMixin: bg=self._colors.field_bg, fg=self._colors.fg, insertbackground=self._colors.fg, + **self._colors.focus_kwargs(), ) ) ) text_widgets.pack(pady=8) for text_widget in text_widgets: _disable_paste(text_widget) + # Tk traps inside a Text (inserts a tab, refocuses itself) and + # no-ops , so without this a keyboard-only user who tabs + # into the justification box can never reach SUBMIT -- inside a lock + # whose only other exit is waiting out the countdown. + escape_text_tab_trap(text_widget) return text_widgets def _update_commitment_forced_delay(self) -> None: diff --git a/screen_locker/_ui_widgets.py b/screen_locker/_ui_widgets.py index d722866..fbfefa1 100644 --- a/screen_locker/_ui_widgets.py +++ b/screen_locker/_ui_widgets.py @@ -6,6 +6,8 @@ import contextlib import tkinter as tk from typing import TYPE_CHECKING +from gatelock import bind_activate, escape_text_tab_trap + if TYPE_CHECKING: from collections.abc import Callable @@ -28,8 +30,23 @@ class UIWidgetsMixin: """Mixin providing low-level widget creation helpers.""" def clear_container(self) -> None: - """Remove all widgets from the main container, on every monitor.""" + """Remove all widgets from the main container, on every monitor. + + Also schedules the surface viewports to re-settle once the new screen + has been painted. Every repaint in the app starts by calling this, so + hooking it here is what makes focus-following and scroll-reset + unforgettable -- there are ~19 repaint sites, and "remember to call + settle after painting" is a rule that would eventually be missed, on a + lock where a missed re-arm means unreachable content. + + ``after_idle`` is what defers the work until *after* the caller has + finished building widgets; running it inline here would settle an empty + container. Repeated calls within one repaint coalesce harmlessly. + """ self.container.clear() + settle = getattr(self, "settle_surfaces", None) + if settle is not None: + self.root.after_idle(settle) def _label( self, @@ -105,8 +122,20 @@ class UIWidgetsMixin: Padding follows the shared 2x horizontal:vertical ratio (rule 22); `relief="flat"` is the one depth technique every button in the app uses now (rule 27), matching what `_heat_skip.py` already did alone. + + Two keyboard properties are wired here rather than per call site, so a + new button cannot omit them: + + * A **visible focus ring** (`focus_kwargs`). Tk's default is a 1px + *black* ring, which on ``bg`` (#211D1B) is invisible -- and + `relief="flat"` removes the depth cue too, so an unringed button + gives a keyboard user no indication of where they are. + * **Enter activates.** Tk's X11 class bindings give ``Button`` only + ````; there is no ```` binding at all. Under a global + grab with no pointer, "the obvious key does nothing" is the + difference between satisfying the lock and being stuck at it. """ - return parent.child_widgets( + buttons = parent.child_widgets( tk.Button, text=text, font=(self._colors.font_family, 24, "bold"), @@ -118,7 +147,13 @@ class UIWidgetsMixin: relief="flat", padx=24, pady=12, + **self._colors.focus_kwargs(), ) + # Every per-monitor copy, not just `.first` -- one flow paints N screens + # and the user may be looking at any of them. + for button in buttons: + bind_activate(button) + return buttons def _button_row(self) -> FrameGroup: """Create and pack a horizontal button container on every monitor.""" @@ -163,9 +198,67 @@ class UIWidgetsMixin: bg=self._colors.field_bg, fg=self._colors.fg, insertbackground=self._colors.fg, + **self._colors.focus_kwargs(), ) entries.pack(side="top", anchor="w", pady=4) for entry in entries: disable_paste(entry) if focus: entries.first.focus_set() + + def _add_label_text( + self, + parent: FrameGroup, + *, + label: str, + height: int = 4, + ) -> WidgetGroup: + """Add a label + multi-line ``Text`` pair on every monitor. + + Use this instead of building a bare ``tk.Text``: it un-traps Tab. + Tk makes ```` insert a literal tab and refocus the widget, and + binds ```` to nothing, so an untreated multi-line field is a + keyboard dead end whose only exits (``Ctrl+Tab``/``Ctrl+Shift+Tab``) are + not advertised anywhere. On a lock surface that means a user who tabs + into it can never reach the submit button. + """ + row = parent.child_frame(bg=self._colors.bg) + row.pack(pady=8, fill="x") + row.child_widgets( + tk.Label, + text=label, + font=(self._colors.font_family, 14), + fg=self._colors.fg, + bg=self._colors.bg, + anchor="w", + ).pack(side="top", anchor="w") + boxes = row.child_widgets( + tk.Text, + width=60, + height=height, + wrap="word", + font=(self._colors.font_family, 14), + bg=self._colors.field_bg, + fg=self._colors.fg, + insertbackground=self._colors.fg, + **self._colors.focus_kwargs(), + ) + boxes.pack(side="top", anchor="w", pady=4) + for box in boxes: + disable_paste(box) + escape_text_tab_trap(box) + return boxes + + def _focus_first_button(self, row: FrameGroup) -> None: + """Give the primary surface's first button keyboard focus. + + The lock takes a global grab and there is no other window on screen, so + a surface that starts with *nothing* focused leaves the user pressing + keys into the void with no visible ring to hint where focus went. The + retry screen (TRY AGAIN / I'm sick / Log Manual Workout) had exactly + that problem, because ``on_focus_ready`` is a documented no-op. + """ + for child in row.first.winfo_children(): + if child.winfo_class() == "Button": + child.focus_set() + return diff --git a/screen_locker/_window_setup.py b/screen_locker/_window_setup.py index 7e9c037..4762b21 100644 --- a/screen_locker/_window_setup.py +++ b/screen_locker/_window_setup.py @@ -12,13 +12,38 @@ from __future__ import annotations import tkinter as tk from typing import TYPE_CHECKING -from gatelock import RANK_SCREEN_LOCKER, Arbiter, LockWindow +from gatelock import ( + RANK_SCREEN_LOCKER, + Arbiter, + LockWindow, + ScrollableSurface, +) from screen_locker._surface_group import FrameGroup if TYPE_CHECKING: from gatelock import SurfaceInfo +# Tk classes that accept keyboard focus by default. Checking ``takefocus`` alone +# is not enough: it is "" on most widgets, which means "ask ::tk::FocusOK", so an +# empty value indicates a default-focusable widget rather than an opted-out one. +_FOCUSABLE_CLASSES = frozenset( + {"Button", "Checkbutton", "Entry", "Listbox", "Radiobutton", "Spinbox", "Text"} +) + + +def _first_focusable(widget: tk.Misc) -> tk.Misc | None: + """Return the first descendant that would accept keyboard focus.""" + for child in widget.winfo_children(): + if str(child.cget("takefocus")) == "0": + continue + if child.winfo_class() in _FOCUSABLE_CLASSES: + return child + deeper = _first_focusable(child) + if deeper is not None: + return deeper + return None + class WindowSetupMixin: """Mixin providing the screen-locker-specific auxiliary windows.""" @@ -27,6 +52,26 @@ class WindowSetupMixin: # annotation mypy cannot infer the attribute's type through the mixin. container: FrameGroup + @property + def _scrollers(self) -> dict[str, ScrollableSurface]: + """Viewports for the live outputs, keyed by output name. + + Held so a repaint can reset the scroll position and re-arm + focus-following on the newly built widgets. + + Created lazily by this mixin rather than initialised by the host, so the + mixin owns its own state: ``build_surface`` can be called before the + host's ``__init__`` finishes (``LockWindow.setup()`` invokes it), and + requiring the host to remember an assignment is the kind of coupling + that breaks quietly when a new host appears. + """ + # hasattr rather than try/except: an AttributeError here would be a real + # bug worth surfacing, and catching it to mean "first call" is exactly + # the swallowed-exception pattern this repo bans. + if not hasattr(self, "_scroller_map"): + self._scroller_map: dict[str, ScrollableSurface] = {} + return self._scroller_map + def _build_lock_window(self) -> LockWindow: """Claim the screen at this app's rank and build the lock. @@ -47,43 +92,99 @@ class WindowSetupMixin: return lock def _ensure_container(self) -> None: - """Give the non-lock windows a container group of exactly one. + """Give the non-lock windows a scrollable container group of one. The verify and relaxed-day windows never build a lock, so nothing - called ``build_surface`` and the group is still empty. They are - ordinary single windows, and one frame is all they need. + called ``build_surface`` and the group is still empty. + + They get the same viewport the lock surfaces do rather than a bare + centred frame: their content already exceeds their window (see + :meth:`_setup_aux_window`), and a centred frame clips symmetrically + instead of scrolling. ``center_when_fits`` preserves the centred look + for the screens that do fit. """ - if not self.container.surfaces: - self.container = FrameGroup.single(self.root, bg=self._colors.bg) + if self.container.surfaces: + return + scroller = ScrollableSurface(self.root, self._colors, center_when_fits=True) + scroller.container.pack(fill="both", expand=True) + self._scrollers["__aux__"] = scroller + self.container = FrameGroup([scroller.content]) def build_surface(self, parent: tk.Misc, surface: SurfaceInfo) -> None: - """Add one centred container for a newly-live output. + """Add one scrollable, keyboard-reachable container for a live output. Only the container is built here, not the screen inside it: which screen is showing depends on flow state the lock knows nothing about, so the caller repaints through the usual flow entry point once the surface exists. + + This was a bare ``place(relx=0.5, rely=0.5, anchor="center")`` frame. + A ``place``d frame takes its *requested* size and is clipped by its + parent, so any screen taller than the display lost content off the top + **and** the bottom simultaneously -- the heading and the submit button + at once -- with no scrollbar and nothing to indicate anything was + missing. On a 768px panel several screens do exceed the display (the + status view stacks a data-independent seven-day block plus four + buttons), and this is a hard lock with a global grab and VT switching + disabled, so unreachable content is unrecoverable. + + ``center_when_fits`` keeps short screens looking exactly as they did. """ - frame = tk.Frame(parent, bg=self._colors.bg) - frame.place(relx=0.5, rely=0.5, anchor="center") - self.container.add(frame, surface.output_name) + scroller = ScrollableSurface(parent, self._colors, center_when_fits=True) + scroller.container.place(relx=0, rely=0, relwidth=1, relheight=1) + self._scrollers[surface.output_name] = scroller + self.container.add(scroller.content, surface.output_name) def teardown_surface(self, surface: SurfaceInfo) -> None: - """Forget the container for an output that went dark.""" + """Forget the container and viewport for an output that went dark.""" self.container.discard(surface.output_name) + self._scrollers.pop(surface.output_name, None) + + def settle_surfaces(self) -> None: + """Re-arm focus-following and reset scroll after a repaint. + + Called once a flow has finished painting a screen. Focus-following has + to be re-armed because it binds ```` per widget, and a repaint + replaces those widgets; the scroll reset matters because a new screen + should start at its top, not wherever the previous one was left. + """ + for scroller in self._scrollers.values(): + scroller.finalize() def on_focus_ready(self, surface: SurfaceInfo | None) -> None: - """Nothing to focus yet: the lock's first screen has no input field. + """Focus the first button so the lock is operable without a pointer. - This fires once, when the lock is mapped and grabbed, and at that - moment the screen is the phone-check status -- buttons only. The - screens that DO take typed input (the manual-workout form, the sick - dialog) are painted later, in response to a button, so each focuses - its own first field as it is built rather than relying on this hook. + This fires once, when the lock is mapped and grabbed. At that moment the + screen is the phone-check status -- buttons only -- which this hook used + to treat as "nothing to focus". That reasoning was wrong: buttons are + exactly what needs focusing. The lock holds a *global* grab with VT + switching disabled and there is no other window on screen, so starting + with nothing focused meant a keyboard-only user had to blind-Tab into a + ring that (before the focus-ring fix) was invisible black-on-black, with + no way to tell what was selected or whether the keypress had landed. - ``surface`` is None when no output is live at all -- the lock is - held with nothing to show, which is still a valid state. + The screens that take typed input (the manual-workout form, the sick + dialog) still focus their own first field as they are built; this only + covers the button-only screens that previously focused nothing. + + ``surface`` is None when no output is live at all -- the lock is held + with nothing to show, which is still a valid state and needs no focus. """ + if surface is None: + return + self._focus_first_focusable() + + def _focus_first_focusable(self) -> None: + """Focus the first keyboard-focusable widget on the primary surface. + + Walks the tree rather than taking a widget reference, because which + screen is painted depends on flow state this mixin does not track. + """ + if not self.container.surfaces: + return + found = _first_focusable(self.container.first) + if found is not None: + found.focus_set() def on_callback_error(self) -> None: """Surfaced via GateRoot's logging already; no extra action yet.""" @@ -93,9 +194,7 @@ class WindowSetupMixin: def _setup_verify_window(self) -> None: """Configure window for post-sick-day workout verification.""" - self.root.geometry("600x400") - self.root.configure(bg=self._colors.bg, cursor="arrow") - self.root.protocol("WM_DELETE_WINDOW", self.close) + self._setup_aux_window(min_width=600, min_height=400) def _setup_demo_close_button(self) -> None: """Add close button for demo mode.""" @@ -112,6 +211,31 @@ class WindowSetupMixin: def _setup_relaxed_day_window(self) -> None: """Configure a small non-locking window for the optional Tue-Thu prompt.""" - self.root.geometry("700x450") + self._setup_aux_window(min_width=700, min_height=450) + + def _setup_aux_window(self, *, min_width: int, min_height: int) -> None: + """Configure a non-locking window that grows to fit its own content. + + These two windows used to hardcode ``geometry("600x400")`` and + ``geometry("700x450")``. Both were too small for what they display: the + relaxed-day prompt's own button row requests **862px** against a 700px + window, so it was sheared at *both* edges -- centred content clipped + symmetrically, exactly as on the lock surfaces, and pre-dating the + keyboard work by a wide margin (Tk's default focus ring accounts for + 4px of it). + + So the size is left to Tk (``geometry("")`` means "fit the content") with + the old values kept only as a *minimum*, and clamped to the display so a + wide screen's content cannot push the window off-screen. Content that + still does not fit scrolls, because ``_ensure_container`` wraps these + windows in a viewport too. + """ self.root.configure(bg=self._colors.bg, cursor="arrow") self.root.protocol("WM_DELETE_WINDOW", self.close) + # "" hands sizing back to the geometry manager instead of pinning it. + self.root.geometry("") + self.root.minsize( + min(min_width, self.root.winfo_screenwidth()), + min(min_height, self.root.winfo_screenheight()), + ) + self.root.maxsize(self.root.winfo_screenwidth(), self.root.winfo_screenheight()) diff --git a/screen_locker/tests/conftest.py b/screen_locker/tests/conftest.py index a8c07d4..8d2f0e2 100644 --- a/screen_locker/tests/conftest.py +++ b/screen_locker/tests/conftest.py @@ -50,6 +50,11 @@ _TK_MODULES = ( "screen_locker.status_view", "screen_locker._heat_skip", "screen_locker._surface_group", + # The scroll viewport the lock surfaces are built from lives in gatelock + # and imports tkinter independently, so it needs the same mock -- otherwise + # container.first is a *real* tk.Frame whose winfo_children() cannot be + # stubbed, and every surface assertion breaks. + "gatelock._scrollable", ) __all__ = [ "FAKE_OUTPUTS", diff --git a/screen_locker/tests/test_verify_workout.py b/screen_locker/tests/test_verify_workout.py index 9bfa8a4..08a40fc 100644 --- a/screen_locker/tests/test_verify_workout.py +++ b/screen_locker/tests/test_verify_workout.py @@ -136,20 +136,30 @@ class TestVerifyOnlyInit: class TestSetupVerifyWindow: """Tests for _setup_verify_window.""" - def test_sets_geometry_and_protocol( + def test_sizes_itself_to_content_with_a_floor( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path, ) -> None: - """Verify window uses 600x400 geometry and WM_DELETE_WINDOW.""" + """Verify window fits its content, with 600x400 as a floor not a cap. + + This asserted ``geometry("600x400")``, which pinned the window to a size + smaller than what it displays -- the same defect that left the + relaxed-day prompt's 862px button row sheared inside a 700px window. + ``geometry("")`` hands sizing to the geometry manager, ``minsize`` keeps + the old dimensions as a floor, and ``maxsize`` stops a wide screen's + content pushing the window off-display. + """ locker = create_locker( mock_tk, tmp_path, verify_only=True, is_sick_day_log=True, ) - locker.root.geometry.assert_called_with("600x400") + locker.root.geometry.assert_called_with("") + locker.root.minsize.assert_called_once() + locker.root.maxsize.assert_called_once() locker.root.configure.assert_called_with( bg=locker._colors.bg, cursor="arrow",