Remove Saved* serde mirror types (migrate ReplayMove to upstream klondike serde) #89

New Issue

2026-06-11T22:31:30Z

funman300 commented

2026-06-11 22:31:30 +00:00

The Saved* mirror types in solitaire_core/src/klondike_adapter.rs (SavedKlondikePile, SavedInstruction, SavedTableau, SavedFoundation, SavedSkipCards, SavedKlondikePileStack, SavedTableauStack, SavedDstFoundation, SavedDstTableau + their From/TryFrom impls — ~240 lines) exist solely because upstream klondike lacked serde. It now has full serde: KlondikeInstruction has a hand-written impl and KlondikePile/Tableau/Foundation derive Serialize/Deserialize.

These mirrors are now pure wrapper duplication. They survive only because the persisted ReplayMove wire format still references SavedKlondikePile (solitaire_data/src/replay.rs), mirrored in solitaire_wasm, plus the v3 save-migration AnyInstruction::V3(SavedInstruction) path in game_state.rs.

Plan: bump the ReplayMove schema (v2 -> v3), switch its move fields from SavedKlondikePile to upstream klondike::KlondikePile serde, update the wasm mirror + in-repo JS (solitaire_server/web/game.js), keep a transparent v2->v3 deserialize shim for old replays, then delete the entire Saved* block.

Coupled with #83 (card.rs / card_to_id): the same ReplayMove/wasm format also carries card_to_id u32 ids; switching those to card_game::Card serde is the same wire-format change. Do #83 and this together as one 'ReplayMove v3' migration.

Wire-format change — needs the v2 replay-load shim so existing replays still open.

The `Saved*` mirror types in `solitaire_core/src/klondike_adapter.rs` (SavedKlondikePile, SavedInstruction, SavedTableau, SavedFoundation, SavedSkipCards, SavedKlondikePileStack, SavedTableauStack, SavedDstFoundation, SavedDstTableau + their From/TryFrom impls — ~240 lines) exist solely because upstream `klondike` lacked serde. It now has full serde: `KlondikeInstruction` has a hand-written impl and `KlondikePile`/`Tableau`/`Foundation` derive Serialize/Deserialize. These mirrors are now pure wrapper duplication. They survive only because the persisted **ReplayMove** wire format still references `SavedKlondikePile` (`solitaire_data/src/replay.rs`), mirrored in `solitaire_wasm`, plus the v3 save-migration `AnyInstruction::V3(SavedInstruction)` path in `game_state.rs`. **Plan:** bump the ReplayMove schema (v2 -> v3), switch its move fields from `SavedKlondikePile` to upstream `klondike::KlondikePile` serde, update the wasm mirror + in-repo JS (`solitaire_server/web/game.js`), keep a transparent v2->v3 deserialize shim for old replays, then delete the entire `Saved*` block. **Coupled with #83** (card.rs / card_to_id): the same ReplayMove/wasm format also carries `card_to_id` u32 ids; switching those to `card_game::Card` serde is the same wire-format change. Do #83 and this together as one 'ReplayMove v3' migration. Wire-format change — needs the v2 replay-load shim so existing replays still open.

funman300 commented

2026-06-12 01:56:35 +00:00

Investigated — this issue's premise is wrong; the Saved pile mirrors are NOT removable.* A multi-agent attempt was reverted after the build proved it.

Root cause: upstream klondike 0.4.0 provides serde for the config types and KlondikeInstruction (hand-written, serializes as a u16 int), but its pile-position types — Tableau, Foundation, KlondikePile, KlondikePileStack — have NO serde derive (bare #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] at lib.rs 142/168/188/292). The Saved* pile mirrors (SavedKlondikePile/SavedTableau/SavedFoundation) exist precisely to serialize pile positions for ReplayMove, and that need is unchanged. Migrating ReplayMove.from/to to KlondikePile fails to compile (KlondikePile: serde::Serialize not satisfied) in both solitaire_data and solitaire_wasm.

The Saved* instruction mirrors (SavedInstruction + SavedDst*/Saved*Stack) could be dropped in favour of KlondikeInstruction serde — but instruction_history() feeds the wasm move_history/debug_move_history JsValue API, so that too is a JS-facing wire-format change, not dead-code removal.

Proper unlock (upstream): add #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] to the four klondike pile types upstream (one line each, matching the config types). Then the entire Saved* block becomes genuinely deletable. Until then these mirrors are necessary glue.

Recommend: close as 'blocked on upstream klondike pile serde', or convert to an upstream klondike issue. The working tree is back at the green #82 commit; no changes landed.

**Investigated — this issue's premise is wrong; the Saved* pile mirrors are NOT removable.** A multi-agent attempt was reverted after the build proved it. Root cause: upstream `klondike 0.4.0` provides serde for the **config types** and `KlondikeInstruction` (hand-written, serializes as a u16 int), but its **pile-position types — `Tableau`, `Foundation`, `KlondikePile`, `KlondikePileStack` — have NO serde derive** (bare `#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]` at lib.rs 142/168/188/292). The `Saved*` pile mirrors (`SavedKlondikePile`/`SavedTableau`/`SavedFoundation`) exist precisely to serialize pile positions for `ReplayMove`, and that need is unchanged. Migrating `ReplayMove.from/to` to `KlondikePile` fails to compile (`KlondikePile: serde::Serialize` not satisfied) in both solitaire_data and solitaire_wasm. The `Saved*` **instruction** mirrors (`SavedInstruction` + `SavedDst*`/`Saved*Stack`) *could* be dropped in favour of `KlondikeInstruction` serde — but `instruction_history()` feeds the wasm `move_history`/`debug_move_history` JsValue API, so that too is a JS-facing wire-format change, not dead-code removal. **Proper unlock (upstream):** add `#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]` to the four klondike pile types upstream (one line each, matching the config types). Then the entire Saved* block becomes genuinely deletable. Until then these mirrors are necessary glue. Recommend: close as 'blocked on upstream klondike pile serde', or convert to an upstream klondike issue. The working tree is back at the green #82 commit; no changes landed.

funman300 commented

2026-06-12 02:59:19 +00:00

Unblocked — revised approach (per upstream author). Asked Rhys to add serde to the klondike pile types; his answer: "It shouldn't need to serialize those for any reason, they are only relevant at runtime." He's right — pile positions are transient; you persist the moves, not the board coordinates.

So the fix is to stop serialising pile coordinates and persist KlondikeInstruction instead (it already has serde). Concretely: ReplayMove was just the instruction history re-expressed in pile coords (instruction_history() -> ReplayMove::Move{from,to,count}, then converted back on playback) — pure round-trip overhead.

Plan now in progress:

solitaire_data::Replay.moves: Vec<ReplayMove> -> Vec<KlondikeInstruction>; delete the ReplayMove enum. StockClick becomes RotateStock (which already IS the stock-click input the engine resolves to draw/recycle).
Playback applies via GameState::apply_instruction; UI pile coords recovered via GameState::instruction_to_piles.
The entire Saved* mirror block + the v3 game_state AnyInstruction migration are deleted.
Hard bump on replays.json (no compat shim). JS is unaffected (it renders WASM snapshots, not raw replay moves).

Being implemented as a verified workspace change; no upstream klondike edits needed.

**Unblocked — revised approach (per upstream author).** Asked Rhys to add serde to the klondike pile types; his answer: *"It shouldn't need to serialize those for any reason, they are only relevant at runtime."* He's right — pile positions are transient; you persist the **moves**, not the board coordinates. So the fix is to stop serialising pile coordinates and persist `KlondikeInstruction` instead (it already has serde). Concretely: `ReplayMove` was just the instruction history re-expressed in pile coords (`instruction_history()` -> `ReplayMove::Move{from,to,count}`, then converted back on playback) — pure round-trip overhead. Plan now in progress: - `solitaire_data::Replay.moves`: `Vec<ReplayMove>` -> `Vec<KlondikeInstruction>`; delete the `ReplayMove` enum. `StockClick` becomes `RotateStock` (which already IS the stock-click input the engine resolves to draw/recycle). - Playback applies via `GameState::apply_instruction`; UI pile coords recovered via `GameState::instruction_to_piles`. - The entire `Saved*` mirror block + the v3 game_state `AnyInstruction` migration are deleted. - Hard bump on replays.json (no compat shim). JS is unaffected (it renders WASM snapshots, not raw replay moves). Being implemented as a verified workspace change; no upstream klondike edits needed.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: funman300/Ferrous-Solitaire#89