Preferences: add an Apply button (#325) (#325)

ATTRIBUTION.md

@@ -30,6 +30,7 @@ The other built-in ICC profiles and the generated LUT looks are produced by code
 |---|---|---|---|---|
 | `docs/brand/` (all files) | ArtCraft name, wordmark and mark | ArtCraft Team | getartcraft.com | Not open source; trademarks of the ArtCraft Team, [`docs/brand/LICENSE-brand.txt`](docs/brand/LICENSE-brand.txt) |
 | `docs/images/photocraft-*.jpg` | PhotoCraft screenshots | PhotoCraft contributors (UI) | Rendered offscreen with the `snapshot` example | MIT OR Apache-2.0 (UI); the artwork in each is public domain, listed below |
+| `docs/images/preferences-apply-*.png` | Preferences before and after adding Apply (no artwork) | PhotoCraft contributors | PhotoCraft control-channel capture and offscreen `snapshot` example | MIT OR Apache-2.0 |
 
 Artwork shown in the screenshots (all public domain, via Wikimedia Commons; details in
 [`docs/images/SOURCES.md`](docs/images/SOURCES.md)):

crates/ui-egui/src/control.rs

@@ -10,6 +10,7 @@
 //! - `ui.set {tool?, panels?, zoom?, center?, dark?}`: change UI state
 //! - `ui.menu.invoke {id}` / `ui.menu.list`: activate a menu item by id; list the menu tree
 //! - `ui.dialog.open {kind, fields?}` (kinds: newDocument, about, layerStyle {effect?}, colorPicker {target: foreground|background}, command {command}) / `ui.dialog.set {dialog, field, value}` / `ui.dialog.confirm {dialog}` / `ui.dialog.cancel {dialog}`
+//! - `ui.dialog.apply {dialog}`: commit Preferences changes without closing the dialog
 //! - `ui.window.open {document?}` / `ui.window.close {window}`: extra document windows
 //! - `ui.pointer {events: [{kind: down|move|up, x, y, pressure?, tiltX?, tiltY?, rotation?}], modifiers?, button?}`: drive the active tool in document coordinates (`button: "secondary"` = the right button: opens the Brush Preset picker, or erases with Preferences › Tools › Right-click with painting tools = erase)
 //! - `ui.click {x, y, button?, count?}` / `ui.move {x, y}`: synthetic pointer input in screen points
@@ -234,10 +235,14 @@ pub fn handle(app: &mut PhotocraftApp, ctx: &egui::Context, req: &ControlRequest
                 None => err(format!("no dialog {id}")),
             }
         }
-        "ui.dialog.confirm" => match u("dialog") {
+        "ui.dialog.confirm" | "ui.dialog.apply" => match u("dialog") {
             Some(id) => {
                 let command = app.ui.dialogs.iter().find(|dialog| dialog.id == id).and_then(|dialog| {
-                    dialog.fields.get("__command").and_then(Value::as_str).map(|command| (command.to_string(), Value::Object(dialog.fields.clone())))
+                    if req.method == "ui.dialog.apply" {
+                        dialog.fields.get("values").map(|values| ("prefs.set".to_string(), json!({"path": "", "value": values})))
+                    } else {
+                        dialog.fields.get("__command").and_then(Value::as_str).map(|command| (command.to_string(), Value::Object(dialog.fields.clone())))
+                    }
                 });
                 if let Some((command, params)) = command
                     && let Some(authorize) = app.services.automation_command.as_ref()
@@ -249,7 +254,7 @@ pub fn handle(app: &mut PhotocraftApp, ctx: &egui::Context, req: &ControlRequest
                 if events_enabled {
                     app.session.edit_prefs(|prefs| prefs.script_events.enabled = false);
                 }
-                let result = crate::dialogs::confirm(app, id);
+                let result = if req.method == "ui.dialog.apply" { crate::prefs_ui::apply(app, id) } else { crate::dialogs::confirm(app, id) };
                 if events_enabled {
                     app.session.edit_prefs(|prefs| prefs.script_events.enabled = true);
                 }
@@ -520,6 +525,43 @@ mod tests {
         assert_eq!(app.session.active().unwrap().revision, rev, "opening a dialog must not edit the document");
     }
 
+    #[test]
+    fn preferences_apply_is_available_over_control_without_closing() {
+        let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());
+        let ctx = egui::Context::default();
+        let r = call(&mut app, &ctx, "ui.menu.invoke", json!({"id": "edit.preferences.interface"}));
+        let id = r["result"]["dialog"].as_u64().unwrap();
+        let mut values = app.ui.dialog_mut(id).unwrap().fields["values"].clone();
+        values["interface"]["uiScale"] = json!("200");
+        assert_eq!(call(&mut app, &ctx, "ui.dialog.set", json!({"dialog": id, "field": "values", "value": values}))["ok"], true);
+        assert_eq!(call(&mut app, &ctx, "ui.dialog.apply", json!({"dialog": id}))["ok"], true);
+        assert_eq!(app.session.prefs().interface.ui_scale, photocraft_engine::prefs::UiScale::P200);
+        assert!(app.ui.dialog_mut(id).is_some());
+        assert_eq!(call(&mut app, &ctx, "ui.dialog.cancel", json!({"dialog": id}))["ok"], true);
+        assert_eq!(app.session.prefs().interface.ui_scale, photocraft_engine::prefs::UiScale::P200);
+        for params in [json!({}), json!({"dialog": "invalid"}), json!({"dialog": u64::MAX})] {
+            assert_eq!(call(&mut app, &ctx, "ui.dialog.apply", params)["ok"], false);
+        }
+    }
+
+    #[test]
+    fn preferences_apply_respects_the_automation_command_policy() {
+        let services = crate::Services {
+            automation_command: Some(Box::new(|id, _| if id == "prefs.set" { Err("preference changes denied".into()) } else { Ok(()) })),
+            ..Default::default()
+        };
+        let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), services);
+        let ctx = egui::Context::default();
+        let before = app.session.prefs().to_json();
+        let id = crate::prefs_ui::open_preferences(&mut app, "interface");
+        app.ui.dialog_mut(id).unwrap().fields.get_mut("values").unwrap()["interface"]["uiScale"] = json!("200");
+        let r = call(&mut app, &ctx, "ui.dialog.apply", json!({"dialog": id}));
+        assert_eq!(r["ok"], false);
+        assert_eq!(r["error"], "preference changes denied");
+        assert_eq!(app.session.prefs().to_json(), before);
+        assert!(app.ui.dialog_mut(id).is_some());
+    }
+
     #[test]
     fn synthetic_shortcuts_use_the_automation_command_policy() {
         let services = crate::Services {

crates/ui-egui/src/dialogs.rs

@@ -43,6 +43,7 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) {
     for d in dialogs {
         let mut fields = d.fields.clone();
         let mut outcome: Option<bool> = None; // Some(true)=OK, Some(false)=Cancel
+        let mut apply_requested = false;
         let title = display_title(&d);
         let id = egui::Id::new(("dialog", d.id));
         // Offset from centre, moved by dragging the title bar (view state only, so egui memory).
@@ -138,6 +139,10 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) {
                     if crate::widgets::primary_button(ui, ok_label, 84.0).clicked() || ui.input(|i| i.key_pressed(egui::Key::Enter)) {
                         outcome = Some(true);
                     }
+                    if d.kind == DialogKind::Command && crate::prefs_ui::is_preferences(&fields) {
+                        let changed = crate::prefs_ui::preferences_changed(app, &fields);
+                        apply_requested = ui.add_enabled_ui(changed, |ui| crate::widgets::secondary_button(ui, tl!("Apply"), 84.0)).inner.clicked();
+                    }
                     if crate::widgets::secondary_button(ui, if d.kind == DialogKind::NewDocument { tl!("Close") } else { tl!("Cancel") }, 84.0).clicked() {
                         outcome = Some(false);
                     }
@@ -163,6 +168,9 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) {
         if let Some(dm) = app.ui.dialog_mut(d.id) {
             dm.fields = fields;
         }
+        if apply_requested && outcome.is_none() {
+            let _ = crate::prefs_ui::apply(app, d.id);
+        }
         match outcome {
             Some(true) => {
                 let _ = confirm(app, d.id);

crates/ui-egui/src/prefs_ui.rs

@@ -5,7 +5,7 @@
 //!
 //! The values live in the engine ([`photocraft_engine::prefs::Preferences`], so agents read and
 //! change them with `prefs.get` / `prefs.set`); this module only edits a working copy in a dialog
-//! and commits it with those commands on OK.
+//! and commits it with those commands on Apply or OK.
 
 use std::collections::{BTreeMap, HashMap};
 
@@ -485,6 +485,37 @@ pub fn owns(fields: &Map<String, Value>) -> bool {
     fields.contains_key("__prefsui")
 }
 
+/// Only Preferences supports applying changes without closing the dialog.
+pub fn is_preferences(fields: &Map<String, Value>) -> bool {
+    fields.get("__prefsui").and_then(Value::as_str) == Some("prefs")
+}
+
+fn preference_values(p: &prefs::Preferences) -> Map<String, Value> {
+    let values = p.to_json();
+    SECTIONS.iter().filter_map(|(id, _)| Some((id.to_string(), values.get(id)?.clone()))).collect()
+}
+
+/// Compare the editable sections, excluding settings managed outside Preferences.
+pub fn preferences_changed(app: &PhotocraftApp, fields: &Map<String, Value>) -> bool {
+    is_preferences(fields) && fields.get("values").and_then(Value::as_object).is_some_and(|values| *values != preference_values(app.session.prefs()))
+}
+
+/// Commit the working copy and keep the current section open. Failure leaves the draft intact.
+pub fn apply(app: &mut PhotocraftApp, id: u64) -> Result<Value, String> {
+    let d = app.ui.dialogs.iter().find(|d| d.id == id).ok_or_else(|| format!("no dialog {id}"))?;
+    if d.kind != DialogKind::Command || !is_preferences(&d.fields) {
+        return Err("Apply is only available for Preferences".into());
+    }
+    let fields = d.fields.clone();
+    let result = confirm(app, &fields)?;
+    let values = Value::Object(preference_values(app.session.prefs()));
+    if let Some(d) = app.ui.dialog_mut(id) {
+        // Use the validated values as the next draft, including any normalisation by prefs.set.
+        d.fields.insert("values".into(), values);
+    }
+    Ok(result)
+}
+
 /// Max dialog width for our dialogs.
 pub fn width(fields: &Map<String, Value>) -> Option<f32> {
     match fields.get("__prefsui").and_then(Value::as_str)? {
@@ -497,8 +528,7 @@ pub fn width(fields: &Map<String, Value>) -> Option<f32> {
 /// Open Edit › Preferences on `section`.
 pub fn open_preferences(app: &mut PhotocraftApp, section: &str) -> u64 {
     let section = if SECTIONS.iter().any(|(id, _)| *id == section) { section } else { "general" };
-    let values = app.session.prefs().to_json();
-    let working: Map<String, Value> = SECTIONS.iter().filter_map(|(id, _)| Some((id.to_string(), values.get(id)?.clone()))).collect();
+    let working = preference_values(app.session.prefs());
     let order = field_order(app.session.prefs(), &working);
     let gpu = app.perf.gpu_info.lines();
     open_kind(app, "prefs", "Preferences", json!({"section": section, "values": working, "__order": order, "__gpuInfo": gpu}))
@@ -1396,6 +1426,109 @@ mod tests {
         assert_eq!(app.session.prefs().performance.history_states, 50);
     }
 
+    #[test]
+    fn preferences_apply_button_saves_without_closing_and_cancel_keeps_applied_values() {
+        use egui_kittest::{
+            Harness,
+            kittest::{NodeT, Queryable},
+        };
+
+        let (mut app, store) = app_with_store();
+        app.run("prefs.set", json!({"values": {"interface.language": "en", "type.smartQuotes": false}})).unwrap();
+        let id = open_preferences(&mut app, "interface");
+        let mut h = Harness::builder().with_size(vec2(1280.0, 800.0)).build_eframe(move |cc| {
+            PhotocraftApp::setup_context(&cc.egui_ctx, Default::default());
+            app
+        });
+        h.run_steps(4);
+        assert!(h.get_by_label("Apply").accesskit_node().is_disabled());
+
+        let values = h.state_mut().ui.dialog_mut(id).unwrap().fields.get_mut("values").unwrap();
+        values["interface"]["theme"] = json!("studioLight");
+        values["performance"]["historyStates"] = json!(12);
+        h.run_steps(2);
+        assert!(!h.get_by_label("Apply").accesskit_node().is_disabled());
+        h.get_by_label("Apply").click();
+        h.run_steps(4);
+
+        assert_eq!(h.state().session.prefs().performance.history_states, 12);
+        assert_eq!(h.state().ui.theme, ThemeKind::StudioLight);
+        assert!(!h.state().session.prefs().type_.smart_quotes, "hidden settings round-trip unchanged");
+        let d = h.state().ui.dialogs.iter().find(|d| d.id == id).unwrap();
+        assert_eq!(d.fields["section"], "interface");
+        assert_eq!(d.fields["values"]["performance"]["historyStates"], 12);
+        assert!(h.get_by_label("Apply").accesskit_node().is_disabled());
+        let saved: Value = serde_json::from_str(store.lock().unwrap().as_ref().unwrap()).unwrap();
+        assert_eq!(saved["performance"]["historyStates"], 12);
+        assert_eq!(saved["interface"]["theme"], "studioLight");
+
+        // Repeated Apply starts from the validated values, not the dialog's original snapshot.
+        h.state_mut().ui.dialog_mut(id).unwrap().fields.get_mut("values").unwrap()["performance"]["historyStates"] = json!(22);
+        h.run_steps(2);
+        h.get_by_label("Apply").click();
+        h.run_steps(4);
+        assert_eq!(h.state().session.prefs().performance.history_states, 22);
+        h.state_mut().ui.dialog_mut(id).unwrap().fields.get_mut("values").unwrap()["performance"]["historyStates"] = json!(33);
+        h.run_steps(2);
+        h.get_by_label("Cancel").click();
+        h.run_steps(4);
+        assert!(h.state().ui.dialogs.iter().all(|d| d.id != id));
+        assert_eq!(h.state().session.prefs().performance.history_states, 22);
+        let saved = store.lock().unwrap().clone().unwrap();
+        let (mut restarted, _) = app_with_saved(Some(saved));
+        tick(&mut restarted, &egui::Context::default());
+        assert_eq!(restarted.session.prefs().performance.history_states, 22);
+        assert_eq!(restarted.ui.theme, ThemeKind::StudioLight);
+    }
+
+    #[test]
+    fn preferences_confirm_after_apply_commits_later_edits() {
+        let (mut app, _) = app_with_store();
+        let id = open_preferences(&mut app, "cursors");
+        app.ui.dialog_mut(id).unwrap().fields.get_mut("values").unwrap()["cursors"]["painting"] = json!("fullSizeTip");
+        apply(&mut app, id).unwrap();
+        app.ui.dialog_mut(id).unwrap().fields.get_mut("values").unwrap()["unitsAndRulers"]["rulers"] = json!("inches");
+        crate::dialogs::confirm(&mut app, id).unwrap();
+        assert!(app.ui.dialogs.iter().all(|d| d.id != id));
+        assert_eq!(app.session.prefs().cursors.painting, prefs::PaintingCursor::FullSizeTip);
+        assert_eq!(app.session.prefs().units_and_rulers.rulers, prefs::Unit::Inches);
+    }
+
+    #[test]
+    fn preferences_apply_rejects_invalid_drafts_and_keeps_them_open() {
+        let (mut app, _) = app_with_store();
+        let before = app.session.prefs().to_json();
+        for invalid in [json!(0), json!("invalid"), Value::Null] {
+            let id = open_preferences(&mut app, "performance");
+            app.ui.dialog_mut(id).unwrap().fields.get_mut("values").unwrap()["performance"]["historyStates"] = invalid;
+            let draft = app.ui.dialog_mut(id).unwrap().fields.clone();
+            assert!(apply(&mut app, id).is_err());
+            assert_eq!(app.session.prefs().to_json(), before);
+            assert_eq!(app.ui.dialog_mut(id).unwrap().fields, draft);
+            app.ui.close_dialog(id);
+        }
+        let id = open_preferences(&mut app, "interface");
+        app.ui.dialog_mut(id).unwrap().fields.remove("values");
+        assert!(apply(&mut app, id).is_err());
+        assert!(app.ui.dialog_mut(id).is_some());
+        assert_eq!(app.session.prefs().to_json(), before);
+    }
+
+    #[test]
+    fn apply_rejects_missing_and_non_preferences_dialogs() {
+        let (mut app, _) = app_with_store();
+        let before = app.session.prefs().to_json();
+        assert!(apply(&mut app, u64::MAX).is_err());
+        let id = open_shortcuts(&mut app, 0);
+        assert!(apply(&mut app, id).is_err());
+        assert!(app.ui.dialog_mut(id).is_some());
+        let fields = json!({"__prefsui": "prefs", "values": preference_values(app.session.prefs())}).as_object().unwrap().clone();
+        let id = app.ui.open_dialog(DialogKind::About, fields);
+        assert!(apply(&mut app, id).is_err());
+        assert!(app.ui.dialog_mut(id).is_some());
+        assert_eq!(app.session.prefs().to_json(), before);
+    }
+
     #[test]
     fn shortcut_dialog_moves_shortcuts_and_shell_dispatch_follows() {
         let (mut app, _) = app_with_store();

docs/control-protocol.md

@@ -38,6 +38,7 @@ The transport is `apps/photocraft/src/control_server.rs`, and the handlers are i
 - `ui.set {tool?, panels?, dockTabs?, dock?, dockWidth?, maskTarget?, selectionMode?, zoom?, center?, fit?, theme?, brushSize?, brushSection?, brushTab?}`: change UI state (`selectionMode` is the selection tools' options-bar mode, 0 New, 1 Add, 2 Subtract, 3 Intersect; `brushSection` indexes the Brush Settings sections, `brushTab` 0 = Brush Settings, 1 = Brushes; `dock` is `{order: ["layers", …], heights: {"properties": 180}, collapsed: ["color"]}`, the right-dock groups top to bottom, their heights in points and the groups collapsed to their tab strip; `dockWidth` sets the right dock width in points, clamped to 250..520)
 - `ui.menu.invoke {id}` / `ui.menu.list`: activate a menu item by id; list the menu tree
 - `ui.dialog.open {kind, fields?}` (kinds `newDocument`, `about`, `layerStyle {effect?}`, `colorPicker {target: foreground|background}`, `command {command}`) / `ui.dialog.set {dialog, field, value}` / `ui.dialog.confirm {dialog}` / `ui.dialog.cancel {dialog}`
+  - Preferences (`ui.menu.invoke {id: "edit.preferences.interface"}`) edits the sections in its `values` field. `ui.dialog.apply {dialog}` saves those values through `prefs.set` and keeps the same dialog and section open. `ui.dialog.confirm` saves and closes; `ui.dialog.cancel` discards only edits made since the last successful Apply. Invalid values return an error without closing the Apply dialog or changing the saved preferences. Settings marked for the next launch still require a restart.
   - Edit › Fill… (`ui.menu.invoke {id: "edit.fill"}`, Shift+F5, Shift+Backspace) opens the Fill dialog; its fields are `edit.fill`'s params (`contents`, `color`, `pattern`, `colorAdaptation`, `mode`, `opacity`, `preserveTransparency`), and OK remembers them in the preferences (`dialogs["edit.fill"]`).
   - A pixel tool pressed on a type, shape, Smart Object or fill layer (e.g. through `ui.pointer`) opens the "Rasterize?" prompt instead of painting: a dialog with `__rasterize` (`type|shape|smartObject|fill`), `message`, `layer`, `tool` and `at`. `ui.dialog.confirm` runs the `layer.rasterize.*` command and then paints at `at` (two history states, `{"rasterized", "painted"}`); `ui.dialog.cancel` does nothing.
 - `ui.window.open {document?}` / `ui.window.close {window}`: extra document windows

docs/ui-design.md

@@ -54,6 +54,13 @@ missing-glyph boxes. Check every new panel with the offscreen snapshot tool (`do
 
 Use `ui.click {x,y}`, `ui.move`, `ui.key` and `ui.type` (synthetic input in screen points) to open menus, popups and context menus, then `ui.screenshot`.
 
+## Preferences
+
+Preferences has **Apply**, **OK** and **Cancel**. Apply saves the edited sections and keeps the
+dialog open; it is disabled when the values match the saved preferences. OK saves and closes.
+Cancel discards only changes made since the last successful Apply. A failed Apply leaves the
+draft open for correction. Settings marked for the next launch still require a restart.
+
 ## High DPI and 4K displays
 
 Edit → Preferences → Interface → UI Scale applies immediately. Auto follows the operating