From a9326be31898aa350baa0396f2a2f35521885dae Mon Sep 17 00:00:00 2001 From: kubuntugamer <52578831+kubuntugamer@users.noreply.github.com> Date: Sat, 10 Oct 2026 00:14:38 -0500 Subject: [PATCH] fix(engine): object.opacity accepts its documented lowercase blend modes (#275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `object.opacity` documents `blend?: normal|multiply|…`, but parsed the value with `serde_json::from_value`, which only accepts the enum's PascalCase variant names. Every documented lowercase form was silently dropped: the command returned `{"changed": 1}` while the stored blend stayed as it was, so a client following the parameter description could save and export the previous appearance without any error (#260). Parse with the codebase's own `BlendMode::parse`, which is already case-, space-, underscore- and hyphen-insensitive and covered by tests, so `multiply`, `MULTIPLY`, `Color Burn`, `soft_light` and the serialized `Multiply` all resolve to the same mode. An unrecognized value is now an error rather than a silent no-op, matching how sibling commands treat a bad parameter — restoring `normal` after `Multiply` really does go back to Normal. Three tests in `blend_mode_tests`: the documented forms apply, the PascalCase names the Properties panel submits still apply, and an unknown value is rejected without changing the item. Fixes #260 🤖 Generated with Codebuff Co-authored-by: Codebuff --- crates/engine/src/cmd/object.rs | 76 ++++++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/object.rs b/crates/engine/src/cmd/object.rs index dba63ca7..7e219f29 100644 --- a/crates/engine/src/cmd/object.rs +++ b/crates/engine/src/cmd/object.rs @@ -4,6 +4,7 @@ use std::collections::HashMap; use std::sync::Arc; +use designcraft_color::BlendMode; use designcraft_doc::{ Content, Document, Fill, Item, ItemId, ParaFormat, Selection, Shape, SpreadRef, Story, StoryId, Stroke, TextFrame, TextFrameOptions, TextSel, }; @@ -219,7 +220,16 @@ pub fn specs() -> Vec { ), cmd!("object.opacity", "Opacity", [], None, "{opacity: 0..1, blend?: normal|multiply|…, ids?}", has_selection, |s, p| { let o = f64_or(p, "opacity", 1.0).clamp(0.0, 1.0) as f32; - let blend = p.get("blend").and_then(|b| serde_json::from_value(b.clone()).ok()); + // `BlendMode::parse` takes the documented lowercase forms (`multiply`), the panel + // labels (`Color Burn`) and the serialized names (`Multiply`). An unknown value is + // an error rather than a silent no-op that still reports `changed` (#260). + let blend = match p.get("blend") { + None | Some(Value::Null) => None, + Some(v) => { + let raw = v.as_str().ok_or_else(|| bad("object.opacity", "blend must be a string"))?; + Some(BlendMode::parse(raw).ok_or_else(|| bad("object.opacity", format!("unknown blend mode `{raw}`")))?) + } + }; set_flag( s, p, @@ -2422,3 +2432,67 @@ mod named_target_tests { assert_eq!(s.execute("conveyor.collect", &json!({"id": r["id"]})).unwrap()["count"], 1); } } + +#[cfg(test)] +mod blend_mode_tests { + use designcraft_color::BlendMode; + + use super::*; + + /// `object.opacity` documents `blend?: normal|multiply|…`, but deserializing the value + /// against the enum only accepted the PascalCase variant names, so the documented + /// lowercase forms were silently dropped: the command reported `changed: 1` while the + /// stored blend stayed as it was (#260). + #[test] + fn lowercase_documented_blend_modes_apply() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let r = s.execute("frame.create", &json!({"rect": [36, 36, 300, 300], "content": "unassigned"})).unwrap(); + let id = ItemId(r["id"].as_u64().unwrap()); + let blend = |s: &Session| s.doc().unwrap().doc.item(id).map(|i| i.blend); + + for (given, want) in [ + ("multiply", BlendMode::Multiply), + ("screen", BlendMode::Screen), + ("MULTIPLY", BlendMode::Multiply), + ("Color Burn", BlendMode::ColorBurn), + ("color-burn", BlendMode::ColorBurn), + ("soft_light", BlendMode::SoftLight), + ("normal", BlendMode::Normal), + ] { + let out = s.execute("object.opacity", &json!({"opacity": 1, "blend": given})).unwrap(); + assert_eq!(out["changed"], 1, "{given}"); + assert_eq!(blend(&s), Some(want), "blend mode {given:?} must be applied"); + } + } + + /// The PascalCase names keep working, so the Properties panel's own submissions are + /// unaffected. + #[test] + fn pascal_case_blend_modes_still_apply() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let r = s.execute("frame.create", &json!({"rect": [36, 36, 300, 300], "content": "unassigned"})).unwrap(); + let id = ItemId(r["id"].as_u64().unwrap()); + s.execute("object.opacity", &json!({"opacity": 0.5, "blend": "Multiply"})).unwrap(); + let it = s.doc().unwrap().doc.item(id).cloned().unwrap(); + assert_eq!(it.blend, BlendMode::Multiply); + assert!((it.opacity - 0.5).abs() < 1e-6, "opacity must still be set"); + } + + /// A blend mode the command doesn't know is an error, not a silent success that leaves + /// the previous appearance in place. + #[test] + fn an_unknown_blend_mode_is_an_error() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let r = s.execute("frame.create", &json!({"rect": [36, 36, 300, 300], "content": "unassigned"})).unwrap(); + let id = ItemId(r["id"].as_u64().unwrap()); + s.execute("object.opacity", &json!({"opacity": 1, "blend": "Multiply"})).unwrap(); + let e = s.execute("object.opacity", &json!({"opacity": 1, "blend": "sparkle"})).unwrap_err().to_string(); + assert!(e.contains("sparkle") && e.contains("blend"), "{e}"); + let it = s.doc().unwrap().doc.item(id).cloned().unwrap(); + assert_eq!(it.blend, BlendMode::Multiply, "a rejected value changes nothing"); + assert_eq!(it.opacity, 1.0); + } +}