From 9f600de52a29f271c1213573759ccbb86b231afa Mon Sep 17 00:00:00 2001 From: ysdirector Date: Sat, 10 Oct 2026 04:51:40 +0800 Subject: [PATCH] fix(control): apply sysvar layer changes atomically --- src/app/control/p1_tests.rs | 46 ++++++++++++++++ src/app/control/sheets.rs | 106 ++++++++++++++++++++++++++++++++++-- 2 files changed, 147 insertions(+), 5 deletions(-) diff --git a/src/app/control/p1_tests.rs b/src/app/control/p1_tests.rs index 331142ce6..1e001b95a 100644 --- a/src/app/control/p1_tests.rs +++ b/src/app/control/p1_tests.rs @@ -125,6 +125,52 @@ fn sysvar_round_trips_and_refuses_unknowns() { assert_eq!(r["code"], "unknown_sysvar"); } +#[test] +fn sysvar_batches_validate_atomically_and_undo_as_one_step() { + let mut app = OpenCADStudio::new_for_test(); + app.automation_op(r#"{"op":"new"}"#); + let state = app.automation_op(r#"{"protocol":1,"op":"state"}"#); + let doc = state["document_id"].as_u64().unwrap(); + mutate( + &mut app, + r#"{"protocol":1,"op":"entities_create","request_id":"sv-atomic-layer","document_id":{doc},"entities":[{"type":"Point","location":[0,0],"layer":"Walls"}]}"#, + ); + let i = app.active_tab; + // Isolate this operation's dirty/history assertions from the layer setup. + app.tabs[i].dirty = false; + let history_before = app.tabs[i].history.undo_stack.len(); + + let r = app.automation_op(&format!( + r#"{{"protocol":1,"op":"sysvar","request_id":"sv-atomic-ok","document_id":{doc},"set":{{"ltscale":2.5,"clayer":"Walls"}}}}"# + )); + assert_eq!(r["ok"], true, "{}", r["error"]); + assert_eq!(app.tabs[i].scene.document.header.current_layer_name, "Walls"); + assert_eq!(app.tabs[i].scene.document.header.linetype_scale, 2.5); + assert_eq!(app.tabs[i].active_layer, "Walls"); + assert_eq!(app.tabs[i].layers.current_layer, "Walls"); + assert_eq!(app.ribbon.active_layer, "Walls"); + assert!(app.tabs[i].dirty); + assert_eq!(app.tabs[i].history.undo_stack.len(), history_before + 1); + + // A bad later value must not leak the valid earlier value or an undo entry. + let r = app.automation_op(&format!( + r#"{{"protocol":1,"op":"sysvar","request_id":"sv-atomic-bad","document_id":{doc},"set":{{"ltscale":9.0,"clayer":"NOPE"}}}}"# + )); + assert_eq!(r["ok"], false); + assert_eq!(r["code"], "invalid_sysvar_value"); + assert_eq!(app.tabs[i].scene.document.header.linetype_scale, 2.5); + assert_eq!(app.tabs[i].scene.document.header.current_layer_name, "Walls"); + assert_eq!(app.tabs[i].history.undo_stack.len(), history_before + 1); + + // One undo restores both header variables and all current-layer mirrors. + app.automation_op(r#"{"op":"undo"}"#); + assert_eq!(app.tabs[i].scene.document.header.linetype_scale, 1.0); + assert_eq!(app.tabs[i].scene.document.header.current_layer_name, "0"); + assert_eq!(app.tabs[i].active_layer, "0"); + assert_eq!(app.tabs[i].layers.current_layer, "0"); + assert_eq!(app.ribbon.active_layer, "0"); +} + #[test] fn layout_create_and_page_setup_configure_sheets() { let mut app = OpenCADStudio::new_for_test(); diff --git a/src/app/control/sheets.rs b/src/app/control/sheets.rs index dd3b49bdc..1a7716ec1 100644 --- a/src/app/control/sheets.rs +++ b/src/app/control/sheets.rs @@ -50,14 +50,38 @@ impl OpenCADStudio { /// "set":{"MIRRTEXT":0}}` writes (one undo step, validated per name). pub(super) fn control_sysvar(&mut self, req: &Value) -> Result, Value> { if let Some(set) = req["set"].as_object() { - self.push_undo_snapshot(self.active_tab, "SYSVAR"); + let i = self.active_tab; + // Validate the complete batch before opening history or changing the + // document. Apart from making the operation atomic, this keeps a + // later bad variable from leaving an earlier variable half-applied. + { + let document = &self.tabs[i].scene.document; + for (name, value) in set { + validate_sysvar(document, name, value)?; + } + } + + self.push_undo_snapshot(i, "SYSVAR"); let mut applied = serde_json::Map::new(); for (name, value) in set { - set_sysvar(&mut self.tabs[self.active_tab].scene.document, name, value) - .map_err(|e| { - // Nothing committed yet — the snapshot is empty. - e + if name.eq_ignore_ascii_case("clayer") { + let layer = value + .as_str() + .expect("validate_sysvar checked CLAYER as a string"); + self.set_current_layer_name(i, layer).map_err(|error| { + failure( + "invalid_sysvar_value", + format!("Layer {layer:?} could not be made current: {error}"), + ) })?; + } else { + set_sysvar(&mut self.tabs[i].scene.document, name, value)?; + } + // System variables are persisted drawing state, not merely a + // UI preference. CLAYER is already marked dirty by the unified + // layer path; keeping this assignment for every variable makes + // mixed batches follow the same contract. + self.tabs[i].dirty = true; applied.insert(name.to_ascii_lowercase(), value.clone()); } self.set_control_result(json!({ "set": applied })); @@ -573,7 +597,79 @@ fn read_sysvar(scene: &crate::scene::Scene, name: &str) -> Option { Some(value) } +fn validate_sysvar(document: &codec::CadDocument, name: &str, value: &Value) -> Result<(), Value> { + let failure = |message: &str| crate::app::control::failure("invalid_sysvar_value", message); + match name.to_ascii_lowercase().as_str() { + "ltscale" => { + positive(value, "ltscale", failure)?; + } + "pdmode" => { + integer(value, "pdmode", failure)?; + } + "pdsize" => { + value + .as_f64() + .filter(|v| v.is_finite()) + .ok_or_else(|| failure("pdsize needs a finite number"))?; + } + "celtscale" => { + positive(value, "celtscale", failure)?; + } + "textsize" => { + positive(value, "textsize", failure)?; + } + "filletrad" => { + value + .as_f64() + .filter(|v| v.is_finite() && *v >= 0.0) + .ok_or_else(|| failure("filletrad needs a finite number >= 0"))?; + } + "mirrtext" => { + // Keep the historical coercion used by set_sysvar: omitted or + // non-integer values resolve to the enabled state. + } + "insunits" => { + integer(value, "insunits", failure)?; + } + "osmode" => { + value + .as_i64() + .and_then(|v| i32::try_from(v).ok()) + .ok_or_else(|| failure("osmode needs an integer"))?; + } + "clayer" => { + let name = value + .as_str() + .filter(|s| !s.is_empty()) + .ok_or_else(|| failure("clayer needs a layer name"))?; + if document.layers.get(name).is_none() { + return Err(failure(&format!("Layer '{name}' does not exist"))); + } + } + "ctextstyle" => { + let name = value + .as_str() + .filter(|s| !s.is_empty()) + .ok_or_else(|| failure("ctextstyle needs a text style name"))?; + if document.text_styles.get(name).is_none() { + return Err(failure(&format!("Text style '{name}' does not exist"))); + } + } + other => { + return Err(crate::app::control::failure( + "unknown_sysvar", + format!( + "System variable '{other}' is read-only or unknown. Writable: {}", + "celtscale, clayer, ctextstyle, filletrad, insunits, ltscale, mirrtext, osmode, pdmode, pdsize, textsize" + ), + )) + } + } + Ok(()) +} + fn set_sysvar(document: &mut codec::CadDocument, name: &str, value: &Value) -> Result<(), Value> { + validate_sysvar(document, name, value)?; let failure = |message: &str| crate::app::control::failure("invalid_sysvar_value", message); let header = &mut document.header; match name.to_ascii_lowercase().as_str() {