Keep paused action policy and spelling receivers bound

This commit is contained in:
yoriswind committed 2026-11-03 19:39:45 -08:00
1 parent 6d247886a7
commit 211efca709
6 files changed
+167 -20

No files matched your search

+1 -1
View File
@@ -840,7 +840,7 @@ fn set_color(s: &mut Session, p: &Value) -> Result<Value> {
/// Kept separate from Playback Options' performance modes until #5212 provides that dialog.
fn set_history(s: &mut Session, p: &Value) -> Result<Value> {
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"))?;
+28 -3
View File
@@ -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<ActionHistory>,
active: Option<DocId>,
documents: Vec<PausedDocument>,
@@ -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());
+2
View File
@@ -207,6 +207,8 @@ pub struct Session {
coalesce_request: Option<String>,
/// The outer action's original document/history states, independent of command transactions.
action_history: Option<actions_cmds::ActionHistory>,
/// Weak leases expire when every copy of a suspended continuation is abandoned.
paused_action_history: Vec<std::sync::Weak<()>>,
/// Tests observe the actual scratch-session boundary without retaining it or changing output.
#[cfg(test)]
batch_history_observer: Option<fn(&Session)>,
+21 -15
View File
@@ -243,18 +243,28 @@ fn play_owned(app: &mut PhotocraftApp, params: &Value, suspended: Option<Value>)
// 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<photocraft_doc::DocId>,
/// 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<Option<photocraft_doc::LayerId>>,
}
@@ -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", &params).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) => {
@@ -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] {
+33 -1
View File
@@ -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");