diff --git a/crates/ocs_plugin_api/src/host.rs b/crates/ocs_plugin_api/src/host.rs index cda9ac2a1..1f8903987 100644 --- a/crates/ocs_plugin_api/src/host.rs +++ b/crates/ocs_plugin_api/src/host.rs @@ -142,6 +142,11 @@ pub enum TableOperation { BlockRename { from: String, to: String }, /// Delete an unreferenced block definition and its contents. BlockDelete { name: String }, + /// Add a new entity to a block definition, validated exactly as a new + /// model-space entity is. The entity's owner is set by the host. Returns the + /// new entity's handle. Edit and delete members with the ordinary entity + /// operations. + BlockEntityAdd { block: String, entity: EntityType }, } /// The style tables `TableOperation` can rename, delete or make current. diff --git a/crates/ocs_plugin_api/src/ipc/v4/protocol.rs b/crates/ocs_plugin_api/src/ipc/v4/protocol.rs index b8454893d..aee1a13f2 100644 --- a/crates/ocs_plugin_api/src/ipc/v4/protocol.rs +++ b/crates/ocs_plugin_api/src/ipc/v4/protocol.rs @@ -141,6 +141,10 @@ mod tests { TableOperation::BlockModify { name: "B".into(), description: None, explodable: Some(false), scale_uniformly: Some(true) }, TableOperation::BlockRename { from: "B".into(), to: "C".into() }, TableOperation::BlockDelete { name: "C".into() }, + TableOperation::BlockEntityAdd { + block: "C".into(), + entity: acadrust::EntityType::Line(acadrust::entities::Line::new()), + }, ] { let bytes = bincode::serialize(&PluginRequest::TableOperation { operation: operation.clone() }).unwrap(); assert!(matches!(bincode::deserialize::(&bytes).unwrap(), diff --git a/docs/plugin-host-model-completion-plan.md b/docs/plugin-host-model-completion-plan.md index f6e1f1d67..0ca0d3740 100644 --- a/docs/plugin-host-model-completion-plan.md +++ b/docs/plugin-host-model-completion-plan.md @@ -227,3 +227,16 @@ unit tests. One more cadcodec finding (DXF drops the block description) is pinne editing a definition's contents in place, and attribute definitions inside a scripted block. Next: linetypes and layouts, then headless commands. +### 2026-09-21: block contents (`create_entity(..., block=)`) + +`TableOperation::BlockEntityAdd` adds a validated entity to a definition: the host sets the owner, runs the same +validation and reference binding as a new model-space entity, refuses viewports, raster images, block markers, +nested attributes and self-nesting inserts (direct or transitive), then reuses the scene's block-editor routing so +the entity gets normal preparation. Editing and deleting members already worked through the entity API; deleting a +member now also drops its handle from the block record, because the core document leaves a removed entity +listed (`entity_handles`). An entity-delta undo of an add still leaves the handle listed (documented scene behaviour), +so consumers must treat `entity_handles` as candidates and confirm with `get_entity`; the Python block records +already do. Evidence: `audit_python_block_contents_over_real_ipc` (9 refusals; create, edit, delete in a block; +attribute definition inside a block; nested insert; live/DWG/DXF persistence; undo), IPC and Python unit tests. +Next: linetypes and layouts, then headless commands. + diff --git a/plugins/opencad-python/PLUGIN.md b/plugins/opencad-python/PLUGIN.md index 63190db8a..4533d285e 100644 --- a/plugins/opencad-python/PLUGIN.md +++ b/plugins/opencad-python/PLUGIN.md @@ -209,9 +209,20 @@ block, a viewport, a non-finite base point, renaming or deleting layout, anonymous or externally referenced blocks, and a case-only rename. Deleting also removes the definition's contents. Records list user blocks only (`handle`, `name`, `description`, `explodable`, `scale_uniformly`, `base_point`, -`insert_count`, `entities` as `{handle, kind}`). Editing a definition's contents -in place and creating attribute definitions inside a block from a script are not -available yet. A DXF save drops the block description (cadcodec); DWG keeps it. +`insert_count`, `entities` as `{handle, kind}`). A DXF save drops the block +description (cadcodec); DWG keeps it. + +**Block contents.** Add to a definition with `create_entity(kind, block="Part", +...)`. It is validated exactly like a model-space entity and the host sets the +owner (so attribute definitions work: `create_entity("AttributeDefinition", +block="Part", tag=..., prompt=..., default_value=..., insertion_point=..., +height=...)`). Members are read, edited (`entity.end = (8, 0, 0)` inside +`doc.transaction(...)`) and deleted (`doc.delete_entity`) with the ordinary entity +API, using the handles in `blocks["Part"]["entities"]`. Refused: an unknown, +layout, anonymous or external block, an invalid entity, a viewport, raster image, +block marker or nested attribute, an entity on a locked layer, and an insert that +would nest a block inside itself directly or through other blocks. Adding while the +block editor is open is refused. Each add or delete is one undo step. ### Historical command-replay experiment (not in the default build) diff --git a/plugins/opencad-python/src/document_model.py b/plugins/opencad-python/src/document_model.py index b414bf8cf..8c1894b88 100644 --- a/plugins/opencad-python/src/document_model.py +++ b/plugins/opencad-python/src/document_model.py @@ -473,11 +473,17 @@ class _Document: def transaction(self, label): return _Transaction(self, label) - def create_entity(self, kind, **properties): - """Create one mapped entity and return its live document descriptor.""" + def create_entity(self, kind, block=None, **properties): + """Create one mapped entity and return its live document descriptor. + With `block="Name"` the entity is added to that block definition + instead of the drawing (its owner is set by the host).""" if "kind" in properties or "handle" in properties: raise ValueError("kind and handle are managed by create_entity") - handle = ocs.add(dict(kind=kind, **_coerce_points(kind, properties))) + entity = dict(kind=kind, **_coerce_points(kind, properties)) + if block is not None: + handle = ocs.add_to_block(str(block), entity) + else: + handle = ocs.add(entity) return self.entities[handle] def delete_entity(self, entity): diff --git a/plugins/opencad-python/src/ocs_module.rs b/plugins/opencad-python/src/ocs_module.rs index 711456616..4e1667330 100644 --- a/plugins/opencad-python/src/ocs_module.rs +++ b/plugins/opencad-python/src/ocs_module.rs @@ -4263,6 +4263,31 @@ mod ocs { .ok_or_else(|| vm.new_runtime_error("ocs: not running inside a PY_ command".to_owned()))? } + /// Add a new entity, built from `entity` like `add`, to the block definition + /// `block`. Returns the new entity's handle. + #[cfg(feature = "experimental-host-model")] + #[pyfunction] + fn add_to_block(block: String, entity: PyObjectRef, vm: &VirtualMachine) -> PyResult { + let dict = entity.try_into_value::(vm)?; + let mut built = dict_to_entity(&dict, vm)?; + let result = host_ctx::with_host(|host| -> PyResult> { + let owner = host + .document() + .block_records + .get(block.trim()) + .map(|record| record.handle) + .ok_or_else(|| vm.new_runtime_error(format!("ocs.add_to_block: block {block:?} does not exist")))?; + built.common_mut().owner_handle = owner; + ocs_plugin_api::entity_coverage::validate_new_canvas_entity(&built) + .map_err(|error| vm.new_value_error(error))?; + Ok(host.table_operation(ocs_plugin_api::host::TableOperation::BlockEntityAdd { block, entity: built })) + }) + .ok_or_else(|| vm.new_runtime_error("ocs: not running inside a PY_ command".to_owned()))??; + result + .map(|handle| handle.value()) + .map_err(|error| vm.new_runtime_error(format!("ocs.add_to_block: {error}"))) + } + /// Update `handle` by merging `entity`'s keys onto its *current* value — /// a key `entity` doesn't mention is left exactly as it was, not reset to /// a type default (Felix's Phase 1 review: reconstructing from a partial diff --git a/plugins/opencad-python/tests/test_document_model.py b/plugins/opencad-python/tests/test_document_model.py index 8823092c4..8e9f5ac3d 100644 --- a/plugins/opencad-python/tests/test_document_model.py +++ b/plugins/opencad-python/tests/test_document_model.py @@ -41,6 +41,7 @@ class DocumentModelTests(unittest.TestCase): style_operation=lambda kind, op, name, options: self.calls.append((kind, op, name, options)) or 9, block_records=lambda: [{"name": n, "entities": []} for n in ("Widget", "Gadget")], block_operation=lambda op, name, options: self.calls.append(("block", op, name, options)) or 9, + add_to_block=lambda block, entity: self.calls.append(("add_to_block", block, entity)) or 1, ) namespace = {"ocs": self.ocs} exec(MODEL.read_text(), namespace) @@ -96,6 +97,14 @@ class DocumentModelTests(unittest.TestCase): dim.set_current("Standard") self.assertEqual(self.calls[-1], ("dim", "set_current", "Standard", None)) + def test_create_entity_in_block(self): + self.doc.create_entity("Line", block="Widget", start={"x": 0, "y": 0, "z": 0}, end={"x": 1, "y": 0, "z": 0}) + self.assertEqual(self.calls[-1][:2], ("add_to_block", "Widget")) + self.assertEqual(self.calls[-1][2]["kind"], "Line") + self.assertNotIn("block", self.calls[-1][2]) + self.doc.create_entity("Line", start={"x": 0, "y": 0, "z": 0}, end={"x": 1, "y": 0, "z": 0}) + self.assertEqual(self.calls[-1][0], "add") + def test_blocks(self): blocks = self.doc.blocks self.assertIn("WIDGET", blocks) diff --git a/src/app/plugin_host.rs b/src/app/plugin_host.rs index ba60109b5..2ae2d1172 100644 --- a/src/app/plugin_host.rs +++ b/src/app/plugin_host.rs @@ -503,9 +503,18 @@ impl<'a> HostSession<'a> { } return removed; } + let owner = self.document().get_entity(handle).map(|entity| entity.common().owner_handle); self.app.tabs[self.tab].scene.erase_entities(&[handle]); let removed = self.document().get_entity(handle).is_none(); if removed { + // The document keeps a block record's member list separately from the + // entity store, and removing an entity leaves its handle listed; drop + // it so a definition never lists a member that no longer exists. + if let Some(owner) = owner { + if let Some(record) = self.document_mut().block_records.iter_mut().find(|record| record.handle == owner) { + record.entity_handles.retain(|member| *member != handle); + } + } self.publish_document_view(); } removed @@ -1248,7 +1257,8 @@ impl<'a> HostSession<'a> { other @ (TableOperation::BlockCreate { .. } | TableOperation::BlockModify { .. } | TableOperation::BlockRename { .. } - | TableOperation::BlockDelete { .. }) => self.block_operation(other), + | TableOperation::BlockDelete { .. } + | TableOperation::BlockEntityAdd { .. }) => self.block_operation(other), other => self.style_operation(other), } } @@ -1336,6 +1346,42 @@ impl<'a> HostSession<'a> { self.finish_block_change(); Ok(handle) } + TableOperation::BlockEntityAdd { block, mut entity } => { + let (name, record_handle) = editable_block(self.document(), block.trim())?; + if matches!( + entity, + E::Viewport(_) | E::Block(_) | E::BlockEnd(_) | E::AttributeEntity(_) | E::Unknown(_) | E::RasterImage(_) + ) { + return Err("an entity of this kind cannot be added to a block from a script".to_owned()); + } + if self.app.tabs[self.tab].scene.block_edit_block.is_some() { + return Err("finish the open block editor before adding entities to a block".to_owned()); + } + if let E::Insert(insert) = &entity { + if insert.block_name.eq_ignore_ascii_case(&name) || block_nests(self.document(), &insert.block_name, &name, 0) { + return Err(format!("inserting {:?} into {name:?} would nest a block inside itself", insert.block_name)); + } + } + entity.common_mut().owner_handle = record_handle; + ocs_plugin_api::entity_coverage::validate_new_canvas_entity(&entity)?; + ocs_plugin_api::entity_coverage::bind_canvas_entity_references(self.document(), &mut entity)?; + if self.document().layers.get(&entity.common().layer).is_some_and(|layer| layer.is_locked()) { + return Err(format!("layer {:?} is locked", entity.common().layer)); + } + self.push_undo("Add to block"); + entity.common_mut().handle = Handle::NULL; + entity.common_mut().owner_handle = record_handle; + // The scene routes an add into the block being edited; reuse that + // path so the entity gets the same preparation as any new entity. + self.app.tabs[self.tab].scene.block_edit_block = Some(record_handle); + let handle = self.add_entity(entity); + self.app.tabs[self.tab].scene.block_edit_block = None; + if handle.is_null() { + return Err(format!("the entity could not be added to block {name:?}")); + } + self.finish_block_change(); + Ok(handle) + } TableOperation::BlockModify { name, description, explodable, scale_uniformly } => { let (name, handle) = editable_block(self.document(), name.trim())?; if description.is_none() && explodable.is_none() && scale_uniformly.is_none() { @@ -1666,6 +1712,22 @@ impl<'a> HostSession<'a> { } } +/// True when block `outer` (transitively) contains an insert of `target`. +fn block_nests(doc: &CadDocument, outer: &str, target: &str, depth: usize) -> bool { + if depth > 64 { + return true; + } + let Some(record) = doc.block_records.get(outer) else { + return false; + }; + record.entity_handles.iter().filter_map(|h| doc.get_entity(*h)).any(|entity| match entity { + EntityType::Insert(insert) => { + insert.block_name.eq_ignore_ascii_case(target) || block_nests(doc, &insert.block_name, target, depth + 1) + } + _ => false, + }) +} + /// A layer name AutoCAD would accept: 1-255 characters, none of `<>/\":;?*|=\``. fn validated_layer_name(raw: &str) -> Result { validated_symbol_name(raw, "a layer name") @@ -7139,6 +7201,115 @@ B.delete('Widget2') assert_eq!(members("Widget2"), Some(1), "undo restored Widget2"); } + /// Block contents through Python over the real runner: add entities + /// (including an attribute definition) to a definition, edit and delete them + /// with the ordinary entity API, refuse bad requests, persist through + /// DWG/DXF and undo. + #[test] + fn audit_python_block_contents_over_real_ipc() { + let Some(plugin_path) = std::env::var_os("OCS_TEST_PYTHON_PLUGIN") else { + return; + }; + let mut app = OpenCADStudio::new_for_test(); + app.tabs[0].is_start = false; + let mut host = HostSession::new(&mut app, 0); + let process = ocs_plugin_api::process::PluginProcess::spawn( + std::path::Path::new(&plugin_path), &mut host, crate::plugin::v4_support::notification_handler(), + ).unwrap(); + let dir = std::env::temp_dir(); + let run = |host: &mut HostSession<'_>, tag: &str, body: &str| { + let script = dir.join(format!("ocs_bcontents_{tag}_{}.py", std::process::id())); + std::fs::write(&script, format!(concat!( + "def P(x, y, z): return {{'x': x, 'y': y, 'z': z}}\n", + "doc = ocs.active_document\nB = doc.blocks\nL = doc.layers\n", + "refused, accepted = [], []\n", + "def check(tag, fn):\n", + " try:\n fn()\n except (RuntimeError, TypeError, ValueError):\n refused.append(tag)\n", + " else:\n accepted.append(tag)\n", + "try:\n{body}\n", + "except Exception as error:\n", + " accepted.append('SCRIPTERROR_' + ''.join(c if c.isalnum() else '_' for c in str(error))[:120])\n", + "L.create('REPORT ' + str(len(refused)) + ' ~ ' + ' '.join(accepted))\n", + ), body = body.lines().map(|line| format!(" {line}")).collect::>().join("\n"))).unwrap(); + assert!(process.dispatch(host, &format!("PY_RUN {}", script.display()), &mut |_| {}).unwrap()); + let _ = std::fs::remove_file(&script); + }; + let report = |document: &CadDocument| -> (usize, Vec) { + let name = document.layers.iter().map(|l| l.name.clone()).filter(|n| n.starts_with("REPORT")).last() + .expect("script report layer"); + let (refused, accepted) = name["REPORT".len()..].split_once('~').unwrap(); + (refused.trim().parse().unwrap(), accepted.split_whitespace().map(str::to_owned).collect()) + }; + + run(&mut host, "build", r#" +seed = doc.create_entity('Circle', center=P(0, 0, 0), radius=1) +B.create('Part', [seed], erase_originals=True) +line = doc.create_entity('Line', start=P(0, 0, 0), end=P(5, 0, 0), block='Part') +arc = doc.create_entity('Arc', center=P(0, 0, 0), radius=5.0, start_angle=0.0, end_angle=1.5, block='Part') +tag = doc.create_entity('AttributeDefinition', block='Part', tag='PART_NO', prompt='Part number', default_value='PN-001', insertion_point=P(1, 2, 0), height=2.5) +with doc.transaction('Lengthen'): + line.end = (8, 0, 0) +doc.delete_entity(arc) +B.create('Outer', [doc.create_entity('Circle', center=P(0, 0, 0), radius=1)], erase_originals=True) +doc.create_entity('Insert', block_name='Part', insert_point=P(0, 0, 0), block='Outer') +check('missing_block', lambda: doc.create_entity('Line', start=P(0, 0, 0), end=P(1, 0, 0), block='Nope')) +check('layout_block', lambda: doc.create_entity('Line', start=P(0, 0, 0), end=P(1, 0, 0), block='*Model_Space')) +check('zero_circle', lambda: doc.create_entity('Circle', center=P(1, 1, 0), radius=0, block='Part')) +check('viewport', lambda: doc.create_entity('Viewport', block='Part')) +check('bad_tag', lambda: doc.create_entity('AttributeDefinition', block='Part', tag=' ', prompt='p', default_value='', insertion_point=P(0, 0, 0), height=1)) +check('bad_height', lambda: doc.create_entity('AttributeDefinition', block='Part', tag='T', prompt='p', default_value='', insertion_point=P(0, 0, 0), height=0)) +check('self_insert', lambda: doc.create_entity('Insert', block_name='Part', insert_point=P(0, 0, 0), block='Part')) +check('cycle', lambda: doc.create_entity('Insert', block_name='Outer', insert_point=P(0, 0, 0), block='Part')) +check('unknown_insert_block', lambda: doc.create_entity('Insert', block_name='Ghost', insert_point=P(0, 0, 0), block='Part')) +"#); + let (refused, accepted) = report(host.document()); + assert!(accepted.is_empty(), "accepted invalid requests: {accepted:?}"); + assert_eq!(refused, 9); + + let check_state = |label: &str, document: &CadDocument| { + let record = document.block_records.get("Part").unwrap_or_else(|| panic!("{label}: Part missing")); + let members: Vec = document.entities() + .filter(|e| e.common().owner_handle == record.handle && !matches!(e, EntityType::Block(_) | EntityType::BlockEnd(_))) + .cloned().collect(); + assert_eq!(members.len(), 3, "{label}: circle, line, attribute definition (the arc was deleted): {:?}", + members.iter().map(|e| format!("{e:?}").chars().take(10).collect::()).collect::>()); + let line = members.iter().find_map(|e| match e { EntityType::Line(l) => Some(l.clone()), _ => None }).unwrap_or_else(|| panic!("{label}: line")); + assert_eq!((line.end.x, line.end.y), (8.0, 0.0), "{label}: edited inside the block"); + assert!(members.iter().any(|e| matches!(e, EntityType::Circle(_))), "{label}: circle from the seed"); + let attdef = members.iter().find_map(|e| match e { EntityType::AttributeDefinition(a) => Some(a.clone()), _ => None }).unwrap_or_else(|| panic!("{label}: attdef")); + assert_eq!((attdef.tag.as_str(), attdef.prompt.as_str(), attdef.default_value.as_str()), ("PART_NO", "Part number", "PN-001"), "{label}"); + assert!(!members.iter().any(|e| matches!(e, EntityType::Arc(_))), "{label}: arc deleted"); + let outer = document.block_records.get("Outer").unwrap_or_else(|| panic!("{label}: Outer missing")); + let nested = document.entities().any(|e| matches!(e, EntityType::Insert(i) if i.block_name == "Part" && i.common.owner_handle == outer.handle)); + assert!(nested, "{label}: Part is inserted inside Outer"); + }; + check_state("live", host.document()); + let dwg = crate::io::load_bytes("bc.dwg", acadrust::DwgWriter::write_to_vec(host.document()).unwrap()).unwrap(); + let dxf = crate::io::load_bytes("bc.dxf", acadrust::DxfWriter::new(host.document()).write_to_vec().unwrap()).unwrap(); + check_state("DWG", &dwg); + check_state("DXF", &dxf); + + // U: one more add, then the report layer; undoing both leaves the block as before. + run(&mut host, "extra", "doc.create_entity('Circle', center=P(9, 9, 0), radius=2, block='Part')"); + assert_eq!(report(host.document()).1.len(), 0); + { + let document = host.document(); + let record = document.block_records.get("Part").unwrap(); + let live: Vec<_> = record.entity_handles.iter().map(|h| (h.value(), document.get_entity(*h).is_some())).collect(); + assert_eq!(record.entity_handles.len(), 4, "record entity_handles (handle, exists): {live:?}"); + } + drop(process); + drop(host); + app.finish_pending_history(0); + app.undo_steps(2); + // Membership bookkeeping is intrinsic to an add and stays listed across an entity-delta + // undo (see `Scene::add_entity_internal`), so count the entities that really exist. + let document = &app.tabs[0].scene.document; + let part = document.block_records.get("Part").unwrap(); + let existing = part.entity_handles.iter().filter(|h| document.get_entity(**h).is_some()).count(); + assert_eq!(existing, 3, "undo removed the added member"); + } + #[test] fn audit_python_raster_image_definition_linkage_over_real_ipc() { use acadrust::objects::ObjectType;