From 394ca2f8295f099e4a25ea899db646dc743945f2 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Thu, 14 May 2026 19:04:30 +0800 Subject: [PATCH] =?UTF-8?q?fix(history):=20DocumentSnapshot=20carries=20va?= =?UTF-8?q?r=5Ftable=20=E2=80=94=20undo=20restores=20variables?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-gate caught a real correctness gap: the previous commit (3c2e7711) wired the ColorPicker's variable-mode commit through `set_color_hex` and pushed `pre_snap` onto undo when the colour changed. But `DocumentSnapshot` only covered `pages / active_page_index / selected / selected_set` — `var_table` wasn't in the snapshot, so `undo()` would restore pages + selection without touching the variable. Variable edits were effectively unrecoverable. `crates/openpencil-shell-core/src/document.rs`: - `DocumentSnapshot` grew `var_table: VariableTable`. Captures every Variable definition + themed entries + active_theme + fill_refs / stroke_refs at snapshot time, so undo can restore the complete colour-token graph. `crates/openpencil-shell-core/src/document/mutators.rs`: - `snapshot()` clones `var_table` into the captured snapshot. - `restore()` writes the snapshot's var_table back to the document. The single literal site for DocumentSnapshot stays consistent across all callers (`snapshot_for_history()`, `commit_history()`, `undo()`, `redo()` — all flow through `snapshot()` + `restore()`). `crates/openpencil-shell-core/src/document/color_picker.rs`: - Dropped the now-redundant `variable_pre_color: Option` parallel field from `ColorPickerState`. It was a workaround for snapshot not carrying var_table; now `close_color_picker` reads `snap.var_table.resolve_color(name)` directly to detect the pre-edit colour. Cleaner single source of truth. - The change-detection in `close_color_picker`'s variable branch now compares `snap.var_table.resolve_color(name)` against the live resolution. Same result as before, fewer parallel fields. Tests (1 added — the regression repro, 295 shell-core total): - `undo_after_variable_edit_restores_pre_edit_color` builds the exact codex BLOCK case: variable starts at `#ff8800`, picker opens, HSV drags to pure red, picker closes (history push). `doc.undo()` runs; the variable must resolve back to the original orange. Pre-fix the snapshot's pages + selection would restore but the variable would still read red — the undo entry was effectively broken. --- crates/openpencil-shell-core/src/document.rs | 12 +++-- .../src/document/color_picker.rs | 47 +++++++++++++++---- .../src/document/mutators.rs | 2 + 3 files changed, 46 insertions(+), 15 deletions(-) diff --git a/crates/openpencil-shell-core/src/document.rs b/crates/openpencil-shell-core/src/document.rs index 896c4d9b2..2787c9039 100644 --- a/crates/openpencil-shell-core/src/document.rs +++ b/crates/openpencil-shell-core/src/document.rs @@ -286,6 +286,13 @@ pub struct DocumentSnapshot { pub active_page_index: usize, pub selected: NodeId, pub selected_set: Vec, + /// Variable table at snapshot time. Covers Variable definitions + /// + themed entries + active_theme + fill_refs / stroke_refs. + /// Without this, ColorPicker variable-mode edits would push an + /// undo entry that couldn't restore the variable (codex stop- + /// gate: "variable edits push undo entries that cannot restore + /// the variable"). + pub var_table: VariableTable, } /// Chrome-level UI state — toggles for collapsible chrome surfaces. @@ -477,11 +484,6 @@ pub struct ColorPickerState { /// the new field parallel avoids touching every ColorTarget /// pattern match. pub variable: Option, - /// In variable mode, the resolved colour captured at open - /// time so `close_color_picker` can detect "did the variable - /// actually change?" without requiring the document snapshot - /// to carry var_table. Unused in node-target mode. - pub variable_pre_color: Option, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/crates/openpencil-shell-core/src/document/color_picker.rs b/crates/openpencil-shell-core/src/document/color_picker.rs index 1d1404e85..fbfa5352a 100644 --- a/crates/openpencil-shell-core/src/document/color_picker.rs +++ b/crates/openpencil-shell-core/src/document/color_picker.rs @@ -29,7 +29,6 @@ impl Document { drag: None, anchor_y, variable: None, - variable_pre_color: None, }); true } @@ -63,8 +62,10 @@ impl Document { drag: None, anchor_y, variable: Some(name), - variable_pre_color: Some(current), }); + let _ = current; // resolved colour was just used to seed HSV + // above; close-time comparison now reads + // from the snapshot's var_table. true } @@ -112,14 +113,13 @@ impl Document { return false; }; if let Some(snap) = state.pre_snap { - // Variable-mode close: did the resolved colour - // actually change? `variable_pre_color` was captured - // when the picker opened; compare against the - // post-HSV-drag resolution. snapshot.var_table doesn't - // exist in the DocumentSnapshot shape, so we can't go - // through `snap`. + // Variable-mode close: compare the resolved colour + // BEFORE the picker opened (snap.var_table) against + // the post-drag resolution. DocumentSnapshot now + // carries `var_table` so undo can actually restore + // the variable (codex stop-gate fix). let changed = if let Some(name) = &state.variable { - let before = state.variable_pre_color; + let before = snap.var_table.resolve_color(name); let after = self.var_table.resolve_color(name); before != after } else { @@ -184,7 +184,7 @@ mod tests { assert!(doc.open_color_picker_for_variable("brand", 100.0)); let state = doc.ui.color_picker.as_ref().expect("picker open"); assert!(state.variable.as_deref() == Some("brand")); - assert!(state.variable_pre_color.is_some()); + assert!(state.pre_snap.is_some(), "undo snapshot must be captured"); // HSV anchor: hue near 32° for #ff8800 (orange). assert!(state.hue > 20.0 && state.hue < 45.0, "hue {}", state.hue); assert!(state.sat > 0.95, "sat {}", state.sat); @@ -221,6 +221,33 @@ mod tests { assert!(resolved.b < 0.01); } + #[test] + fn undo_after_variable_edit_restores_pre_edit_color() { + // Codex stop-gate: variable edits push undo entries that + // must actually restore the variable. Pre-fix the + // DocumentSnapshot didn't carry var_table, so the undo + // entry would restore pages + selection but leave the + // variable change in place. Now var_table is snapshotted + + // restored — undo round-trips the variable colour. + let mut doc = doc_with_color_var("brand", "#ff8800"); // orange + assert!(doc.open_color_picker_for_variable("brand", 0.0)); + // Drag HSV to pure red. + assert!(doc.color_picker_set_hsv(0.0, 1.0, 1.0)); + assert!(doc.close_color_picker()); + // Mid-state: variable resolves to red. + let after_edit = doc.var_table.resolve_color("brand").unwrap(); + assert!((after_edit.r - 1.0).abs() < 0.01 && after_edit.g < 0.01); + // Undo must restore the original orange. + assert!(doc.undo()); + let restored = doc.var_table.resolve_color("brand").unwrap(); + assert!( + (restored.r - 1.0).abs() < 0.01 + && (restored.g - 0x88 as f32 / 255.0).abs() < 0.01 + && restored.b < 0.01, + "undo must restore original #ff8800, got {restored:?}" + ); + } + #[test] fn close_color_picker_after_variable_edit_pushes_history_only_when_changed() { let mut doc = doc_with_color_var("brand", "#ff8800"); diff --git a/crates/openpencil-shell-core/src/document/mutators.rs b/crates/openpencil-shell-core/src/document/mutators.rs index 6fd62b0c1..ff7c3a24b 100644 --- a/crates/openpencil-shell-core/src/document/mutators.rs +++ b/crates/openpencil-shell-core/src/document/mutators.rs @@ -114,6 +114,7 @@ impl Document { active_page_index: self.active_page_index, selected: self.selected, selected_set: self.selected_set.clone(), + var_table: self.var_table.clone(), } } @@ -122,6 +123,7 @@ impl Document { self.active_page_index = snap.active_page_index; self.selected = snap.selected; self.selected_set = snap.selected_set; + self.var_table = snap.var_table; } /// Capture without pushing; use with `history_push_past`.