app
KiCad - the KiCad Bridge
Public Made by Adomby adom
Reference implementation of the KiCad bridge: multi-instance Python server, forward path via kicad-cli, reverse path via in-process plugin. Most complex of the three bundled bridges.
← All Pull Requests
Fix post-commit deletion checks, zone refill accounting and saved-zone equality #13
Merged opened by John Lauer 2026-09-18
Merges
astra/native-commit-readback → masterKiCad defers DeleteItems until the native transaction commits. The previous zone and silkscreen paths read absence inside the transaction, saw the old board, and rolled back valid deletions. Both now verify absence after commit; failures report committed:true, success:false and postCommitDeletionError instead of pretending rollback. Removal-only text batches skip an empty bounds request.
Zone-plan refill is now a separate caller operation: default no refill, refillRequired:true after changes, and explicit refill:true refuses before mutation. This intentionally changes the previous implicit-refill behavior, keeping the zone transaction one Undo step with a usable revision. Call kicad_zone_state {refill:true} separately and use its completed revision. This is the leave-refilling-to-the-caller option from the #9 review.
Native saved-zone comparison normalizes metadata ordering, fill-state yes, the omitted legacy filled_areas_thickness no marker, zero priority and unlocked state. The offline candidate explicitly carries island_removal_mode 0, matching the existing IPC IRM_ALWAYS. Different island settings, geometry, holes, arcs and unknown fields remain significant; unknown defaults can still conservatively rewrite. Copper candidate DRC now matches the IPC island policy for add-zone callers too.
Base: 363e9080a6e9a3c5a36a53b79196152b0b037c72, current 1.0.22 plus merged PR #12. Real branch PR, not file-set replacement. No module, verb or existing test removed. Only the two changed IPC functions were rustfmt-formatted; unrelated source preserved.
Validation: production cargo check --workspace --bins passed; cargo test --workspace --lib --bins passed (25 bridge + 230 core). Added native-save normalization/idempotence and pre-IPC refill refusal tests; existing shape/rules/ownership tests retained. Current-head/immutable-base preflight passed. Combined source contains PRs #7-#11. Native Windows acceptance is NOT yet performed on this patch; no shared runtime replaced. docs/native-zone-plan.md specifies create/reapply/update/remove/Undo/revision and mixed-text-batch acceptance. Please run it on the owning development bridge before publication. Addresses review findings on #95/#104 but does not close those pending native acceptance.
Diff Skip to comments (1)
docs/native-zone-plan.mdadded+17@@ -0,0 +1,17 @@+# Native zone-plan reconciliation++Ownership is explicit in `ownedItemIds`; names do not confer ownership. Plans require `expectedRevision`, validate the full candidate with native DRC, and preserve foreign zones.++Deletion absence is read only AFTER commit. `deletionVerified` is true on confirmed absence. A failed check returns `postCommitDeletionError`, `success:false` and `committed:true`; do not blindly retry or assume rollback. The same rule applies to standalone silkscreen text removal. A removal-only text batch does not request bounding boxes for an empty ID list.++## Refill is a separate native operation++The zone-plan edit is one Undo step. It now defaults to no refill and returns `refilled:false`, `refillRequired:true` for a changed plan. Explicit `refill:true` refuses BEFORE mutation with `separate_refill_required`. After applying, call `kicad_zone_state` with `refill:true`, wait for completion, and use that operation's revision. Refilling can introduce its own Undo step. This intentionally changes the previous implicit-refill behavior that falsely reported one Undo step and a usable final revision.++## Equality after native serialization++Known non-geometric serialization differences are normalized: order of metadata, fill-state `yes`, omitted legacy `filled_areas_thickness no`, explicit zero priority and unlocked state. The offline copper candidate now explicitly requests `island_removal_mode 0`, matching IPC IRM_ALWAYS. Geometry, holes, arcs, layers, nets, thermal/clearance/island settings and unknown properties remain significant. This is not an area comparison. Unsupported unknown defaults can still conservatively cause a rewrite.++## Native acceptance required++On a disposable board: create, reapply the identical owned plan (expect unchanged IDs and zero Undo steps), update it, remove it, and undo each mutation once. Verify removed IDs absent after commit, foreign zones intact, and revision equals a fresh state read. Verify `refill:true` refuses without edits; run separate refill and read its revision. Repeat silkscreen removal with duplicate strings and a mixed create/update/remove batch, then one Undo. Source tests alone do not establish these native results.
rust/crates/kicad-bridge/src/verbs_routing.rs+2−2@@ -80,9 +80,9 @@ pub static VERBS: &[Verb] = &[ name: "kicad_apply_zone_plan", summary: "Reconcile a complete explicit-owned zone plan as one native Undo step after full-candidate DRC.", mechanism: Mechanism::Ipc, risk: "write", timeout_sec: 130,- input: "{filePath, expectedRevision, ownedItemIds:[], zones:[{name,net,layers,polygon,...}], dryRun?, refill?, save?}",+ input: "{filePath, expectedRevision, ownedItemIds:[], zones:[{name,net,layers,polygon,...}], dryRun?, refill:false?, save?}", example: "kicad_apply_zone_plan {filePath,expectedRevision,ownedItemIds:[],zones:[]}",- hint: "Use exact owned item IDs from prior successful replies. Unowned zones are preserved; ambiguous names refuse. Dry run has no edit or save. Returns unchangedIds, removedIds and ownedItemIds. Full native definitions, not area, determine equality.",+ hint: "Use exact owned item IDs from prior successful replies. Unowned zones are preserved; ambiguous names refuse. Dry run has no edit or save. Returns unchangedIds, removedIds and ownedItemIds. Native definitions with known serialization defaults normalized, not area, determine equality. This verb never refills; refill:true refuses before mutation. Call kicad_zone_state with refill:true separately and use its returned revision.", related: &["kicad_zone_state", "kicad_add_zone"], pitfalls: &["Desired holes and arcs are refused by this polygon request schema; existing unowned holes/arcs remain untouched.", "Inspect postCommitError before retrying a committed edit."], },
rust/crates/kicad-core/src/ipc.rs+271−70@@ -909,45 +909,131 @@ pub fn add_zone(ctx: &Ctx, args: &Value) -> RResult<Value> { /// Reconcile only explicitly owned zones in one native undo transaction. Always DRC /// the complete candidate first; preserve unrelated zones and require a revision. pub fn apply_zone_plan(ctx: &Ctx, args: &Value) -> RResult<Value> {- if args["expectedRevision"].as_str().filter(|s|!s.is_empty()).is_none() {- return Err(RoutingError::new("revision_required","Read kicad_zone_state first and pass its expectedRevision"));- }- let s=connect(ctx,args)?;- let (text,board,revision)=s.snapshot()?;check_revision(args,&revision)?;- let plan=crate::zone_plan::prepare(&text,&board,args)?;- let before=drc_snapshot(ctx.kicad_cli.as_deref(),&s.source_path(),&text)?;- let check=drc_snapshot(ctx.kicad_cli.as_deref(),&s.source_path(),&plan.candidate)?;- let added=new_errors(&before,&check)?;- if !added.is_empty(){return Err(RoutingError::new("drc_rejected","Full zone plan adds native DRC errors; nothing changed").with("violations",json!(added)).with("mutated",json!(false)));}- if args["dryRun"]==true || (plan.remove.is_empty() && plan.create.is_empty()) {- return Ok(json!({"success":true,"mutated":false,"dryRun":args["dryRun"]==true,"revision":revision,"unchangedIds":plan.unchanged,"wouldRemoveIds":plan.remove,"wouldCreate":plan.create.iter().map(zones::ZoneSpec::public).collect::<Vec<_>>(),"drc":check,"undoSteps":0}));- }- let nets=s.client.get_nets().map_err(map_err)?;let mut items=Vec::new();+ if args["expectedRevision"]+ .as_str()+ .filter(|s| !s.is_empty())+ .is_none()+ {+ return Err(RoutingError::new(+ "revision_required",+ "Read kicad_zone_state first and pass its expectedRevision",+ ));+ }+ // Refill is a separate asynchronous native edit and Undo step. Keep this+ // transaction's revision usable by leaving refill to an explicit caller step.+ if args["refill"] == true {+ return Err(RoutingError::new("separate_refill_required", "Apply with refill:false (the default), then call kicad_zone_state with refill:true and read its final revision").with("mutated", json!(false)));+ }+ let s = connect(ctx, args)?;+ let (text, board, revision) = s.snapshot()?;+ check_revision(args, &revision)?;+ let plan = crate::zone_plan::prepare(&text, &board, args)?;+ let before = drc_snapshot(ctx.kicad_cli.as_deref(), &s.source_path(), &text)?;+ let check = drc_snapshot(ctx.kicad_cli.as_deref(), &s.source_path(), &plan.candidate)?;+ let added = new_errors(&before, &check)?;+ if !added.is_empty() {+ return Err(RoutingError::new(+ "drc_rejected",+ "Full zone plan adds native DRC errors; nothing changed",+ )+ .with("violations", json!(added))+ .with("mutated", json!(false)));+ }+ if args["dryRun"] == true || (plan.remove.is_empty() && plan.create.is_empty()) {+ return Ok(+ json!({"success":true,"mutated":false,"dryRun":args["dryRun"]==true,"revision":revision,"unchangedIds":plan.unchanged,"wouldRemoveIds":plan.remove,"wouldCreate":plan.create.iter().map(zones::ZoneSpec::public).collect::<Vec<_>>(),"drc":check,"undoSteps":0}),+ );+ }+ let nets = s.client.get_nets().map_err(map_err)?;+ let mut items = Vec::new(); for z in &plan.create {- let net_code=if z.keepout.is_some(){0}else{nets.iter().find(|n|n.name==z.net.name).ok_or_else(||RoutingError::new("unknown_net",format!("No native net {:?}",z.net.name)))?.code};- let layers=z.layers.iter().map(|l|BoardLayerInfo::id_from_name(l).ok_or_else(||RoutingError::new("unknown_layer",l.clone()))).collect::<RResult<Vec<_>>>()?;- items.push(prost_types::Any{type_url:zones::ZONE_TYPE_URL.into(),value:zones::zone_bytes(z,&layers,net_code)});+ let net_code = if z.keepout.is_some() {+ 0+ } else {+ nets.iter()+ .find(|n| n.name == z.net.name)+ .ok_or_else(|| {+ RoutingError::new("unknown_net", format!("No native net {:?}", z.net.name))+ })?+ .code+ };+ let layers = z+ .layers+ .iter()+ .map(|l| {+ BoardLayerInfo::id_from_name(l)+ .ok_or_else(|| RoutingError::new("unknown_layer", l.clone()))+ })+ .collect::<RResult<Vec<_>>>()?;+ items.push(prost_types::Any {+ type_url: zones::ZONE_TYPE_URL.into(),+ value: zones::zone_bytes(z, &layers, net_code),+ }); }- check_revision(args,&s.revision()?)?;- let created=transaction(&s,"Adom apply zone plan",||{- if !plan.remove.is_empty(){+ check_revision(args, &s.revision()?)?;+ let created = transaction(&s, "Adom apply zone plan", || {+ if !plan.remove.is_empty() { s.client.delete_items(plan.remove.clone())?;- // KiCad versions may return no DeleteItems rows. Verify actual item- // absence within the transaction instead of trusting a row count.- let remaining=s.client.get_items_raw_by_type_codes(vec![PcbObjectTypeCode::new_zone().code])?;- if remaining.iter().filter_map(|a|zones::zone_id(&a.value)).any(|id|plan.remove.contains(&id)){return Err(IpcFailure::from("Owned zones remain after deletion; rollback requested".to_string()));}+ // KiCad applies deletions at commit. Readback here sees the old board.+ }+ let created = if items.is_empty() {+ Vec::new()+ } else {+ s.client.create_items(items.clone(), None)?+ };+ if created.len() != items.len() {+ return Err(IpcFailure::from(+ "Not every planned zone was created; rollback requested".to_string(),+ )); }- let created=if items.is_empty(){Vec::new()}else{s.client.create_items(items.clone(),None)?};- if created.len()!=items.len(){return Err(IpcFailure::from("Not every planned zone was created; rollback requested".to_string()));} Ok(created) })?;- let ids:Vec<String>=created.iter().filter_map(|a|zones::zone_id(&a.value)).collect();- let mut owned=plan.unchanged.clone();owned.extend(ids.clone());- let mut out=json!({"success":true,"committed":true,"mutated":true,"undoSteps":1,"itemIds":ids,"ownedItemIds":owned,"unchangedIds":plan.unchanged,"removedIds":plan.remove,"drc":check,"saved":false,"refilled":false,"_hint":"Full plan committed as one native Undo step. Inspect postCommitError and kicad_zone_state before retrying. Ownership is only the returned ownedItemIds; never infer it from a zone name or age."});- if ids.len()!=plan.create.len(){out["postCommitError"]=json!("Native creation returned incomplete zone IDs; inspect the board before any retry");}- if args["refill"]!=false {match s.client.refill_zones(Vec::new()){Ok(())=>out["refilled"]=json!(true),Err(e)=>out["postCommitError"]=json!(e.to_string())}}- match s.revision(){Ok(r)=>out["revision"]=json!(r),Err(e)=>out["postCommitError"]=json!(e.message)}- if args["save"]==true {match s.client.save_document(){Ok(())=>out["saved"]=json!(true),Err(e)=>out["postCommitError"]=json!(e.to_string())}}+ let ids: Vec<String> = created+ .iter()+ .filter_map(|a| zones::zone_id(&a.value))+ .collect();+ let mut owned = plan.unchanged.clone();+ owned.extend(ids.clone());+ let mut out = json!({"success":true,"committed":true,"mutated":true,"undoSteps":1,"itemIds":ids,"ownedItemIds":owned,"unchangedIds":plan.unchanged,"removedIds":plan.remove,"drc":check,"saved":false,"refilled":false,"_hint":"Full plan committed as one native Undo step. Inspect postCommitError and kicad_zone_state before retrying. Ownership is only the returned ownedItemIds; never infer it from a zone name or age."});+ if ids.len() != plan.create.len() {+ out["postCommitError"] = json!(+ "Native creation returned incomplete zone IDs; inspect the board before any retry"+ );+ }+ out["refillRequired"] = json!(true);+ if !plan.remove.is_empty() {+ match s+ .client+ .get_items_raw_by_type_codes(vec![PcbObjectTypeCode::new_zone().code])+ {+ Ok(rows) => {+ let remaining: Vec<_> = rows+ .iter()+ .filter_map(|a| zones::zone_id(&a.value))+ .filter(|id| plan.remove.contains(id))+ .collect();+ out["deletionVerified"] = json!(remaining.is_empty());+ if !remaining.is_empty() {+ out["postCommitDeletionError"] = json!({"remainingIds": remaining});+ out["success"] = json!(false);+ }+ }+ Err(e) => {+ out["postCommitDeletionError"] = json!(e.to_string());+ out["success"] = json!(false);+ }+ }+ }+ match s.revision() {+ Ok(r) => out["revision"] = json!(r),+ Err(e) => out["postCommitError"] = json!(e.message),+ }+ if args["save"] == true {+ match s.client.save_document() {+ Ok(()) => out["saved"] = json!(true),+ Err(e) => out["postCommitError"] = json!(e.to_string()),+ }+ } Ok(out) } @@ -1245,6 +1331,15 @@ fn merge(into: &mut Value, from: &Value) { #[cfg(test)] mod tests {+ #[test]+ fn zone_plan_refill_refuses_before_ipc_or_mutation() {+ let error = super::apply_zone_plan(&super::Ctx::default(), &serde_json::json!({+ "expectedRevision": "test", "refill": true+ })).unwrap_err();+ assert_eq!(error.code, "separate_refill_required");+ assert_eq!(super::error_response(&error)["mutated"], false);+ }+ use super::*; use std::cell::RefCell; @@ -1658,42 +1753,148 @@ pub fn text_bounds(ctx:&Ctx,args:&Value)->RResult<Value>{ } /// Update standalone silk text by stable UUID, in one native Undo transaction.-pub fn silk_text_batch(ctx:&Ctx,args:&Value)->RResult<Value>{- use kicad_ipc_rs::BoardTextItem;- let s=connect(ctx,args)?;let (text,_,revision)=s.snapshot()?;check_revision(args,&revision)?;- let plan=crate::silk_text::prepare(&text,args)?;- // Silkscreen clearance findings are often warnings. Compare all severities, not only errors.- let all_severities=|mut v:Value| {if let Some(rows)=v["violations"].as_array_mut(){for row in rows {row["severity"]=json!("error");}}v};- let before=drc_snapshot(ctx.kicad_cli.as_deref(),&s.source_path(),&text)?;- let after=drc_snapshot(ctx.kicad_cli.as_deref(),&s.source_path(),&plan.candidate)?;- let added=new_errors(&all_severities(before),&all_severities(after.clone()))?;- if !added.is_empty(){return Err(RoutingError::new("drc_rejected","Silkscreen candidate adds native DRC findings; no live text changed").with("violations",json!(added)).with("mutated",json!(false)));}- if args["dryRun"]==true{return Ok(json!({"success":true,"mutated":false,"dryRun":true,"revision":revision,"drc":after}));}- let item=|t:&crate::silk_text::Text| {- let mut item=BoardTextItem::from_proto(Default::default());item.set_layer_id(BoardLayerInfo::id_from_name(&t.layer).unwrap());- let p=item.proto_mut();p.id=Some(Default::default());p.id.as_mut().unwrap().value=t.id.clone();p.locked=1;p.knockout=false;- p.text=Some(Default::default());let text=p.text.as_mut().unwrap();text.text=t.text.clone();text.position=Some(Default::default());let pos=text.position.as_mut().unwrap();pos.x_nm=nm(t.x);pos.y_nm=nm(t.y);- text.attributes=Some(Default::default());let a=text.attributes.as_mut().unwrap();a.horizontal_alignment=match t.align.as_str(){"left"=>1,"right"=>3,_=>2};a.vertical_alignment=2;- a.angle=Some(Default::default());a.angle.as_mut().unwrap().value_degrees=t.rotation;- a.line_spacing=1.;a.stroke_width=Some(Default::default());a.stroke_width.as_mut().unwrap().value_nm=nm(t.stroke);- a.size=Some(Default::default());a.size.as_mut().unwrap().x_nm=nm(t.width);a.size.as_mut().unwrap().y_nm=nm(t.height);- a.mirrored=t.mirrored;a.multiline=t.text.contains('\n');a.visible=true;- EditablePcbItem::BoardText(item)- };- let created_ids:Vec<String>=plan.create.iter().map(|t|t.id.clone()).collect();let updated_ids:Vec<String>=plan.update.iter().map(|t|t.id.clone()).collect();- let validate_reply=|actual:Vec<EditablePcbItem>,wanted:&[String]|->Result<(),IpcFailure>{let actual:HashSet<_>=actual.iter().filter_map(|t|t.id().map(str::to_string)).collect();if actual!=wanted.iter().cloned().collect(){return Err(IpcFailure::from("KiCad reply did not preserve every requested text UUID".to_string()));}Ok(())};- check_revision(args,&s.revision()?)?;- transaction(&s,"Adom silkscreen text",||{- if !plan.remove.is_empty(){s.client.delete_items(plan.remove.clone())?;let remaining=s.client.get_items_by_type_codes(vec![PcbObjectTypeCode::new_text().code])?;if remaining.iter().filter_map(item_id).any(|id|plan.remove.iter().any(|x|x==id)){return Err(IpcFailure::from("KiCad still reports deleted text".to_string()));}}- if !plan.update.is_empty(){validate_reply(s.client.update_editable_items(plan.update.iter().map(&item).collect())?,&updated_ids)?;}- if !plan.create.is_empty(){validate_reply(s.client.create_editable_items(plan.create.iter().map(&item).collect(),None)?,&created_ids)?;}- Ok(())- })?;- let mut result=json!({"success":true,"mutated":true,"committed":true,"undoSteps":1,"createdIds":created_ids,"updatedIds":updated_ids,"removedIds":plan.remove,"drc":after,"saved":false,"source":"live-editor","_hint":"One native Undo step. Updates require full text definitions and preserve UUIDs; no content-based replacement. Inspect returned revision/bounds and refresh the linked 3D viewer. A postCommitError means committed: inspect state, never blindly replay."});- match s.revision(){Ok(rev)=>result["revision"]=json!(rev),Err(e)=>result["postCommitError"]=json!(e.message)}- let ids:Vec<_>=created_ids.iter().chain(&updated_ids).cloned().collect();- match s.client.get_item_bounding_boxes(ids,false){Ok(boxes)=>result["bounds"]=json!(boxes.iter().map(|b|json!({"id":b.item_id,"boxMm":[b.x_nm as f64/1e6,b.y_nm as f64/1e6,(b.x_nm+b.width_nm) as f64/1e6,(b.y_nm+b.height_nm) as f64/1e6]})).collect::<Vec<_>>()),Err(e)=>result["postCommitBoundsError"]=json!(e.to_string())}- if args["save"]==true {match s.client.save_document(){Ok(())=>result["saved"]=json!(true),Err(e)=>result["postCommitSaveError"]=json!(e.to_string())}}- Ok(result)+pub fn silk_text_batch(ctx: &Ctx, args: &Value) -> RResult<Value> {+ use kicad_ipc_rs::BoardTextItem;+ let s = connect(ctx, args)?;+ let (text, _, revision) = s.snapshot()?;+ check_revision(args, &revision)?;+ let plan = crate::silk_text::prepare(&text, args)?;+ // Silkscreen clearance findings are often warnings. Compare all severities, not only errors.+ let all_severities = |mut v: Value| {+ if let Some(rows) = v["violations"].as_array_mut() {+ for row in rows {+ row["severity"] = json!("error");+ }+ }+ v+ };+ let before = drc_snapshot(ctx.kicad_cli.as_deref(), &s.source_path(), &text)?;+ let after = drc_snapshot(ctx.kicad_cli.as_deref(), &s.source_path(), &plan.candidate)?;+ let added = new_errors(&all_severities(before), &all_severities(after.clone()))?;+ if !added.is_empty() {+ return Err(RoutingError::new(+ "drc_rejected",+ "Silkscreen candidate adds native DRC findings; no live text changed",+ )+ .with("violations", json!(added))+ .with("mutated", json!(false)));+ }+ if args["dryRun"] == true {+ return Ok(+ json!({"success":true,"mutated":false,"dryRun":true,"revision":revision,"drc":after}),+ );+ }+ let item = |t: &crate::silk_text::Text| {+ let mut item = BoardTextItem::from_proto(Default::default());+ item.set_layer_id(BoardLayerInfo::id_from_name(&t.layer).unwrap());+ let p = item.proto_mut();+ p.id = Some(Default::default());+ p.id.as_mut().unwrap().value = t.id.clone();+ p.locked = 1;+ p.knockout = false;+ p.text = Some(Default::default());+ let text = p.text.as_mut().unwrap();+ text.text = t.text.clone();+ text.position = Some(Default::default());+ let pos = text.position.as_mut().unwrap();+ pos.x_nm = nm(t.x);+ pos.y_nm = nm(t.y);+ text.attributes = Some(Default::default());+ let a = text.attributes.as_mut().unwrap();+ a.horizontal_alignment = match t.align.as_str() {+ "left" => 1,+ "right" => 3,+ _ => 2,+ };+ a.vertical_alignment = 2;+ a.angle = Some(Default::default());+ a.angle.as_mut().unwrap().value_degrees = t.rotation;+ a.line_spacing = 1.;+ a.stroke_width = Some(Default::default());+ a.stroke_width.as_mut().unwrap().value_nm = nm(t.stroke);+ a.size = Some(Default::default());+ a.size.as_mut().unwrap().x_nm = nm(t.width);+ a.size.as_mut().unwrap().y_nm = nm(t.height);+ a.mirrored = t.mirrored;+ a.multiline = t.text.contains('\n');+ a.visible = true;+ EditablePcbItem::BoardText(item)+ };+ let created_ids: Vec<String> = plan.create.iter().map(|t| t.id.clone()).collect();+ let updated_ids: Vec<String> = plan.update.iter().map(|t| t.id.clone()).collect();+ let validate_reply =+ |actual: Vec<EditablePcbItem>, wanted: &[String]| -> Result<(), IpcFailure> {+ let actual: HashSet<_> = actual+ .iter()+ .filter_map(|t| t.id().map(str::to_string))+ .collect();+ if actual != wanted.iter().cloned().collect() {+ return Err(IpcFailure::from(+ "KiCad reply did not preserve every requested text UUID".to_string(),+ ));+ }+ Ok(())+ };+ check_revision(args, &s.revision()?)?;+ transaction(&s, "Adom silkscreen text", || {+ if !plan.remove.is_empty() {+ s.client.delete_items(plan.remove.clone())?;+ }+ if !plan.update.is_empty() {+ validate_reply(+ s.client+ .update_editable_items(plan.update.iter().map(&item).collect())?,+ &updated_ids,+ )?;+ }+ if !plan.create.is_empty() {+ validate_reply(+ s.client+ .create_editable_items(plan.create.iter().map(&item).collect(), None)?,+ &created_ids,+ )?;+ }+ Ok(())+ })?;+ let mut result = json!({"success":true,"mutated":true,"committed":true,"undoSteps":1,"createdIds":created_ids,"updatedIds":updated_ids,"removedIds":plan.remove,"drc":after,"saved":false,"source":"live-editor","_hint":"One native Undo step. Updates require full text definitions and preserve UUIDs; no content-based replacement. Inspect returned revision/bounds and refresh the linked 3D viewer. A postCommitError means committed: inspect state, never blindly replay."});+ if !plan.remove.is_empty() {+ match s+ .client+ .get_items_by_type_codes(vec![PcbObjectTypeCode::new_text().code])+ {+ Ok(rows) => {+ let remaining: Vec<_> = rows+ .iter()+ .filter_map(item_id)+ .filter(|id| plan.remove.iter().any(|wanted| wanted == id))+ .collect();+ result["deletionVerified"] = json!(remaining.is_empty());+ if !remaining.is_empty() {+ result["postCommitDeletionError"] = json!({"remainingIds": remaining});+ result["success"] = json!(false);+ }+ }+ Err(e) => {+ result["postCommitDeletionError"] = json!(e.to_string());+ result["success"] = json!(false);+ }+ }+ }++ match s.revision() {+ Ok(rev) => result["revision"] = json!(rev),+ Err(e) => result["postCommitError"] = json!(e.message),+ }+ let ids: Vec<_> = created_ids.iter().chain(&updated_ids).cloned().collect();+ if !ids.is_empty() {+ match s.client.get_item_bounding_boxes(ids,false){Ok(boxes)=>result["bounds"]=json!(boxes.iter().map(|b|json!({"id":b.item_id,"boxMm":[b.x_nm as f64/1e6,b.y_nm as f64/1e6,(b.x_nm+b.width_nm) as f64/1e6,(b.y_nm+b.height_nm) as f64/1e6]})).collect::<Vec<_>>()),Err(e)=>result["postCommitBoundsError"]=json!(e.to_string())}+ }+ if args["save"] == true {+ match s.client.save_document() {+ Ok(()) => result["saved"] = json!(true),+ Err(e) => result["postCommitSaveError"] = json!(e.to_string()),+ }+ }+ Ok(result) }
rust/crates/kicad-core/src/zone_plan.rs+55−2@@ -14,8 +14,34 @@ fn fail(message: impl Into<String>) -> RoutingError { RoutingError::new("zone_plan_refused",message).with("mutated",json!(false)) } fn canonical(n: &Node) -> Value {- if n.is_list() { json!(n.children().iter().map(canonical).collect::<Vec<_>>()) }- else { json!(n.value()) }+ if !n.is_list() {+ return json!(n.value());+ }+ let head = n.head().unwrap_or("");+ let mut children: Vec<Value> = n+ .children()+ .iter()+ .filter(|c| {+ // KiCad 10 omits this legacy no-thickness marker and zero priority.+ !matches!(+ (c.head(), c.arg(0).as_deref()),+ (Some("filled_areas_thickness"), Some("no"))+ | (Some("priority"), Some("0"))+ | (Some("locked"), Some("no"))+ )+ })+ .map(canonical)+ .collect();+ // Fill yes describes generated fill state, not the requested copper definition.+ if head == "fill" {+ children.retain(|v| v != "yes");+ }+ // Metadata order is insignificant. Polygon/point order and all unknown+ // properties remain significant; never collapse holes or arcs to area.+ if matches!(head, "zone" | "fill" | "connect_pads" | "keepout") {+ children.sort_by_key(Value::to_string);+ }+ json!(children) } fn definition_node(n: &Node) -> Node { let mut copy=n.clone();@@ -94,5 +120,32 @@ pub fn prepare(text:&str, board:&Board, args:&Value) -> Result<Prepared,RoutingE assert_ne!(definition(&a),definition(&crate::schematic::parse_node(&changed).unwrap())); } }++ #[test]+ fn native_saved_zone_defaults_are_idempotent() {+ let (text, board, request) = fixture();+ let first = prepare(&text, &board, &json!({"ownedItemIds": [], "zones": [request.clone()]})).unwrap();+ let doc = Document::parse(&first.candidate).unwrap();+ let zone = doc.root.children().iter().find(|n| n.head() == Some("zone")).unwrap();+ let id = zone.child_value("uuid").unwrap();+ // KiCad's saved copper zone omits the legacy filled_areas marker,+ // moves island mode after thermal settings, and carries generated fills.+ let mut saved = zone.clone();+ saved.children_mut().unwrap().retain(|n| n.head() != Some("filled_areas_thickness"));+ let fill = saved.children_mut().unwrap().iter_mut().find(|n| n.head() == Some("fill")).unwrap();+ let children = fill.children_mut().unwrap();+ let index = children.iter().position(|n| n.head() == Some("island_removal_mode")).unwrap();+ let mode = children.remove(index); children.push(mode);+ saved.children_mut().unwrap().push(crate::schematic::parse_node("(filled_polygon (layer F.Cu) (pts (xy 2 2) (xy 8 2) (xy 8 8)))").unwrap());+ assert_eq!(definition(zone), definition(&saved));+ let native_text = first.candidate.replace(&zone.to_text(), &saved.to_text());+ let again = prepare(&native_text, &board, &json!({"ownedItemIds": [id.clone()], "zones": [request]})).unwrap();+ assert_eq!(again.unchanged, vec![id]);+ assert!(again.remove.is_empty() && again.create.is_empty());+ for extra in ["(island_removal_mode 1)", "(island_removal_mode 2)"] {+ let other = saved.to_text().replace("(island_removal_mode 0)", extra);+ assert_ne!(definition(&saved), definition(&crate::schematic::parse_node(&other).unwrap()));+ }+ } }
rust/crates/kicad-core/src/zones.rs+1−1@@ -242,7 +242,7 @@ pub fn zone_sexpr(z: &ZoneSpec) -> String { } s.push_str(&format!("\t\t(min_thickness {})\n", pcb::num(z.min_thickness))); s.push_str("\t\t(filled_areas_thickness no)\n");- s.push_str(&format!("\t\t(fill yes\n\t\t\t(thermal_gap {})\n\t\t\t(thermal_bridge_width {})\n\t\t)\n", pcb::num(z.thermal_gap), pcb::num(z.thermal_bridge)));+ s.push_str(&format!("\t\t(fill yes\n\t\t\t(island_removal_mode 0)\n\t\t\t(thermal_gap {})\n\t\t\t(thermal_bridge_width {})\n\t\t)\n", pcb::num(z.thermal_gap), pcb::num(z.thermal_bridge))); let pts: Vec<String> = z.polygon.iter().map(|(x, y)| format!("(xy {} {})", pcb::num(*x), pcb::num(*y))).collect(); s.push_str(&format!("\t\t(polygon\n\t\t\t(pts\n\t\t\t\t{}\n\t\t\t)\n\t\t)\n", pts.join(" "))); s.push_str("\t)");
Comments
Log in to comment.
Merged on the wiki (952ce32), mirrored into the build tree, shipped as KiCad Bridge 1.0.23 (insiders), and accepted on ConfRoomROG (KiCad 10.0.5) with your own checklist from docs/native-zone-plan.md. Everything in it passes.
Zones (disposable unrouted ESC copy, two foreign inner planes untouched throughout)
refill:trueseparate_refill_requiredbefore any mutation; live revision unchangedcommitted:true,undoSteps:1,refillRequired:true, returned revision equals a fresh readmutated:false,undoSteps:0,unchangedIdscarries the id: the equality normalisation works on what KiCad actually wrotecommitted:true, old id inremovedIds, new id initemIds,deletionVerified:truezones: [])committed:true,deletionVerified:true, revision back to the original valueSilkscreen text
deletionVerified:true, no bounds request; the other string still present and editablecommitted:true,undoSteps:1,deletionVerified:true, bounds for the two live itemsOne residual, small.
kicad_zone_state {refill:true}answeredrefilled:truewith the revision from BEFORE the fill landed: a fresh read a moment later gave a different value. Same asynchronous-fill root cause you moved out of the apply verb, now isolated to the refill call itself. Either wait for the fill to settle before reading the revision, or say in the reply that the caller must re-read. Not blocking; the apply path is clean.Thanks for the branch PR and the checklist; both made this a fifteen-minute acceptance instead of an afternoon.