From adb446049b131a6943f2cdca4b3338defc6b6b77 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 1 Nov 2026 07:54:14 -0800 Subject: [PATCH] Variables: Undo and Redo clear a highlight that names no variable Undo, Redo and a canceled undo group put back an earlier document but left the Variables panel highlight as it was. After renaming the highlighted variable and undoing, the highlight still named the new name: a variable defined later under that name showed highlighted without a click, and Delete removed it. Each of these paths now clears a highlight whose variable the restored document does not define, as Delete Variables and Load Variable Library already did. --- crates/engine/src/cmd/edit.rs | 2 + crates/engine/src/cmd/variablecmds.rs | 4 +- crates/engine/src/cmd/variablecmds/library.rs | 4 +- crates/engine/src/doc_state.rs | 8 ++++ crates/engine/src/session/execute.rs | 1 + crates/engine/src/session/undo.rs | 2 + crates/engine/src/tests/variables.rs | 43 +++++++++++++++++++ 7 files changed, 58 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 62a17bda3..f252d09ff 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -498,6 +498,7 @@ fn undo(s: &mut Session, _: &Value) -> Result { super::symboledit::follow_history(st, editing_symbol.as_ref()); leave_isolation_if_gone(st); super::globaledit::follow_history(st); + st.drop_stale_variables_highlight(); Ok(json!({ "undone": label })) } @@ -525,6 +526,7 @@ fn redo(s: &mut Session, _: &Value) -> Result { super::symboledit::follow_history(st, editing_symbol.as_ref()); leave_isolation_if_gone(st); super::globaledit::follow_history(st); + st.drop_stale_variables_highlight(); Ok(json!({ "redone": label })) } diff --git a/crates/engine/src/cmd/variablecmds.rs b/crates/engine/src/cmd/variablecmds.rs index 9e589e883..33799bc85 100644 --- a/crates/engine/src/cmd/variablecmds.rs +++ b/crates/engine/src/cmd/variablecmds.rs @@ -231,9 +231,7 @@ fn delete(s: &mut Session, p: &Value) -> Result { let n = s.edit("Delete Variables", |d, _| Ok(d.variables.prune(&names)))?; // A highlight of a variable that is gone highlights nothing. let st = s.doc_mut()?; - if st.variables_highlight.as_ref().is_some_and(|h| st.doc.variables.variable(h).is_none()) { - st.variables_highlight = None; - } + st.drop_stale_variables_highlight(); st.revision += 1; Ok(json!({"deleted": n})) } diff --git a/crates/engine/src/cmd/variablecmds/library.rs b/crates/engine/src/cmd/variablecmds/library.rs index 937d4155e..b410af042 100644 --- a/crates/engine/src/cmd/variablecmds/library.rs +++ b/crates/engine/src/cmd/variablecmds/library.rs @@ -277,9 +277,7 @@ pub(super) fn load_library(s: &mut Session, p: &Value) -> Result { })?; // A highlight of a variable that is gone highlights nothing. let st = s.doc_mut()?; - if st.variables_highlight.as_ref().is_some_and(|h| st.doc.variables.variable(h).is_none()) { - st.variables_highlight = None; - } + st.drop_stale_variables_highlight(); st.revision += 1; let skipped: Vec = lib.other.iter().map(|(name, t)| json!({"name": name, "trait": t})).collect(); let mut out = json!({ diff --git a/crates/engine/src/doc_state.rs b/crates/engine/src/doc_state.rs index 30cf1e7e8..dc1df88be 100644 --- a/crates/engine/src/doc_state.rs +++ b/crates/engine/src/doc_state.rs @@ -352,6 +352,14 @@ impl DocState { self.revision += 1; } } + /// Clear the Variables panel highlight when the document defines no variable of that name + /// (deleted, or taken out by Undo or Redo), so a variable defined later under that name is + /// not highlighted without a click. + pub(crate) fn drop_stale_variables_highlight(&mut self) { + if self.variables_highlight.as_ref().is_some_and(|h| self.doc.variables.variable(h).is_none()) { + self.variables_highlight = None; + } + } /// Count the document as modified, as if never saved (a document restored by Data Recovery). pub fn mark_unsaved(&mut self) { self.saved_doc = Arc::new(Document::new(1.0, 1.0)); diff --git a/crates/engine/src/session/execute.rs b/crates/engine/src/session/execute.rs index d3ffe4163..f4ebd87c4 100644 --- a/crates/engine/src/session/execute.rs +++ b/crates/engine/src/session/execute.rs @@ -219,6 +219,7 @@ impl Session { st.interaction = interaction; st.history = history; cmd::globaledit::follow_history(st); + st.drop_stale_variables_highlight(); st.revision += 1; } if let Some(i) = active.and_then(|uid| self.docs.iter().position(|d| d.uid == uid)) { diff --git a/crates/engine/src/session/undo.rs b/crates/engine/src/session/undo.rs index 2f750dc13..c5f3f1595 100644 --- a/crates/engine/src/session/undo.rs +++ b/crates/engine/src/session/undo.rs @@ -102,6 +102,7 @@ impl Session { deselect_hidden(&self.menu, &st.doc, &mut st.selection); st.sync_new_art(); cmd::globaledit::follow_history(st); + st.drop_stale_variables_highlight(); st.revision += 1; // The journal drops only the canceled groups' own entries and keeps the ones made in other documents meanwhile. journal.extend(g.journal); @@ -230,6 +231,7 @@ impl Session { st.doc = doc; st.selection = selection; cmd::globaledit::follow_history(st); + st.drop_stale_variables_highlight(); deselect_hidden(&menu, &st.doc, &mut st.selection); st.history.recost_last(&st.doc); st.revision += 1; diff --git a/crates/engine/src/tests/variables.rs b/crates/engine/src/tests/variables.rs index e1bfd3e2f..5e0176f70 100644 --- a/crates/engine/src/tests/variables.rs +++ b/crates/engine/src/tests/variables.rs @@ -893,6 +893,49 @@ fn highlight_follows_rename_and_clears_with_delete() { assert!(s.doc().unwrap().variables_highlight.is_none(), "a highlight of a deleted variable highlights nothing"); } +/// Undo of a rename takes the new name away, so the highlight that followed the rename goes +/// too: a variable defined later under that name is not highlighted, and Delete without names +/// does not delete it. +#[test] +fn undoing_a_rename_clears_the_highlight_of_the_new_name() { + let mut s = session(); + s.execute("variable.define", &json!({"name": "A", "kind": "text"})).unwrap(); + s.execute("variable.highlight", &json!({"variable": "A"})).unwrap(); + s.execute("variable.rename", &json!({"name": "A", "newName": "B"})).unwrap(); + s.execute("edit.undo", &json!({})).unwrap(); + assert!(s.doc().unwrap().variables_highlight.is_none()); + s.execute("variable.define", &json!({"name": "B", "kind": "visibility"})).unwrap(); + assert!(s.doc().unwrap().variables_highlight.is_none(), "B was never clicked"); + assert!(s.execute("variable.delete", &json!({})).is_err(), "no variable highlighted"); + let vars = s.execute("variable.list", &json!({})).unwrap(); + assert_eq!(vars["variables"].as_array().unwrap().len(), 2); +} + +/// Undo of New Variable, Redo of Delete Variables and a canceled undo group each leave the +/// highlighted name undefined, and the highlight goes with it. +#[test] +fn undo_redo_and_a_canceled_group_clear_a_highlight_of_a_variable_they_remove() { + let mut s = session(); + s.execute("variable.define", &json!({"name": "N", "kind": "text"})).unwrap(); + s.execute("variable.highlight", &json!({"variable": "N"})).unwrap(); + s.execute("edit.undo", &json!({})).unwrap(); + assert!(s.doc().unwrap().variables_highlight.is_none(), "undo of New Variable"); + + s.execute("edit.redo", &json!({})).unwrap(); + s.execute("variable.delete", &json!({"names": ["N"]})).unwrap(); + s.execute("edit.undo", &json!({})).unwrap(); + s.execute("variable.highlight", &json!({"variable": "N"})).unwrap(); + s.execute("edit.redo", &json!({})).unwrap(); + assert!(s.doc().unwrap().variables_highlight.is_none(), "redo of Delete Variables"); + + s.execute("edit.undoGroup.begin", &json!({})).unwrap(); + s.execute("variable.define", &json!({"name": "G", "kind": "text"})).unwrap(); + s.execute("variable.highlight", &json!({"variable": "G"})).unwrap(); + s.execute("edit.undoGroup.end", &json!({"cancel": true})).unwrap(); + assert!(s.doc().unwrap().doc.variables.variable("G").is_none(), "the cancel took G back"); + assert!(s.doc().unwrap().variables_highlight.is_none(), "a canceled undo group"); +} + /// A row that hides an object must be able to show it again, and hiding it on the artboard /// (as the Layers panel does) is what a capture records: a visibility variable reads and /// writes its object's own visibility, which therefore never makes the object off limits.