Merge pull request #5131 from dotCooCoo/fix/vars-p4

Variables: Undo and Redo clear a highlight that names no variable

crates/engine/src/cmd/edit.rs

@@ -498,6 +498,7 @@ fn undo(s: &mut Session, _: &Value) -> Result<Value> {
     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<Value> {
     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 }))
 }
 

crates/engine/src/cmd/variablecmds.rs

@@ -231,9 +231,7 @@ fn delete(s: &mut Session, p: &Value) -> Result<Value> {
     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}))
 }

crates/engine/src/cmd/variablecmds/library.rs

@@ -277,9 +277,7 @@ pub(super) fn load_library(s: &mut Session, p: &Value) -> Result<Value> {
     })?;
     // 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<Value> = lib.other.iter().map(|(name, t)| json!({"name": name, "trait": t})).collect();
     let mut out = json!({

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));

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)) {

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;

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.