From a77c47bea3ee048ca3065614f57306e0e950d007 Mon Sep 17 00:00:00 2001
From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com>
Date: Mon, 19 Oct 2026 09:49:10 +0800
Subject: [PATCH] M6: a move the destination refuses leaves the field in place
 (#2696)

Review on #2957: set_props took the field out of the tree before attach
checked the destination, so a refused move (a parent holding /MaxLen, say)
left it out of /Fields for a direct caller of the forms API. The read-only
check is now its own function, run before the field is detached.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 crates/forms/src/author.rs | 24 +++++++++++++++++-------
 crates/forms/src/tests.rs  | 10 ++++++++++
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/crates/forms/src/author.rs b/crates/forms/src/author.rs
index edb6f58f6..cdbd24793 100644
--- a/crates/forms/src/author.rs
+++ b/crates/forms/src/author.rs
@@ -545,13 +545,11 @@ fn child_named(doc: &Document, af: ObjRef, parent: Option<ObjRef>, name: &str) -
 /// the field a neutral one of its own, as `/Ff` and `/Q` can.
 const PASSED_VALUES: [&[u8]; 3] = [b"V", b"DV", b"MaxLen"];
 
-/// Put `field` in the field tree under the full name `parts`: its own `/T` is the last part,
-/// and the parts before it name parent fields, reused where the tree has them and made where
-/// not (Acrobat makes `a.b` a field `b` under a field `a`).
-fn attach(doc: &mut Document, af: ObjRef, field: ObjRef, parts: &[&str]) -> Result<(), FormError> {
+/// Whether `field` may go in the field tree under the full name `parts`, changing nothing. The
+/// field list holds only terminal fields, so a parent left empty by a move isn't in it: a node
+/// of the same name there is found here.
+fn check_place(doc: &Document, af: ObjRef, field: ObjRef, parts: &[&str]) -> Result<(), FormError> {
     let Some((own, parents)) = parts.split_last() else { return invalid("field names can't be empty") };
-    // First check the place, changing nothing. The field list holds only terminal fields, so a
-    // parent left empty by a move isn't in it: a node of the same name there is found here.
     let owned = doc.get(field).as_dict().cloned().unwrap_or_default();
     let mut at: Option<ObjRef> = None;
     let mut whole_path = true;
@@ -567,10 +565,20 @@ fn attach(doc: &mut Document, af: ObjRef, field: ObjRef, parts: &[&str]) -> Resu
         }
         at = Some(r);
     }
-    // `field` is new or already out of the tree, so a node found is another.
+    // A field being moved is still in the tree here, but under another full name (a move to its
+    // own name changes nothing and doesn't get here), so a node found is another.
     if whole_path && child_named(doc, af, at, own).is_some() {
         return invalid(format!("a field named {:?} already exists", parts.join(".")));
     }
+    Ok(())
+}
+
+/// Put `field` in the field tree under the full name `parts`: its own `/T` is the last part,
+/// and the parts before it name parent fields, reused where the tree has them and made where
+/// not (Acrobat makes `a.b` a field `b` under a field `a`).
+fn attach(doc: &mut Document, af: ObjRef, field: ObjRef, parts: &[&str]) -> Result<(), FormError> {
+    let Some((own, parents)) = parts.split_last() else { return invalid("field names can't be empty") };
+    check_place(doc, af, field, parts)?;
     let mut parent: Option<ObjRef> = None;
     let mut inherits = false;
     for part in parents {
@@ -1107,6 +1115,8 @@ pub fn set_props(doc: &mut Document, name: &str, props: &FieldProps) -> Result<S
             }
             let af = ensure_form(doc)?;
             own_inherited(doc, f.obj)?;
+            // Before it leaves its place, so a refused move leaves it where it was.
+            check_place(doc, af, f.obj, &parts)?;
             detach(doc, f.obj)?;
             attach(doc, af, f.obj, &parts)?;
             new_name = n.to_string();
diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs
index 2d9d08b2d..de37e418c 100644
--- a/crates/forms/src/tests.rs
+++ b/crates/forms/src/tests.rs
@@ -3163,6 +3163,16 @@ fn dotted_field_names_are_made_with_parent_fields() {
         set_props(&mut doc.clone(), "address.city", &FieldProps { name: Some("grp".into()), ..FieldProps::default() }).is_err(),
         "address.grp is taken"
     );
+    // A move the destination refuses leaves the field where it was (review on #2957): it was
+    // taken out of the tree before the destination was checked, and stayed out.
+    doc.update_dict(address, |d| d.set(b"MaxLen".to_vec(), Object::Int(3))).unwrap();
+    let at_top = top(&doc);
+    assert!(set_props(&mut doc, "name", &FieldProps { name: Some("address.name".into()), ..FieldProps::default() }).is_err());
+    assert_eq!(top(&doc), at_top, "name is still in the form's /Fields");
+    doc.update_dict(address, |d| {
+        d.remove(b"MaxLen");
+    })
+    .unwrap();
 
     for bad in ["MASTER", "MASTER.street1.x", "MASTER..x", ".x", "x.", "MASTER. "] {
         assert!(add_field(&mut doc, 0, [300.0, 200.0, 450.0, 220.0], &text, Some(bad)).is_err(), "{bad:?}");