From 211efca709c1764bed633bdd0573b24caaa5bcbc Mon Sep 17 00:00:00 2001 From: yoriswind Date: Tue, 3 Nov 2026 19:39:45 -0800 Subject: [PATCH] Keep paused action policy and spelling receivers bound --- crates/engine/src/cmd/actions_cmds.rs | 2 +- crates/engine/src/cmd/actions_cmds/history.rs | 31 ++++++- crates/engine/src/lib.rs | 2 + crates/ui-egui/src/panels/actions.rs | 36 ++++---- crates/ui-egui/src/panels/actions/tests.rs | 82 +++++++++++++++++++ crates/ui-egui/src/panels/type_panels_ui.rs | 34 +++++++- 6 files changed, 167 insertions(+), 20 deletions(-) diff --git a/crates/engine/src/cmd/actions_cmds.rs b/crates/engine/src/cmd/actions_cmds.rs index be535797d7..7fca1b3064 100644 --- a/crates/engine/src/cmd/actions_cmds.rs +++ b/crates/engine/src/cmd/actions_cmds.rs @@ -840,7 +840,7 @@ fn set_color(s: &mut Session, p: &Value) -> Result { /// Kept separate from Playback Options' performance modes until #5212 provides that dialog. fn set_history(s: &mut Session, p: &Value) -> Result { not_playing(s, "actions.setHistory")?; - if s.action_history.is_some() { + if s.action_history.is_some() || s.paused_action_history.iter().any(|lease| lease.strong_count() != 0) { return Err(bad("actions.setHistory", "cannot change action history settings during playback")); } let enabled = p.get("singleState").and_then(Value::as_bool).ok_or_else(|| bad("actions.setHistory", "singleState must be a boolean"))?; diff --git a/crates/engine/src/cmd/actions_cmds/history.rs b/crates/engine/src/cmd/actions_cmds/history.rs index da175cf5c3..01c19ac607 100644 --- a/crates/engine/src/cmd/actions_cmds/history.rs +++ b/crates/engine/src/cmd/actions_cmds/history.rs @@ -223,6 +223,7 @@ struct PausedDocument { /// Exact document/history binding of a dialog continuation. Never stored in dialog JSON. #[derive(Clone, Debug)] pub struct ActionHistoryContinuation { + _policy_lease: Arc<()>, group: Option, active: Option, documents: Vec, @@ -286,6 +287,9 @@ impl ActionHistoryContinuation { /// Publish the prefix and capture the same document/history/source binding for both callers. fn capture(s: &mut Session) -> Self { + s.paused_action_history.retain(|lease| lease.strong_count() != 0); + let policy_lease = Arc::new(()); + s.paused_action_history.push(Arc::downgrade(&policy_lease)); let group = s.action_history.take(); if let Some(group) = &group { consolidate(s, group); @@ -297,7 +301,7 @@ impl ActionHistoryContinuation { .filter(|st| Some(st.doc.id) == active || group.as_ref().is_some_and(|g| g.before.iter().any(|b| b.id == st.doc.id))) .map(|st| PausedDocument { id: st.doc.id, doc: st.doc.clone(), history: HistoryBinding::of(&st.history), source_snapshot: st.source_snapshot }) .collect(); - Self { group, active, documents } + Self { _policy_lease: policy_lease, group, active, documents } } } @@ -321,6 +325,26 @@ mod tests { use super::*; use serde_json::json; + #[test] + fn history_policy_stays_fixed_until_all_paused_continuations_are_released() { + for single in [false, true] { + let mut s = Session::new(); + s.execute("actions.setHistory", json!({"singleState":single})).unwrap(); + begin_document_history(&mut s, "Paused"); + let continuation = ActionHistoryContinuation::capture(&mut s); + let copy = continuation.clone(); + assert!(s.execute("actions.setHistory", json!({"singleState":!single})).is_err()); + assert_eq!(s.prefs().actions.create_single_history_state, single); + continuation.resume(&mut s).unwrap(); + finish_document_history(&mut s); + drop(continuation); + assert!(s.execute("actions.setHistory", json!({"singleState":!single})).is_err()); + drop(copy); + s.execute("actions.setHistory", json!({"singleState":!single})).unwrap(); + assert_eq!(s.prefs().actions.create_single_history_state, !single); + } + } + #[test] fn idle_dialog_rebind_keeps_first_edit_and_clear_boundaries() { for single in [false, true] { @@ -365,8 +389,9 @@ mod tests { s.execute("actions.setHistory", json!({"singleState":single})).unwrap(); begin_document_history(&mut s, "Spell"); let continuation = ActionHistoryContinuation::capture(&mut s); - s.execute("actions.setHistory", json!({"singleState":true})).unwrap(); - begin_document_history(&mut s, "Parent"); + // An independently owned parent scope must never be taken, even for a paused + // per-step action whose policy is now protected from changes. + s.action_history = Some(ActionHistory { name: "Parent".into(), external: true, before: Vec::new() }); let before = s.active().unwrap().doc.clone(); let rows = s.active().unwrap().history.entries(); assert!(continuation.suspend_after_edit(&mut s).is_err()); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 4bcf49b642..321480e479 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -207,6 +207,8 @@ pub struct Session { coalesce_request: Option, /// The outer action's original document/history states, independent of command transactions. action_history: Option, + /// Weak leases expire when every copy of a suspended continuation is abandoned. + paused_action_history: Vec>, /// Tests observe the actual scratch-session boundary without retaining it or changing output. #[cfg(test)] batch_history_observer: Option, diff --git a/crates/ui-egui/src/panels/actions.rs b/crates/ui-egui/src/panels/actions.rs index 03d646af32..d1b35dccd9 100644 --- a/crates/ui-egui/src/panels/actions.rs +++ b/crates/ui-egui/src/panels/actions.rs @@ -243,18 +243,28 @@ fn play_owned(app: &mut PhotocraftApp, params: &Value, suspended: Option) // from the next step when it is confirmed (`resume`). Agents play headlessly. if action.dialogs.contains(&i) && !app.automation_input && (crate::filter_dialog::has_dialog(id) || crate::adjust_dialog::has_dialog(id)) { let label = label_of(id); - let filter_receiver = (id.starts_with("filter.") && p.get("layer").is_none()).then(|| app.session.active().and_then(|st| st.active_layer)); + let active_layer = app.session.active().and_then(|st| st.active_layer); let dialog = crate::dialogs::open_command_dialog(app, id, &label); let token = app.paused_playbacks.pause(action.name.clone(), i + 1, app.session.authorize); - if let Some(paused) = app.paused_playbacks.paused.get_mut(&token) { - paused.filter_receiver = filter_receiver; - } + let mut receiver = None; if let Some(d) = app.ui.dialog_mut(dialog) { if let Some(o) = p.as_object() { // `__` keys are the dialog's own markers (`params_of` drops them), not step values. d.fields.extend(o.iter().filter(|(k, _)| !k.starts_with("__")).map(|(k, v)| (k.clone(), v.clone()))); } d.fields.insert(RESUME.into(), json!(token)); + // Mask/color-component targets still belong to a layer; alpha channels and + // Quick Mask belong to the document. Inspect the target the dialog captured. + let document_target = d + .fields + .get("target") + .is_some_and(|target| target.as_str() == Some("quickMask") || target.get("channel").and_then(Value::as_u64).is_some()); + if p.get("layer").is_none_or(Value::is_null) && !document_target { + receiver = Some(active_layer); + } + } + if let Some(paused) = app.paused_playbacks.paused.get_mut(&token) { + paused.filter_receiver = receiver; } paused = Some(json!({"step": i, "id": id, "dialog": dialog})); break; @@ -854,7 +864,7 @@ pub(crate) struct PausedPlayback { /// The document a pause at Check Spelling was playing on. That dialog doesn't block the /// document tabs, so Done ([`resume_spelling`]) continues only on this document. doc: Option, - /// An implicit filter step resolves this active layer at the pause. The outer Option marks + /// An implicit filter/adjustment step resolves this active layer at the pause. The outer Option marks /// that binding even when there was no active layer; explicit-layer and other dialogs omit it. filter_receiver: Option>, } @@ -890,7 +900,7 @@ pub(crate) fn run_step_dialog(app: &mut PhotocraftApp, fields: &serde_json::Map< if let Some(receiver) = paused.filter_receiver && app.session.active().and_then(|st| st.active_layer) != receiver { - return Err("the paused filter's receiver changed while its dialog was open".into()); + return Err("the paused command's receiver changed while its dialog was open".into()); } let captured = paused.gate; let saved = app.session.authorize; @@ -938,16 +948,12 @@ pub(crate) fn spelling_change(app: &mut PhotocraftApp, params: Value) -> Result< return Err("the active document changed while Check Spelling was open".into()); } let history = paused.history.clone(); - let gate = paused.gate.or(app.session.authorize); - if let Some(authorize) = gate { - authorize("edit.checkSpelling", ¶ms).map_err(|error| error.to_string())?; - } let fields = serde_json::Map::from_iter([(RESUME.to_string(), json!(token))]); - prepare_history(app, &fields)?; - let saved_gate = app.session.authorize; - app.session.authorize = gate; - let result = app.run("edit.checkSpelling", params); - app.session.authorize = saved_gate; + let result = run_step_dialog(app, &fields, "edit.checkSpelling", params); + // A rejected admission did not resume this continuation. Keep the original token intact. + if app.paused_playbacks.paused.get(&token).is_some_and(|paused| paused.history.is_some()) { + return result; + } if let Some(history) = history { match history.suspend_after_edit(&mut app.session) { Ok(history) => { diff --git a/crates/ui-egui/src/panels/actions/tests.rs b/crates/ui-egui/src/panels/actions/tests.rs index 83c5576860..d06d826eb9 100644 --- a/crates/ui-egui/src/panels/actions/tests.rs +++ b/crates/ui-egui/src/panels/actions/tests.rs @@ -2341,6 +2341,88 @@ fn a_stricter_current_gate_still_denies_the_captured_dialog_edit() { } } +#[test] +fn paused_spelling_checks_the_live_gate_before_resuming_history() { + fn deny_correction(id: &str, p: &Value) -> photocraft_engine::Result<()> { + if id == "edit.checkSpelling" && p["action"] == "changeAll" { Err(photocraft_engine::EngineError::Other("live spelling gate".into())) } else { Ok(()) } + } + for service in 0..3 { + let (mut app, _, _) = paused_history_spelling(true, Some(deny_second_spelling_change)); + let before = app.session.active().unwrap().doc.clone(); + let rows = app.session.active().unwrap().history.entries(); + let token = app.ui.type_panels.spell.as_ref().unwrap().resume.unwrap(); + app.session.authorize = None; + match service { + 0 => app.session.authorize = Some(deny_correction), + 1 => { + app.automation_input = true; + app.services.automation_authorize = Some(deny_correction); + } + _ => { + app.automation_input = true; + app.services.automation_command = Some(Box::new(|id, _| if id == "edit.checkSpelling" { Err("live spelling gate".into()) } else { Ok(()) })); + } + } + assert!(spelling_change(&mut app, json!({"action":"changeAll","word":"Teh","replace":"The"})).is_err()); + assert_eq!(app.session.active().unwrap().doc.as_ref(), before.as_ref()); + assert_eq!(app.session.active().unwrap().history.entries(), rows); + assert!(app.paused_playbacks.paused.get(&token).unwrap().history.is_some(), "denial must leave the bound continuation untouched"); + assert!(app.session.execute("actions.setHistory", json!({"singleState":false})).is_err()); + let fields = serde_json::Map::from_iter([(RESUME.to_string(), json!(token))]); + abandoned(&mut app, &fields); + app.session.execute("actions.setHistory", json!({"singleState":false})).unwrap(); + } +} + +#[test] +fn paused_adjustments_bind_implicit_pixel_and_mask_receivers() { + for explicit in [false, true] { + for mask in [false, true] { + let mut app = gated_blur_fixture(true, false); + let a = app.session.active().unwrap().active_layer.unwrap(); + app.run("layer.layerMask.revealAll", json!({})).unwrap(); + let b = photocraft_doc::LayerId(app.run("layer.new.layer", json!({"name":"B"})).unwrap()["layer"].as_u64().unwrap()); + app.session.select_layer(a).unwrap(); + let mut params = json!({"brightness":40}); + if mask { + params["target"] = json!("mask"); + } + if explicit { + params["layer"] = json!(a.0); + } + app.session.actions.list[0].steps[0] = ("image.adjustments.brightnessContrast".into(), params); + let dialog = pause_gated_blur(&mut app, None); + app.session.select_layer(b).unwrap(); + let before = app.session.active().unwrap().doc.clone(); + let result = crate::dialogs::confirm(&mut app, dialog); + if explicit { + result.unwrap(); + assert_eq!(app.session.active().unwrap().doc.layer(b).unwrap(), before.layer(b).unwrap()); + } else { + assert!(result.is_err_and(|e| e.contains("receiver changed"))); + assert_eq!(app.session.active().unwrap().doc.as_ref(), before.as_ref()); + } + } + } +} + +#[test] +fn a_document_channel_adjustment_survives_an_active_layer_change() { + let mut app = gated_blur_fixture(true, false); + let a = app.session.active().unwrap().active_layer.unwrap(); + let b = photocraft_doc::LayerId(app.run("layer.new.layer", json!({"name":"B"})).unwrap()["layer"].as_u64().unwrap()); + app.run("channel.new", json!({"name":"Alpha","fill":"black"})).unwrap(); + app.session.select_layer(a).unwrap(); + app.session.actions.list[0].steps[0] = ("image.adjustments.brightnessContrast".into(), json!({"brightness":40,"target":{"channel":0}})); + let dialog = pause_gated_blur(&mut app, None); + app.session.select_layer(b).unwrap(); + let before = app.session.active().unwrap().doc.clone(); + crate::dialogs::confirm(&mut app, dialog).unwrap(); + let after = &app.session.active().unwrap().doc; + assert_eq!(after.layers, before.layers); + assert_ne!(after.channels, before.channels); +} + #[test] fn an_implicit_paused_filter_refuses_a_public_api_receiver_change() { for single in [false, true] { diff --git a/crates/ui-egui/src/panels/type_panels_ui.rs b/crates/ui-egui/src/panels/type_panels_ui.rs index f6a3becb53..0574083a6e 100644 --- a/crates/ui-egui/src/panels/type_panels_ui.rs +++ b/crates/ui-egui/src/panels/type_panels_ui.rs @@ -802,6 +802,14 @@ pub fn close_spelling(app: &mut PhotocraftApp) { } } +fn spelling_change_all_params(item: &Value, word: &str, replace: &str, all_layers: bool) -> Value { + let mut params = json!({"action":"changeAll", "word":word, "replace":replace, "allLayers":all_layers}); + if !all_layers { + params["layer"] = item["layer"].clone(); + } + params +} + fn spelling_window(app: &mut PhotocraftApp, ctx: &egui::Context) { let t = Tokens::get(ctx); let Some(d) = app.ui.type_panels.spell.clone() else { return }; @@ -930,7 +938,7 @@ fn spelling_window(app: &mut PhotocraftApp, ctx: &egui::Context) { let p = if a == "change" { json!({ "action": "change", "layer": i["layer"], "start": i["start"], "end": i["end"], "word": word, "replace": change_to }) } else { - json!({ "action": "changeAll", "word": word, "replace": change_to, "allLayers": all_layers }) + spelling_change_all_params(i, &word, &change_to, all_layers) }; if let Err(error) = crate::actions::spelling_change(app, p) { app.ui.status = error; @@ -971,6 +979,30 @@ mod tests { assert!(app.ui.type_panels.glyphs); } + #[test] + fn change_all_keeps_the_listed_layer_after_a_session_selection_change() { + for all_layers in [false, true] { + let mut app = app_with_text("Teh cat"); + let listed = app.session.active().unwrap().active_layer.unwrap(); + app.run("type.create", json!({"x":10,"y":90,"text":"Teh dog"})).unwrap(); + let other = app.session.active().unwrap().active_layer.unwrap(); + app.session.select_layer(listed).unwrap(); + menu(&mut app, "edit.checkSpelling", &json!({"allLayers":false})).unwrap().unwrap(); + let item = app.ui.type_panels.spell.as_ref().unwrap().items[0].clone(); + app.session.select_layer(other).unwrap(); + let params = spelling_change_all_params(&item, "Teh", "The", all_layers); + if all_layers { + assert!(params.get("layer").is_none()); + } + crate::actions::spelling_change(&mut app, params).unwrap(); + let doc = &app.session.active().unwrap().doc; + for (id, expected) in [(listed, "The cat"), (other, if all_layers { "The dog" } else { "Teh dog" })] { + let photocraft_doc::LayerContent::Text(text) = &doc.layer(id).unwrap().content else { panic!("not text") }; + assert_eq!(text.text, expected); + } + } + } + #[test] fn glyph_insertion_and_recent() { let mut app = app_with_text("ab");