fix(history): DocumentSnapshot carries var_table — undo restores variables
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<Color>`
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.
This commit is contained in:
parent
6fd6f63a6f
commit
394ca2f829
|
|
@ -286,6 +286,13 @@ pub struct DocumentSnapshot {
|
|||
pub active_page_index: usize,
|
||||
pub selected: NodeId,
|
||||
pub selected_set: Vec<NodeId>,
|
||||
/// 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<String>,
|
||||
/// 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<crate::Color>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -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`.
|
||||
|
|
|
|||
Loading…
Reference in a new issue