1b-agent/OneCAD-Draft
1
0 Fork 0
forked from 1b-agent/opencadstudio-upstream
Code Pull Requests Activity

refactor(chprop): convert choice arms to apply_property_op, fix CTB snapshot leak

Browse Source 
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.
This commit is contained in:
Karim Jerbi committed 2026-09-24 23:16:29 +01:00
1 parent acc3e05001
commit bd1fdcc1c5
1 file changed
+350 -227
Download Patch File Download Diff File Expand all files Collapse all files
src/app/update/command.rs
+350 -227
View File
@@ -2412,18 +2412,247 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task<Message> {
return Task::none(); 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<acadrust::Handle> = 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) { if crate::scene::model::solid_history::is_surface_property_choice(field) {
use crate::scene::model::solid_history::{ use crate::scene::model::solid_history::{
PROP_SURFACE_MAINTAIN_ASSOCIATIVITY, PROP_SURFACE_SHOW_ASSOCIATIVITY, PROP_SURFACE_MAINTAIN_ASSOCIATIVITY, PROP_SURFACE_SHOW_ASSOCIATIVITY,
PROP_SURFACE_WIREFRAME_TYPE, PROP_SURFACE_WIREFRAME_TYPE,
}; };
for &handle in &handles { self.apply_property_op(i, "CHPROP", &handles, |app, handle| {
if self.tabs[i].scene.is_layer_locked(handle) { if app.tabs[i].scene.is_layer_locked(handle) {
continue; return;
} }
let Some(mut state) = self.tabs[i] let Some(mut state) = app.tabs[i]
.scene .scene
.document .document
.get_entity(handle) .get_entity(handle)
@@ -2434,7 +2663,7 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task<Message> {
_ => None, _ => None,
}) })
else { else {
continue; return;
}; };
match field { match field {
PROP_SURFACE_WIREFRAME_TYPE => { PROP_SURFACE_WIREFRAME_TYPE => {
@@ -2446,67 +2675,22 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task<Message> {
PROP_SURFACE_SHOW_ASSOCIATIVITY => { PROP_SURFACE_SHOW_ASSOCIATIVITY => {
state.show_associativity = value.eq_ignore_ascii_case("Yes") state.show_associativity = value.eq_ignore_ascii_case("Yes")
} }
_ => continue, _ => return,
} }
crate::scene::view::dispatch::set_entity_xdata( crate::scene::view::dispatch::set_entity_xdata(
&mut self.tabs[i].scene.document, &mut app.tabs[i].scene.document,
handle, handle,
crate::entities::solid3d::SURFACE_PROPERTIES_APP, crate::entities::solid3d::SURFACE_PROPERTIES_APP,
Some(crate::entities::solid3d::surface_property_xdata_values(state)), Some(crate::entities::solid3d::surface_property_xdata_values(state)),
); );
if field == PROP_SURFACE_WIREFRAME_TYPE { 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) { self.tabs[i].properties.edit_choice_open = false;
// These choices change the generated body, not just history return Task::none();
// flags. Use the same transactional rebuild as numeric edits. }
for &handle in &handles { if field == "tol_dim_style" {
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" {
let style = self.tabs[i] let style = self.tabs[i]
.scene .scene
.document .document
@@ -2516,116 +2700,86 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task<Message> {
.map(|entry| (entry.handle, entry.name.clone(), entry.annotative)); .map(|entry| (entry.handle, entry.name.clone(), entry.annotative));
if let Some((style_handle, style_name, annotative)) = style { if let Some((style_handle, style_name, annotative)) = style {
let scale = self.tabs[i].scene.creation_annotation_scale_handle(); let scale = self.tabs[i].scene.creation_annotation_scale_handle();
for &handle in &handles { self.apply_property_op(i, "CHPROP", &handles, |app, handle| {
if self.tabs[i].scene.is_layer_locked(handle) { if app.tabs[i].scene.is_layer_locked(handle) {
continue; return;
} }
if let Some(acadrust::EntityType::Tolerance(tolerance)) = 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_handle = Some(style_handle);
tolerance.dimension_style_name = style_name.clone(); tolerance.dimension_style_name = style_name.clone();
} else { } else {
continue; return;
} }
crate::scene::annotative::set_entity_annotative( crate::scene::annotative::set_entity_annotative(
&mut self.tabs[i].scene.document, &mut app.tabs[i].scene.document,
handle, handle,
annotative, annotative,
); );
if annotative { if annotative {
if let Some(scale) = scale { if let Some(scale) = scale {
crate::scene::annotative::create_annotation_context( crate::scene::annotative::create_annotation_context(
&mut self.tabs[i].scene.document, &mut app.tabs[i].scene.document,
handle, handle,
scale, scale,
); );
} }
} }
} });
} else {
self.invalidate_property_targets(i, &handles);
self.refresh_properties();
} }
} else if field.starts_with("dim_") { self.tabs[i].properties.edit_choice_open = false;
for &handle in &handles { return Task::none();
if self.tabs[i].scene.is_layer_locked(handle) { }
continue; if field == "tbl_style_handle" {
} let style_handle = self.tabs[i]
if matches!( .scene
self.tabs[i].scene.document.get_entity(handle), .document
Some(acadrust::EntityType::Dimension(_)) .objects
) { .iter()
let applied = crate::entities::dim_override::set_property( .find_map(|(handle, object)| match object {
&mut self.tabs[i].scene.document, acadrust::objects::ObjectType::TableStyle(style)
handle, if style.name.eq_ignore_ascii_case(value.trim()) =>
field, {
&value, Some(*handle)
); }
if applied && field == "dim_text_inside" { _ => None,
if let Some(acadrust::EntityType::Dimension( });
acadrust::entities::Dimension::LargeRadial(dimension), if let Some(style_handle) = style_handle {
)) = self.tabs[i].scene.document.get_entity_mut(handle) self.apply_property_op(i, "CHPROP", &handles, |app, handle| {
{ if let Some(acadrust::EntityType::Table(table)) =
dimension.base.text_user_positioned = false; 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" { self.tabs[i].properties.edit_choice_open = false;
for &handle in &handles { return Task::none();
if matches!( }
self.tabs[i].scene.document.get_entity(handle), self.push_undo_snapshot(i, "CHPROP");
Some(acadrust::EntityType::Viewport(_))
) { if matches!(
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!(
field, field,
"mleader_style" "mleader_style"
| "text_style_handle" | "text_style_handle"
@@ -2887,43 +3041,7 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task<Message> {
self.tabs[i].scene.bump_entities(&changes); 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" { } 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 // Named plot-style pick: ByLayer / ByBlock clear the
// handle; a named style resolves through the drawing's // handle; a named style resolves through the drawing's
// ACAD_PLOTSTYLENAME dictionary to its placeholder handle. // ACAD_PLOTSTYLENAME dictionary to its placeholder handle.
@@ -2966,50 +3084,6 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task<Message> {
} }
} }
} }
} 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<acadrust::Handle> = 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 { } else {
let plane = if self.tabs[i].editing_model_space() { let plane = if self.tabs[i].editing_model_space() {
self.tabs[i].ucs_xform().working_plane() self.tabs[i].ucs_xform().working_plane()
@@ -4361,3 +4435,52 @@ mod layer_rename_tests {
assert_ne!(app.tabs[i].scene.geometry_epoch, epoch); 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"
);
}
}