From 60d8c3361b712610d05d7c8d25aa562131cf8e83 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Mon, 13 Jul 2026 10:09:18 +0200 Subject: [PATCH] Redesign manual-workout form: two-column grid, drop phone-only fields The manual-workout evidence form (shown on the lock screen and the status window via the shared ManualWorkoutDialogMixin) was a cramped, narrow centered column of tiny inputs with large empty margins, and included two fields that make no sense to fill from a locked PC. - Remove the vestigial "Google Maps link" (location_maps_link) and "Screenshot path" (proof_screenshot_path) fields from the form, the ManualWorkoutDraft dataclass, and build_entry(). Both were written to workout_log.json but never read by any code. The now-empty "Evidence" section is gone too. reservation_phone is kept. - Lay the form out as a two-column grid that fills ~90% of the screen (the fullscreen container was shrink-wrapping to its widest child) with larger labels/inputs; reflection boxes span full width. Activity-specific fields now sit directly under their section header. - Extract the low-level field/grid builders into a new ManualWorkoutFormWidgetsMixin (_manual_workout_widgets.py) so the dialog module stays under the 400-line cap; the dialog mixin inherits it, so the host classes need no change. Tests updated for the removed fields and the pack->grid sport swap; added a unit test for the grid cursor's odd-row bump. Full suite green at 100%. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01PM3auNTifaVgcXWDizZrjH --- screen_locker/_manual_workout.py | 4 - screen_locker/_manual_workout_dialog.py | 170 ++++++----------- screen_locker/_manual_workout_widgets.py | 176 ++++++++++++++++++ screen_locker/tests/conftest.py | 8 +- screen_locker/tests/test_manual_workout.py | 4 - .../tests/test_manual_workout_dialog.py | 36 +++- 6 files changed, 264 insertions(+), 134 deletions(-) create mode 100644 screen_locker/_manual_workout_widgets.py diff --git a/screen_locker/_manual_workout.py b/screen_locker/_manual_workout.py index 4b250c2..20c1b3d 100644 --- a/screen_locker/_manual_workout.py +++ b/screen_locker/_manual_workout.py @@ -142,9 +142,7 @@ class ManualWorkoutDraft: went_well: str to_improve: str overall_feeling: str - location_maps_link: str = "" # optional — awkward to fill from a locked PC reservation_phone: str = "" - proof_screenshot_path: str = "" # optional — phone-oriented field techniques_practiced: str = "" warm_up_minutes: str = "" pain_or_injury: str = "none" @@ -278,11 +276,9 @@ def build_entry(draft: ManualWorkoutDraft) -> dict[str, object]: "end_time": draft.end_time.strip(), "duration_minutes": f"{duration:.1f}" if duration is not None else "", "location_name": draft.location_name.strip(), - "location_maps_link": draft.location_maps_link.strip(), "transport_method": draft.transport_method.strip(), "cost": draft.cost.strip(), "reservation_phone": draft.reservation_phone.strip(), - "proof_screenshot_path": draft.proof_screenshot_path.strip(), "rpe": int(draft.rpe), "techniques_practiced": draft.techniques_practiced.strip(), "warm_up_minutes": draft.warm_up_minutes.strip(), diff --git a/screen_locker/_manual_workout_dialog.py b/screen_locker/_manual_workout_dialog.py index a18f07a..857a9c4 100644 --- a/screen_locker/_manual_workout_dialog.py +++ b/screen_locker/_manual_workout_dialog.py @@ -23,10 +23,8 @@ from screen_locker import _manual_workout from screen_locker._constants import ( MANUAL_WORKOUT_DESCRIPTION_MIN_CHARS, MANUAL_WORKOUT_REFLECTION_MIN_CHARS, - MANUAL_WORKOUT_RPE_MAX, - MANUAL_WORKOUT_RPE_MIN, ) -from screen_locker._ui_widgets import disable_paste +from screen_locker._manual_workout_widgets import ManualWorkoutFormWidgetsMixin _logger = logging.getLogger(__name__) @@ -35,7 +33,7 @@ _SPORT_LABEL_TO_CODE = { } -class ManualWorkoutDialogMixin: +class ManualWorkoutDialogMixin(ManualWorkoutFormWidgetsMixin): """Renders the manual-workout evidence form and handles submission.""" def _show_manual_workout_form(self) -> None: @@ -60,22 +58,53 @@ class ManualWorkoutDialogMixin: self._text(_manual_workout.budget_summary(self.log_file), color="#88ccff") self._build_manual_workout_form() - def _build_manual_workout_form(self) -> None: - """Build the scrollable evidence form and submit/back buttons.""" + def _mw_scrollable_form(self) -> tk.Frame: + """Create the scrollable canvas + inner two-column form frame. + + 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. + """ outer = tk.Frame(self.container, bg="#1a1a1a") outer.pack(fill="both", expand=True, pady=10) canvas = tk.Canvas(outer, bg="#1a1a1a", highlightthickness=0) scrollbar = tk.Scrollbar(outer, orient="vertical", command=canvas.yview) form = tk.Frame(canvas, bg="#1a1a1a") + 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")), ) - canvas.create_window((0, 0), window=form, anchor="nw") + 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: + """Build the two-column evidence form and submit/back buttons. + + Large labels/inputs and a screen-filling grid (see + ``_mw_scrollable_form``) so the form is usable with a keyboard while + locked out, rather than a cramped narrow column. + """ + form = self._mw_scrollable_form() + + # Independent 2-column grid cursor per master (form + sport sub-frames). + self._mw_grid_counters: dict[int, int] = {} self._mw_vars: dict[str, tk.StringVar] = {} self._mw_int_vars: dict[str, tk.IntVar] = {} self._mw_rpe_var = tk.IntVar(value=5) @@ -88,10 +117,6 @@ class ManualWorkoutDialogMixin: self._mw_section(form, "Location & logistics") self._mw_vars["location_name"] = self._mw_entry(form, "Location name:") - self._mw_vars["location_maps_link"] = self._mw_entry( - form, - "Google Maps link (optional — easier to fill from your phone):", - ) self._mw_vars["transport_method"] = self._mw_entry( form, "How did you get there?" ) @@ -100,16 +125,18 @@ class ManualWorkoutDialogMixin: form, "Reservation phone number (if booked by phone, optional):" ) - self._mw_section(form, "Evidence") - self._mw_vars["proof_screenshot_path"] = self._mw_entry( - form, - "Screenshot path as proof of arrangement " - "(optional — phone-oriented field):", - ) - self._mw_section(form, "Activity details") + # Both sport sub-frames occupy the same full-width grid slot; only the + # selected one is shown (see _on_mw_sport_changed). + act_row = self._mw_next_full_row(form) self._mw_tt_frame = tk.Frame(form, bg="#1a1a1a") self._mw_other_frame = tk.Frame(form, bg="#1a1a1a") + for sport_frame in (self._mw_tt_frame, self._mw_other_frame): + sport_frame.grid_columnconfigure(0, weight=1, uniform="mwact") + sport_frame.grid_columnconfigure(1, weight=1, uniform="mwact") + sport_frame.grid( + row=act_row, column=0, columnspan=2, sticky="ew", padx=12, pady=6 + ) self._build_table_tennis_fields(self._mw_tt_frame) self._build_other_sport_fields(self._mw_other_frame) self._mw_rpe_row(form) @@ -158,28 +185,16 @@ class ManualWorkoutDialogMixin: width=12, ).pack(side="left", padx=10) - def _mw_section(self, parent: tk.Widget, title: str) -> None: - """Add a section heading inside the form.""" - tk.Label( - parent, - text=title, - font=("Arial", 16, "bold"), - fg="#88ccff", - bg="#1a1a1a", - anchor="w", - ).pack(fill="x", pady=(15, 2)) - def _mw_sport_row(self, parent: tk.Widget) -> None: """Add the sport-selector dropdown; swaps the activity-details section.""" self._mw_sport_var = tk.StringVar( value=_manual_workout.SPORT_LABELS[_manual_workout.SPORT_TABLE_TENNIS] ) row = tk.Frame(parent, bg="#1a1a1a") - row.pack(pady=5, fill="x") tk.Label( row, text="Sport:", - font=("Arial", 14), + font=("Arial", 16), fg="white", bg="#1a1a1a", ).pack(side="left", padx=5) @@ -189,18 +204,23 @@ class ManualWorkoutDialogMixin: *_manual_workout.SPORT_LABELS.values(), command=self._on_mw_sport_changed, ).pack(side="left", padx=5) + self._mw_grid(parent, row, full=True) def _on_mw_sport_changed(self, selected_label: str) -> None: - """Show the fields for the newly-selected sport, hide the other's.""" + """Show the fields for the newly-selected sport, hide the other's. + + Both frames share one grid slot; ``grid_remove`` hides a frame while + remembering its cell options so ``grid`` restores it in place. + """ sport = _SPORT_LABEL_TO_CODE.get( selected_label, _manual_workout.SPORT_TABLE_TENNIS ) if sport == _manual_workout.SPORT_TABLE_TENNIS: - self._mw_other_frame.pack_forget() - self._mw_tt_frame.pack(fill="x") + self._mw_other_frame.grid_remove() + self._mw_tt_frame.grid() else: - self._mw_tt_frame.pack_forget() - self._mw_other_frame.pack(fill="x") + self._mw_tt_frame.grid_remove() + self._mw_other_frame.grid() def _build_table_tennis_fields(self, parent: tk.Widget) -> None: """Build the table-tennis-specific score/equipment fields.""" @@ -224,82 +244,6 @@ class ManualWorkoutDialogMixin: parent, "Equipment used (optional):" ) - def _mw_entry(self, parent: tk.Widget, label: str) -> tk.StringVar: - """Add a label+entry field and return its backing StringVar.""" - var = tk.StringVar() - self._add_label_entry(parent, label=label, variable=var) - return var - - def _mw_int_field( - self, parent: tk.Widget, label: str, *, frm: int = 0, to: int = 99 - ) -> tk.IntVar: - """Add a label + numeric Spinbox field and return its backing IntVar.""" - var = tk.IntVar(value=0) - row = tk.Frame(parent, bg="#1a1a1a") - row.pack(pady=5, fill="x") - tk.Label( - row, - text=label, - font=("Arial", 14), - fg="white", - bg="#1a1a1a", - ).pack(side="left", padx=5) - tk.Spinbox( - row, - from_=frm, - to=to, - textvariable=var, - width=4, - font=("Arial", 14), - ).pack(side="left", padx=5) - return var - - def _mw_textbox(self, parent: tk.Widget, label: str) -> tk.Text: - """Add a label + multi-line Text field and return the widget.""" - tk.Label( - parent, - text=label, - font=("Arial", 14), - fg="white", - bg="#1a1a1a", - anchor="w", - ).pack(fill="x", pady=(5, 0)) - text_widget = tk.Text( - parent, - width=60, - height=4, - font=("Arial", 12), - bg="#2a2a2a", - fg="white", - insertbackground="white", - ) - text_widget.pack(pady=2, fill="x") - disable_paste(text_widget) - return text_widget - - def _mw_rpe_row(self, parent: tk.Widget) -> None: - """Add the RPE (rate of perceived exertion) spinbox row.""" - row = tk.Frame(parent, bg="#1a1a1a") - row.pack(pady=5, fill="x") - tk.Label( - row, - text=( - f"RPE — perceived exertion " - f"({MANUAL_WORKOUT_RPE_MIN}-{MANUAL_WORKOUT_RPE_MAX}):" - ), - font=("Arial", 14), - fg="white", - bg="#1a1a1a", - ).pack(side="left", padx=5) - tk.Spinbox( - row, - from_=MANUAL_WORKOUT_RPE_MIN, - to=MANUAL_WORKOUT_RPE_MAX, - textvariable=self._mw_rpe_var, - width=4, - font=("Arial", 14), - ).pack(side="left", padx=5) - def _current_mw_sport(self) -> str: """Return the internal sport code for the currently-selected label.""" return _SPORT_LABEL_TO_CODE.get( @@ -331,9 +275,7 @@ class ManualWorkoutDialogMixin: went_well=self._mw_text_widgets["went_well"].get("1.0", "end"), to_improve=self._mw_text_widgets["to_improve"].get("1.0", "end"), overall_feeling=self._mw_text_widgets["overall_feeling"].get("1.0", "end"), - location_maps_link=self._mw_vars["location_maps_link"].get(), reservation_phone=self._mw_vars["reservation_phone"].get(), - proof_screenshot_path=self._mw_vars["proof_screenshot_path"].get(), techniques_practiced=self._mw_vars["techniques_practiced"].get(), warm_up_minutes=self._mw_vars["warm_up_minutes"].get(), pain_or_injury=self._mw_vars["pain_or_injury"].get() or "none", diff --git a/screen_locker/_manual_workout_widgets.py b/screen_locker/_manual_workout_widgets.py new file mode 100644 index 0000000..5526cfa --- /dev/null +++ b/screen_locker/_manual_workout_widgets.py @@ -0,0 +1,176 @@ +"""Low-level widget/grid builders for the manual-workout evidence form. + +Split out of ``_manual_workout_dialog`` so the dialog module stays under the +repo's file-length cap. These helpers build individual form cells and place +them into a two-column grid; the orchestration (which fields, in what order, +and submission) lives in ``ManualWorkoutDialogMixin``, which inherits this. + +The host is expected to have initialised ``self._mw_grid_counters`` (per-master +grid cursors) and, for ``_mw_rpe_row``, ``self._mw_rpe_var``. +""" + +from __future__ import annotations + +import tkinter as tk + +from screen_locker._constants import ( + MANUAL_WORKOUT_RPE_MAX, + MANUAL_WORKOUT_RPE_MIN, +) +from screen_locker._ui_widgets import disable_paste + + +class ManualWorkoutFormWidgetsMixin: + """Two-column grid placement + individual field-cell builders.""" + + def _mw_grid( + self, parent: tk.Widget, widget: tk.Widget, *, full: bool = False + ) -> None: + """Place ``widget`` into ``parent``'s two-column grid cursor. + + A half-width item (``full=False``) fills the next cell (left, then + right); a ``full`` item starts a fresh row and spans both columns. Each + master keeps its own cursor (keyed by ``id(parent)``) so nested sport + frames grid independently of the main form. + """ + counters = self._mw_grid_counters + idx = counters.get(id(parent), 0) + if full and idx % 2 == 1: + idx += 1 # a full-width item can't share a row — bump to a new one + row, col = divmod(idx, 2) + widget.grid( + row=row, + column=col, + columnspan=2 if full else 1, + sticky="ew", + padx=12, + pady=6, + ) + counters[id(parent)] = idx + (2 if full else 1) + + def _mw_next_full_row(self, parent: tk.Widget) -> int: + """Reserve a fresh full-width row for a manually-gridded widget. + + Used for the twin sport frames, which share one slot and are gridded + directly (not through ``_mw_grid``); this advances the cursor past that + row so following fields resume cleanly. + """ + counters = self._mw_grid_counters + idx = counters.get(id(parent), 0) + if idx % 2 == 1: + idx += 1 + counters[id(parent)] = idx + 2 + return idx // 2 + + def _mw_section(self, parent: tk.Widget, title: str) -> None: + """Add a full-width section heading inside the form.""" + label = tk.Label( + parent, + text=title, + font=("Arial", 20, "bold"), + fg="#88ccff", + bg="#1a1a1a", + anchor="w", + ) + self._mw_grid(parent, label, full=True) + + def _mw_entry(self, parent: tk.Widget, label: str) -> tk.StringVar: + """Add a half-width label+entry cell and return its backing StringVar. + + Builds its own cell inline (rather than reusing ``_add_label_entry``, + which is pack-based and shared with the sick dialog) so it slots into + the two-column grid. Paste stays disabled via ``disable_paste``. + """ + var = tk.StringVar() + cell = tk.Frame(parent, bg="#1a1a1a") + tk.Label( + cell, + text=label, + font=("Arial", 16), + fg="white", + bg="#1a1a1a", + anchor="w", + ).pack(fill="x") + entry = tk.Entry( + cell, + textvariable=var, + font=("Arial", 18), + bg="#2a2a2a", + fg="white", + insertbackground="white", + ) + entry.pack(fill="x", pady=2) + disable_paste(entry) + self._mw_grid(parent, cell) + return var + + def _mw_int_field( + self, parent: tk.Widget, label: str, *, frm: int = 0, to: int = 99 + ) -> tk.IntVar: + """Add a half-width label + numeric Spinbox and return its IntVar.""" + var = tk.IntVar(value=0) + row = tk.Frame(parent, bg="#1a1a1a") + tk.Label( + row, + text=label, + font=("Arial", 16), + fg="white", + bg="#1a1a1a", + ).pack(side="left", padx=5) + tk.Spinbox( + row, + from_=frm, + to=to, + textvariable=var, + width=4, + font=("Arial", 16), + ).pack(side="left", padx=5) + self._mw_grid(parent, row) + return var + + def _mw_textbox(self, parent: tk.Widget, label: str) -> tk.Text: + """Add a full-width label + multi-line Text cell and return the widget.""" + cell = tk.Frame(parent, bg="#1a1a1a") + tk.Label( + cell, + text=label, + font=("Arial", 16), + fg="white", + bg="#1a1a1a", + anchor="w", + ).pack(fill="x", pady=(5, 0)) + text_widget = tk.Text( + cell, + height=3, + font=("Arial", 14), + bg="#2a2a2a", + fg="white", + insertbackground="white", + ) + text_widget.pack(pady=2, fill="x") + disable_paste(text_widget) + self._mw_grid(parent, cell, full=True) + return text_widget + + def _mw_rpe_row(self, parent: tk.Widget) -> None: + """Add the RPE (rate of perceived exertion) spinbox row.""" + row = tk.Frame(parent, bg="#1a1a1a") + tk.Label( + row, + text=( + f"RPE — perceived exertion " + f"({MANUAL_WORKOUT_RPE_MIN}-{MANUAL_WORKOUT_RPE_MAX}):" + ), + font=("Arial", 16), + fg="white", + bg="#1a1a1a", + ).pack(side="left", padx=5) + tk.Spinbox( + row, + from_=MANUAL_WORKOUT_RPE_MIN, + to=MANUAL_WORKOUT_RPE_MAX, + textvariable=self._mw_rpe_var, + width=4, + font=("Arial", 16), + ).pack(side="left", padx=5) + self._mw_grid(parent, row) diff --git a/screen_locker/tests/conftest.py b/screen_locker/tests/conftest.py index 3b24803..2d4acb9 100644 --- a/screen_locker/tests/conftest.py +++ b/screen_locker/tests/conftest.py @@ -28,14 +28,14 @@ if TYPE_CHECKING: from typing import Literal -# Every module that imports ``tkinter as tk`` and calls it directly. The UI was -# split across these modules, so each ``tk`` reference must be patched — both to -# guarantee no test can touch a real display and so a test holding ``mock_tk`` -# sees widgets created on that same mock (not a divergent autouse mock). +# Every module that imports ``tkinter as tk`` and calls it directly. The UI is +# split across these, so each ``tk`` must be patched — so no test touches a real +# display and a ``mock_tk`` holder sees widgets made on that same mock. _TK_MODULES = ( "screen_locker.screen_lock", "screen_locker._sick_dialog", "screen_locker._manual_workout_dialog", + "screen_locker._manual_workout_widgets", "screen_locker._ui_widgets", "screen_locker._window_setup", "screen_locker.status_view", diff --git a/screen_locker/tests/test_manual_workout.py b/screen_locker/tests/test_manual_workout.py index 52c229a..3ec3963 100644 --- a/screen_locker/tests/test_manual_workout.py +++ b/screen_locker/tests/test_manual_workout.py @@ -291,10 +291,6 @@ class TestBuildEntry: assert entry["location_name"] == "Solec" assert entry["racket"] == "spin" - def test_optional_location_maps_link_defaults_empty(self) -> None: - entry = build_entry(_tt_draft()) - assert entry["location_maps_link"] == "" - def test_pain_or_injury_defaults_to_none(self) -> None: entry = build_entry(_tt_draft()) assert entry["pain_or_injury"] == "none" diff --git a/screen_locker/tests/test_manual_workout_dialog.py b/screen_locker/tests/test_manual_workout_dialog.py index 841f90b..1087a99 100644 --- a/screen_locker/tests/test_manual_workout_dialog.py +++ b/screen_locker/tests/test_manual_workout_dialog.py @@ -50,8 +50,8 @@ class TestSportToggle: locker._mw_other_frame = MagicMock() locker._mw_sport_var = MagicMock() locker._on_mw_sport_changed("Other") - locker._mw_tt_frame.pack_forget.assert_called_once() - locker._mw_other_frame.pack.assert_called_once_with(fill="x") + locker._mw_tt_frame.grid_remove.assert_called_once() + locker._mw_other_frame.grid.assert_called_once_with() def test_selecting_table_tennis_shows_tt_frame( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path @@ -61,8 +61,8 @@ class TestSportToggle: locker._mw_other_frame = MagicMock() locker._mw_sport_var = MagicMock() locker._on_mw_sport_changed("Table tennis") - locker._mw_other_frame.pack_forget.assert_called_once() - locker._mw_tt_frame.pack.assert_called_once_with(fill="x") + locker._mw_other_frame.grid_remove.assert_called_once() + locker._mw_tt_frame.grid.assert_called_once_with() def test_unknown_label_defaults_to_table_tennis( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path @@ -75,6 +75,30 @@ class TestSportToggle: assert locker._current_mw_sport() == _manual_workout.SPORT_TABLE_TENNIS +class TestGridCursor: + """Tests for the two-column grid cursor helper _mw_next_full_row.""" + + def test_full_row_from_even_cursor_advances_by_a_row( + self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path + ) -> None: + locker = create_locker(mock_tk, tmp_path) + locker._mw_grid_counters = {} + parent = object() + # Fresh (even) cursor: reserves row 0, then row 1 on the next call. + assert locker._mw_next_full_row(parent) == 0 + assert locker._mw_next_full_row(parent) == 1 + + def test_full_row_from_odd_cursor_bumps_to_next_row( + self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path + ) -> None: + locker = create_locker(mock_tk, tmp_path) + parent = object() + # An odd cursor (a half-width cell already placed on the left) must be + # bumped to a fresh row before a full-width item is placed. + locker._mw_grid_counters = {id(parent): 1} + assert locker._mw_next_full_row(parent) == 1 + + class TestSubmitManualWorkoutForm: """Tests for _submit_manual_workout_form.""" @@ -90,11 +114,9 @@ class TestSubmitManualWorkoutForm: "start_time": "12:00", "end_time": "14:00", "location_name": "Osrodek Solec", - "location_maps_link": "", "transport_method": "bike", "cost": "60 PLN", "reservation_phone": "", - "proof_screenshot_path": "", "techniques_practiced": "", "warm_up_minutes": "", "pain_or_injury": "none", @@ -120,11 +142,9 @@ class TestSubmitManualWorkoutForm: "start_time", "end_time", "location_name", - "location_maps_link", "transport_method", "cost", "reservation_phone", - "proof_screenshot_path", "techniques_practiced", "warm_up_minutes", "pain_or_injury",