From acc3e050017cee4fa7977be096153f2f3570fa88 Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Thu, 24 Sep 2026 23:12:45 +0100 Subject: [PATCH 1/3] refactor(clipboard): share table and block merge copies merge_dependencies and merge_clipboard_blocks duplicated the table-record and block-definition merge logic already ported into paste_entities_kernel. Extract merge_table_records and merge_block_defs as the single shared copies; both the methods and the kernel delegate to them. The method keeps one batched geometry rebuild instead of one per definition. --- src/app/command_driver/clipboard.rs | 78 ++++++++++++++++------------- src/app/command_driver/mod.rs | 32 +----------- 2 files changed, 46 insertions(+), 64 deletions(-) diff --git a/src/app/command_driver/clipboard.rs b/src/app/command_driver/clipboard.rs index cec941d04..b5f313592 100644 --- a/src/app/command_driver/clipboard.rs +++ b/src/app/command_driver/clipboard.rs @@ -252,19 +252,11 @@ impl OpenCADStudio { return; } let blocks = self.clipboard_deps.blocks.clone(); - for def in blocks { - if self.tabs[i] - .scene - .document - .block_records - .get(&def.name) - .is_some() - { - continue; - } - self.tabs[i] - .scene - .define_block_raw(&def.name, def.base_point, def.entities); + // Single shared copy — see `merge_block_defs`. One batched geometry + // rebuild instead of one per definition; same end state as the + // per-definition bumps `Scene::define_block_raw` used to run. + if merge_block_defs(&mut self.tabs[i].scene.document, &blocks) > 0 { + self.tabs[i].scene.bump_geometry(); } } @@ -415,25 +407,11 @@ impl OpenCADStudio { /// PASTECLIP kernel: paste a clipboard payload (`entities` + `deps`, as /// captured by [`copy_to_clipboard_kernel`]) into `doc`, optionally /// translated, returning the new handles index-aligned with `entities` -/// (`NULL` where an add failed) plus the source-handle → pasted-handle map. -/// -/// Document-level work lives here; Scene/render concerns stay caller-side in -/// `finalize_paste` (tessellation bumps, `populate_missing_meshes`, -/// `recreate_groups`, `duplicate_parametric_constraints_for`, -/// `sync_displayed_annotation_context`, undo/dirty/selection/echo/panels). -/// See the per-helper report in `finalize_paste`. -pub(crate) fn paste_entities_kernel( - doc: &mut acadrust::CadDocument, - entities: &[acadrust::EntityType], - deps: &crate::app::ClipboardDeps, - translate: Option<&crate::command::EntityTransform>, -) -> ( - Vec, - rustc_hash::FxHashMap, -) { +/// Recreate missing layer/linetype/text-style/dim-style records with fresh +/// handles from the target document. Shared by `merge_dependencies` (PASTEBLOCK +/// path) and `paste_entities_kernel` — one copy, not a port per caller. +pub(crate) fn merge_table_records(doc: &mut acadrust::CadDocument, deps: &crate::app::ClipboardDeps) { use acadrust::TableEntry; - // Port of `merge_dependencies`: recreate missing table records with fresh - // handles from the target document. Pure `&mut CadDocument` work. for rec in &deps.layers { if !doc.layers.contains(rec.name()) { let mut r = rec.clone(); @@ -462,11 +440,43 @@ pub(crate) fn paste_entities_kernel( let _ = doc.dim_styles.add(r); } } - // Port of `merge_clipboard_blocks` / `Scene::define_block_raw` minus the - // trailing `bump_geometry` (render cache — caller-side). - for def in &deps.blocks { +} + +/// Recreate block definitions the pasted INSERTs reference but the target +/// document lacks. Shared by `merge_clipboard_blocks` and +/// `paste_entities_kernel`. Returns how many definitions were created, so +/// render-cache callers can bump once instead of per definition. +pub(crate) fn merge_block_defs(doc: &mut acadrust::CadDocument, blocks: &[crate::app::BlockDef]) -> usize { + let mut created = 0; + for def in blocks { + let before = doc.block_records.len(); define_block_raw_doc(doc, &def.name, def.base_point, &def.entities); + created += usize::from(doc.block_records.len() > before); } + created +} + +/// (`NULL` where an add failed) plus the source-handle → pasted-handle map. +/// +/// Document-level work lives here; Scene/render concerns stay caller-side in +/// `finalize_paste` (tessellation bumps, `populate_missing_meshes`, +/// `recreate_groups`, `duplicate_parametric_constraints_for`, +/// `sync_displayed_annotation_context`, undo/dirty/selection/echo/panels). +/// See the per-helper report in `finalize_paste`. +pub(crate) fn paste_entities_kernel( + doc: &mut acadrust::CadDocument, + entities: &[acadrust::EntityType], + deps: &crate::app::ClipboardDeps, + translate: Option<&crate::command::EntityTransform>, +) -> ( + Vec, + rustc_hash::FxHashMap, +) { + // Shared table-record merge (also used by `merge_dependencies`): recreate + // missing records with fresh handles from the target document. + merge_table_records(doc, deps); + // Shared block-definition merge (also used by `merge_clipboard_blocks`). + merge_block_defs(doc, &deps.blocks); let by_index: Vec = entities .iter() diff --git a/src/app/command_driver/mod.rs b/src/app/command_driver/mod.rs index ed583fb7a..e1bcebfd8 100644 --- a/src/app/command_driver/mod.rs +++ b/src/app/command_driver/mod.rs @@ -1473,39 +1473,11 @@ impl OpenCADStudio { /// handle from the target document so it can't collide with an existing /// one. No-op for same-document pastes (the records already exist). (#129) pub(super) fn merge_dependencies(&mut self, i: usize, deps: &crate::app::ClipboardDeps) { - use acadrust::TableEntry; if deps.is_empty() { return; } - let doc = &mut self.tabs[i].scene.document; - for rec in &deps.layers { - if !doc.layers.contains(rec.name()) { - let mut r = rec.clone(); - r.set_handle(doc.allocate_handle()); - let _ = doc.layers.add(r); - } - } - for rec in &deps.linetypes { - if !doc.line_types.contains(rec.name()) { - let mut r = rec.clone(); - r.set_handle(doc.allocate_handle()); - let _ = doc.line_types.add(r); - } - } - for rec in &deps.text_styles { - if !doc.text_styles.contains(rec.name()) { - let mut r = rec.clone(); - r.set_handle(doc.allocate_handle()); - let _ = doc.text_styles.add(r); - } - } - for rec in &deps.dim_styles { - if !doc.dim_styles.contains(rec.name()) { - let mut r = rec.clone(); - r.set_handle(doc.allocate_handle()); - let _ = doc.dim_styles.add(r); - } - } + // Single shared copy — see `clipboard::merge_table_records`. + clipboard::merge_table_records(&mut self.tabs[i].scene.document, deps); } From bd1fdcc1c56a3841a1e3003df319f2a6eb8176ea Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Thu, 24 Sep 2026 23:12:45 +0100 Subject: [PATCH 2/3] refactor(chprop): convert choice arms to apply_property_op, fix CTB snapshot leak Converts 12 selection arms of on_prop_geom_choice_changed to the shared helper: transparency, material, tol text and dim styles, dim overrides, viewport scale, UCS, named views, loft, history, surface and table style. Miss paths no longer push no-op undo entries. Fixes a leaked undo snapshot: changing plot style in CTB mode is a no-op but pushed an entry. Guard hoisted before the shared snapshot, with a regression test. Left manual by design: mleader, leader, block and fallback arms, all no-selection arms, and on_prop_geom_commit, whose constraint-aware invalidation the helper cannot express. --- src/app/update/command.rs | 577 +++++++++++++++++++++++--------------- 1 file changed, 350 insertions(+), 227 deletions(-) diff --git a/src/app/update/command.rs b/src/app/update/command.rs index 8fa8c4f6d..c6d86d326 100644 --- a/src/app/update/command.rs +++ b/src/app/update/command.rs @@ -2412,18 +2412,247 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task { return Task::none(); } } - self.push_undo_snapshot(i, "CHPROP"); - + // CTB mode has no named plot styles, so the `plot_style` arm + // below is a no-op early return. Guard here — before the shared + // snapshot — so the no-op path pushes no undo entry. (No other + // arm returns early after the snapshot, so nothing else depends + // on it existing at this point.) + if field == "plot_style" && self.tabs[i].scene.document.header.plotstyle_mode { + self.refresh_properties(); + return Task::none(); + } + if field == "transparency" { + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if app.tabs[i].scene.is_layer_locked(handle) { + return; + } + if let Some(entity) = app.tabs[i].scene.document.get_entity_mut(handle) { + crate::scene::view::dispatch::apply_common_prop( + entity, + "transparency", + &value, + ); + } + }); + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if field == "material" { + // Material source: ByLayer / ByBlock clear the handle; a + // named material sets flag 3 + its handle (resolved here + // because the update loop holds the document). + let mat_handle: Option = self.tabs[i] + .scene + .document + .objects + .iter() + .find_map(|(h, o)| match o { + acadrust::objects::ObjectType::Material(m) if m.name == value => { + Some(*h) + } + _ => None, + }); + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if app.tabs[i].scene.is_layer_locked(handle) { + return; + } + if let Some(entity) = app.tabs[i].scene.document.get_entity_mut(handle) + { + let common = entity.common_mut(); + match value.as_str() { + "ByLayer" => { + common.material_flags = 0; + common.material_handle = None; + } + "ByBlock" => { + common.material_flags = 1; + common.material_handle = None; + } + "Global" => { + common.material_flags = 2; + common.material_handle = None; + } + _ => { + if let Some(h) = mat_handle { + common.material_flags = 3; + common.material_handle = Some(h); + } + } + } + } + }); + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if field == "tol_text_style" { + use crate::entities::dim_override as dov; + use acadrust::xdata::XDataValue; + let style_handle = self.tabs[i] + .scene + .document + .text_styles + .iter() + .find(|entry| entry.name.eq_ignore_ascii_case(&value)) + .map(|entry| entry.handle); + if let Some(style_handle) = style_handle { + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if app.tabs[i].scene.is_layer_locked(handle) + || !matches!( + app.tabs[i].scene.document.get_entity(handle), + Some(acadrust::EntityType::Tolerance(_)) + ) + { + return; + } + dov::set( + &mut app.tabs[i].scene.document, + handle, + dov::DIMTXSTY, + Some(XDataValue::Handle(style_handle)), + ); + }); + } else { + self.invalidate_property_targets(i, &handles); + self.refresh_properties(); + } + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if field.starts_with("dim_") { + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if app.tabs[i].scene.is_layer_locked(handle) { + return; + } + if matches!( + app.tabs[i].scene.document.get_entity(handle), + Some(acadrust::EntityType::Dimension(_)) + ) { + let applied = crate::entities::dim_override::set_property( + &mut app.tabs[i].scene.document, + handle, + field, + &value, + ); + if applied && field == "dim_text_inside" { + if let Some(acadrust::EntityType::Dimension( + acadrust::entities::Dimension::LargeRadial(dimension), + )) = app.tabs[i].scene.document.get_entity_mut(handle) + { + dimension.base.text_user_positioned = false; + } + } + } + }); + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if field == "vscale_std" { + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if matches!( + app.tabs[i].scene.document.get_entity(handle), + Some(acadrust::EntityType::Viewport(_)) + ) { + let _ = app.tabs[i] + .scene + .set_viewport_scale_named_for(handle, &value); + } + }); + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if field == "vp_ucs_name" { + // Resolve UCS name → cloned data, then mutate viewports. + let ucs_data = self.tabs[i] + .scene + .document + .ucss + .iter() + .find(|u| u.name == value) + .cloned(); + if let Some(ucs) = ucs_data { + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if let Some(acadrust::EntityType::Viewport(vp)) = + app.tabs[i].scene.document.get_entity_mut(handle) + { + vp.ucs_handle = ucs.handle; + vp.ucs_origin = ucs.origin.clone(); + vp.ucs_x_axis = ucs.x_axis.clone(); + vp.ucs_y_axis = ucs.y_axis.clone(); + vp.ucs_per_viewport = true; + } + }); + } else { + self.invalidate_property_targets(i, &handles); + self.refresh_properties(); + } + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if field == "vp_named_view" { + // Assign a named view to viewport(s): copy camera parameters. + let view_data = self.tabs[i] + .scene + .document + .views + .iter() + .find(|v| v.name == value) + .cloned(); + if let Some(view) = view_data { + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if let Some(acadrust::EntityType::Viewport(vp)) = + app.tabs[i].scene.document.get_entity_mut(handle) + { + vp.view_target = view.target.clone(); + vp.view_direction = view.direction.clone(); + if view.height > 0.0 { + vp.view_height = view.height; + } + } + }); + self.tabs[i].scene.camera_generation += 1; + } else { + self.invalidate_property_targets(i, &handles); + self.refresh_properties(); + } + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if crate::scene::model::solid_history::is_loft_geometry_choice(field) { + // These choices change the generated body, not just history + // flags. Use the same transactional rebuild as numeric edits. + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if app.tabs[i].scene.is_layer_locked(handle) { + return; + } + app.tabs[i] + .scene + .apply_solid_history_property(handle, field, &value); + }); + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if crate::scene::model::solid_history::is_history_choice(field) { + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if app.tabs[i].scene.is_layer_locked(handle) { + return; + } + app.tabs[i] + .scene + .apply_solid_history_choice(handle, field, &value); + }); + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } if crate::scene::model::solid_history::is_surface_property_choice(field) { use crate::scene::model::solid_history::{ PROP_SURFACE_MAINTAIN_ASSOCIATIVITY, PROP_SURFACE_SHOW_ASSOCIATIVITY, PROP_SURFACE_WIREFRAME_TYPE, }; - for &handle in &handles { - if self.tabs[i].scene.is_layer_locked(handle) { - continue; + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if app.tabs[i].scene.is_layer_locked(handle) { + return; } - let Some(mut state) = self.tabs[i] + let Some(mut state) = app.tabs[i] .scene .document .get_entity(handle) @@ -2434,7 +2663,7 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task { _ => None, }) else { - continue; + return; }; match field { PROP_SURFACE_WIREFRAME_TYPE => { @@ -2446,67 +2675,22 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task { PROP_SURFACE_SHOW_ASSOCIATIVITY => { state.show_associativity = value.eq_ignore_ascii_case("Yes") } - _ => continue, + _ => return, } crate::scene::view::dispatch::set_entity_xdata( - &mut self.tabs[i].scene.document, + &mut app.tabs[i].scene.document, handle, crate::entities::solid3d::SURFACE_PROPERTIES_APP, Some(crate::entities::solid3d::surface_property_xdata_values(state)), ); if field == PROP_SURFACE_WIREFRAME_TYPE { - self.tabs[i].scene.reseed_derived_caches(handle); + app.tabs[i].scene.reseed_derived_caches(handle); } - } - } else if crate::scene::model::solid_history::is_loft_geometry_choice(field) { - // These choices change the generated body, not just history - // flags. Use the same transactional rebuild as numeric edits. - for &handle in &handles { - if self.tabs[i].scene.is_layer_locked(handle) { - continue; - } - self.tabs[i] - .scene - .apply_solid_history_property(handle, field, &value); - } - } else if crate::scene::model::solid_history::is_history_choice(field) { - for &handle in &handles { - if self.tabs[i].scene.is_layer_locked(handle) { - continue; - } - self.tabs[i] - .scene - .apply_solid_history_choice(handle, field, &value); - } - } else if field == "tol_text_style" { - use crate::entities::dim_override as dov; - use acadrust::xdata::XDataValue; - let style_handle = self.tabs[i] - .scene - .document - .text_styles - .iter() - .find(|entry| entry.name.eq_ignore_ascii_case(&value)) - .map(|entry| entry.handle); - if let Some(style_handle) = style_handle { - for &handle in &handles { - if self.tabs[i].scene.is_layer_locked(handle) - || !matches!( - self.tabs[i].scene.document.get_entity(handle), - Some(acadrust::EntityType::Tolerance(_)) - ) - { - continue; - } - dov::set( - &mut self.tabs[i].scene.document, - handle, - dov::DIMTXSTY, - Some(XDataValue::Handle(style_handle)), - ); - } - } - } else if field == "tol_dim_style" { + }); + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if field == "tol_dim_style" { let style = self.tabs[i] .scene .document @@ -2516,116 +2700,86 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task { .map(|entry| (entry.handle, entry.name.clone(), entry.annotative)); if let Some((style_handle, style_name, annotative)) = style { let scale = self.tabs[i].scene.creation_annotation_scale_handle(); - for &handle in &handles { - if self.tabs[i].scene.is_layer_locked(handle) { - continue; + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if app.tabs[i].scene.is_layer_locked(handle) { + return; } if let Some(acadrust::EntityType::Tolerance(tolerance)) = - self.tabs[i].scene.document.get_entity_mut(handle) + app.tabs[i].scene.document.get_entity_mut(handle) { tolerance.dimension_style_handle = Some(style_handle); tolerance.dimension_style_name = style_name.clone(); } else { - continue; + return; } crate::scene::annotative::set_entity_annotative( - &mut self.tabs[i].scene.document, + &mut app.tabs[i].scene.document, handle, annotative, ); if annotative { if let Some(scale) = scale { crate::scene::annotative::create_annotation_context( - &mut self.tabs[i].scene.document, + &mut app.tabs[i].scene.document, handle, scale, ); } } - } + }); + } else { + self.invalidate_property_targets(i, &handles); + self.refresh_properties(); } - } else if field.starts_with("dim_") { - for &handle in &handles { - if self.tabs[i].scene.is_layer_locked(handle) { - continue; - } - if matches!( - self.tabs[i].scene.document.get_entity(handle), - Some(acadrust::EntityType::Dimension(_)) - ) { - let applied = crate::entities::dim_override::set_property( - &mut self.tabs[i].scene.document, - handle, - field, - &value, - ); - if applied && field == "dim_text_inside" { - if let Some(acadrust::EntityType::Dimension( - acadrust::entities::Dimension::LargeRadial(dimension), - )) = self.tabs[i].scene.document.get_entity_mut(handle) - { - dimension.base.text_user_positioned = false; - } + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + if field == "tbl_style_handle" { + let style_handle = self.tabs[i] + .scene + .document + .objects + .iter() + .find_map(|(handle, object)| match object { + acadrust::objects::ObjectType::TableStyle(style) + if style.name.eq_ignore_ascii_case(value.trim()) => + { + Some(*handle) + } + _ => None, + }); + if let Some(style_handle) = style_handle { + self.apply_property_op(i, "CHPROP", &handles, |app, handle| { + if let Some(acadrust::EntityType::Table(table)) = + app.tabs[i].scene.document.get_entity_mut(handle) + { + table.table_style_handle = Some(style_handle); + // Shared-tail `tbl_` epilogue, now local: drop + // stale block_record refs so the new style takes + // effect (same final state as style loop + tail + // clearing, independent fields). + table.block_record_handle = None; + } + }); + } else { + // Unresolvable name: preserve the old tail's + // block_record clearing without pushing a snapshot. + for &handle in &handles { + if let Some(acadrust::EntityType::Table(table)) = + self.tabs[i].scene.document.get_entity_mut(handle) + { + table.block_record_handle = None; } } + self.invalidate_property_targets(i, &handles); + self.refresh_properties(); } - } else if field == "vscale_std" { - for &handle in &handles { - if matches!( - self.tabs[i].scene.document.get_entity(handle), - Some(acadrust::EntityType::Viewport(_)) - ) { - let _ = self.tabs[i] - .scene - .set_viewport_scale_named_for(handle, &value); - } - } - } else if field == "vp_ucs_name" { - // Resolve UCS name → cloned data, then mutate viewports. - let ucs_data = self.tabs[i] - .scene - .document - .ucss - .iter() - .find(|u| u.name == value) - .cloned(); - if let Some(ucs) = ucs_data { - for handle in &handles { - if let Some(acadrust::EntityType::Viewport(vp)) = - self.tabs[i].scene.document.get_entity_mut(*handle) - { - vp.ucs_handle = ucs.handle; - vp.ucs_origin = ucs.origin.clone(); - vp.ucs_x_axis = ucs.x_axis.clone(); - vp.ucs_y_axis = ucs.y_axis.clone(); - vp.ucs_per_viewport = true; - } - } - } - } else if field == "vp_named_view" { - // Assign a named view to viewport(s): copy camera parameters. - let view_data = self.tabs[i] - .scene - .document - .views - .iter() - .find(|v| v.name == value) - .cloned(); - if let Some(view) = view_data { - for handle in &handles { - if let Some(acadrust::EntityType::Viewport(vp)) = - self.tabs[i].scene.document.get_entity_mut(*handle) - { - vp.view_target = view.target.clone(); - vp.view_direction = view.direction.clone(); - if view.height > 0.0 { - vp.view_height = view.height; - } - } - } - self.tabs[i].scene.camera_generation += 1; - } - } else if matches!( + self.tabs[i].properties.edit_choice_open = false; + return Task::none(); + } + self.push_undo_snapshot(i, "CHPROP"); + + if matches!( field, "mleader_style" | "text_style_handle" @@ -2887,43 +3041,7 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task { self.tabs[i].scene.bump_entities(&changes); } } - } else if field == "tbl_style_handle" { - let style_handle = self.tabs[i] - .scene - .document - .objects - .iter() - .find_map(|(handle, object)| match object { - acadrust::objects::ObjectType::TableStyle(style) - if style.name.eq_ignore_ascii_case(value.trim()) => - { - Some(*handle) - } - _ => None, - }); - if let Some(style_handle) = style_handle { - for &handle in &handles { - if let Some(acadrust::EntityType::Table(table)) = - self.tabs[i].scene.document.get_entity_mut(handle) - { - table.table_style_handle = Some(style_handle); - } - } - } - } else if field == "transparency" { - for &handle in &handles { - if self.tabs[i].scene.is_layer_locked(handle) { - continue; - } - if let Some(entity) = self.tabs[i].scene.document.get_entity_mut(handle) { - crate::scene::view::dispatch::apply_common_prop(entity, "transparency", &value); - } - } } else if field == "plot_style" { - if self.tabs[i].scene.document.header.plotstyle_mode { - self.refresh_properties(); - return Task::none(); - } // Named plot-style pick: ByLayer / ByBlock clear the // handle; a named style resolves through the drawing's // ACAD_PLOTSTYLENAME dictionary to its placeholder handle. @@ -2966,50 +3084,6 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task { } } } - } else if field == "material" { - // Material source: ByLayer / ByBlock clear the handle; a - // named material sets flag 3 + its handle (resolved here - // because the update loop holds the document). - let mat_handle: Option = self.tabs[i] - .scene - .document - .objects - .iter() - .find_map(|(h, o)| match o { - acadrust::objects::ObjectType::Material(m) if m.name == value => { - Some(*h) - } - _ => None, - }); - for &handle in &handles { - if self.tabs[i].scene.is_layer_locked(handle) { - continue; - } - if let Some(entity) = self.tabs[i].scene.document.get_entity_mut(handle) - { - let common = entity.common_mut(); - match value.as_str() { - "ByLayer" => { - common.material_flags = 0; - common.material_handle = None; - } - "ByBlock" => { - common.material_flags = 1; - common.material_handle = None; - } - "Global" => { - common.material_flags = 2; - common.material_handle = None; - } - _ => { - if let Some(h) = mat_handle { - common.material_flags = 3; - common.material_handle = Some(h); - } - } - } - } - } } else { let plane = if self.tabs[i].editing_model_space() { self.tabs[i].ucs_xform().working_plane() @@ -4361,3 +4435,52 @@ mod layer_rename_tests { assert_ne!(app.tabs[i].scene.geometry_epoch, epoch); } } + +#[cfg(test)] +mod plot_style_ctb_guard_tests { + use crate::app::OpenCADStudio; + use acadrust::entities::Line; + use acadrust::types::Vector3; + + fn line_handle(app: &mut OpenCADStudio) -> acadrust::Handle { + let mut line = Line::new(); + line.start = Vector3::ZERO; + line.end = Vector3::new(1.0, 0.0, 0.0); + app.commit_entity_handle(acadrust::EntityType::Line(line)) + .expect("line should commit") + } + + #[test] + fn ctb_mode_plot_style_choice_pushes_no_undo_entry() { + let mut app = OpenCADStudio::new_for_test(); + let i = app.active_tab; + let h = line_handle(&mut app); + // Mirror `chprop_integration_tests`: seed `source_handles` so + // `property_target_handles` returns the line. + app.tabs[i].properties.source_handles = vec![h]; + // CTB mode: the B15 `plot_style` arm is a no-op early return. + app.tabs[i].scene.document.header.plotstyle_mode = true; + // Settle any pending snapshot left by the fixture setup. + app.finish_pending_history(i); + assert!( + app.tabs[i].history.pending.is_none(), + "test setup: no pending snapshot before the choice change" + ); + let before = app.tabs[i].history.undo_stack.len(); + // Drive the handler directly: `update()` closes the pending + // transaction at the message boundary (`finish_all_pending_history`), + // which silently discards the leaked no-op snapshot and would mask + // the bug. The direct call exposes whether a snapshot was pushed. + let _ = app.on_prop_geom_choice_changed("plot_style", "ByLayer".to_string()); + assert!( + app.tabs[i].history.pending.is_none(), + "CTB-mode plot_style choice is a no-op and must not push an undo snapshot" + ); + app.finish_pending_history(i); + assert_eq!( + app.tabs[i].history.undo_stack.len(), + before, + "CTB-mode plot_style choice must not add an undo entry" + ); + } +} From 631c0c97aaeecc9bb4a987525e3218d2a2a9d07d Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Thu, 24 Sep 2026 23:38:01 +0100 Subject: [PATCH 3/3] fix(command-line): Enter runs the highlighted autocomplete pre-selection --- src/ui/command_line.rs | 73 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 5 deletions(-) diff --git a/src/ui/command_line.rs b/src/ui/command_line.rs index 3bce5460d..f27022a18 100644 --- a/src/ui/command_line.rs +++ b/src/ui/command_line.rs @@ -164,8 +164,9 @@ pub struct CommandLine { pub history_open: bool, /// Persisted height of the full-history editor in logical pixels. pub history_height: f32, - /// Index of the currently-highlighted autocomplete suggestion, or - /// `None` before keyboard navigation begins. Reset when input changes. + /// Index of the currently-highlighted autocomplete suggestion. `None` + /// means the first match is pre-selected (highlighted) before keyboard + /// navigation begins. Reset when input changes. pub autocomplete_cursor: Option, /// Command names contributed by loaded plugins, refreshed whenever the /// enabled-plugin set changes. Merged into autocomplete alongside the @@ -617,11 +618,20 @@ impl CommandLine { true } - /// The command name explicitly highlighted in the autocomplete popup. + /// The command name highlighted in the autocomplete popup. Before any + /// arrow-key navigation the first match is pre-selected (the popup renders + /// `autocomplete_cursor.unwrap_or(0)` as highlighted), so Enter runs that + /// entry — never a different command resolved through another path. pub fn selected_suggestion(&self) -> Option { let matches = self.autocomplete_matches(); - self.autocomplete_cursor - .and_then(|index| matches.get(index).cloned()) + if matches.is_empty() { + return None; + } + let index = self + .autocomplete_cursor + .unwrap_or(0) + .min(matches.len() - 1); + matches.get(index).cloned() } /// Autocomplete suggestions for the current input — see @@ -1230,6 +1240,59 @@ mod tests { assert_eq!(m.first().map(String::as_str), Some("LINE"), "got {m:?}"); } + #[test] + fn preselected_top_suggestion_is_returned_without_navigation() { + // The popup highlights the first match before any arrow-key navigation + // (`unwrap_or(0)` in `view`); Enter must run that same entry. Typing + // `LT` with no alias table highlights `LTSCALE`, so the pre-selection + // must be `LTSCALE` — not `None` (which would fall through to alias / + // closest-match resolution and could run a different command). + let mut line = CommandLine::new(); + line.clear_history(); + line.command_aliases = FxHashMap::default(); + line.input = "LT".to_string(); + line.autocomplete_cursor = None; + let matches = line.autocomplete_matches(); + assert_eq!( + matches.first().map(String::as_str), + Some("LTSCALE"), + "got {matches:?}" + ); + assert_eq!( + line.selected_suggestion().as_deref(), + Some("LTSCALE"), + "Enter must run the highlighted pre-selection" + ); + } + + #[test] + fn preselected_alias_target_is_returned_without_navigation() { + // With the `LT` → `LINETYPE` alias, the forced top entry is `LINETYPE` + // and Enter must run it without requiring arrow-key navigation. + let mut line = CommandLine::new(); + line.clear_history(); + line.command_aliases = aliases(&[("LT", "LINETYPE"), ("LTS", "LTSCALE")]); + line.input = "LT".to_string(); + line.autocomplete_cursor = None; + assert_eq!( + line.selected_suggestion().as_deref(), + Some("LINETYPE"), + "got {:?}", + line.autocomplete_matches() + ); + } + + #[test] + fn no_preselection_without_matches_or_input() { + let mut line = CommandLine::new(); + line.clear_history(); + line.input = String::new(); + line.autocomplete_cursor = None; + assert_eq!(line.selected_suggestion(), None); + line.input = "ZZZ_NO_SUCH_COMMAND".to_string(); + assert_eq!(line.selected_suggestion(), None); + } + #[test] fn history_markers_and_copy_text_are_clean() { let mut line = CommandLine::new();