mirror of
https://github.com/storytold/photocraft.git
synced 2026-10-14 19:26:51 -04:00
* Fix #695: validate Layer Style effect fields Reject non-object effect parameter fields before preview or confirmation, render a visible validation message, and keep the canvas on the original document when preview input is invalid. Co-authored-by: Copilot <[email protected]> Copilot-Session: e77cd34a-ce3e-4981-996f-4d37d49df90d * fix(ui): keep global light preview fields compatible Co-authored-by: Copilot <[email protected]> Copilot-Session: e77cd34a-ce3e-4981-996f-4d37d49df90d * Layer Style: show an invalid-parameters error once Co-Authored-By: Claude Opus 5.5 <[email protected]> --------- Co-authored-by: Brandon Thomas <[email protected]> Co-authored-by: Claude Opus 5.5 <[email protected]> Copilot-Session: e77cd34a-ce3e-4981-996f-4d37d49df90d
This commit is contained in:
3 files changed
+162
-75
No files matched your search
@@ -478,9 +478,12 @@ fn display_doc(app: &mut PhotocraftApp, idx: usize) -> (std::sync::Arc<Document>
|
||||
let key = (crate::layer_style::preview_hash(&d.fields) ^ st.revision.wrapping_mul(0x9e37_79b9_7f4a_7c15)) | 1 << 63;
|
||||
if app.style_preview.as_ref().map(|p| p.0) != Some(key) {
|
||||
let shown = crate::layer_style::preview_document(&st.doc, &app.session.patterns, &d.fields).map(std::sync::Arc::new);
|
||||
if let Err(error) = &shown {
|
||||
log::warn!("Layer Style preview: {error}");
|
||||
}
|
||||
app.style_preview = Some((key, shown));
|
||||
}
|
||||
if let Some((_, Some(doc))) = &app.style_preview {
|
||||
if let Some((_, Ok(doc))) = &app.style_preview {
|
||||
return (doc.clone(), key);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -326,13 +326,30 @@ pub fn confirm(app: &mut PhotocraftApp, f: &Map<String, Value>) -> Result<Value,
|
||||
apply(f, |id, p| app.run(id, p))
|
||||
}
|
||||
|
||||
/// Every parameter group is edited as an object. Validate before preview or confirmation so
|
||||
/// malformed values supplied through `ui.dialog.set` are reported before any command runs.
|
||||
fn validate_params(f: &Map<String, Value>) -> Result<(), String> {
|
||||
for kind in std::iter::once(BLENDING).chain(KINDS.iter().map(|(kind, _)| *kind)) {
|
||||
let key = format!("p:{kind}");
|
||||
if let Some(value) = f.get(&key)
|
||||
&& !value.is_object()
|
||||
{
|
||||
return Err(format!("invalid layer style parameters: `{key}` must be an object"));
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Runs the dialog's commands through `run`: blending options, clear, then each enabled effect.
|
||||
fn apply(f: &Map<String, Value>, mut run: impl FnMut(&str, Value) -> Result<Value, String>) -> Result<Value, String> {
|
||||
validate_params(f)?;
|
||||
let layer = f.get("layer").cloned().unwrap_or(Value::Null);
|
||||
let initial_light_angle = f.get("globalLight").and_then(Value::as_f64);
|
||||
if let Some(Value::Object(bo)) = f.get(&format!("p:{BLENDING}")) {
|
||||
if let Some(bo) = f.get(&format!("p:{BLENDING}")).and_then(Value::as_object) {
|
||||
let mut p = Value::Object(bo.clone());
|
||||
p["layer"] = layer.clone();
|
||||
if let Some(params) = p.as_object_mut() {
|
||||
params.insert("layer".into(), layer.clone());
|
||||
}
|
||||
run("layer.layerStyle.blendingOptions", p)?;
|
||||
}
|
||||
let _ = run("layer.layerStyle.clear", json!({"layer": layer}));
|
||||
@@ -342,7 +359,9 @@ fn apply(f: &Map<String, Value>, mut run: impl FnMut(&str, Value) -> Result<Valu
|
||||
for &(kind, _) in KINDS {
|
||||
if f.get(&format!("on:{kind}")).and_then(Value::as_bool) == Some(true) {
|
||||
let mut p = f.get(&format!("p:{kind}")).cloned().unwrap_or_else(|| json!({}));
|
||||
p["layer"] = layer.clone();
|
||||
if let Some(params) = p.as_object_mut() {
|
||||
params.insert("layer".into(), layer.clone());
|
||||
}
|
||||
if p.get("useGlobalLight").and_then(Value::as_bool) == Some(true)
|
||||
&& let Some(angle) = p.get("angle").and_then(Value::as_f64)
|
||||
&& initial_light_angle.is_none_or(|initial| angle != initial)
|
||||
@@ -371,18 +390,19 @@ pub fn preview_document(
|
||||
doc: &photocraft_doc::Document,
|
||||
patterns: &photocraft_engine::pattern_cmds::PatternLibrary,
|
||||
f: &Map<String, Value>,
|
||||
) -> Option<photocraft_doc::Document> {
|
||||
) -> Result<photocraft_doc::Document, String> {
|
||||
let mut s = photocraft_engine::Session::new();
|
||||
s.patterns = patterns.clone();
|
||||
s.add_document(doc.clone(), None);
|
||||
apply(f, |id, p| s.execute(id, p).map_err(|e| e.to_string())).ok()?;
|
||||
s.active().map(|d| (*d.doc).clone())
|
||||
apply(f, |id, p| s.execute(id, p).map_err(|e| e.to_string()))?;
|
||||
s.active().map(|d| (*d.doc).clone()).ok_or_else(|| "no preview document".into())
|
||||
}
|
||||
|
||||
/// Dialog body (left list, right parameters).
|
||||
pub fn body(ui: &mut egui::Ui, f: &mut Map<String, Value>) {
|
||||
let t = Tokens::get(ui.ctx());
|
||||
let selected = f.get("selected").and_then(Value::as_str).unwrap_or("dropShadow").to_string();
|
||||
let invalid = validate_params(f).err();
|
||||
ui.horizontal_top(|ui| {
|
||||
// Left: effect list.
|
||||
ui.vertical(|ui| {
|
||||
@@ -440,79 +460,85 @@ pub fn body(ui: &mut egui::Ui, f: &mut Map<String, Value>) {
|
||||
ui.label(RichText::new(tl!(&label)).font(crate::theme::semibold(14.0)).color(t.text));
|
||||
ui.add_space(6.0);
|
||||
let pkey = format!("p:{selected}");
|
||||
let mut p = f.get(&pkey).cloned().unwrap_or_else(|| defaults(&selected));
|
||||
for &(key, label, kind) in spec(&selected) {
|
||||
match kind {
|
||||
P::Slider(min, max, unit) => {
|
||||
let mut v = p.get(key).and_then(Value::as_f64).unwrap_or(min as f64) as f32;
|
||||
if widgets::slider_row(ui, label, &mut v, min..=max, unit, None).changed() {
|
||||
p[key] = json!(v.round());
|
||||
// Only reachable through `ui.dialog.set`: say what is wrong once, keep the bad value.
|
||||
if let Some(error) = &invalid {
|
||||
ui.colored_label(t.danger, error);
|
||||
}
|
||||
let params = f.get(&pkey).cloned().unwrap_or_else(|| defaults(&selected));
|
||||
if let Some(mut p) = params.as_object().map(|o| Value::Object(o.clone())) {
|
||||
for &(key, label, kind) in spec(&selected) {
|
||||
match kind {
|
||||
P::Slider(min, max, unit) => {
|
||||
let mut v = p.get(key).and_then(Value::as_f64).unwrap_or(min as f64) as f32;
|
||||
if widgets::slider_row(ui, label, &mut v, min..=max, unit, None).changed() {
|
||||
p[key] = json!(v.round());
|
||||
}
|
||||
}
|
||||
P::Color => {
|
||||
ui.horizontal(|ui| {
|
||||
ui.label(RichText::new(tl!(&label)).color(t.text_dim));
|
||||
let hexs = p.get(key).and_then(Value::as_str).unwrap_or("#000000").to_string();
|
||||
let mut c = parse_hex(&hexs);
|
||||
if ui.color_edit_button_srgba(&mut c).changed() {
|
||||
p[key] = json!(format!("#{:02x}{:02x}{:02x}", c.r(), c.g(), c.b()));
|
||||
}
|
||||
});
|
||||
}
|
||||
P::Blend => {
|
||||
ui.horizontal(|ui| {
|
||||
ui.label(RichText::new(tl!(&label)).color(t.text_dim));
|
||||
let mut cur = p.get(key).and_then(Value::as_str).unwrap_or("Normal").to_string();
|
||||
let opts: Vec<(String, &str)> =
|
||||
photocraft_color::BlendMode::LAYER_MODES.iter().map(|m| (m.label().to_string(), m.label())).collect();
|
||||
if widgets::dropdown(ui, &format!("fx-blend-{selected}"), &mut cur, &opts, 150.0) {
|
||||
p[key] = json!(cur);
|
||||
}
|
||||
});
|
||||
}
|
||||
P::Choice(options) => {
|
||||
ui.horizontal(|ui| {
|
||||
ui.label(RichText::new(tl!(&label)).color(t.text_dim));
|
||||
let mut cur = p.get(key).and_then(Value::as_str).unwrap_or(options[0].0).to_string();
|
||||
let opts: Vec<(String, &str)> = options.iter().map(|(v, l)| (v.to_string(), *l)).collect();
|
||||
if widgets::dropdown(ui, &format!("fx-{selected}-{key}"), &mut cur, &opts, 150.0) {
|
||||
p[key] = json!(cur);
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
P::Color => {
|
||||
ui.horizontal(|ui| {
|
||||
ui.label(RichText::new(tl!(&label)).color(t.text_dim));
|
||||
let hexs = p.get(key).and_then(Value::as_str).unwrap_or("#000000").to_string();
|
||||
let mut c = parse_hex(&hexs);
|
||||
if ui.color_edit_button_srgba(&mut c).changed() {
|
||||
p[key] = json!(format!("#{:02x}{:02x}{:02x}", c.r(), c.g(), c.b()));
|
||||
}
|
||||
});
|
||||
}
|
||||
P::Blend => {
|
||||
ui.horizontal(|ui| {
|
||||
ui.label(RichText::new(tl!(&label)).color(t.text_dim));
|
||||
let mut cur = p.get(key).and_then(Value::as_str).unwrap_or("Normal").to_string();
|
||||
let opts: Vec<(String, &str)> =
|
||||
photocraft_color::BlendMode::LAYER_MODES.iter().map(|m| (m.label().to_string(), m.label())).collect();
|
||||
if widgets::dropdown(ui, &format!("fx-blend-{selected}"), &mut cur, &opts, 150.0) {
|
||||
p[key] = json!(cur);
|
||||
}
|
||||
});
|
||||
}
|
||||
P::Choice(options) => {
|
||||
ui.horizontal(|ui| {
|
||||
ui.label(RichText::new(tl!(&label)).color(t.text_dim));
|
||||
let mut cur = p.get(key).and_then(Value::as_str).unwrap_or(options[0].0).to_string();
|
||||
let opts: Vec<(String, &str)> = options.iter().map(|(v, l)| (v.to_string(), *l)).collect();
|
||||
if widgets::dropdown(ui, &format!("fx-{selected}-{key}"), &mut cur, &opts, 150.0) {
|
||||
p[key] = json!(cur);
|
||||
}
|
||||
});
|
||||
}
|
||||
P::Pattern => {
|
||||
let list: Vec<(String, String)> = f
|
||||
.get("patternList")
|
||||
.and_then(Value::as_array)
|
||||
.map(|a| a.iter().filter_map(|e| Some((e.get(0)?.as_str()?.to_string(), e.get(1)?.as_str()?.to_string()))).collect())
|
||||
.unwrap_or_default();
|
||||
ui.horizontal(|ui| {
|
||||
ui.label(RichText::new(tl!(&label)).color(t.text_dim));
|
||||
let mut cur = p
|
||||
.get(key)
|
||||
.and_then(Value::as_str)
|
||||
.filter(|c| !c.is_empty())
|
||||
.map(str::to_string)
|
||||
.or_else(|| list.first().map(|l| l.0.clone()))
|
||||
P::Pattern => {
|
||||
let list: Vec<(String, String)> = f
|
||||
.get("patternList")
|
||||
.and_then(Value::as_array)
|
||||
.map(|a| a.iter().filter_map(|e| Some((e.get(0)?.as_str()?.to_string(), e.get(1)?.as_str()?.to_string()))).collect())
|
||||
.unwrap_or_default();
|
||||
let opts: Vec<(String, &str)> = list.iter().map(|(id, n)| (id.clone(), n.as_str())).collect();
|
||||
if widgets::dropdown(ui, &format!("fx-{selected}-{key}"), &mut cur, &opts, 180.0)
|
||||
|| p.get(key).and_then(Value::as_str).is_none_or(str::is_empty)
|
||||
{
|
||||
p[key] = json!(cur);
|
||||
ui.horizontal(|ui| {
|
||||
ui.label(RichText::new(tl!(&label)).color(t.text_dim));
|
||||
let mut cur = p
|
||||
.get(key)
|
||||
.and_then(Value::as_str)
|
||||
.filter(|c| !c.is_empty())
|
||||
.map(str::to_string)
|
||||
.or_else(|| list.first().map(|l| l.0.clone()))
|
||||
.unwrap_or_default();
|
||||
let opts: Vec<(String, &str)> = list.iter().map(|(id, n)| (id.clone(), n.as_str())).collect();
|
||||
if widgets::dropdown(ui, &format!("fx-{selected}-{key}"), &mut cur, &opts, 180.0)
|
||||
|| p.get(key).and_then(Value::as_str).is_none_or(str::is_empty)
|
||||
{
|
||||
p[key] = json!(cur);
|
||||
}
|
||||
});
|
||||
}
|
||||
P::Check => {
|
||||
let mut b = p.get(key).and_then(Value::as_bool).unwrap_or(false);
|
||||
if widgets::checkbox(ui, &mut b, label).changed() {
|
||||
p[key] = json!(b);
|
||||
}
|
||||
});
|
||||
}
|
||||
P::Check => {
|
||||
let mut b = p.get(key).and_then(Value::as_bool).unwrap_or(false);
|
||||
if widgets::checkbox(ui, &mut b, label).changed() {
|
||||
p[key] = json!(b);
|
||||
}
|
||||
}
|
||||
ui.add_space(2.0);
|
||||
}
|
||||
ui.add_space(2.0);
|
||||
f.insert(pkey, p);
|
||||
}
|
||||
f.insert(pkey, p);
|
||||
});
|
||||
});
|
||||
}
|
||||
@@ -576,6 +602,64 @@ mod tests {
|
||||
assert_eq!((fx(&shown), fx(&st.doc)), (2, 0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn preview_rejects_non_object_effect_params() {
|
||||
let mut s = photocraft_engine::Session::new();
|
||||
s.execute("file.new", json!({"width": 16, "height": 16})).unwrap();
|
||||
s.execute("layer.new.layer", json!({})).unwrap();
|
||||
let st = s.active().unwrap();
|
||||
let mut f = initial_fields(st.doc.layer(st.active_layer.unwrap()).unwrap(), Some("colorOverlay"), st.doc.global_light.angle);
|
||||
f.insert("p:colorOverlay".into(), json!("invalid"));
|
||||
|
||||
assert!(preview_document(&st.doc, &s.patterns, &f).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apply_rejects_non_object_params_before_running_commands() {
|
||||
let mut f = Map::new();
|
||||
f.insert("p:stroke".into(), json!([1, 2]));
|
||||
f.insert("on:stroke".into(), json!(true));
|
||||
let mut calls = 0;
|
||||
|
||||
let result = apply(&f, |_, _| {
|
||||
calls += 1;
|
||||
Ok(Value::Null)
|
||||
});
|
||||
|
||||
assert!(result.is_err());
|
||||
assert_eq!(calls, 0, "invalid params must not partially apply the style");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn body_keeps_invalid_params_and_renders_without_panicking() {
|
||||
let ctx = egui::Context::default();
|
||||
PhotocraftApp::setup_context(&ctx, crate::theme::ThemeKind::ALL[0]);
|
||||
let mut fields = Map::new();
|
||||
fields.insert("selected".into(), json!("dropShadow"));
|
||||
fields.insert("p:dropShadow".into(), json!(7));
|
||||
|
||||
let mut out = ctx.run_ui(Default::default(), |ui| body(ui, &mut fields));
|
||||
out.textures_delta.clear();
|
||||
|
||||
assert_eq!(fields.get("p:dropShadow"), Some(&json!(7)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirm_rejects_invalid_params_and_applies_valid_effects() {
|
||||
let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default());
|
||||
app.run("file.new", json!({"width": 16, "height": 16})).unwrap();
|
||||
app.run("layer.new.layer", json!({})).unwrap();
|
||||
let layer = app.session.active().unwrap().doc.layer(app.session.active().unwrap().active_layer.unwrap()).unwrap().clone();
|
||||
let mut invalid = initial_fields(&layer, Some("colorOverlay"), 0.0);
|
||||
invalid.insert("p:colorOverlay".into(), json!(null));
|
||||
assert!(confirm(&mut app, &invalid).is_err());
|
||||
assert!(app.session.active().unwrap().doc.layer(layer.id).unwrap().effects.items.is_empty());
|
||||
|
||||
let valid = initial_fields(&layer, Some("colorOverlay"), 0.0);
|
||||
confirm(&mut app, &valid).unwrap();
|
||||
assert_eq!(app.session.active().unwrap().doc.layer(layer.id).unwrap().effects.items.len(), 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn percent_fields_round_trip_through_the_engine() {
|
||||
let mut s = photocraft_engine::Session::new();
|
||||
|
||||
@@ -340,8 +340,8 @@ pub struct PhotocraftApp {
|
||||
pub(crate) transform_preview: Option<transform_tool::TransformPreview>,
|
||||
/// Move-tool ⇧/⌥ drag state (move_mods).
|
||||
pub(crate) move_mods: move_mods::MoveDrag,
|
||||
/// Live Layer Style dialog preview: (key over revision + style fields, document with the style applied).
|
||||
pub(crate) style_preview: Option<(u64, Option<std::sync::Arc<Document>>)>,
|
||||
/// Live Layer Style dialog preview: (key over revision + style fields, preview or validation error).
|
||||
pub(crate) style_preview: Option<(u64, Result<std::sync::Arc<Document>, String>)>,
|
||||
/// Liquify dialog, Puppet Warp and Perspective Warp sessions (distort_ui).
|
||||
pub(crate) distort: distort_ui::Distort,
|
||||
/// Gradient tool live-mode drags and previews (gradient_ui).
|
||||
|
||||
Reference in new issue
Block a user