From 90e24d9711068c4cc6383cfd5c0e9b9c3d17254e Mon Sep 17 00:00:00 2001 From: funman300 Date: Fri, 8 May 2026 16:06:02 -0700 Subject: [PATCH] feat(replay): wire ESC accelerator for stop, gate pause modal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ESC during an active replay now stops it (mirrors the existing Stop button click). UI-first contract from CLAUDE.md §3.3 holds for the keyboard accelerator: every keybind the footer surfaces points at a wired action. Cross-plugin coordination: pause_plugin's `toggle_pause` already listens for ESC and would otherwise open the pause modal on the same press. Resolved by adding a fourth defer-if check to the existing modal-stack pattern in `toggle_pause` — `replay_state.is_some_and(|s| s.is_playing())` slots in right after `other_modal_scrims` and before `selection`. Symmetric shape to the existing forfeit / modal-scrim / selection / game-over / drag gates. Footer hint extended from `[SPACE] pause/resume` to `[SPACE] pause/resume · [ESC] stop` in lockstep — the "only-wired-keybinds" discipline holds. 3 new tests: - esc_keyboard_stops_active_replay (positive: Esc → Inactive, overlay despawns next frame) - esc_keyboard_is_noop_when_not_playing (negative: doesn't fire on Inactive state, lets global Esc listeners own those frames) - keybind_footer_hint_lists_space_and_esc (footer text contains both keybinds) Plus updated helper-pin test for the new hint string. Existing pause_plugin tests unaffected (they don't insert a ReplayPlaybackState resource so the new gate is a no-op for them). Tests: 1240 → 1243 (+3). Clippy clean. Co-Authored-By: Claude Opus 4.7 --- solitaire_engine/src/pause_plugin.rs | 11 +++ solitaire_engine/src/replay_overlay.rs | 121 +++++++++++++++++++++++-- 2 files changed, 126 insertions(+), 6 deletions(-) diff --git a/solitaire_engine/src/pause_plugin.rs b/solitaire_engine/src/pause_plugin.rs index ffb66a4..f84f373 100644 --- a/solitaire_engine/src/pause_plugin.rs +++ b/solitaire_engine/src/pause_plugin.rs @@ -30,6 +30,7 @@ use crate::events::{ use crate::font_plugin::FontResource; use crate::game_plugin::{GameOverScreen, GameStatePath}; use crate::progress_plugin::ProgressResource; +use crate::replay_playback::ReplayPlaybackState; use crate::resources::{DragState, GameStateResource}; use crate::selection_plugin::{SelectionKeySet, SelectionState}; use crate::settings_plugin::{SettingsChangedEvent, SettingsResource, SettingsStoragePath}; @@ -154,6 +155,7 @@ fn toggle_pause( mut drag: Option>, mut changed: MessageWriter, selection: Option>, + replay_state: Option>, ) { let PauseModalQueries { pause_screens: screens, @@ -184,6 +186,15 @@ fn toggle_pause( if !other_modal_scrims.is_empty() { return; } + // If a replay is currently playing, let `replay_overlay::handle_stop_keyboard` + // own the Esc press — that handler stops the replay. Without this guard a + // single Esc both stops the replay AND opens the pause modal on top of the + // (now empty) board, leaving the player on a screen they didn't ask for. + // The HUD-button path is gated too; clicking Pause while watching a replay + // is almost always an accident. + if replay_state.is_some_and(|s| s.is_playing()) { + return; + } // If a card is currently selected, let SelectionPlugin handle this Escape // (it will clear the selection). Pause must not also open in the same frame. if selection.is_some_and(|s| s.selected_pile.is_some()) { diff --git a/solitaire_engine/src/replay_overlay.rs b/solitaire_engine/src/replay_overlay.rs index cd8d059..cad10de 100644 --- a/solitaire_engine/src/replay_overlay.rs +++ b/solitaire_engine/src/replay_overlay.rs @@ -286,6 +286,7 @@ impl Plugin for ReplayOverlayPlugin { handle_pause_button, handle_step_button, handle_pause_keyboard, + handle_stop_keyboard, handle_stop_button, ) .chain(), @@ -776,11 +777,12 @@ fn keybind_footer_mode_text() -> &'static str { /// Pure helper — returns the keybind-hint text shown on the right /// side of the keybind-hint footer row. Lists only the keys that /// are *actually wired* today: the Space accelerator for -/// pause/resume. Future commits that wire ESC for stop or ← / → for -/// scrub will extend this string — the footer never lists -/// unimplemented keybinds (would lie to users). +/// pause/resume and the ESC accelerator for stop. Future commits +/// that wire ← / → for prev/next move will extend this string — +/// the footer never lists unimplemented keybinds (would lie to +/// users). fn keybind_footer_hint_text() -> &'static str { - "[SPACE] pause/resume" + "[SPACE] pause/resume \u{00B7} [ESC] stop" // · separator } /// Pure helper — returns the WIN MOVE marker's left-edge position as @@ -1083,6 +1085,37 @@ fn handle_pause_keyboard( toggle_pause_replay_playback(&mut state); } +/// Watches `Esc` for the keyboard stop accelerator. UI-first +/// contract from CLAUDE.md §3.3 is satisfied by the on-screen +/// Stop button; this is the optional accelerator. +/// +/// Cross-plugin coordination: `pause_plugin::toggle_pause` also +/// listens for `Esc` and would otherwise open the pause modal on +/// the same press. The conflict is resolved by `toggle_pause` +/// gating itself on `ReplayPlaybackState::is_playing()` — +/// symmetrical to the existing `forfeit_screens` / +/// `other_modal_scrims` defer-if pattern in that system. So during +/// an active replay this handler owns the `Esc` press and the +/// pause modal stays closed. +/// +/// No-op when the playback isn't `Playing` (the resource may still +/// exist as `Inactive` or `Completed`; only `Playing` means a +/// replay is on screen for the player to stop). +fn handle_stop_keyboard( + mut commands: Commands, + keys: Option>>, + mut state: ResMut, +) { + let Some(keys) = keys else { return }; + if !keys.just_pressed(KeyCode::Escape) { + return; + } + if !state.is_playing() { + return; + } + stop_replay_playback(&mut commands, &mut state); +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -1916,8 +1949,8 @@ mod tests { ); assert_eq!( keybind_footer_hint_text(), - "[SPACE] pause/resume", - "hint text must list only wired keybinds (Space → pause/resume)", + "[SPACE] pause/resume \u{00B7} [ESC] stop", + "hint text must list both wired keybinds (Space → pause/resume, Esc → stop) separated by a middle dot", ); } @@ -2167,6 +2200,82 @@ mod tests { } } + /// Pressing Esc while a replay is playing resets the state to + /// `Inactive` (same end-state as clicking the Stop button). + /// Mirrors `space_keyboard_toggles_paused_flag` for the stop + /// accelerator. + #[test] + fn esc_keyboard_stops_active_replay() { + let mut app = headless_app(); + // The keyboard handler reads `Option>>` + // and no-ops when missing — provide it for this test. + app.init_resource::>(); + set_state(&mut app, running_state(5, 0)); + app.update(); + assert_eq!(overlay_root_count(&mut app), 1); + + app.world_mut() + .resource_mut::>() + .press(KeyCode::Escape); + app.update(); + + assert!( + matches!( + app.world().resource::(), + ReplayPlaybackState::Inactive + ), + "Esc must reset state to Inactive while replay is Playing", + ); + + // One more tick — `react_to_state_change` despawns the overlay + // in response to the state going Inactive. + app.update(); + assert_eq!( + overlay_root_count(&mut app), + 0, + "overlay must despawn the frame after Esc stops the replay", + ); + } + + /// Esc is a no-op when the replay isn't `Playing` — covers + /// `Inactive` (no replay attached) and `Completed` (auto-clear + /// underway). The handler must stay quiet so the global Esc + /// listeners (pause modal, etc.) own those frames. + #[test] + fn esc_keyboard_is_noop_when_not_playing() { + let mut app = headless_app(); + app.init_resource::>(); + // Resource defaults to Inactive — no replay attached. + app.update(); + + app.world_mut() + .resource_mut::>() + .press(KeyCode::Escape); + app.update(); + + // State stays Inactive — no spurious mutation. + assert!(matches!( + app.world().resource::(), + ReplayPlaybackState::Inactive + )); + } + + /// The keybind-footer hint text now lists both wired + /// accelerators (Space + Esc). Lock the format so a future edit + /// that drops one or the other has to also update this test. + #[test] + fn keybind_footer_hint_lists_space_and_esc() { + let hint = keybind_footer_hint_text(); + assert!( + hint.contains("[SPACE]"), + "hint must surface the Space accelerator; got {hint:?}", + ); + assert!( + hint.contains("[ESC]"), + "hint must surface the Esc accelerator; got {hint:?}", + ); + } + #[test] fn space_keyboard_toggles_paused_flag() { let mut app = headless_app();