From ebdddbe1f52a488ff1c17af024a90ccb236f0ac4 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Thu, 22 Oct 2026 15:06:29 +0100 Subject: [PATCH] Stop automation re-routing dialogs to file-writing handlers `dialogs::confirm` picks a dialog's handler by marker fields (`__diskChanged`, `__export`, `__web`, `__print`, ...). Only the app sets them now: - `ui.dialog.set` and `ui.dialog.open`'s `fields` refuse to add a key any of confirm's or cancel's `owns()` predicates route on (`dialogs::is_route_marker`, which probes the predicates themselves). A dialog's own fields, private ones included, stay settable. - `dialogs::confirm`, when driven by automation (a control request's gate or synthetic input), judges a handler that writes files itself as the file command it stands for: file.save, file.export.exportAs, file.export.saveForWebLegacy or file.print. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/control.rs | 76 +++++++++++++++++++++++++++++++++++ crates/ui-egui/src/dialogs.rs | 70 ++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+) diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index c30ed4b8f1..8e5c8d0898 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -902,6 +902,9 @@ fn dispatch(app: &mut PhotocraftApp, ctx: &egui::Context, req: &ControlRequest) }; let mut fields = if kind == DialogKind::NewDocument { app.new_document_fields() } else { Default::default() }; if let Some(f) = p.get("fields").and_then(Value::as_object) { + if let Some(marker) = f.keys().find(|k| crate::dialogs::is_route_marker(k)) { + return err(format!("`{marker}` is set only by the dialog it belongs to")); + } fields.extend(f.clone()); } ok(json!({"dialog": app.ui.open_dialog(kind, fields)})) @@ -955,6 +958,10 @@ fn dispatch(app: &mut PhotocraftApp, ctx: &egui::Context, req: &ControlRequest) let (Some(id), Some(field)) = (u("dialog"), s("field")) else { return err("need `dialog` and `field`") }; let value = p.get("value").cloned().unwrap_or(Value::Null); match app.ui.dialog_mut(id) { + // A marker picks the handler OK runs: a dialog keeps the ones it was opened with. + Some(d) if !d.fields.contains_key(field) && crate::dialogs::is_route_marker(field) => { + err(format!("`{field}` is set only by the dialog it belongs to")) + } Some(d) => { d.fields.insert(field.to_string(), value); ok(Value::Null) @@ -2661,6 +2668,75 @@ mod tests { } } + /// An app whose automation gates refuse file commands, with a document open, and the paths + /// its writer was asked to write. + fn gated_app_recording_writes() -> (PhotocraftApp, egui::Context, std::rc::Rc>>, u64) { + let written: std::rc::Rc>> = std::rc::Rc::default(); + let w = written.clone(); + let services = crate::Services { + automation_authorize: Some(deny_ambient_file), + automation_command: Some(Box::new(|id, params| deny_ambient_file(id, params).map_err(|e| e.to_string()))), + export: Some(Box::new(|_: &photocraft_doc::Document, _: &str, _: &crate::ExportSettings| Ok((b"out".to_vec(), Vec::new())))), + write: Some(Box::new(move |path: &str, _: &[u8]| { + w.borrow_mut().push(path.to_string()); + Ok(()) + })), + ..Default::default() + }; + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), services); + app.background_jobs = false; + app.session.execute("file.new", json!({"width": 4, "height": 4})).unwrap(); + let doc = app.session.active().unwrap().doc.id.0; + (app, egui::Context::default(), written, doc) + } + + fn outside_target() -> String { + std::env::temp_dir().join("outside").join("x.png").to_string_lossy().into_owned() + } + + /// `ui.dialog.set` and `ui.dialog.open` never add a marker that picks a dialog's OK handler: + /// a View › Zoom In dialog can't be made to confirm as the disk-change prompt's save. + #[test] + fn automation_cannot_add_a_marker_that_reroutes_a_dialog() { + let (mut app, ctx, written, doc) = gated_app_recording_writes(); + let target = outside_target(); + let r = call(&mut app, &ctx, "ui.dialog.open", json!({"kind": "command", "command": "view.zoomIn"})); + let dialog = r["result"]["dialog"].clone(); + for (field, value) in [("document", json!(doc)), ("path", json!(target)), ("flat", json!(false))] { + let r = call(&mut app, &ctx, "ui.dialog.set", json!({"dialog": dialog, "field": field, "value": value})); + assert!(r.get("error").is_none(), "{r}"); + } + let injected = call(&mut app, &ctx, "ui.dialog.set", json!({"dialog": dialog, "field": "__diskChanged", "value": true})); + call(&mut app, &ctx, "ui.dialog.confirm", json!({"dialog": dialog})); + assert!(written.borrow().is_empty(), "nothing is written: {:?}", written.borrow()); + assert!(injected["error"].as_str().is_some_and(|e| e.contains("set only by the dialog")), "{injected}"); + let r = call(&mut app, &ctx, "ui.dialog.open", json!({"kind": "about", "fields": {"__export": true}})); + assert!(r["error"].as_str().is_some_and(|e| e.contains("set only by the dialog")), "{r}"); + } + + /// A handler that writes files itself (the disk-change prompt's save) is judged as the file + /// command it stands for, through a control request or synthetic input alike. + #[test] + fn automation_confirming_a_file_writing_dialog_is_judged_as_its_file_command() { + let (mut app, ctx, written, doc) = gated_app_recording_writes(); + let target = outside_target(); + // A real disk-change prompt keeps its own fields settable, but its save is a file command. + let fields = json!({"__diskChanged": true, "__label": "Save", "message": "", "document": doc, "path": target, "flat": false}); + let prompt = app.ui.open_dialog(DialogKind::Command, fields.as_object().cloned().unwrap()); + let r = call(&mut app, &ctx, "ui.dialog.set", json!({"dialog": prompt, "field": "__diskChanged", "value": true})); + assert!(r.get("error").is_none(), "{r}"); + let r = call(&mut app, &ctx, "ui.dialog.confirm", json!({"dialog": prompt})); + assert!(written.borrow().is_empty(), "nothing is written: {:?}", written.borrow()); + assert!(r["error"].as_str().is_some_and(|e| e.contains("file.save")), "{r}"); + // Synthetic input (Enter or OK) reaches the same handler without the control pre-check. + let synthetic = app.ui.open_dialog(DialogKind::Command, fields.as_object().cloned().unwrap()); + app.automation_input = true; + let r = crate::dialogs::confirm(&mut app, synthetic); + app.automation_input = false; + assert!(written.borrow().is_empty(), "nothing is written: {:?}", written.borrow()); + assert!(r.is_err_and(|e| e.contains("file.save"))); + } + #[test] fn app_open_opens_a_directory_pcraft_and_falls_back_for_the_zip_form() { use photocraft_doc::Document; diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index acf77870ef..b58f1ec89e 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -688,11 +688,81 @@ pub fn title(d: &Dialog) -> String { } } +type Fields = serde_json::Map; + +/// The markers [`confirm`] and [`cancel`] choose a dialog's handler by: keys the code opening a +/// dialog puts in its fields. Keep in step with the arms of `confirm`. +const ROUTES: &[fn(&Fields) -> bool] = &[ + crate::guide_layout_ui::owns, + crate::brush_preset_dialog::owns, + crate::fill_ui::owns, + crate::pattern_fill_ui::owns, + crate::stroke_ui::owns, + crate::channel_options_ui::owns, + crate::channel_options_ui::owns_new_spot, + crate::shape_dialog::owns, + crate::delete_layer_prompt::owns, + crate::disk_change_prompt::owns, + crate::actions::owns_insert_stop, + crate::new_layer_ui::owns, + crate::mask_trash_prompt::owns, + crate::rasterize_prompt::owns, + crate::duplicate_layer_ui::owns, + crate::stack_dialog::owns, + crate::tool_reset::owns, + crate::crop_presets::owns, + crate::variables_ui::owns, + crate::file_ui::owns, + crate::new_guide_ui::owns, + crate::color_picker_ui::owns, + crate::color_range_ui::owns, + crate::prefs_ui::owns, + crate::open_prompt::owns, + crate::layer_style::color_picker::owns, + |f| f.contains_key("__export"), + |f| f.contains_key("__smartFilter"), + |f| f.contains_key(crate::actions::RESUME), +]; + +/// Whether `key` is a marker that picks a dialog's handler. Automation may change the values of +/// a dialog's fields, private ones included, but never add a marker: a dialog it opened would +/// otherwise confirm through another dialog's handler, one its command was never judged for. +pub fn is_route_marker(key: &str) -> bool { + let probe = Fields::from_iter([(key.to_string(), Value::Bool(true))]); + ROUTES.iter().any(|owns| owns(&probe)) +} + +/// The file command a dialog's handler stands for when it reads or writes files itself, outside +/// the engine (a disk-change save, Export As, Save for Web, Print), so automation is judged for +/// that command rather than for the dialog's `__command`. +fn file_command(fields: &Fields) -> Option<&'static str> { + if crate::disk_change_prompt::owns(fields) { + Some("file.save") + } else if fields.contains_key("__export") { + Some("file.export.exportAs") + } else if fields.contains_key("__web") { + Some("file.export.saveForWebLegacy") + } else if fields.contains_key("__print") { + Some("file.print") + } else { + None + } +} + /// Confirm a dialog: run its action and close it. Used by the OK button and by automation. pub fn confirm(app: &mut PhotocraftApp, id: u64) -> Result { if crate::layer_style::color_picker::has_child(app, id) { return Err("confirm or cancel the Layer Style color picker first".into()); } + // A control request installs the session gate; synthetic input sets `automation_input`. + // Either way the handler's own file access is judged before it runs, as its command would be. + if (app.automation_input || app.session.authorize.is_some()) + && let Some(d) = app.ui.dialogs.iter().find(|d| d.id == id) + && let Some(command) = file_command(&d.fields) + && let Some(authorize) = app.services.automation_command.as_ref() + { + authorize(command, &Value::Object(d.fields.clone()))?; + } if let Some(d) = app.ui.dialogs.iter().find(|d| d.id == id).filter(|d| crate::guide_layout_ui::owns(&d.fields)) { if app.ui.dialogs.iter().any(|child| crate::color_picker_ui::owns(&child.fields) && child.fields.get("__dialog").and_then(Value::as_u64) == Some(id)) { return Err("confirm or cancel the guide color picker first".into());