From e5c4f51a6e468d5e929403e983bdf38582ca837f Mon Sep 17 00:00:00 2001 From: funman300 Date: Fri, 8 May 2026 16:50:59 -0700 Subject: [PATCH] =?UTF-8?q?feat(replay):=20wire=20=E2=86=90=20/=20?= =?UTF-8?q?=E2=86=92=20keyboard=20accelerators=20for=20paused=20stepping?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit → during a paused replay advances by one move (mirrors the Stop button's existing forward-step semantics). ← decrements the cursor and dispatches `UndoRequestEvent`, which the game's `handle_undo` reads next frame to reverse its most-recent move — hooking the existing undo system rather than replaying forward from cursor 0 (every replay-applied move pushes to the undo stack the same way a player move would, so undo is the right reversal primitive). Both accelerators are paused-only — backwards via a new `step_backwards_replay_playback` in `replay_playback.rs` that hard-gates with the same destructure pattern as `step_replay_playback`. Pressing → during running playback or ← at cursor 0 are silent no-ops; the player learns "pause first, then arrow." The mockup labels these `[← →] scrub` (continuous fast scan). Single-move step is the closest behaviour shippable today — continuous scrub would need either a key-held event source or an internal speed-up loop. Footer hint reads `[← →] step` to match what's wired rather than the aspirational "scrub." Footer hint extended in lockstep: `[SPACE] pause/resume · [ESC] stop · [← →] step` — the only-wired-keybinds discipline holds. ReplayOverlayPlugin gains `add_message::()` defensively so the plugin can run under MinimalPlugins without GamePlugin attached (idempotent registration; harmless when GamePlugin is also present). 6 new tests (2 hint pins + 4 keyboard scenarios) + 1 helper-pin update for the new hint string. Pre-existing flake noted: `daily_challenge_plugin::tests:: check_system_fires_warning_event_only_once_per_day` is failing because wall-clock UTC is currently within 30 minutes of midnight, inside the daily-expiry warning window the test asserts against. Verified pre-existing by stashing all changes and re-running — failure persists. Same shape as the `winnable_seed_search` flake the handoff documented earlier this session: time-dependent, deterministically passes under different clock conditions. Not introduced by this commit. Clippy clean. Co-Authored-By: Claude Opus 4.7 --- solitaire_engine/src/replay_overlay.rs | 198 ++++++++++++++++++++++-- solitaire_engine/src/replay_playback.rs | 48 +++++- 2 files changed, 236 insertions(+), 10 deletions(-) diff --git a/solitaire_engine/src/replay_overlay.rs b/solitaire_engine/src/replay_overlay.rs index 1cf42e9..571318c 100644 --- a/solitaire_engine/src/replay_overlay.rs +++ b/solitaire_engine/src/replay_overlay.rs @@ -28,9 +28,10 @@ use chrono::Datelike; use crate::font_plugin::FontResource; use crate::layout::LayoutResource; -use crate::events::{DrawRequestEvent, MoveRequestEvent}; +use crate::events::{DrawRequestEvent, MoveRequestEvent, UndoRequestEvent}; use crate::replay_playback::{ - step_replay_playback, stop_replay_playback, toggle_pause_replay_playback, ReplayPlaybackState, + step_backwards_replay_playback, step_replay_playback, stop_replay_playback, + toggle_pause_replay_playback, ReplayPlaybackState, }; use solitaire_data::ReplayMove; use crate::ui_modal::{spawn_modal_button, ButtonVariant}; @@ -275,6 +276,7 @@ impl Plugin for ReplayOverlayPlugin { // in production (alongside `replay_playback`) is harmless. app.add_message::() .add_message::() + .add_message::() .add_systems( Update, ( @@ -288,6 +290,7 @@ impl Plugin for ReplayOverlayPlugin { handle_step_button, handle_pause_keyboard, handle_stop_keyboard, + handle_arrow_keyboard, handle_stop_button, ) .chain(), @@ -786,12 +789,11 @@ 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 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). +/// pause/resume, the ESC accelerator for stop, and the ← / → +/// accelerators for paused single-move stepping. The footer never +/// lists unimplemented keybinds (would lie to users). fn keybind_footer_hint_text() -> &'static str { - "[SPACE] pause/resume \u{00B7} [ESC] stop" // · separator + "[SPACE] pause/resume \u{00B7} [ESC] stop \u{00B7} [\u{2190}\u{2192}] step" // · separator } /// Pure helper — returns the WIN MOVE marker's left-edge position as @@ -1094,6 +1096,37 @@ fn handle_pause_keyboard( toggle_pause_replay_playback(&mut state); } +/// Watches the arrow keys for the paused single-step +/// accelerators. UI-first contract from CLAUDE.md §3.3 is +/// satisfied by the on-screen Step button (forward only); these +/// are the optional accelerators that also surface a backwards +/// step. +/// +/// Both keys are paused-only — the underlying step helpers +/// hard-gate via destructure on `paused: true`. Pressing → during +/// running playback or ← at cursor 0 are silent no-ops; the +/// player learns "pause first, then arrow." +/// +/// The mockup labels these `[← →] scrub` (continuous fast scan). +/// Single-move step is the closest behaviour shippable today; the +/// footer hint reads `[← →] step` to match what's wired rather +/// than the aspirational "scrub." +fn handle_arrow_keyboard( + keys: Option>>, + mut state: ResMut, + mut moves_writer: MessageWriter, + mut draws_writer: MessageWriter, + mut undo_writer: MessageWriter, +) { + let Some(keys) = keys else { return }; + if keys.just_pressed(KeyCode::ArrowRight) { + step_replay_playback(&mut state, &mut moves_writer, &mut draws_writer); + } + if keys.just_pressed(KeyCode::ArrowLeft) { + step_backwards_replay_playback(&mut state, &mut undo_writer); + } +} + /// 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. @@ -1958,8 +1991,8 @@ mod tests { ); assert_eq!( keybind_footer_hint_text(), - "[SPACE] pause/resume \u{00B7} [ESC] stop", - "hint text must list both wired keybinds (Space → pause/resume, Esc → stop) separated by a middle dot", + "[SPACE] pause/resume \u{00B7} [ESC] stop \u{00B7} [\u{2190}\u{2192}] step", + "hint text must list all three wired keybind groups (Space → pause/resume, Esc → stop, ←→ → step) separated by middle dots", ); } @@ -2313,6 +2346,153 @@ mod tests { ); } + /// Hint must also list the arrow-key step accelerators. + /// Pinned separately from the Space + Esc test so a future + /// regression that drops only the arrows is caught here even + /// if the Space + Esc check still passes. + #[test] + fn keybind_footer_hint_lists_arrow_steps() { + let hint = keybind_footer_hint_text(); + assert!( + hint.contains("\u{2190}\u{2192}"), + "hint must surface the ←→ step accelerators; got {hint:?}", + ); + assert!( + hint.contains("step"), + "hint must label the arrow accelerators as 'step' \ + (matches what's wired — single-move step, not continuous scrub); got {hint:?}", + ); + } + + /// Pressing → while paused advances the cursor by exactly one + /// — same end-state as clicking the on-screen Step button. + #[test] + fn arrow_right_keyboard_advances_cursor_while_paused() { + let mut app = headless_app(); + app.init_resource::>(); + set_state(&mut app, pressed_paused_state(5, 0)); + app.update(); + + app.world_mut() + .resource_mut::>() + .press(KeyCode::ArrowRight); + app.update(); + + match app.world().resource::() { + ReplayPlaybackState::Playing { cursor, paused, .. } => { + assert_eq!( + *cursor, 1, + "→ must advance the cursor by exactly one while paused", + ); + assert!( + *paused, + "→ must leave the paused flag untouched", + ); + } + other => panic!("expected Playing, got {other:?}"), + } + } + + /// Pressing → while running is a no-op — the existing + /// `step_replay_playback` guard prevents racing the tick loop. + #[test] + fn arrow_right_keyboard_is_noop_while_running() { + let mut app = headless_app(); + app.init_resource::>(); + set_state(&mut app, running_state(5, 0)); + app.update(); + + app.world_mut() + .resource_mut::>() + .press(KeyCode::ArrowRight); + app.update(); + + match app.world().resource::() { + ReplayPlaybackState::Playing { cursor, paused, .. } => { + assert_eq!(*cursor, 0, "→ must not race the tick loop"); + assert!(!*paused); + } + other => panic!("expected Playing, got {other:?}"), + } + } + + /// Pressing ← while paused with cursor > 0 decrements the + /// cursor by exactly one. The corresponding game-state reversal + /// happens when `handle_undo` reads the dispatched + /// `UndoRequestEvent` — that's covered in the playback core's + /// integration test, not here. + #[test] + fn arrow_left_keyboard_decrements_cursor_while_paused() { + let mut app = headless_app(); + app.init_resource::>(); + // Start paused at cursor=3 so there's room to step backwards. + set_state(&mut app, pressed_paused_state(5, 3)); + app.update(); + + app.world_mut() + .resource_mut::>() + .press(KeyCode::ArrowLeft); + app.update(); + + match app.world().resource::() { + ReplayPlaybackState::Playing { cursor, paused, .. } => { + assert_eq!( + *cursor, 2, + "← must decrement the cursor by exactly one while paused", + ); + assert!( + *paused, + "← must leave the paused flag untouched", + ); + } + other => panic!("expected Playing, got {other:?}"), + } + } + + /// Pressing ← at cursor 0 is a no-op (nothing to rewind past). + #[test] + fn arrow_left_keyboard_is_noop_at_cursor_zero() { + let mut app = headless_app(); + app.init_resource::>(); + set_state(&mut app, pressed_paused_state(5, 0)); + app.update(); + + app.world_mut() + .resource_mut::>() + .press(KeyCode::ArrowLeft); + app.update(); + + match app.world().resource::() { + ReplayPlaybackState::Playing { cursor, .. } => { + assert_eq!(*cursor, 0, "← at cursor 0 must be a no-op"); + } + other => panic!("expected Playing, got {other:?}"), + } + } + + /// Pressing ← while running is a no-op — same hard-gate + /// rationale as the forward-step paused-only check. + #[test] + fn arrow_left_keyboard_is_noop_while_running() { + let mut app = headless_app(); + app.init_resource::>(); + set_state(&mut app, running_state(5, 3)); + app.update(); + + app.world_mut() + .resource_mut::>() + .press(KeyCode::ArrowLeft); + app.update(); + + match app.world().resource::() { + ReplayPlaybackState::Playing { cursor, paused, .. } => { + assert_eq!(*cursor, 3, "← must not race the tick loop"); + assert!(!*paused); + } + other => panic!("expected Playing, got {other:?}"), + } + } + #[test] fn space_keyboard_toggles_paused_flag() { let mut app = headless_app(); diff --git a/solitaire_engine/src/replay_playback.rs b/solitaire_engine/src/replay_playback.rs index d9afa5e..8ae4064 100644 --- a/solitaire_engine/src/replay_playback.rs +++ b/solitaire_engine/src/replay_playback.rs @@ -42,7 +42,7 @@ use bevy::prelude::*; use solitaire_data::{Replay, ReplayMove}; -use crate::events::{DrawRequestEvent, MoveRequestEvent, StateChangedEvent}; +use crate::events::{DrawRequestEvent, MoveRequestEvent, StateChangedEvent, UndoRequestEvent}; use crate::game_plugin::{GameMutation, RecordingReplay}; use crate::resources::GameStateResource; use crate::settings_plugin::SettingsResource; @@ -284,6 +284,52 @@ pub fn step_replay_playback( true } +/// Steps the replay **backwards** by exactly one move while paused. +/// +/// Strategy: the live game's undo system is the source of truth for +/// reversing moves. Every move the replay forward-stepped (via +/// [`step_replay_playback`] or the auto-advance loop in +/// [`tick_replay_playback`]) was dispatched as a canonical +/// [`MoveRequestEvent`] / [`DrawRequestEvent`], which the game +/// applied and pushed onto its undo stack. So a backwards step here +/// is simply: decrement the cursor (so the about-to-apply move +/// re-points at the one we're rewinding past) and fire an +/// [`UndoRequestEvent`] so the game reverses its most-recent move +/// next frame. +/// +/// Hard-gated to the paused state via destructure pattern — +/// matches the existing [`step_replay_playback`] gate so the +/// player can only scrub one direction at a time and the tick +/// loop never races a manual rewind. +/// +/// Returns `false` and is a no-op in three cases: +/// - State isn't `Playing` (no replay attached). +/// - State is `Playing` but not paused (the tick loop owns the cursor). +/// - Cursor is already at 0 (nothing to rewind past). +/// +/// Returns `true` on a successful step; the actual game-state +/// reversal happens next frame when `handle_undo` reads the +/// `UndoRequestEvent`. +pub fn step_backwards_replay_playback( + state: &mut ResMut, + undo_writer: &mut MessageWriter, +) -> bool { + let ReplayPlaybackState::Playing { + cursor, + paused: true, + .. + } = state.as_mut() + else { + return false; + }; + if *cursor == 0 { + return false; + } + *cursor -= 1; + undo_writer.write(UndoRequestEvent); + true +} + /// Tick system. Runs every frame; only does work when /// [`ReplayPlaybackState::is_playing`]. ///