diff --git a/crates/algo/src/magnetic.rs b/crates/algo/src/magnetic.rs index f2dab08816..54759237cb 100644 --- a/crates/algo/src/magnetic.rs +++ b/crates/algo/src/magnetic.rs @@ -512,12 +512,11 @@ fn direction_cost(tp: [f32; 2], tq: [f32; 2], dx: i32, dy: i32, len: f32) -> f32 if tp == [0.0; 2] || tq == [0.0; 2] { return NEUTRAL_DIRECTION; } - let mut l = [dx as f32 / len, dy as f32 / len]; - if tp[0] * l[0] + tp[1] * l[1] < 0.0 { - l = [-l[0], -l[1]]; - } - let dp = (tp[0] * l[0] + tp[1] * l[1]).clamp(-1.0, 1.0); - let dq = (l[0] * tq[0] + l[1] * tq[1]).clamp(-1.0, 1.0); + // An edge tangent is an axis, not a directed vector: reversing the strongest channel's + // gradient must not make an otherwise straight edge expensive to follow. + let l = [dx as f32 / len, dy as f32 / len]; + let dp = (tp[0] * l[0] + tp[1] * l[1]).abs().clamp(0.0, 1.0); + let dq = (l[0] * tq[0] + l[1] * tq[1]).abs().clamp(0.0, 1.0); 2.0 / (3.0 * PI) * (dp.acos() + dq.acos()) } diff --git a/crates/algo/src/magnetic/tests.rs b/crates/algo/src/magnetic/tests.rs index ccfc73bc04..ee187bc781 100644 --- a/crates/algo/src/magnetic/tests.rs +++ b/crates/algo/src/magnetic/tests.rs @@ -55,6 +55,39 @@ fn snaps_to_the_most_prominent_edge() { assert!((p[0] - 18.0).abs() < 0.01, "{p:?}"); } +#[test] +fn gradient_polarity_does_not_make_a_straight_edge_look_like_a_turn() { + // A gradient can reverse across adjacent pixels when another colour channel becomes the + // strongest one. The edge tangent is an axis, so its sign must not change the path cost. + let forward = direction_cost([1.0, 0.0], [1.0, 0.0], 1, 0, 1.0); + let reversed = direction_cost([1.0, 0.0], [-1.0, 0.0], 1, 0, 1.0); + assert!(forward < 1e-6, "aligned edge: {forward}"); + assert!(reversed < 1e-6, "same edge with reversed gradient polarity: {reversed}"); +} + +#[test] +fn traces_a_straight_edge_through_a_gradient_polarity_change() { + let read = Cell::new(0); + // The contrast stays high, but the edge changes from dark-to-light to light-to-dark halfway + // down. The horizontal seam intersects it; the guide stays on the vertical contour. + let mut f = image( + |x, y| { + let left = x < 32; + let top = y < 32; + let white = left != top; + grey(if white { 255 } else { 0 }) + }, + &read, + ); + let mut t = Tracer::new(Rect::new(0, 0, 64, 64)); + let path = t.trace(&mut f, [32.0, 5.0], [32.0, 59.0], &[[32.0, 32.0]], Settings::default()); + assert_eq!(path.first(), Some(&[32.0, 5.0])); + assert_eq!(path.last(), Some(&[32.0, 59.0])); + for p in &path { + assert!((p[0] - 32.0).abs() < 1.0, "left the high-contrast edge: {p:?}; {path:?}"); + } +} + #[test] fn trace_follows_a_curved_edge() { let read = Cell::new(0); diff --git a/crates/automation/src/rpc.rs b/crates/automation/src/rpc.rs index 5fea450270..65e710e9d3 100644 --- a/crates/automation/src/rpc.rs +++ b/crates/automation/src/rpc.rs @@ -24,7 +24,9 @@ use std::io::{BufRead, Write}; use std::net::TcpListener; use std::path::PathBuf; +use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex, PoisonError}; +use std::time::{Duration, Instant}; use base64::Engine as _; use serde_json::{Value, json}; @@ -325,32 +327,67 @@ fn serve_tcp_connection(h: &Mutex, stream: std::net::TcpStream, token: } } +/// Headless TCP daemons with no clients should eventually release their listening port (#3682). +const TCP_IDLE_TIMEOUT: Duration = Duration::from_secs(30 * 60); + /// Serve authenticated JSON lines on a loopback TCP address with one shared session. -/// Refuses non-loopback addresses and caps active connections and request bytes. +/// Refuses non-loopback addresses, caps connections and request bytes, and closes the listener +/// after an idle period with no connections. Active clients are never evicted by this timer. pub fn serve_tcp(addr: &str, h: Arc>, token: String, ready: impl FnOnce(std::net::SocketAddr)) -> Result<(), AutomationError> { + serve_tcp_until_idle(addr, h, token, ready, TCP_IDLE_TIMEOUT) +} + +fn serve_tcp_until_idle( + addr: &str, + h: Arc>, + token: String, + ready: impl FnOnce(std::net::SocketAddr), + idle: Duration, +) -> Result<(), AutomationError> { let listener = TcpListener::bind(addr).map_err(|e| AutomationError::Io(format!("bind {addr}: {e}")))?; let local = listener.local_addr().map_err(|e| AutomationError::Io(e.to_string()))?; if !local.ip().is_loopback() { return Err(bad(format!("{addr} is not a loopback address"))); } + listener.set_nonblocking(true).map_err(|e| AutomationError::Io(e.to_string()))?; ready(local); let limiter = ConnectionLimiter::new(MAX_CONNECTIONS); let token = Arc::new(token); - for stream in listener.incoming() { - let Ok(mut stream) = stream else { continue }; - let Some(permit) = limiter.try_acquire() else { - let _ = configure_stream(&stream); - let _ = writeln!(stream, "{}", json!({"id": null, "ok": false, "error": "connection limit reached"})); - continue; - }; - let h = h.clone(); - let token = Arc::clone(&token); - std::thread::spawn(move || { - let _permit = permit; - let _ = serve_tcp_connection(&h, stream, &token, AUTH_DEADLINE); - }); + let active = Arc::new(AtomicUsize::new(0)); + let mut last_active = Instant::now(); + loop { + match listener.accept() { + Ok((mut stream, _)) => { + last_active = Instant::now(); + let Some(permit) = limiter.try_acquire() else { + let _ = configure_stream(&stream); + let _ = writeln!(stream, "{}", json!({"id": null, "ok": false, "error": "connection limit reached"})); + continue; + }; + // The listener is nonblocking; individual requests still use blocking socket I/O. + let _ = stream.set_nonblocking(false); + let h = h.clone(); + let token = Arc::clone(&token); + let active = Arc::clone(&active); + active.fetch_add(1, Ordering::AcqRel); + std::thread::spawn(move || { + let _permit = permit; + let _ = serve_tcp_connection(&h, stream, &token, AUTH_DEADLINE); + active.fetch_sub(1, Ordering::AcqRel); + }); + } + Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => { + if active.load(Ordering::Acquire) != 0 { + last_active = Instant::now(); + } else if last_active.elapsed() >= idle { + return Ok(()); + } + std::thread::sleep(Duration::from_millis(100)); + } + Err(e) if matches!(e.kind(), std::io::ErrorKind::Interrupted | std::io::ErrorKind::ConnectionAborted) => {} + Err(e) => return Err(AutomationError::Io(e.to_string())), + } } - Ok(()) } #[cfg(test)] @@ -1146,6 +1183,38 @@ mod tests { assert!(h.handle("methods", Value::Null).is_ok()); } + #[test] + fn idle_tcp_daemon_releases_its_port_but_keeps_active_clients() { + use std::io::BufReader; + const TOKEN: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + let h = Arc::new(Mutex::new(Headless::new())); + let (ready_tx, ready_rx) = std::sync::mpsc::channel(); + let (done_tx, done_rx) = std::sync::mpsc::channel(); + let server = std::thread::spawn(move || { + let result = serve_tcp_until_idle("127.0.0.1:0", h, TOKEN.to_owned(), move |addr| ready_tx.send(addr).unwrap(), Duration::from_millis(200)); + done_tx.send(result).unwrap(); + }); + let addr = ready_rx.recv_timeout(Duration::from_secs(2)).unwrap(); + let mut client = std::net::TcpStream::connect(addr).unwrap(); + client.set_read_timeout(Some(Duration::from_secs(2))).unwrap(); + let mut reader = BufReader::new(client.try_clone().unwrap()); + writeln!(client, r#"{{"id":"auth","method":"auth","params":{{"token":"{TOKEN}"}}}}"#).unwrap(); + let mut line = String::new(); + reader.read_line(&mut line).unwrap(); + let auth: Value = serde_json::from_str(&line).unwrap(); + assert_eq!(auth["result"]["authenticated"], true); + // The port stays owned well past the idle limit while a client is connected. + assert!( + matches!(done_rx.recv_timeout(Duration::from_millis(450)), Err(std::sync::mpsc::RecvTimeoutError::Timeout)), + "daemon exited with a live client" + ); + drop(reader); + drop(client); + assert!(done_rx.recv_timeout(Duration::from_secs(3)).unwrap().is_ok(), "idle daemon did not exit cleanly"); + server.join().unwrap(); + assert!(TcpListener::bind(addr).is_ok(), "idle listener still owns its port"); + } + #[test] fn tcp_serves_loopback() { use std::io::{BufReader, Write as _}; diff --git a/crates/engine/src/align_cmds.rs b/crates/engine/src/align_cmds.rs index d4cc56b2bd..99e3c8a590 100644 --- a/crates/engine/src/align_cmds.rs +++ b/crates/engine/src/align_cmds.rs @@ -44,6 +44,14 @@ fn selected_pixel_layers(s: &Session) -> Vec { st.selected_layers().into_iter().filter(|id| st.doc.layer(*id).is_some_and(|l| matches!(l.content, LayerContent::Raster(_)))).collect() } +/// Whether a lock keeps the layer `id` from being moved or warped: lock all, position or pixels. +/// The Background's own position lock doesn't count: like Free Transform, Auto-Align turns the +/// Background into a normal layer when it has to move it (Photoshop). +fn pinned(doc: &Document, id: LayerId) -> bool { + let l = doc.effective_locks(id); + l.all || l.pixels || (l.position && !doc.is_background(id)) +} + fn two_layers(s: &Session) -> std::result::Result<(), String> { s.active().ok_or("no document open")?; if selected_pixel_layers(s).len() < 2 { Err("select two or more pixel layers in the Layers panel".into()) } else { Ok(()) } @@ -63,7 +71,8 @@ fn auto_align(s: &mut Session, p: &Value) -> Result { let reference = match p.get("reference").and_then(Value::as_u64) { Some(r) if ids.contains(&LayerId(r)) => LayerId(r), Some(r) => return Err(bad(cmd, format!("reference {r} is not a selected pixel layer"))), - None => ids[0], + // As in Photoshop, a locked layer stays put: it becomes the reference (#7080). + None => ids.iter().copied().find(|id| pinned(&doc, *id)).unwrap_or(ids[0]), }; let ref_idx = ids.iter().position(|i| *i == reference).unwrap_or(0); let area = doc.bounds(); @@ -100,6 +109,9 @@ fn auto_align(s: &mut Session, p: &Value) -> Result { if moves.is_empty() { return Err(EngineError::Other("Auto-Align couldn't find enough matching detail between the layers".into())); } + if let Some(l) = moves.iter().find(|&&i| pinned(&doc, ids[i])).and_then(|&i| doc.layer(ids[i])) { + return Err(EngineError::Other(format!("Auto-Align can't move the locked layer \"{}\"", l.name))); + } let warped: Vec<(LayerId, Surface, Option)> = moves .iter() .map(|&i| { @@ -122,6 +134,8 @@ fn auto_align(s: &mut Session, p: &Value) -> Result { .collect::>()?; s.edit("Auto-Align Layers", |doc, _| { for (id, px, mask) in &warped { + // Photoshop turns the Background into a normal layer before moving it. + crate::extra_cmds::background_to_layer(doc, *id); let l = doc.layer_mut(*id).ok_or(EngineError::NoLayer(*id))?; if let Some(surf) = l.surface_mut() { *surf = px.clone(); @@ -331,7 +345,7 @@ pub fn specs() -> Vec { spec!( "edit.autoAlignLayers", "Auto-Align Layers…", - r##"{"projection":"auto|perspective|cylindrical|spherical|collage|reposition","reference":layer id?=bottom selected layer,"geometricCorrection":bool=false,"interpolation":"bicubic|bilinear|nearest","registration":"auto|features|intensity"}"##, + r##"{"projection":"auto|perspective|cylindrical|spherical|collage|reposition","reference":layer id?=the locked selected layer, else the bottom selected layer,"geometricCorrection":bool=false,"interpolation":"bicubic|bilinear|nearest","registration":"auto|features|intensity"}"##, auto_align ), spec!( @@ -350,8 +364,15 @@ mod tests { /// Deterministic texture written into a layer at offset (dx, dy). fn texture_layer(s: &mut Session, name: &str, dx: i32, dy: i32) -> LayerId { s.execute("layer.new.layer", json!({"name": name})).unwrap(); - s.edit("tex", |doc, active| { - let surf = doc.layer_mut(active.unwrap()).unwrap().surface_mut().unwrap(); + let id = s.active().unwrap().active_layer.unwrap(); + paint_texture(s, id, dx, dy); + id + } + + /// Writes the deterministic texture into layer `id` at offset (dx, dy). + fn paint_texture(s: &mut Session, id: LayerId, dx: i32, dy: i32) { + s.edit("tex", |doc, _| { + let surf = doc.layer_mut(id).unwrap().surface_mut().unwrap(); let mut seed = 99u64; let mut rnd = || { seed = seed.wrapping_mul(6364136223846793005).wrapping_add(1442695040888963407); @@ -367,7 +388,6 @@ mod tests { Ok(()) }) .unwrap(); - s.active().unwrap().active_layer.unwrap() } fn select(s: &mut Session, ids: &[LayerId]) { @@ -411,6 +431,86 @@ mod tests { assert!(s.execute("edit.autoAlignLayers", json!({"reference": u64::MAX})).is_err()); } + /// As in Photoshop, a locked layer stays where it is: it becomes the reference and the + /// others move onto it (#7080). + #[test] + fn auto_align_keeps_a_locked_layer_as_the_reference() { + for lock in ["all", "position", "pixels"] { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 300, "height": 240, "background": "transparent"})).unwrap(); + let a = texture_layer(&mut s, "A", 20, 20); + let b = texture_layer(&mut s, "B", 31, 13); + s.edit("lock", |doc, _| { + doc.layer_mut(b).unwrap().locks.set(lock, true); + Ok(()) + }) + .unwrap(); + select(&mut s, &[a, b]); + let r = s.execute("edit.autoAlignLayers", json!({"projection": "reposition"})).unwrap(); + assert_eq!(r["reference"], b.0, "{lock}: {r}"); + let doc = &s.active().unwrap().doc; + let bb = doc.layer(b).unwrap().surface().unwrap().content_bounds(); + assert_eq!((bb.x0, bb.y0), (31, 13), "{lock}: the locked layer stays put"); + let ab = doc.layer(a).unwrap().surface().unwrap().content_bounds(); + assert!((ab.x0 - 31).abs() <= 1 && (ab.y0 - 13).abs() <= 1, "{lock}: {ab:?} {r}"); + // An explicit reference that would move the locked layer is refused. + s.undo(); + assert!(s.execute("edit.autoAlignLayers", json!({"projection": "reposition", "reference": a.0})).is_err()); + // Two locked layers: nothing may move. + s.edit("lock", |doc, _| { + doc.layer_mut(a).unwrap().locks.set(lock, true); + Ok(()) + }) + .unwrap(); + assert!(s.execute("edit.autoAlignLayers", json!({"projection": "reposition"})).is_err()); + let bb = s.active().unwrap().doc.layer(a).unwrap().surface().unwrap().content_bounds(); + assert_eq!((bb.x0, bb.y0), (20, 20)); + } + } + + /// The Background doesn't block Auto-Align: when a projection reprojects every layer + /// (Cylindrical, Spherical, or Auto choosing one), or another layer is the reference, it + /// becomes a normal layer ("Layer 0") and moves, as in Photoshop and as Free Transform does. + #[test] + fn auto_align_moves_the_background_as_a_normal_layer() { + let setup = || { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 300, "height": 240, "background": "white"})).unwrap(); + let bg = s.active().unwrap().doc.background().unwrap().id; + paint_texture(&mut s, bg, 20, 20); + let b = texture_layer(&mut s, "B", 31, 13); + select(&mut s, &[bg, b]); + (s, bg, b) + }; + for projection in ["cylindrical", "spherical", "auto"] { + let (mut s, bg, b) = setup(); + let r = s.execute("edit.autoAlignLayers", json!({"projection": projection})).unwrap_or_else(|e| panic!("{projection}: {e}")); + let doc = &s.active().unwrap().doc; + let moved = r["aligned"].as_array().unwrap().iter().any(|a| a["layer"] == bg.0); + assert_eq!(moved, projection != "auto" || r["layout"] != "perspective", "{projection}: {r}"); + assert_eq!(doc.is_background(bg), !moved, "{projection}: a moved Background is a normal layer: {r}"); + if moved { + assert_eq!(doc.layer(bg).unwrap().name, "Layer 0", "{projection}"); + } + assert!(doc.layer(b).is_some(), "{projection}"); + s.undo(); + assert!(s.active().unwrap().doc.is_background(bg), "{projection}: undo restores the Background"); + } + // Planar, with the other layer as the reference: the Background moves onto it. + let (mut s, bg, b) = setup(); + s.execute("edit.autoAlignLayers", json!({"projection": "reposition", "reference": b.0})).unwrap(); + let doc = &s.active().unwrap().doc; + assert!(!doc.is_background(bg)); + // Default reference: the Background (bottom) stays put and stays the Background. + let (mut s, bg, b) = setup(); + let r = s.execute("edit.autoAlignLayers", json!({"projection": "reposition"})).unwrap(); + assert_eq!(r["reference"], bg.0, "{r}"); + let doc = &s.active().unwrap().doc; + assert!(doc.is_background(bg)); + let bb = doc.layer(b).unwrap().surface().unwrap().content_bounds(); + assert!((bb.x0 - 20).abs() <= 1 && (bb.y0 - 20).abs() <= 1, "{bb:?} {r}"); + } + /// The moving layer's mask is warped as a mask: one grey channel whose outside keeps its /// default (reveal-all stays revealed, hide-all stays hidden), for planar and mesh warps. #[test] diff --git a/crates/engine/src/atn_cmds.rs b/crates/engine/src/atn_cmds.rs index 0acaa9cf3a..12b4994feb 100644 --- a/crates/engine/src/atn_cmds.rs +++ b/crates/engine/src/atn_cmds.rs @@ -782,46 +782,32 @@ fn selective_from(d: &Descriptor) -> Option { } fn selective_to(p: &Value) -> Option { - // Each range, as the command reads it (`adjust_cmds::selective_from_params`): the array form - // `"reds": [c, m, y, k]` first, then the scalar form, which names one range in `"colors"` and - // gives the channels it changes. Writing only the arrays dropped the scalar form's values, and - // the step exported as an adjustment that changes nothing (#7672). - let mut rows: Vec> = vec![None; SELECTIVE_RANGES.len()]; - for (row, (_, name)) in rows.iter_mut().zip(SELECTIVE_RANGES) { - if let Some(a) = p.get(*name).and_then(Value::as_array) { - let v = |i: usize| a.get(i).and_then(Value::as_f64).unwrap_or(0.0); - *row = Some([v(0), v(1), v(2), v(3)]); - } - } - if let Some(named) = jstr(p, "colors") { - let i = SELECTIVE_RANGES.iter().position(|(_, name)| *name == named)?; - let row = rows.get_mut(i)?; - let mut v = row.unwrap_or([0.0; 4]); - for (slot, key) in v.iter_mut().zip(["cyan", "magenta", "yellow", "black"]) { - // A channel that is given but is not a number is an error in the command - // (`adjust_cmds::selective_from_params`), so a step carrying one cannot be written - // either: ignoring it would export a different adjustment than the one recorded. - // `null` counts as absent there, and so here. - match p.get(key) { - None | Some(Value::Null) => {} - Some(_) => *slot = jnum(p, key)?, - } - } - *row = Some(v); - } + // Match command execution: scalar components override only the selected range, `relative` + // takes precedence over `method`, and invalid values must not become a different edit. + let photocraft_doc::Adjustment::SelectiveColor { relative, adjustments } = crate::adjust_cmds::selective_from_params(p, None).ok()? else { + return None; + }; let mut list = Vec::new(); - for (row, (ps, _)) in rows.iter().zip(SELECTIVE_RANGES) { - if let Some(v) = row { + for (name, row) in crate::adjust_cmds::RANGES.into_iter().zip(adjustments) { + if p.get(name).is_some_and(|v| !v.is_null()) || jstr(p, "colors") == Some(name) { + let ps = lookup_back(SELECTIVE_RANGES, name)?; + let [cyan, magenta, yellow, black] = row.map(f64::from); list.push(D::Descriptor( - desc("ClrC").with("Clrs", enm("Clrs", ps)).with("Cyn ", pct(v[0])).with("Mgnt", pct(v[1])).with("Yllw", pct(v[2])).with("Blck", pct(v[3])), + desc("ClrC") + .with("Clrs", enm("Clrs", ps)) + .with("Cyn ", pct(cyan)) + .with("Mgnt", pct(magenta)) + .with("Yllw", pct(yellow)) + .with("Blck", pct(black)), )); } } - let mut d = desc("SlcC").with("presetKind", enm("presetKindType", "presetKindCustom")); - if let Some(m) = jstr(p, "method") { - d = d.with("Mthd", enm("CrcM", if m == "absolute" { "Absl" } else { "Rltv" })); - } - Some(d.with("ClrC", D::List(list))) + Some( + desc("SlcC") + .with("presetKind", enm("presetKindType", "presetKindCustom")) + .with("Mthd", enm("CrcM", if relative { "Rltv" } else { "Absl" })) + .with("ClrC", D::List(list)), + ) } /// Gradient Map: the gradient's colour stops at 0..4096 → `stops: [[0..1, "#rrggbb"], …]`. diff --git a/crates/engine/src/edit_cmds.rs b/crates/engine/src/edit_cmds.rs index 1ae19fcdad..1a6c3a4072 100644 --- a/crates/engine/src/edit_cmds.rs +++ b/crates/engine/src/edit_cmds.rs @@ -450,15 +450,35 @@ fn paste_layers(s: &mut Session, layers: &Document, dx: i32, dy: i32) -> Result< /// How far a paste moves the clipboard pixels: not at all in place (or when they lie on the /// canvas and no `center` is given), else onto `center` (the view centre from the UI) or the -/// canvas centre. +/// canvas centre, then kept on the canvas as Photoshop keeps it: pixels that fit stay wholly +/// inside, larger ones start at the canvas's top-left corner (#6759). `"clampToCanvas": false` +/// skips that, for Paste Into / Outside, which centre on the selection. pub(crate) fn paste_offset(clip: &Clip, canvas: Rect, p: &Value, in_place: bool) -> (i32, i32) { if in_place || (clip.bounds.intersect(&canvas) == clip.bounds && p.get("center").is_none()) { return (0, 0); } let c = p.get("center").and_then(Value::as_array).filter(|a| a.len() >= 2).map(|a| (a[0].as_f64().unwrap_or(0.0), a[1].as_f64().unwrap_or(0.0))); - let (cx, cy) = c.unwrap_or(((f64::from(canvas.x0) + f64::from(canvas.x1)) / 2.0, (f64::from(canvas.y0) + f64::from(canvas.y1)) / 2.0)); + let (cx, cy) = c.unwrap_or((mid(canvas.x0, canvas.x1), mid(canvas.y0, canvas.y1))); let b = clip.bounds; - ((cx - (f64::from(b.x0) + f64::from(b.x1)) / 2.0).round() as i32, (cy - (f64::from(b.y0) + f64::from(b.y1)) / 2.0).round() as i32) + let (dx, dy) = ((cx - mid(b.x0, b.x1)).round(), (cy - mid(b.y0, b.y1)).round()); + if p.get("clampToCanvas").and_then(Value::as_bool) == Some(false) || b.is_empty() { + return (dx as i32, dy as i32); + } + (clamp_shift(dx, b.x0, b.x1, canvas.x0, canvas.x1), clamp_shift(dy, b.y0, b.y1, canvas.y0, canvas.y1)) +} + +/// The midpoint of `a..b`, each edge converted before adding so extreme coordinates can't overflow. +fn mid(a: i32, b: i32) -> f64 { + (f64::from(a) + f64::from(b)) / 2.0 +} + +/// Shift `d` of the span `b0..b1` along one axis, changed as little as needed to keep the span +/// inside `c0..c1` when it fits; a span longer than the canvas starts at `c0`. +fn clamp_shift(d: f64, b0: i32, b1: i32, c0: i32, c1: i32) -> i32 { + let (b0, b1, c0, c1) = (i64::from(b0), i64::from(b1), i64::from(c0), i64::from(c1)); + // `as` saturates a NaN or out-of-range shift, then the clamp brings it into range. + let start = if b1 - b0 <= c1 - c0 { b0.saturating_add(d as i64).clamp(c0, c1 - (b1 - b0)) } else { c0 }; + i32::try_from(start - b0).unwrap_or(if start > b0 { i32::MAX } else { i32::MIN }) } pub(crate) fn shifted(surface: &Surface, dx: i32, dy: i32) -> Surface { @@ -994,7 +1014,7 @@ pub fn specs() -> Vec { &["Edit"], Some("Cmd+V"), concat!( - r#"{"center":[x,y]? (view centre; default keeps the position when it overlaps the canvas),"#, + r#"{"center":[x,y]? (view centre; default keeps the position when it overlaps the canvas),"clampToCanvas":bool=true (keep the paste on the canvas, or at its top-left when larger),"#, paste_target!(), "} (with no document open: a new document from the clipboard)" ), @@ -1288,6 +1308,78 @@ mod tests { assert_eq!(s.active().unwrap().doc.layers.len(), 4); } + /// A `w`×`h` document with an opaque `cw`×`ch` image from another app on the clipboard. + fn external_clip_session(w: i32, h: i32, cw: i32, ch: i32) -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": w, "height": h})).unwrap(); + let mut surface = Surface::new(PixelFormat::RGBA8); + surface.fill_rect(Rect::new(0, 0, cw, ch), &[1.0, 0.0, 0.0, 1.0]); + s.clipboard = Some(Clip { surface, bounds: Rect::new(0, 0, cw, ch), layers: None, resolution: None }); + s + } + + /// #6759: wherever the view is centred, a paste that fits the canvas stays wholly on it. + #[test] + fn paste_that_fits_stays_on_the_canvas() { + for (w, h, center, want) in [ + (60, 100, [40.0, 45.0], Rect::new(0, 0, 60, 100)), + (60, 100, [-500.0, 900.0], Rect::new(0, 0, 60, 100)), + (100, 100, [10.0, 80.0], Rect::new(0, 0, 60, 100)), + (100, 100, [95.0, 50.0], Rect::new(40, 0, 100, 100)), + (100, 120, [50.0, 60.0], Rect::new(20, 10, 80, 110)), + ] { + let mut s = external_clip_session(w, h, 60, 100); + s.execute("edit.paste", json!({ "center": center })).unwrap(); + assert_eq!(active_bounds(&s), want, "{w}x{h} canvas, view centre {center:?}"); + } + } + + /// #6759: a paste larger than the canvas starts at its top-left corner, wherever the view is. + #[test] + fn paste_larger_than_the_canvas_starts_at_its_top_left() { + for center in [[0.0, 0.0], [25.0, 20.0], [1000.0, 1000.0], [-1000.0, 30.0]] { + let mut s = external_clip_session(50, 40, 60, 100); + s.execute("edit.paste", json!({ "center": center })).unwrap(); + assert_eq!(active_bounds(&s), Rect::new(0, 0, 60, 100), "view centre {center:?}"); + } + // Wider but not taller: at the left edge, and wholly inside vertically. + for (center, want) in + [([500.0, 500.0], Rect::new(0, 100, 60, 200)), ([-500.0, -500.0], Rect::new(0, 0, 60, 100)), ([25.0, 120.0], Rect::new(0, 70, 60, 170))] + { + let mut s = external_clip_session(50, 200, 60, 100); + s.execute("edit.paste", json!({ "center": center })).unwrap(); + assert_eq!(active_bounds(&s), want, "view centre {center:?}"); + } + } + + /// Paste Into centres on the selection even where that runs off the canvas. + #[test] + fn paste_into_is_not_clamped_to_the_canvas() { + let mut s = session(); + s.execute("select.rect", json!({"x": 10, "y": 10, "width": 40, "height": 20})).unwrap(); + s.execute("edit.copy", json!({})).unwrap(); + s.execute("select.rect", json!({"x": 90, "y": 90, "width": 10, "height": 10})).unwrap(); + s.execute("edit.pasteSpecial.pasteInto", json!({})).unwrap(); + assert_eq!(active_bounds(&s), Rect::new(75, 85, 115, 105)); + } + + /// The clamp never overflows or panics: NaN, infinite and `i32`-limit shifts saturate (Rule 9). + #[test] + fn paste_offset_clamp_survives_extreme_coordinates() { + assert_eq!(clamp_shift(f64::NAN, 0, 10, 0, 100), 0); + assert_eq!(clamp_shift(f64::INFINITY, 0, 10, 0, 100), 90); + assert_eq!(clamp_shift(f64::NEG_INFINITY, i32::MIN, i32::MAX, 0, 1), i32::MAX); + assert_eq!(clamp_shift(1e30, i32::MIN, i32::MIN + 1, i32::MAX - 1, i32::MAX), i32::MAX); + let clip = Clip { + surface: Surface::new(PixelFormat::RGBA8), + bounds: Rect::new(i32::MAX - 2, i32::MAX - 2, i32::MAX, i32::MAX), + layers: None, + resolution: None, + }; + let (dx, dy) = paste_offset(&clip, Rect::new(i32::MIN, i32::MIN, i32::MIN + 4, i32::MIN + 4), &json!({}), false); + assert_eq!((dx, dy), (i32::MIN, i32::MIN), "an unrepresentable shift saturates instead of wrapping"); + } + #[test] fn paste_with_malformed_center_does_not_panic() { // A short/empty `center` array must not panic (Rule 9) — it falls back to the canvas centre. diff --git a/crates/engine/src/extra_cmds.rs b/crates/engine/src/extra_cmds.rs index c24f30e515..db5c93fcfe 100644 --- a/crates/engine/src/extra_cmds.rs +++ b/crates/engine/src/extra_cmds.rs @@ -336,6 +336,9 @@ fn can_reselect(s: &Session) -> std::result::Result<(), String> { reselect_target(s).map(|_| ()).ok_or_else(|| "there is no selection to restore".into()) } +/// Edit › Paste Special › Paste Into (or Outside): the clipboard centred on the selection, not +/// clamped to the canvas, as a new layer masked to the selection (or its inverse), or into the +/// targeted mask or channel. One history step. fn paste_into(s: &mut Session, p: &Value, outside: bool) -> Result { let d = s.active().ok_or(EngineError::NoDocument)?; let selection = d.doc.selection.clone().ok_or(EngineError::Other("Paste Into needs a selection".into()))?; @@ -353,7 +356,8 @@ fn paste_into(s: &mut Session, p: &Value, outside: bool) -> Result { selection }; let label = if outside { "Paste Outside" } else { "Paste Into" }; - let mut params = json!({"center": [(b.x0 + b.x1) as f64 / 2.0, (b.y0 + b.y1) as f64 / 2.0]}); + // Centred on the selection, even where that runs off the canvas (not clamped like a paste). + let mut params = json!({"center": [(b.x0 + b.x1) as f64 / 2.0, (b.y0 + b.y1) as f64 / 2.0], "clampToCanvas": false}); if let Some(c) = p.get("center") { params["center"] = c.clone(); } diff --git a/crates/engine/src/image_cmds.rs b/crates/engine/src/image_cmds.rs index d52522ca35..6353f1f2cc 100644 --- a/crates/engine/src/image_cmds.rs +++ b/crates/engine/src/image_cmds.rs @@ -1,6 +1,8 @@ //! Image menu: Image Size, Canvas Size, Crop, Trim, Mode (colour model and //! bit depth) conversions, Duplicate. +use std::convert::Infallible; + use photocraft_algo::resample::{Resample, crop_surface, resize_surface_in_canvas, translate_surface}; use photocraft_color::{ColorMode, SampleType}; use photocraft_doc::{Document, Effect, Effects, FxPaint, Layer, LayerContent, LayerId, Size}; @@ -297,6 +299,7 @@ pub(crate) fn resample_doc(doc: &mut Document, nw: u32, nh: u32, filter: Resampl } // Content that reaches the canvas edge keeps it: a Background stays opaque to the border. let canvas = Rect::from_xywh(0, 0, ow, oh); + let taken = crate::smart_cmds::take_caches(&mut doc.layers); for_each_surface(&mut doc.layers, true, &mut |surf, is_mask| { *surf = resize_surface_in_canvas(surf, sx, sy, if is_mask { Resample::Bilinear } else { filter }, canvas); }); @@ -321,6 +324,7 @@ pub(crate) fn resample_doc(doc: &mut Document, nw: u32, nh: u32, filter: Resampl crate::canvas_geom::transform_geometry(doc, &photocraft_geom::Affine { m: [sx, 0.0, 0.0, sy, 0.0, 0.0] }); doc.size = Size::new(nw, nh); crate::canvas_geom::refresh(doc, crate::canvas_geom::Refresh::All); + let Ok(()) = crate::smart_cmds::restore_unrendered(doc, taken, |c| Ok::<_, Infallible>(resize_surface_in_canvas(c, sx, sy, filter, canvas))); } fn bad(cmd: &str, msg: impl Into) -> EngineError { @@ -371,6 +375,26 @@ pub(crate) fn translate_doc(doc: &mut Document, cmd: &str, dx: i32, dy: i32) -> Ok(()) } +/// Delete Cropped Pixels on a layer tree: crops pixel-layer data only to `r`. Type and smart +/// objects move by shifting their rendered pixels, so cutting those would clip them at the old +/// edge. A video layer's stored frames are its pixels too: the displayed content is only the frame +/// at the playhead, so the others would bring the cropped pixels back when scrubbing (#4956). +fn delete_cropped_pixels(layers: &mut [Layer], r: Rect) { + for_each_layer(layers, &mut |l| { + if let Some(fc) = &mut l.fill_cache { + fc.surface = crop_surface(&fc.surface, r); + } + if let Some(v) = &mut l.video { + for frame in v.frames.iter_mut().chain(v.original_frames.iter_mut().flatten()) { + *frame = crop_surface(frame, r); + } + } + if let LayerContent::Raster(s) = &mut l.content { + *s = crop_surface(s, r); + } + }); +} + /// Crops the document to `r` (in current document coordinates). /// A frame that extends past the canvas fills the Background's new area with `extension` (the /// background colour, as Photoshop's Crop tool does); other layers get transparent pixels there. @@ -382,7 +406,7 @@ fn crop_doc(doc: &mut Document, cmd: &str, r: Rect, delete_pixels: bool, extensi let old_canvas = doc.bounds(); let extends = r.x0 < old_canvas.x0 || r.y0 < old_canvas.y0 || r.x1 > old_canvas.x1 || r.y1 > old_canvas.y1; if delete_pixels { - for_each_surface(&mut doc.layers, false, &mut |surf, _| *surf = crop_surface(surf, r)); + delete_cropped_pixels(&mut doc.layers, r); } translate_doc(doc, cmd, dx, dy)?; doc.size = Size::new(r.width(), r.height()); @@ -753,7 +777,14 @@ fn rotated_crop(s: &mut Session, r: Rect, angle: f64, delete: bool, content_awar if delete { let keep = turned_crop_source(size, new, angle, target); s.edit("Crop", |doc, _| { - for_each_surface(&mut doc.layers, true, &mut |surf, _| *surf = crop_surface(surf, keep)); + // Type and smart objects stay whole, as in the crop itself: the rotation re-renders + // them, but a smart object whose source is unavailable keeps its cache. + delete_cropped_pixels(&mut doc.layers, keep); + for_each_layer(&mut doc.layers, &mut |l| { + if let Some(m) = &mut l.mask { + m.surface = crop_surface(&m.surface, keep); + } + }); for ch in doc.channels.iter_mut().chain(doc.quick_mask.as_mut()) { ch.surface = crop_surface(&ch.surface, keep); } @@ -1093,6 +1124,37 @@ mod tests { } } + /// Delete Cropped Pixels on a shrinking crop deletes the cropped area from every stored video + /// frame (and the original footage), not just the displayed one (#4956). + #[test] + fn delete_cropped_pixels_crops_stored_video_frames() { + let mut s = session(); + s.edit("video", |doc, active| { + let l = doc.layer_mut(active.unwrap()).unwrap(); + let red = l.surface().unwrap().clone(); + let mut blue = red.clone(); + blue.fill_rect(Rect::new(0, 0, 40, 20), &[0.0, 0.0, 1.0, 1.0]); + let mut v = photocraft_doc::VideoData::new(vec![red.clone(), blue.clone()], 24.0); + v.original_frames = Some(vec![red, blue]); + l.video = Some(v); + Ok(()) + }) + .unwrap(); + s.execute("image.crop", json!({"x": 15, "y": 0, "width": 25, "height": 20, "deleteCroppedPixels": true})).unwrap(); + assert_eq!(doc(&s).size, Size::new(25, 20)); + let v = doc(&s).layers[1].video.as_ref().unwrap(); + let originals = v.original_frames.as_ref().unwrap(); + assert_eq!((v.frames.len(), originals.len()), (2, 2)); + for frames in [&v.frames, originals] { + // The red square (10..20, 5..15) keeps only its right half; the full blue frame + // keeps exactly the 25×20 crop. + assert_eq!(frames[0].content_bounds(), Rect::new(0, 5, 5, 15)); + assert_eq!(frames[0].pixel(2, 10), vec![1.0, 0.0, 0.0, 1.0]); + assert_eq!(frames[1].content_bounds(), Rect::new(0, 0, 25, 20)); + assert_eq!(frames[1].pixel(24, 19), vec![0.0, 0.0, 1.0, 1.0]); + } + } + /// Resample on with only a new resolution keeps the print size, as Photoshop's Image Size /// does (an action's `imageSize` with `Rslt` alone); without `keepPrintSize` only the /// resolution changes. @@ -1400,6 +1462,51 @@ mod tests { assert_eq!(doc(&s).layers[0].surface().unwrap().rgba(1, 8), [0.0; 4]); } + /// Delete Cropped Pixels deletes pixel-layer data only: type and smart objects keep what lies + /// outside the frame, so moving them later doesn't show them cut at the old edge. + #[test] + fn delete_cropped_pixels_keeps_type_and_smart_objects_whole() { + let mut s = session(); + s.execute("layer.smartObjects.convertToSmartObject", json!({})).unwrap(); + s.execute("type.create", json!({"x": 10, "y": 15, "text": "Hi", "size": 12})).unwrap(); + let bounds = |s: &Session| -> Vec { doc(s).layers[1..].iter().map(|l| l.surface().unwrap().content_bounds()).collect() }; + let before = bounds(&s); + assert!(before.iter().all(|b| b.x0 < 15 && b.x1 > 15), "both cross the crop edge: {before:?}"); + s.execute("image.crop", json!({"x": 15, "y": 0, "width": 25, "height": 20})).unwrap(); + assert!(matches!(doc(&s).layers[1].content, LayerContent::Smart(_)) && matches!(doc(&s).layers[2].content, LayerContent::Text(_))); + assert_eq!(bounds(&s), before.iter().map(|b| b.translate(-15, 0)).collect::>()); + } + + /// A turned crop deleting pixels keeps type and smart objects whole too, also a smart object + /// whose source is unavailable (the rotation can't re-render it, so it keeps its cache): each + /// ends up exactly as the same turned crop with Delete Cropped Pixels off leaves it. + #[test] + fn turned_crop_deleting_pixels_keeps_type_and_smart_objects_whole() { + let [deleted, kept] = [true, false].map(|delete| { + let mut s = session(); + s.execute("layer.smartObjects.convertToSmartObject", json!({})).unwrap(); + s.edit("unlink", |doc, active| { + if let LayerContent::Smart(sm) = &mut doc.layer_mut(active.unwrap()).unwrap().content { + sm.source = photocraft_doc::SmartSource::Linked { path: "missing-photocraft-test-source.psd".into() }; + } + Ok(()) + }) + .unwrap(); + s.execute("type.create", json!({"x": 10, "y": 15, "text": "Hi", "size": 12})).unwrap(); + s.execute("image.crop", json!({"x": 20, "y": 0, "width": 20, "height": 20, "angle": 2.0, "deleteCroppedPixels": delete})).unwrap(); + s + }); + let (a, b) = (doc(&deleted), doc(&kept)); + assert!(matches!(a.layers[1].content, LayerContent::Smart(_)) && matches!(a.layers[2].content, LayerContent::Text(_))); + for i in [1, 2] { + let (sa, sb) = (a.layers[i].surface().unwrap(), b.layers[i].surface().unwrap()); + let r = sb.content_bounds(); + assert!(r.x0 < 0, "layer {i} reaches past the crop's left edge: {r:?}"); + assert_eq!(sa.content_bounds(), r, "layer {i}"); + assert_eq!(sa.read_region(r), sb.read_region(r), "layer {i}"); + } + } + /// The Background keeps the pixels a crop with Delete Cropped Pixels off hid: extending the /// canvas over them again (Reveal All, Canvas Size) shows them rather than painting them over /// with the background colour (#7086). diff --git a/crates/engine/src/smart_cmds.rs b/crates/engine/src/smart_cmds.rs index c93c55ffac..9949bddea0 100644 --- a/crates/engine/src/smart_cmds.rs +++ b/crates/engine/src/smart_cmds.rs @@ -186,6 +186,33 @@ pub fn source_decode_count(bytes: &[u8], fmt: PixelFormat) -> u32 { SOURCE_CACHE.lock().ok().and_then(|c| c.iter().find(|e| e.key == key).map(|e| e.decodes)).unwrap_or(0) } +pub(crate) fn take_caches(layers: &mut [Layer]) -> Vec<(LayerId, Surface)> { + let mut out = Vec::new(); + for l in layers { + match &mut l.content { + LayerContent::Smart(sm) => out.extend(sm.cache.take().map(|c| (l.id, c))), + LayerContent::Group(g) => out.extend(take_caches(&mut g.children)), + _ => {} + } + } + out +} + +pub(crate) fn restore_unrendered( + doc: &mut Document, + taken: Vec<(LayerId, Surface)>, + mut fallback: impl FnMut(&Surface) -> std::result::Result, +) -> std::result::Result<(), E> { + for (id, cache) in taken { + if let Some(LayerContent::Smart(sm)) = doc.layer_mut(id).map(|l| &mut l.content) + && sm.cache.is_none() + { + sm.cache = Some(fallback(&cache)?); + } + } + Ok(()) +} + fn buffer_to_surface(buf: &photocraft_compose::Buffer, fmt: PixelFormat) -> Surface { let n = fmt.channels(); let mut data = vec![0.0f32; buf.px.len() * n]; diff --git a/crates/engine/src/transform_cmds.rs b/crates/engine/src/transform_cmds.rs index a1b1aac379..1df4b1acee 100644 --- a/crates/engine/src/transform_cmds.rs +++ b/crates/engine/src/transform_cmds.rs @@ -838,13 +838,17 @@ fn transform_doc( // Photoshop turns the Background into a normal layer before transforming it. crate::extra_cmds::background_to_layer(doc, id); let l = doc.layer_mut(id).ok_or(EngineError::NoLayer(id))?; - transform_layer_with(if is_group { None } else { sel.as_ref() }, group, l, h, affine, interp, ctl)?; + let layer_sel = if is_group { None } else { sel.as_ref() }; + check_layer_budget(layer_sel, l, h)?; + let taken = if h.is_identity() { Vec::new() } else { crate::smart_cmds::take_caches(std::slice::from_mut(l)) }; + apply_transform(layer_sel, group, l, h, affine, interp, ctl)?; if !h.is_identity() { // Type layers re-render from their new transform. let snapshot = doc.clone(); if let Some(l) = doc.layer_mut(id) { refresh_text(&snapshot, l); } + crate::smart_cmds::restore_unrendered(doc, taken, |c| warp_surface(c, c.content_bounds(), h, interp))?; // The selection outline moves with the pixels. if let Some(sel) = &doc.selection { doc.selection = Some(warp_gray(sel, h, Interp::Bilinear)?).filter(|s| !s.content_bounds().is_empty()); diff --git a/crates/engine/tests/it/atn_selective_color.rs b/crates/engine/tests/it/atn_selective_color.rs new file mode 100644 index 0000000000..454846f59e --- /dev/null +++ b/crates/engine/tests/it/atn_selective_color.rs @@ -0,0 +1,147 @@ +//! #7672: strict ATN export must preserve both accepted Selective Color parameter forms. +use photocraft_engine::actions_cmds::Action; +use photocraft_engine::{Session, atn_cmds}; +use serde_json::{Value, json}; + +const RANGES: [&str; 9] = ["reds", "yellows", "greens", "cyans", "blues", "magentas", "whites", "neutrals", "blacks"]; +const COMMANDS: [&str; 2] = ["image.adjustments.selectiveColor", "layer.newAdjustmentLayer.selectiveColor"]; + +fn fixture(depth: u32) -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 10, "height": 2, "depth": depth})).unwrap(); + let palette = [ + [0.85, 0.10, 0.05, 1.0], + [0.85, 0.80, 0.05, 1.0], + [0.05, 0.85, 0.10, 1.0], + [0.05, 0.80, 0.85, 1.0], + [0.10, 0.05, 0.85, 1.0], + [0.85, 0.05, 0.80, 1.0], + [0.90, 0.93, 0.95, 1.0], + [0.45, 0.50, 0.55, 1.0], + [0.05, 0.08, 0.10, 1.0], + [0.65, 0.20, 0.70, 0.4], + ]; + s.edit("synthetic colours", |doc, active| { + let surface = doc.layer_mut(active.unwrap()).unwrap().surface_mut().unwrap(); + let format = surface.format(); + let bytes: Vec<_> = palette.iter().cycle().take(20).flat_map(|rgba| photocraft_raster::from_rgba(&format, *rgba)).collect(); + surface.write_region(doc_bounds(), &bytes); + Ok(()) + }) + .unwrap(); + s +} + +fn doc_bounds() -> photocraft_doc::Rect { + photocraft_doc::Rect::from_xywh(0, 0, 10, 2) +} + +fn pixels(s: &Session) -> Vec<[f32; 4]> { + photocraft_compose::flatten(&s.active().unwrap().doc).px +} + +fn check_cases(id: &str, cases: &[Value], require_effect: bool) { + let mut failures = Vec::new(); + for depth in [8, 16, 32] { + for params in cases { + let action = Action::new("Selective Color", vec![(id.into(), params.clone())]); + let mut direct = fixture(depth); + let before = pixels(&direct); + direct.execute(id, params.clone()).unwrap(); + let expected = pixels(&direct); + if require_effect { + assert_ne!(expected, before, "ineffective fixture: {id}, depth {depth}, {params}"); + } + + let mut native = fixture(depth); + native.actions.list.push(action.clone()); + native.execute("actions.play", json!({"action": action.name})).unwrap(); + assert_eq!(pixels(&native), expected, "native replay: {id}, depth {depth}, {params}"); + + let (bytes, skipped) = atn_cmds::export("Regression", &[&action], &[], false).unwrap(); + assert!(skipped.is_empty()); + let imported = atn_cmds::import(&bytes).unwrap(); + assert!(imported.unsupported.is_empty()); + assert!(imported.warnings.is_empty()); + assert_eq!(imported.actions.len(), 1); + assert_eq!(imported.actions[0].steps.len(), 1); + assert_eq!(imported.actions[0].steps[0].0, id); + + let mut replay = fixture(depth); + replay.actions.list = imported.actions; + replay.execute("actions.play", json!({"action": action.name})).unwrap(); + if pixels(&replay) != expected { + failures.push(format!("{id}, depth {depth}, {params}")); + } + } + } + assert!(failures.is_empty(), "ATN playback changed the result:\n{}", failures.join("\n")); +} + +fn scalar_cases() -> Vec { + RANGES + .iter() + .flat_map(|range| { + ["relative", "absolute"].map(|method| json!({"method": method, "colors": range, "cyan": 35, "magenta": -12.5, "yellow": 17.75, "black": 9})) + }) + .collect() +} + +#[test] +fn scalar_selective_color_actions_preserve_pixels_at_every_depth() { + let mut cases = scalar_cases(); + cases.push(json!({"method": "absolute", "colors": "reds", "cyan": 35})); + check_cases(COMMANDS[0], &cases, true); +} + +#[test] +fn scalar_selective_color_adjustment_layers_preserve_pixels_at_every_depth() { + check_cases(COMMANDS[1], &scalar_cases(), true); +} + +#[test] +fn mixed_forms_method_precedence_nulls_and_clamping_keep_their_effect() { + let cases = [ + json!({"method": "absolute", "reds": [10, 20, 30, 40], "blues": [-10, 5, 0, 4], "colors": "reds", "cyan": 0, "yellow": -17.5}), + json!({"method": "relative", "relative": false, "colors": "reds", "cyan": 35}), + json!({"method": "absolute", "relative": true, "colors": "neutrals", "black": 15}), + json!({"colors": "blues", "magenta": -25}), + json!({"method": null, "relative": null, "reds": null, "colors": "reds", "cyan": 35, "black": null}), + json!({"method": "absolute", "colors": "reds", "cyan": 175, "magenta": -150}), + json!({"method": "absolute", "reds": [175, -150, 0, 0]}), + json!({"method": "absolute", "reds": [10, 20, 30, 40], "colors": "reds", "cyan": null}), + json!({}), + json!({"colors": "reds"}), + ]; + for id in COMMANDS { + check_cases(id, &cases, false); + } +} + +#[test] +fn per_range_array_controls_still_preserve_pixels() { + let cases: Vec<_> = RANGES.iter().map(|range| json!({"method": "absolute", *range: [35, -12.5, 17.75, 9]})).collect(); + for id in COMMANDS { + check_cases(id, &cases, true); + } +} + +#[test] +fn malformed_selective_color_params_cannot_be_silently_exported() { + for params in [ + json!({"colors": "purples", "cyan": 35}), + json!({"colors": 3}), + json!({"colors": "reds", "cyan": "35"}), + json!({"method": "sideways"}), + json!({"relative": "yes"}), + json!({"reds": [10, 20]}), + json!({"reds": [10, "20", 0, 0]}), + ] { + for id in COMMANDS { + let action = Action::new("Invalid", vec![(id.into(), params.clone())]); + assert!(atn_cmds::export("Regression", &[&action], &[], false).is_err(), "strict export accepted {id} {params}"); + let (_, skipped) = atn_cmds::export("Regression", &[&action], &[], true).unwrap(); + assert_eq!(skipped.len(), 1, "skipUnsupported must report {id} {params}"); + } + } +} diff --git a/crates/engine/tests/it/main.rs b/crates/engine/tests/it/main.rs index 1d35b8c2b7..25455e539c 100644 --- a/crates/engine/tests/it/main.rs +++ b/crates/engine/tests/it/main.rs @@ -4,6 +4,7 @@ mod affinity; mod artboard_export_names; mod atn_crystallize_cell_size; mod atn_ripple_amount; +mod atn_selective_color; mod atn_stroke_width; mod background_and_mask_depth; mod background_layer_consistency; diff --git a/crates/engine/tests/smart_rerender_fallback.rs b/crates/engine/tests/smart_rerender_fallback.rs new file mode 100644 index 0000000000..1359ec0aeb --- /dev/null +++ b/crates/engine/tests/smart_rerender_fallback.rs @@ -0,0 +1,77 @@ +use std::sync::Arc; + +use photocraft_doc::{LayerContent, SmartObject, SmartSource}; +use photocraft_engine::{EngineError, Session}; +use serde_json::{Value, json}; + +fn deny_blur(id: &str, _: &Value) -> photocraft_engine::Result<()> { + if id == "filter.blur.gaussianBlur" { Err(EngineError::Other("denied".into())) } else { Ok(()) } +} + +fn edits() -> [(&'static str, Value); 2] { + [("edit.transform", json!({"scaleX": 50, "scaleY": 100})), ("image.imageSize", json!({"width": 100, "height": 50}))] +} + +fn blurred_smart_object() -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 200, "height": 100})).unwrap(); + s.execute("layer.new.layer", json!({})).unwrap(); + s.execute("select.rect", json!({"x": 20, "y": 20, "width": 100, "height": 50})).unwrap(); + s.execute("edit.fill", json!({"contents": "color", "color": "#ff0000"})).unwrap(); + s.execute("select.deselect", json!({})).unwrap(); + s.execute("layer.smartObjects.convertToSmartObject", json!({})).unwrap(); + s.execute("filter.blur.gaussianBlur", json!({"radius": 2})).unwrap(); + s +} + +fn cache_width(s: &Session) -> i32 { + let st = s.active().unwrap(); + let LayerContent::Smart(sm) = &st.doc.layer(st.active_layer.unwrap()).unwrap().content else { panic!("not a smart object") }; + sm.cache.as_ref().unwrap().content_bounds().width() as i32 +} + +fn edit_smart_object(s: &mut Session, edit: impl FnOnce(&mut SmartObject)) { + let st = s.active_mut().unwrap(); + let id = st.active_layer.unwrap(); + let LayerContent::Smart(sm) = &mut Arc::make_mut(&mut st.doc).layer_mut(id).unwrap().content else { panic!("not a smart object") }; + edit(sm); +} + +#[test] +fn a_smart_object_whose_filters_are_denied_keeps_the_resampled_fallback() { + for (cmd, params) in edits() { + let mut s = blurred_smart_object(); + let before = cache_width(&s); + s.authorize = Some(deny_blur); + s.execute(cmd, params).unwrap(); + let after = cache_width(&s); + assert!(after < before * 3 / 4, "{cmd}: the fallback stayed at {after} px wide (was {before})"); + } +} + +#[test] +fn a_smart_object_that_fails_to_re_render_keeps_the_resampled_fallback() { + let unrenderable = + [SmartSource::Linked { path: "missing.psd".into() }, SmartSource::Embedded { file_name: "broken.png".into(), bytes: Arc::new(vec![0; 16]) }]; + for (cmd, params) in edits() { + for source in &unrenderable { + let mut s = blurred_smart_object(); + let before = cache_width(&s); + edit_smart_object(&mut s, |sm| sm.source = source.clone()); + s.execute(cmd, params.clone()).unwrap(); + let after = cache_width(&s); + assert!(after < before * 3 / 4, "{cmd} with {source:?}: the fallback stayed at {after} px wide (was {before})"); + } + } +} + +#[test] +fn an_allowed_smart_object_still_re_renders_at_the_new_size() { + for (cmd, params) in edits() { + let mut s = blurred_smart_object(); + let before = cache_width(&s); + s.execute(cmd, params).unwrap(); + let after = cache_width(&s); + assert!(after < before * 3 / 4, "{cmd}: {after} px wide (was {before})"); + } +} diff --git a/crates/format/src/atomic.rs b/crates/format/src/atomic.rs index 635bf924a2..8cd349efff 100644 --- a/crates/format/src/atomic.rs +++ b/crates/format/src/atomic.rs @@ -106,7 +106,7 @@ impl AtomicFs for RealFs { fn sync_dir(&self, dir: &Path) -> io::Result<()> { #[cfg(unix)] { - sync_file(&std::fs::File::open(dir)?) + open_dir_for_sync(dir)?.map_or(Ok(()), |d| sync_file(&d)) } #[cfg(windows)] { @@ -254,14 +254,79 @@ pub fn atomic_write_with_progress(fs: &dyn AtomicFs, retry: RenameRetry, path: & let _ = fs.remove(&tmp); return Err(rename_error(e, path)); } + // The new bytes are in place, but until the directory is flushed the rename may not survive + // a power loss (#4968). Windows fails the save on any failed flush (#7858). Elsewhere a file + // system that can't flush directories at all counts as done; any other failure is reported, + // so the caller keeps the document unsaved and its recovery data instead of discarding them + // on a save that may not last. #[cfg(windows)] fs.sync_dir(&dir).map_err(|e| publication_error(e, path))?; - // Keep the existing Unix policy here; #5420 separately changes its error handling. #[cfg(not(windows))] - let _ = fs.sync_dir(&dir); + dir_synced(fs.sync_dir(&dir)).map_err(|e| unconfirmed_error(e, path))?; Ok(()) } +/// `dir` opened to flush it, or `None` when it can't be opened for that because it has no read +/// permission (a write-and-search-only folder): no flush can be attempted there, so none failed. +/// Any other error is returned. +#[cfg(unix)] +fn open_dir_for_sync(dir: &Path) -> io::Result> { + match std::fs::File::open(dir) { + Ok(d) => Ok(Some(d)), + Err(e) if e.kind() == io::ErrorKind::PermissionDenied => Ok(None), + Err(e) => Err(e), + } +} + +/// `result` of flushing a directory, with "this file system can't flush directories" counted as +/// success: `EINVAL` (Linux file systems without directory fsync), `ENOTSUP`/`EOPNOTSUPP` and, +/// on Apple platforms, `ENOTTY`, or [`io::ErrorKind::Unsupported`]. Waiting or retrying won't +/// change those, and failing every save there would make saving impossible. Any other error is +/// returned: the flush was attempted and failed. +#[cfg(not(windows))] +pub(crate) fn dir_synced(result: io::Result<()>) -> io::Result<()> { + match result { + Err(e) if dir_sync_unsupported(&e) => Ok(()), + other => other, + } +} + +#[cfg(not(windows))] +fn dir_sync_unsupported(e: &io::Error) -> bool { + if e.kind() == io::ErrorKind::Unsupported { + return true; + } + // errno values: EINVAL is 22 everywhere; ENOTSUP and EOPNOTSUPP are 95 on Linux and + // Android, 45 and 102 on Apple platforms, 45 on the BSDs; ENOTTY is 25. Elsewhere 45 means + // something else (illumos: EDEADLK), so only EINVAL and `Unsupported` count there. + const EINVAL: i32 = 22; + let unsupported: &[i32] = if cfg!(any(target_os = "linux", target_os = "android")) { + &[EINVAL, 95] + } else if cfg!(target_vendor = "apple") { + &[EINVAL, 45, 102, 25] + } else if cfg!(any(target_os = "freebsd", target_os = "openbsd", target_os = "netbsd", target_os = "dragonfly")) { + &[EINVAL, 45] + } else if cfg!(unix) { + &[EINVAL] + } else { + &[] + }; + e.raw_os_error().is_some_and(|code| unsupported.contains(&code)) +} + +/// The error for a file that was published but whose directory flush failed: same kind, saying +/// the file is in place but may not survive a power loss. (Windows reports `publication_error`.) +#[cfg(not(windows))] +fn unconfirmed_error(e: io::Error, path: &Path) -> io::Error { + context( + e, + format!( + "{} was written, but the folder could not be flushed to disk, so the save may not survive a power loss or crash; save again or to another folder", + path.display() + ), + ) +} + /// Run `rename`, retrying with backoff while it fails with `PermissionDenied` (see /// [`RenameRetry`]). For callers that rename through their own handles (capability directories). pub fn retry_rename(retry: RenameRetry, mut rename: impl FnMut() -> io::Result<()>) -> io::Result<()> { diff --git a/crates/format/src/atomic/tests.rs b/crates/format/src/atomic/tests.rs index d48cf7d9a6..bc26ae6d0a 100644 --- a/crates/format/src/atomic/tests.rs +++ b/crates/format/src/atomic/tests.rs @@ -41,8 +41,9 @@ struct Faulty { /// Fail the first N renames with PermissionDenied (a transient lock), then succeed. locked_renames: Cell, renames: Cell, - /// Failure of the durability barrier after publication (#7858). - fail_sync_dir: bool, + /// Fail the directory flush (the durability barrier after publication, #4968, #7858) + /// with this raw OS error, or with this kind. + fail_sync_dir: Option>, synced: Cell, } @@ -72,10 +73,11 @@ impl AtomicFs for Faulty { /// Count publication barriers and optionally fail before the real directory flush. fn sync_dir(&self, dir: &Path) -> io::Result<()> { self.synced.set(self.synced.get() + 1); - if self.fail_sync_dir { - return Err(io::Error::other("injected publication flush failure")); + match self.fail_sync_dir { + Some(Ok(code)) => Err(io::Error::from_raw_os_error(code)), + Some(Err(kind)) => Err(io::Error::new(kind, "injected directory flush failure")), + None => RealFs.sync_dir(dir), } - RealFs.sync_dir(dir) } fn copy_metadata(&self, from: &Path, to: &Path) -> io::Result<()> { RealFs.copy_metadata(from, to) @@ -92,7 +94,7 @@ fn a_failed_publication_flush_is_reported_and_can_be_retried() { let d = TempDir::new("publication-flush"); let p = d.0.join("document.pcraft"); std::fs::write(&p, ORIGINAL).unwrap(); - let fs = Faulty { fail_sync_dir: true, ..Default::default() }; + let fs = Faulty { fail_sync_dir: Some(Err(io::ErrorKind::Other)), ..Default::default() }; let error = atomic_write_with(&fs, NO_RETRY, &p, b"new document").unwrap_err(); assert_eq!(fs.synced.get(), 1, "{error}"); @@ -444,3 +446,38 @@ fn sync_file_flushes_a_local_file() { let file = std::fs::OpenOptions::new().write(true).create_new(true).open(&path).unwrap(); sync_file(&file).unwrap(); } + +/// #4968: the rename succeeded but the directory flush failed. The new bytes are in place, and +/// the save is reported as unconfirmed (same kind, saying so) instead of as a success, so the +/// caller keeps the document unsaved and its recovery data. +#[test] +fn a_failed_directory_flush_is_reported_not_swallowed() { + let d = TempDir::new("syncdir"); + let p = d.0.join("doc.pcraft"); + std::fs::write(&p, ORIGINAL).unwrap(); + let fs = Faulty { fail_sync_dir: Some(Err(io::ErrorKind::Other)), ..Default::default() }; + let e = atomic_write_with(&fs, NO_RETRY, &p, b"new").unwrap_err(); + assert_eq!(e.kind(), io::ErrorKind::Other); + let msg = e.to_string(); + assert!(msg.contains("doc.pcraft") && msg.contains("may not survive"), "{msg}"); + assert_eq!(std::fs::read(&p).unwrap(), b"new", "published: the new bytes are in place"); + assert_no_temp(&d.0); +} + +/// A file system that can't flush directories at all (Linux's EINVAL, ENOTSUP, or an +/// `Unsupported` error) still saves: failing every save there would make saving impossible. +/// Windows fails the save on any failed flush (#7858), so this is the Unix rule. +#[cfg(not(windows))] +#[test] +fn a_directory_flush_the_file_system_cannot_do_still_saves() { + let d = TempDir::new("syncdir-unsupported"); + let p = d.0.join("doc.pcraft"); + let fs = Faulty { fail_sync_dir: Some(Err(io::ErrorKind::Unsupported)), ..Default::default() }; + atomic_write_with(&fs, NO_RETRY, &p, b"new").unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"new"); + // EINVAL (22) means "can't flush a directory" only on Unix; EIO (5) is a real failure. + assert_eq!(dir_synced(Err(io::Error::from_raw_os_error(22))).is_ok(), cfg!(unix)); + assert!(dir_synced(Err(io::Error::from_raw_os_error(5))).is_err()); + let fs = Faulty { fail_sync_dir: Some(Ok(5)), ..Default::default() }; + assert!(atomic_write_with(&fs, NO_RETRY, &p, b"newer").is_err()); +} diff --git a/crates/format/src/store.rs b/crates/format/src/store.rs index 18e6a9b432..84124407e4 100644 --- a/crates/format/src/store.rs +++ b/crates/format/src/store.rs @@ -587,6 +587,50 @@ fn validate_bundle_file(root: &Path, path: &Path) -> Result<()> { plain_file_or_missing(path) } +/// Flush `dir`, the folder `written` was just renamed into, as `atomic_write` does (with the +/// full-flush fallback of `sync_file`, #1336). Windows flushes through the injected `AtomicFs` +/// and fails on any error (#7858). +#[cfg(not(windows))] +fn flush_bundle_dir(dir: &Path, written: &Path) -> std::io::Result<()> { + use crate::atomic::AtomicFs; + #[cfg(test)] + { + FLUSHED.with(|f| f.borrow_mut().push(dir.to_path_buf())); + let named = |p: Option<&Path>, name: &str| p.and_then(Path::file_name).is_some_and(|n| n == name); + if FAIL_DIR_FLUSH.with(|f| { + let hit = f.get().is_some_and(|name| named(Some(written), name) || named(written.parent(), name)); + if hit { + f.set(None); + } + hit + }) { + return Err(std::io::Error::other("injected folder flush failure")); + } + } + let _ = written; + crate::atomic::RealFs.sync_dir(dir) +} + +/// [`flush_bundle_dir`], as the save's error when the flush was attempted and failed: `written` +/// is in place, but may not survive a power loss or crash (#4968). +#[cfg(not(windows))] +fn confirm_flushed(dir: &Path, written: &Path) -> Result<()> { + crate::atomic::dir_synced(flush_bundle_dir(dir, written)).map_err(|e| { + FormatError::Io(std::io::Error::new( + e.kind(), + format!("{} was written, but its folder could not be flushed to disk, so the save may not survive a power loss or crash: {e}", written.display()), + )) + }) +} + +#[cfg(all(test, not(windows)))] +thread_local! { + /// Tests: the bundle file, or folder, whose next folder flush fails, once, on this thread. + static FAIL_DIR_FLUSH: std::cell::Cell> = const { std::cell::Cell::new(None) }; + /// Tests: the folders flushed on this thread, in order. + static FLUSHED: std::cell::RefCell> = const { std::cell::RefCell::new(Vec::new()) }; +} + /// Publish a validated bundle entry through a flushed temporary file and replacing rename. /// On Windows, propagate a failed directory barrier even though the new entry is already visible. fn write_bundle_file(root: &Path, path: &Path, data: &[u8], fs: &dyn crate::atomic::AtomicFs) -> Result<()> { @@ -639,10 +683,10 @@ fn write_bundle_file(root: &Path, path: &Path, data: &[u8], fs: &dyn crate::atom { fs.sync_dir(parent).map_err(|e| crate::atomic::publication_error(e, path))?; } - #[cfg(unix)] - { - let _ = std::fs::File::open(parent).and_then(|d| d.sync_all()); - } + // Elsewhere a failed flush is reported too (#4968), but a file system that can't flush a + // directory at all still saves, as in `atomic_write`. + #[cfg(not(windows))] + confirm_flushed(parent, path)?; Ok(()) })(); if result.is_err() && validate_bundle_file(root, &tmp).is_ok() { @@ -994,6 +1038,13 @@ impl PcraftWriter { fs.sync_dir(&dir.join(sub)).map_err(|e| crate::atomic::publication_error(e, &dir.join(sub)))?; } } + #[cfg(not(windows))] + for sub in ["tiles", "blobs"] { + let folder = dir.join(sub); + if folder.is_dir() { + confirm_flushed(&folder, &folder)?; + } + } write_bundle_file(dir, &dir.join(MANIFEST), &p.manifest, fs)?; // Omitted previews belong to the previous manifest until the new one commits. // After commit, a locked leftover is only housekeeping, just like unused objects. @@ -1156,6 +1207,72 @@ fn gc_pinned_directory_handles_resist_subdirectory_swap() { std::fs::remove_dir_all(root).unwrap(); } +/// #4968: a manifest whose folder can't be flushed fails the save, and the objects the previous +/// manifest references stay in place; the next save that flushes removes what is unreferenced. +#[cfg(all(test, not(windows)))] +#[test] +fn a_failed_manifest_flush_keeps_the_previous_objects() { + use std::sync::atomic::{AtomicU64, Ordering}; + + static NEXT: AtomicU64 = AtomicU64::new(0); + let dir = std::env::temp_dir().join(format!("pcraft-flush-{}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed))); + std::fs::create_dir_all(&dir).unwrap(); + let dir = std::fs::canonicalize(dir).unwrap(); + let mut doc = Document::new("flush", photocraft_doc::Size::new(64, 64), photocraft_color::ColorMode::Rgb, SampleType::U8); + let id = doc.insert_above(None, photocraft_doc::Layer::raster("paint", doc.pixel_format())); + let paint = |doc: &mut Document, red: f32| { + doc.layer_mut(id).unwrap().surface_mut().unwrap().fill_rect(photocraft_geom::Rect::new(0, 0, 64, 64), &[red, 0.5, 0.25, 1.0]); + }; + paint(&mut doc, 1.0); + let mut w = PcraftWriter::new(); + w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap(); + let old = list_objects(&dir).unwrap(); + paint(&mut doc, 0.0); + FAIL_DIR_FLUSH.with(|f| f.set(Some(MANIFEST))); + let e = w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap_err(); + assert!(e.to_string().contains("could not be flushed"), "{e}"); + let now = list_objects(&dir).unwrap(); + assert!(old.is_subset(&now), "the previous manifest's objects are kept"); + assert!(now.len() > old.len(), "the new objects were written"); + let stats = w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap(); + assert!(stats.objects_removed > 0, "the next save removes the unreferenced objects"); + assert_eq!(crate::load_path(&dir).unwrap(), doc); + std::fs::remove_dir_all(dir).unwrap(); +} + +/// An object whose folder flush failed is reused by the retried save, which writes no new object +/// there: that save still flushes the object folder before committing a manifest that names it. +#[cfg(all(test, not(windows)))] +#[test] +fn a_retried_save_flushes_the_folder_of_a_reused_object() { + use std::sync::atomic::{AtomicU64, Ordering}; + + static NEXT: AtomicU64 = AtomicU64::new(0); + let dir = std::env::temp_dir().join(format!("pcraft-retry-{}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed))); + std::fs::create_dir_all(&dir).unwrap(); + let dir = std::fs::canonicalize(dir).unwrap(); + // One layer of one tile: the retry finds its only object already written. + let mut doc = Document::new("retry", photocraft_doc::Size::new(16, 16), photocraft_color::ColorMode::Rgb, SampleType::U8); + let id = doc.insert_above(None, photocraft_doc::Layer::raster("paint", doc.pixel_format())); + doc.layer_mut(id).unwrap().surface_mut().unwrap().fill_rect(photocraft_geom::Rect::new(0, 0, 16, 16), &[1.0, 0.5, 0.25, 1.0]); + let mut w = PcraftWriter::new(); + FAIL_DIR_FLUSH.with(|f| f.set(Some("tiles"))); + let e = w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap_err(); + assert!(e.to_string().contains("could not be flushed"), "{e}"); + assert!(!dir.join(MANIFEST).exists(), "nothing committed"); + let written = list_objects(&dir).unwrap(); + assert_eq!(written.iter().filter(|o| o.starts_with("tiles/")).count(), 1, "{written:?}"); + FLUSHED.with(|f| f.borrow_mut().clear()); + let stats = w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap(); + assert_eq!(stats.tiles_written, 0, "the tile is reused"); + let flushed = FLUSHED.with(|f| f.borrow().clone()); + let tiles = flushed.iter().position(|d| *d == dir.join("tiles")); + let manifest = flushed.iter().rposition(|d| *d == dir); + assert!(tiles.is_some() && tiles < manifest, "tiles flushed before the manifest: {flushed:?}"); + assert_eq!(crate::load_path(&dir).unwrap(), doc); + std::fs::remove_dir_all(dir).unwrap(); +} + fn list_objects(dir: &Path) -> Result> { let mut out = HashSet::new(); for sub in ["tiles", "blobs"] { diff --git a/crates/io/src/adjust_map.rs b/crates/io/src/adjust_map.rs index b604c12a8c..3a9d18f94e 100644 --- a/crates/io/src/adjust_map.rs +++ b/crates/io/src/adjust_map.rs @@ -139,6 +139,19 @@ pub fn label(key: &[u8; 4]) -> &'static str { } } +/// Keys of a Brightness/Contrast `CgEd` descriptor: the ones [`write`] emits plus Photoshop's +/// preset bookkeeping. +const BRIT_CGED_KEYS: [&str; 9] = ["Vrsn", "Brgh", "Cntr", "means", "Lab ", "useLegacy", "Auto", "presetKind", "presetFileName"]; + +/// Whether a `brit` layer's `CgEd` descriptor holds settings Brightness/Contrast has no place +/// for. Photoshop 2026's Light adjustment (Exposure, Contrast, Highlights, Shadows, Whites, +/// Blacks) is stored under the `brit` key; read as Brightness/Contrast it kept only Contrast and +/// lost the rest without a word (#6542), so such a layer stays [`Adjustment::Unsupported`]. +pub fn is_extended_brit(cged: Option<&[u8]>) -> bool { + cged.and_then(|c| VersionedDescriptor::parse_prefix(c).ok()) + .is_some_and(|(v, _)| v.descriptor.items.iter().any(|(k, _)| !BRIT_CGED_KEYS.iter().any(|n| k.is(n)))) +} + /// Why [`parse`] keeps a block as [`Adjustment::Unsupported`], when there is more to say than /// "it could not be read" (so far: Color Lookup). pub fn unreadable_reason(key: &[u8; 4], data: &[u8]) -> Option { @@ -204,6 +217,8 @@ fn parse_any(key: &[u8; 4], data: &[u8], cged: Option<&[u8]>) -> Adjustment { b"nvrt" => Some(Adjustment::Invert), b"thrs" => be16(data, 0).map(|v| Adjustment::Threshold { level: f32::from(v) / 255.0 }), b"post" => be16(data, 0).map(|v| Adjustment::Posterize { levels: u32::from(v) }), + // Kept raw (with its `CgEd`, which the PSD writer keeps while the layer is unchanged). + b"brit" if is_extended_brit(cged) => None, b"brit" => { let modern = cged.and_then(|c| VersionedDescriptor::parse_prefix(c).ok()).and_then(|(v, _)| { let d = v.descriptor; diff --git a/crates/io/src/blocks.rs b/crates/io/src/blocks.rs index 34f9654a0a..0cfe0b5e8e 100644 --- a/crates/io/src/blocks.rs +++ b/crates/io/src/blocks.rs @@ -182,6 +182,17 @@ pub fn color_from_desc(d: &Descriptor) -> Option { Color::rgb(g("Rd ")? / 255.0, g("Grn ")? / 255.0, g("Bl ")? / 255.0) } }) + .map(|c| with_pc_alpha(c, d)) +} + +/// PSD colour descriptors have no alpha. PhotoCraft writes `pcAl` (0..1) so shape fill +/// opacity round-trips (#6180). Photoshop ignores the key. +fn with_pc_alpha(mut c: Color, d: &Descriptor) -> Color { + // A hostile file's NaN or infinity would survive the clamp: keep the colour opaque instead. + if let Some(a) = num(d.get("pcAl")).filter(|a| a.is_finite()) { + c.alpha = (a as f32).clamp(0.0, 1.0); + } + c } /// [`Color`] → descriptor in its own model. @@ -204,6 +215,13 @@ pub fn color_to_desc(c: &Color) -> Descriptor { } } +/// Solid shape fills have no other alpha slot. Write `pcAl` only here so gradient stops +/// keep using `Trns` alone (#6180). +fn color_to_desc_with_pc_alpha(c: &Color) -> Descriptor { + let desc = color_to_desc(c); + if c.alpha.is_finite() && (c.alpha - 1.0).abs() > 1e-6 { desc.with("pcAl", Value::Double(f64::from(c.alpha.clamp(0.0, 1.0)))) } else { desc } +} + pub(crate) fn get_desc<'a>(d: &'a Descriptor, key: &str) -> Option<&'a Descriptor> { match d.get(key)? { Value::Descriptor(x) | Value::GlobalObject(x) => Some(x), @@ -499,7 +517,7 @@ pub fn write_fill(f: &Fill) -> ([u8; 4], Vec) { /// A fill as (block key, descriptor with class `null`). pub fn fill_to_desc(f: &Fill) -> ([u8; 4], Descriptor) { match f { - Fill::Solid(c) => (*b"SoCo", Descriptor::new("null").with("Clr ", Value::Descriptor(color_to_desc(c)))), + Fill::Solid(c) => (*b"SoCo", Descriptor::new("null").with("Clr ", Value::Descriptor(color_to_desc_with_pc_alpha(c)))), Fill::Gradient { stops, angle, scale, style, reverse, opacity_stops, midpoints, offset, dither, align } => { let prc = |v: f32| Value::UnitFloat { unit: *b"#Prc", value: f64::from(v * 100.0) }; let mut d = Descriptor::new("null") @@ -1025,6 +1043,55 @@ mod tests { assert!(super::color_from_desc(&Descriptor::new("HSBC").with("Strt", Value::Double(50.0))).is_none()); } + #[test] + fn color_descriptor_keeps_photocraft_alpha() { + use photocraft_color::Color; + use photocraft_doc::Fill; + let c = Color::rgba(0.2, 0.4, 0.6, 0.25); + let (k, d) = super::fill_to_desc(&Fill::Solid(c)); + let Fill::Solid(back) = super::fill_from_desc(&k, &d).unwrap() else { panic!("solid") }; + assert!((back.alpha - 0.25).abs() < 1e-5); + assert!((back.to_rgb()[0] - 0.2).abs() < 1e-3); + let (k, d) = super::fill_to_desc(&Fill::Solid(Color::rgb(1.0, 0.0, 0.0))); + let Fill::Solid(opaque) = super::fill_from_desc(&k, &d).unwrap() else { panic!("solid") }; + assert!((opaque.alpha - 1.0).abs() < 1e-6); + assert!(super::color_to_desc(&c).get("pcAl").is_none(), "generic colour descriptors have no pcAl"); + } + + #[test] + fn non_finite_photocraft_alpha_is_ignored() { + use photocraft_color::Color; + for bad in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + let d = super::color_to_desc(&Color::rgb(0.2, 0.4, 0.6)).with("pcAl", Value::Double(bad)); + let c = super::color_from_desc(&d).unwrap(); + assert_eq!(c.alpha, 1.0, "pcAl {bad}"); + } + let d = super::color_to_desc(&Color::rgb(0.2, 0.4, 0.6)).with("pcAl", Value::Double(0.5)); + assert!((super::color_from_desc(&d).unwrap().alpha - 0.5).abs() < 1e-6); + } + + #[test] + fn gradient_stop_alpha_round_trips_once() { + use photocraft_color::Color; + use photocraft_doc::Fill; + let fill = Fill::Gradient { + stops: vec![(0.0, Color::rgba(1.0, 0.0, 0.0, 0.5)), (1.0, Color::WHITE)], + angle: 90.0, + scale: 1.0, + style: photocraft_doc::GradientStyle::Linear, + reverse: false, + opacity_stops: vec![], + midpoints: vec![], + offset: (0.0, 0.0), + dither: false, + align: true, + }; + let (k, d) = super::fill_to_desc(&fill); + let Fill::Gradient { stops, opacity_stops, .. } = super::fill_from_desc(&k, &d).unwrap() else { panic!("grad") }; + assert!((stops[0].1.alpha - 1.0).abs() < 1e-4, "colour alpha lives in Trns, not the stop colour"); + assert!(opacity_stops.iter().any(|(_, a)| (*a - 0.5).abs() < 1e-3), "{opacity_stops:?}"); + } + #[test] fn blending_ranges_map_to_blend_if() { use photocraft_doc::{BlendIf, BlendRange}; diff --git a/crates/io/src/hue_sat_filter_map.rs b/crates/io/src/hue_sat_filter_map.rs index d7ce04938d..05af178c27 100644 --- a/crates/io/src/hue_sat_filter_map.rs +++ b/crates/io/src/hue_sat_filter_map.rs @@ -173,7 +173,8 @@ pub(super) fn export_item(f: &SmartFilter) -> Result { Descriptor::new("Hst2").with("H ", master.0).with("Strt", master.1).with("Lght", setting(p, "lightness", 0.0, -100.0, 100.0)?), )]; for (i, key) in RANGES.iter().enumerate() { - if let Some(o) = p.get(*key) { + // A `null` range is an untouched one, like a missing key. + if let Some(o) = p.get(*key).filter(|o| !o.is_null()) { adjs.extend(range_entry(i, o)?); } } @@ -303,6 +304,7 @@ mod tests { assert_eq!(export(json!({"hue": 12.4})), json!({"colorize": false, "hue": 12, "saturation": 0, "lightness": 0})); assert_eq!(export(json!({"hue": -400, "lightness": 101, "layer": 3})), json!({"colorize": false, "hue": -180, "saturation": 0, "lightness": 100})); assert_eq!(export(J::Null), json!({"colorize": false, "hue": 0, "saturation": 0, "lightness": 0})); + assert_eq!(export(json!({"reds": null})), json!({"colorize": false, "hue": 0, "saturation": 0, "lightness": 0})); // Colorize starts at 25 % saturation and wraps the hue into 0..360. assert_eq!(export(json!({"colorize": true, "hue": -30})), json!({"colorize": true, "hue": 330, "saturation": 25, "lightness": 0})); // The full set (`to_params`): neutral ranges at their default extent are left out, reds diff --git a/crates/io/src/psd_export.rs b/crates/io/src/psd_export.rs index c174708c2c..8f1eb8b533 100644 --- a/crates/io/src/psd_export.rs +++ b/crates/io/src/psd_export.rs @@ -2171,6 +2171,42 @@ mod tests { assert_eq!(density, 0.5); } + #[test] + fn light_adjustment_is_kept_verbatim_and_warned() { + // #6542: Photoshop 2026's Light layer is a `brit` block whose `CgEd` descriptor carries + // more than Brightness/Contrast. Synthetic block: the extra key names are placeholders. + use photocraft_psd::descriptor::{Descriptor, Value, VersionedDescriptor}; + let brit = vec![0, 0, 0xff, 0xfe, 0, 127, 0, 0]; + let d = Descriptor::new("null") + .with("Vrsn", Value::Integer(1)) + .with("Brgh", Value::Integer(0)) + .with("Cntr", Value::Integer(-2)) + .with("Exps", Value::Double(-2.3)) + .with("Hghl", Value::Integer(-64)) + .with("useLegacy", Value::Boolean(false)); + let cged = VersionedDescriptor::new(d).to_bytes(); + let adjustment = crate::adjust_map::parse(b"brit", &brit, Some(&cged), crate::adjust_map::Channels::Rgb); + assert_eq!(adjustment, photocraft_doc::Adjustment::Unsupported { psd_key: "brit".into(), raw: brit.clone() }); + let mut doc = Document::with_background("Light", photocraft_geom::Size::new(8, 8), ColorMode::Rgb, SampleType::U8, photocraft_doc::Color::WHITE); + let mut layer = Layer::new("Light 1", LayerContent::Adjustment(adjustment)); + layer.psd_blocks = vec![(*b"brit", std::sync::Arc::new(brit.clone())), (*b"CgEd", std::sync::Arc::new(cged.clone()))]; + doc.layers.push(layer); + let block = |file: &PsdFile, key: &[u8; 4]| { + let records = &file.layer_info.as_ref().unwrap().layers; + records.iter().flat_map(|r| r.blocks.iter()).find(|b| b.key == *key).map(|b| b.data.clone()) + }; + let file = PsdFile::from_bytes(&document_to_psd(&doc).to_bytes().unwrap()).unwrap(); + let (restored, warnings) = crate::psd_import::psd_to_document(&file); + assert!(warnings.iter().any(|w| w.contains("\"Light 1\"") && w.contains("Light settings")), "{warnings:?}"); + // Saved again unchanged, both blocks are written back byte for byte. + let again = PsdFile::from_bytes(&document_to_psd(&restored).to_bytes().unwrap()).unwrap(); + assert_eq!(block(&again, b"brit").as_deref(), Some(&brit[..])); + assert_eq!(block(&again, b"CgEd").as_deref(), Some(&cged[..])); + // A plain Brightness/Contrast descriptor still reads as Brightness/Contrast. + let plain = crate::adjust_map::write(&photocraft_doc::Adjustment::BrightnessContrast { brightness: 5.0, contrast: -2.0, legacy: false }); + assert!(!crate::adjust_map::is_extended_brit(Some(&plain[1].1))); + } + #[test] fn size_limit_boundary_keeps_limit_and_switches_above_it() { assert!(!exceeds_psd_size_limit(Some(PSD_MAX_FILE_SIZE))); diff --git a/crates/io/src/psd_import.rs b/crates/io/src/psd_import.rs index eb2fa72ec7..68cb903e73 100644 --- a/crates/io/src/psd_import.rs +++ b/crates/io/src/psd_import.rs @@ -391,10 +391,16 @@ impl Ctx<'_> { // Kept verbatim for saving, but it renders as nothing: say so rather than open the // layer silently without its effect (#1763). if matches!(adj, photocraft_doc::Adjustment::Unsupported { .. }) { - let why = adjust_map::unreadable_reason(k, &data).map(|r| format!(" ({r})")).unwrap_or_default(); + // Photoshop 2026's Light adjustment shares the Brightness/Contrast key (#6542). + let light = k == b"brit" && adjust_map::is_extended_brit(cged); + let why = if light { + " (Light adjustments, with Exposure, Highlights, Shadows, Whites and Blacks, aren't supported yet)".to_string() + } else { + adjust_map::unreadable_reason(k, &data).map(|r| format!(" ({r})")).unwrap_or_default() + }; self.warn(format!( "layer \"{name}\": its {} settings could not be read{why}; the layer is kept as saved but has no effect", - adjust_map::label(k) + if light { "Light" } else { adjust_map::label(k) } )); } LayerContent::Adjustment(adj) diff --git a/crates/io/src/smart_map.rs b/crates/io/src/smart_map.rs index b62e836c43..fcc9eec3f9 100644 --- a/crates/io/src/smart_map.rs +++ b/crates/io/src/smart_map.rs @@ -1820,6 +1820,13 @@ mod tests { sf("image.adjustments.levels", json!({"inBlack": 20, "inWhite": 220, "gamma": 1.4, "red": {"inBlack": 0, "inWhite": 255, "gamma": 0.8}})), sf("image.adjustments.curves", json!({"points": [[0.0, 0.0], [64.0, 40.0], [255.0, 255.0]], "blue": [[0.0, 20.0], [255.0, 235.0]]})), sf("image.adjustments.shadowsHighlights", json!({})), + sf("filter.sharpen.sharpen", json!({})), + // Photoshop stores Dust & Scratches' radius in whole pixels (a `long`). + sf("filter.noise.dustAndScratches", json!({"radius": 4, "threshold": 12})), + sf( + "image.adjustments.hueSaturation", + json!({"colorize": false, "hue": 10, "saturation": -20, "lightness": 5, "reds": {"hue": 5, "saturation": 0, "lightness": -3, "range": [315, 345, 15, 45]}}), + ), // A pencil-drawn curve: 256 output levels (`Mpng`). sf("image.adjustments.curves", json!({"pencil": true, "points": (0..256).rev().collect::>()})), sf("filter.distort.pinch", json!({"amount": -28})), @@ -2877,6 +2884,68 @@ mod tests { } /// A filter PhotoCraft doesn't model, or a known one with settings it doesn't, is kept verbatim; only its visibility and blend change. + #[test] + fn photoshop_sharpen_without_fltr_imports_as_sharpen() { + // #6267: Photoshop's Sharpen item holds only its name, blending and `filterID`. + let item = Descriptor::new("filterFX") + .with("Nm ", Value::Text(UnicodeString::new_nul("Sharpen"))) + .with("blendOptions", Value::Descriptor(Descriptor::new("blendOptions").with("Opct", unit(b"#Prc", 100.0)).with("Md ", enumv("BlnM", "normal")))) + .with("enab", Value::Boolean(true)) + .with("hasoptions", Value::Boolean(false)) + .with("filterID", Value::Integer(1_399_353_968)); + let f = filter_from_item(&item); + assert_eq!((f.command.as_str(), &f.params), ("filter.sharpen.sharpen", &json!({}))); + // Hue/Saturation with an unnumbered range and Dust & Scratches with a missing setting fail + // cleanly instead of panicking. + let bad = SmartFilter { + command: "image.adjustments.hueSaturation".into(), + params: json!({"reds": {"range": [1, "x", 3, 4]}}), + blend: BlendMode::Normal, + opacity: 1.0, + visible: true, + }; + assert!(item_for_filter(&bad).is_err()); + let dust = Descriptor::new("filterFX") + .with("Fltr", Value::Descriptor(Descriptor::new("DstS").with("Rds ", Value::Integer(3)))) + .with("filterID", Value::Integer(code(b"DstS"))); + assert_eq!(filter_from_item(&dust).command, UNSUPPORTED_FILTER); + } + + #[test] + fn hue_saturation_ranges_are_not_lost() { + let hue_sat = |params: J| item_for_filter(&sf("image.adjustments.hueSaturation", params)); + // A range that is not an object is an error, not a silently dropped setting. + assert!(hue_sat(json!({"reds": 5})).is_err()); + assert!(hue_sat(json!({"reds": null})).is_ok()); + // Colorize's 25% default is the master saturation's only: a range's stays 0. + let d = hue_sat(json!({"colorize": true, "reds": {"hue": 5}})).unwrap(); + let adjs = adjustment_list(get_desc(&d, "Fltr").unwrap()).unwrap(); + let saturations: Vec<_> = adjs.iter().map(|a| int(a, "Strt")).collect(); + assert_eq!(saturations, [Some(25), Some(0)]); + // A repeated range (or master) would overwrite the first: the filter is kept verbatim. + let entry = |range: Option| { + let e = Descriptor::new("Hst2").with("H ", Value::Integer(0)).with("Strt", Value::Integer(0)).with("Lght", Value::Integer(0)); + let Some(r) = range else { return Value::Descriptor(e) }; + let e = ["BgnR", "BgnS", "EndS", "EndR"].iter().fold(e.with("LclR", Value::Integer(r)), |e, k| e.with(k, Value::Integer(10))); + Value::Descriptor(e) + }; + for adjs in [vec![entry(None), entry(Some(1)), entry(Some(1))], vec![entry(None), entry(None)]] { + let fltr = Descriptor::new("HStr").with("Clrz", Value::Boolean(false)).with("Adjs", Value::List(adjs)); + let item = Descriptor::new("filterFX") + .with("Nm ", Value::Text(UnicodeString::new_nul("Hue/Saturation..."))) + .with( + "blendOptions", + Value::Descriptor(Descriptor::new("blendOptions").with("Opct", unit(b"#Prc", 100.0)).with("Md ", enumv("BlnM", "normal"))), + ) + .with("enab", Value::Boolean(true)) + .with("Fltr", Value::Descriptor(fltr)) + .with("filterID", Value::Integer(code(b"HStr"))); + let f = filter_from_item(&item); + assert_eq!(f.command, UNSUPPORTED_FILTER); + assert_eq!(item_for_filter(&f).unwrap(), item); + } + } + #[test] fn unknown_filters_keep_their_descriptor() { let item = Descriptor::new("filterFX") diff --git a/crates/io/src/vector_map.rs b/crates/io/src/vector_map.rs index bb830dbd68..621edf47b8 100644 --- a/crates/io/src/vector_map.rs +++ b/crates/io/src/vector_map.rs @@ -258,10 +258,20 @@ pub fn parse_vstk(data: &[u8], dpi: f32) -> Option { _ => Vec::new(), }; let paint = get_desc(&d, "strokeStyleContent").and_then(content_fill).unwrap_or(Fill::Solid(photocraft_color::Color::BLACK)); + let mut opacity = num(d.get("strokeStyleOpacity")).map_or(1.0, |v| v as f32 / 100.0); + // Write folds colour alpha into strokeStyleOpacity for Photoshop. Restore both Appearance + // fields when pcAl is present so PhotoCraft does not show a maxed colour alpha (#6180). + if let Fill::Solid(c) = &paint + && c.alpha.is_finite() + && c.alpha > 1e-6 + && (c.alpha - 1.0).abs() > 1e-6 + { + opacity = (opacity / c.alpha).clamp(0.0, 1.0); + } let stroke = ShapeStroke { width: unit_px(d.get("strokeStyleLineWidth"), dpi).unwrap_or(1.0) as f32, paint, - opacity: num(d.get("strokeStyleOpacity")).map_or(1.0, |v| v as f32 / 100.0), + opacity, align, cap, join, @@ -276,10 +286,28 @@ fn en(ty: &str, v: &str) -> Value { Value::Enumerated { type_id: Id::new(ty), value: Id::new(v) } } +/// Photoshop's stroke colour has no alpha. Fold the colour's alpha into `strokeStyleOpacity` +/// so Appearance still matches after a PSD round-trip (#6180). +fn fold_stroke_color_alpha(s: &ShapeStroke) -> ShapeStroke { + let mut out = s.clone(); + if let Fill::Solid(c) = &mut out.paint + && c.alpha.is_finite() + && (c.alpha - 1.0).abs() > 1e-6 + { + out.opacity = (out.opacity * c.alpha).clamp(0.0, 1.0); + c.alpha = 1.0; + } + out +} + pub fn vstk_bytes(stroke: Option<&ShapeStroke>, fill_enabled: bool, dpi: f32) -> Vec { let default = ShapeStroke::default(); - let s = stroke.unwrap_or(&default); - let (_, content) = fill_to_desc(&s.paint); + let folded = stroke.map(fold_stroke_color_alpha); + let s = folded.as_ref().unwrap_or(&default); + // Keep pcAl on the original colour so Appearance can restore fill/stroke alphas separately + // from the folded Photoshop opacity (#6180). + let paint = stroke.map(|st| &st.paint).unwrap_or(&s.paint); + let (_, content) = fill_to_desc(paint); let class = match s.paint { Fill::Solid(_) => "solidColorLayer", Fill::Gradient { .. } => "gradientLayer", @@ -660,6 +688,20 @@ mod tests { assert!(off.stroke.is_none() && off.fill_enabled); } + #[test] + fn vstk_folds_color_alpha_into_stroke_opacity() { + let s = ShapeStroke { width: 2.0, paint: Fill::Solid(Color::rgba(0.0, 0.0, 1.0, 0.4)), opacity: 0.5, ..ShapeStroke::default() }; + let bytes = vstk_bytes(Some(&s), true, 72.0); + let raw = crate::blocks::parse_prefix_versioned(&bytes).unwrap(); + let file_opacity = crate::blocks::num(raw.get("strokeStyleOpacity")).unwrap() as f32 / 100.0; + assert!((file_opacity - 0.2).abs() < 1e-4, "Photoshop sees the product in strokeStyleOpacity"); + let got = parse_vstk(&bytes, 72.0).unwrap().stroke.unwrap(); + assert!((got.opacity - 0.5).abs() < 1e-4); + let Fill::Solid(c) = got.paint else { panic!("solid") }; + assert!((c.alpha - 0.4).abs() < 1e-5); + assert!((c.to_rgb()[2] - 1.0).abs() < 1e-3); + } + #[test] fn vogk_roundtrip() { for s in [ diff --git a/crates/io/tests/it/vector.rs b/crates/io/tests/it/vector.rs index e32cbed3fa..71bf66823c 100644 --- a/crates/io/tests/it/vector.rs +++ b/crates/io/tests/it/vector.rs @@ -135,6 +135,37 @@ fn shape_layer_typed_roundtrip() { } } +/// #6180: Appearance fill and stroke alphas survive PSD save/load, and a redraw +/// (move/crop) uses those values instead of snapping to opaque. +#[test] +fn rectangle_appearance_alphas_survive_psd_and_redraw() { + let mut d = doc(); + let sh = ShapeLayer { + path: photocraft_vector::shapes::rect(16.0, 16.0, 128.0, 96.0), + fill: Some(Fill::Solid(Color::rgba(1.0, 0.0, 0.0, 0.5))), + stroke: Some(ShapeStroke { width: 4.0, paint: Fill::Solid(Color::rgba(0.0, 0.0, 1.0, 0.4)), opacity: 0.5, ..ShapeStroke::default() }), + live: Some(LiveShape::Rect { rect: [16.0, 16.0, 128.0, 96.0], radii: [0.0; 4] }), + ..Default::default() + }; + d.layers.push(shape_layer(sh)); + let b = roundtrip(&d); + let LayerContent::Shape(s) = &b.layers[1].content else { panic!("not a shape") }; + let Fill::Solid(fill) = s.fill.as_ref().unwrap() else { panic!("solid fill") }; + assert!((fill.alpha - 0.5).abs() < 1e-4, "fill appearance alpha {}", fill.alpha); + let st = s.stroke.as_ref().unwrap(); + let Fill::Solid(stroke_c) = &st.paint else { panic!("solid stroke") }; + assert!((stroke_c.alpha - 0.4).abs() < 1e-4, "stroke colour alpha {}", stroke_c.alpha); + assert!((st.opacity - 0.5).abs() < 1e-4, "stroke opacity {}", st.opacity); + let mut redrawn = s.clone(); + redrawn.cache = Some(photocraft_vector::render_shape(&redrawn, photocraft_color::PixelFormat::RGBA8, Rect::new(0, 0, 256, 256))); + let Fill::Solid(fill2) = redrawn.fill.as_ref().unwrap() else { panic!("solid fill") }; + assert!((fill2.alpha - 0.5).abs() < 1e-4); + let st2 = redrawn.stroke.as_ref().unwrap(); + let Fill::Solid(stroke2) = &st2.paint else { panic!("solid stroke") }; + assert!((stroke2.alpha - 0.4).abs() < 1e-4); + assert!((st2.opacity - 0.5).abs() < 1e-4); +} + #[test] fn custom_stroke_styles_roundtrip_through_psd_vstk() { for align in [StrokeAlign::Inside, StrokeAlign::Center, StrokeAlign::Outside] { diff --git a/crates/paint/src/replace.rs b/crates/paint/src/replace.rs index 3cb02e9a24..20619cd48a 100644 --- a/crates/paint/src/replace.rs +++ b/crates/paint/src/replace.rs @@ -127,6 +127,8 @@ pub fn color_replacement_effect(pre: Surface, rs: ReplaceSettings) -> impl FnMut return; } let (w, h) = (r.width() as usize, r.height() as usize); + let mut pre_px = vec![[0.0f32; 4]; w * h]; + pre.read_rgba_into(r, &mut pre_px); // Per-pixel match weight. let mut weight = vec![0.0f32; w * h]; for y in r.y0..r.y1 { @@ -136,7 +138,7 @@ pub fn color_replacement_effect(pre: Surface, rs: ReplaceSettings) -> impl FnMut } // Match against the pre-stroke colours, so pixels recoloured by earlier dabs of // this stroke still count as the sampled colour. - let c = pre.rgba(x, y); + let c = pre_px[(y - r.y0) as usize * w + (x - r.x0) as usize]; if c[3] <= 0.0 { continue; } diff --git a/crates/raster/src/lib.rs b/crates/raster/src/lib.rs index 6ce0ea688d..fb6a6fd388 100644 --- a/crates/raster/src/lib.rs +++ b/crates/raster/src/lib.rs @@ -1116,7 +1116,8 @@ impl Surface { /// Composite-friendly RGBA read (converts from the surface's model). pub fn rgba(&self, x: i32, y: i32) -> [f32; 4] { - let p = self.pixel(x, y); + let mut p = [0.0f32; 5]; + self.read_pixel(x, y, &mut p); to_rgba(&self.format, &p) } } diff --git a/crates/ui-egui/src/adjust_dialog.rs b/crates/ui-egui/src/adjust_dialog.rs index acb56b46aa..1c45da190b 100644 --- a/crates/ui-egui/src/adjust_dialog.rs +++ b/crates/ui-egui/src/adjust_dialog.rs @@ -629,6 +629,84 @@ mod tests { } } + /// #8984: the Curves dialog has a resize grip; its graph grows with the dialog, the controls + /// stay inside it, and it reopens at the size it was left at. + #[test] + fn curves_dialog_resizes_with_its_graph_and_reopens_at_that_size() { + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1100.0)).build_ui_state(|ui, app| crate::dialogs::show(app, ui.ctx()), app_with_image()); + PhotocraftApp::setup_context(&h.ctx, Default::default()); + let graph = |h: &Harness<'_, PhotocraftApp>| h.ctx.data(|d| d.get_temp::(dialog_mem("curves").with("curves-graph"))).unwrap(); + let window = |h: &Harness<'_, PhotocraftApp>, id: u64| h.ctx.memory(|m| m.area_rect(egui::Id::new(("dialog", id)))).unwrap(); + let id = open(h.state_mut(), "image.adjustments.curves").unwrap(); + h.run_steps(4); + let (opened, small) = (window(&h, id), graph(&h)); + assert!(small.width() > 270.0 && small.width() < 300.0, "opens at the 300-point graph: {small:?}"); + // Drag the grip (bottom-right corner) 300 points right and 200 down. + let corner = opened.max - egui::vec2(12.0, 12.0); + let by = egui::vec2(300.0, 200.0); + h.event(egui::Event::PointerMoved(corner)); + h.step(); + h.event(egui::Event::PointerButton { pos: corner, button: egui::PointerButton::Primary, pressed: true, modifiers: egui::Modifiers::NONE }); + h.step(); + for i in 1..=4 { + h.event(egui::Event::PointerMoved(corner + by * (i as f32 / 4.0))); + h.step(); + } + h.event(egui::Event::PointerButton { pos: corner + by, button: egui::PointerButton::Primary, pressed: false, modifiers: egui::Modifiers::NONE }); + h.run_steps(6); + let (wide, big) = (window(&h, id), graph(&h)); + assert!(big.width() > small.width() + 250.0, "the graph follows the dialog: {small:?} → {big:?}"); + assert!((big.width() - big.height()).abs() < 1.0, "still square: {big:?}"); + assert!(wide.contains_rect(big) && wide.right() - big.right() < 40.0, "no blank column: window {wide:?}, graph {big:?}"); + for label in ["Channel:", "Input:", "Output:", "Preview", "OK", "Cancel"] { + assert!(wide.contains_rect(h.get_by_label(crate::i18n::t(label)).rect()), "{label} inside the window"); + } + // Cancel, open again: the size it was left at. + h.get_by_label("Cancel").click(); + h.run_steps(3); + assert!(h.state().ui.dialogs.is_empty()); + let again = open(h.state_mut(), "image.adjustments.curves").unwrap(); + h.run_steps(4); + assert!((graph(&h).width() - big.width()).abs() < 2.0, "reopened at {:?}, left at {big:?}", graph(&h)); + assert!((window(&h, again).width() - wide.width()).abs() < 2.0); + } + + /// #8984: however tall the graph's width would make it, the dialog still fits the window. + #[test] + fn curves_graph_never_pushes_the_dialog_off_a_short_window() { + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 700.0)).build_ui_state(|ui, app| crate::dialogs::show(app, ui.ctx()), app_with_image()); + PhotocraftApp::setup_context(&h.ctx, Default::default()); + h.ctx.data_mut(|m| m.insert_persisted(egui::Id::new(("dialog-place", "image.adjustments.curves")).with("size"), egui::vec2(1200.0, 1200.0))); + let id = open(h.state_mut(), "image.adjustments.curves").unwrap(); + h.run_steps(5); + let graph = h.ctx.data(|d| d.get_temp::(dialog_mem("curves").with("curves-graph"))).unwrap(); + let window = h.ctx.memory(|m| m.area_rect(egui::Id::new(("dialog", id)))).unwrap(); + assert!(graph.height() <= 700.0 - 360.0, "{graph:?}"); + assert!(window.height() <= 700.0, "{window:?}"); + assert!(window.contains_rect(h.get_by_label("OK").rect())); + // The size the window squeezed it to is not remembered: a larger window gets 1200 again. + let kept = h.ctx.data_mut(|m| m.get_persisted::(egui::Id::new(("dialog-place", "image.adjustments.curves")).with("size"))); + assert_eq!(kept, Some(egui::vec2(1200.0, 1200.0))); + // Dragged far wider, the dialog stops where the graph stops: no blank column. + let corner = window.max - egui::vec2(12.0, 12.0); + h.event(egui::Event::PointerMoved(corner)); + h.step(); + h.event(egui::Event::PointerButton { pos: corner, button: egui::PointerButton::Primary, pressed: true, modifiers: egui::Modifiers::NONE }); + h.step(); + h.event(egui::Event::PointerMoved(corner + egui::vec2(600.0, 0.0))); + h.step(); + h.event(egui::Event::PointerButton { + pos: corner + egui::vec2(600.0, 0.0), + button: egui::PointerButton::Primary, + pressed: false, + modifiers: egui::Modifiers::NONE, + }); + h.run_steps(5); + let graph = h.ctx.data(|d| d.get_temp::(dialog_mem("curves").with("curves-graph"))).unwrap(); + let window = h.ctx.memory(|m| m.area_rect(egui::Id::new(("dialog", id)))).unwrap(); + assert!(window.right() - graph.right() < 40.0, "no blank column: window {window:?}, graph {graph:?}"); + } + #[test] fn cancel_leaves_the_document_untouched() { let mut harness = diff --git a/crates/ui-egui/src/adjust_editors.rs b/crates/ui-egui/src/adjust_editors.rs index 877525ffb8..f646ddd452 100644 --- a/crates/ui-egui/src/adjust_editors.rs +++ b/crates/ui-egui/src/adjust_editors.rs @@ -357,6 +357,27 @@ fn gradient_bar(p: &egui::Painter, r: Rect, from: Color32, to: Color32, vertical const CURVE_HANDLE_SIZE: f32 = 7.0; const CURVE_GRAPH_PADDING: f32 = 5.0; +/// The room left on screen for the Curves dialog's other rows (title, channel and mode, Input and +/// Output, eyedroppers, buttons) when its graph is as tall as it can be. +const CURVES_CHROME: f32 = 360.0; +/// The smallest and the default largest side of the Curves graph: 300 points was the fixed +/// size until #8984. +const CURVE_SIDE: (f32, f32) = (120.0, 300.0); + +/// The Curves graph's side (#8984): the width it is given, in the Properties panel and in the +/// resizable dialog, and no taller than the window leaves room for beside the other rows. +fn curve_side(ui: &egui::Ui) -> f32 { + let tallest = curve_side_max(ui.ctx()); + let width = ui.available_width(); + if width.is_finite() { width.clamp(CURVE_SIDE.0, tallest) } else { CURVE_SIDE.1 } +} + +/// The largest the Curves graph gets in this window: as tall as the window leaves room for beside +/// the dialog's other rows, and never under the old 300 points. The Curves dialog is no wider. +pub(crate) fn curve_side_max(ctx: &egui::Context) -> f32 { + (ctx.content_rect().height() - CURVES_CHROME).max(CURVE_SIDE.1) +} + fn curve_graph(full: Rect, side: f32) -> Rect { let base = Rect::from_min_size(full.min + vec2(14.0, 0.0), vec2(side - 14.0, side - 14.0)); Rect::from_min_max(base.min + vec2(CURVE_GRAPH_PADDING, CURVE_GRAPH_PADDING), base.max - vec2(CURVE_GRAPH_PADDING, CURVE_GRAPH_PADDING)) @@ -461,7 +482,7 @@ fn curves(ui: &mut egui::Ui, v: &mut Value, cx: &EditorCx) -> Edit { let pencil = v.get("pencil").and_then(Value::as_bool).unwrap_or(false); let mut pts = read_curve(v, key); let mut map = read_map(v, key); - let side = ui.available_width().clamp(120.0, 300.0); + let side = curve_side(ui); ui.add_space(4.0); let (full, resp) = ui.allocate_exact_size(vec2(side, side + 14.0), Sense::click_and_drag()); let graph = curve_graph(full, side); diff --git a/crates/ui-egui/src/adjust_editors_tests.rs b/crates/ui-egui/src/adjust_editors_tests.rs index d8c6e7953a..7f669186bb 100644 --- a/crates/ui-egui/src/adjust_editors_tests.rs +++ b/crates/ui-egui/src/adjust_editors_tests.rs @@ -206,6 +206,27 @@ fn curve_of(a: &Adjustment) -> Vec<[f32; 2]> { } } +/// #8984: the Curves graph in Properties grows with the panel instead of stopping at 300 +/// points, stays square, and edits where it is clicked at its new size. +#[test] +fn curves_graph_grows_with_the_properties_panel() { + let mut h = app_harness("curves", "rgb", 8); + let (id, _) = layer(&h); + let narrow = curves_graph(&h, id); + assert!(narrow.width() < 300.0, "{narrow:?}"); + crate::panels::request_dock_width(&h.ctx, 760.0); + h.run_steps(8); + let wide = curves_graph(&h, id); + // 760 points of panel, but the 900-point window leaves room for 540 beside the other rows. + assert!(wide.width() > narrow.width() + 250.0 && wide.height() <= 900.0 - 360.0, "{narrow:?} → {wide:?}"); + assert!((wide.width() - wide.height()).abs() < 1.0, "square: {wide:?}"); + // A point added at the graph's centre lands at 128, 128 of the curve. + click(&mut h, wide.center()); + let c = curve_of(&layer(&h).1); + assert_eq!(c.len(), 3, "{c:?}"); + assert!((c[1][0] - 127.5).abs() < 2.0 && (c[1][1] - 127.5).abs() < 2.0, "{c:?}"); +} + /// Issue #43: pressing on a point selects it (no new point), Delete/Backspace and ⌘/Ctrl-click /// remove points, dragging off the graph removes one, endpoints stay. #[test] diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 708f985fe4..bb77cb0add 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1445,6 +1445,7 @@ pub(crate) fn display_doc(app: &mut PhotocraftApp, idx: usize) -> (std::sync::Ar } if let (Some(t), Some(pv)) = (&app.ui.transform, &app.transform_preview) && app.session.active_index() == Some(idx) + && pv.doc.id == st.doc.id && st.doc.layer(photocraft_doc::LayerId(t.layer)).is_some() { return (pv.doc.clone(), (1 << 40) + pv.session); diff --git a/crates/ui-egui/src/color_picker_ui.rs b/crates/ui-egui/src/color_picker_ui.rs index 18ebefede7..ece2fbafb3 100644 --- a/crates/ui-egui/src/color_picker_ui.rs +++ b/crates/ui-egui/src/color_picker_ui.rs @@ -484,7 +484,7 @@ fn color_field(ui: &mut egui::Ui, f: &mut Map) { // Photoshop updates the colour on the press itself, not only when the button is released // (#6937): `is_pointer_button_down_on` is true from the press frame onwards (it covers // dragging too), while `clicked` fires on release and `dragged` only after a move. - if let Some(p) = resp.interact_pointer_pos().filter(|_| resp.is_pointer_button_down_on() || resp.clicked()) { + if let Some(p) = widgets::pick_pos(&resp) { let (x, y) = (((p.x - field.left()) / field.width()).clamp(0.0, 1.0), ((p.y - field.top()) / field.height()).clamp(0.0, 1.0)); let c = field_color(&mode, fz, x, y); let keep = match mode.as_str() { @@ -503,7 +503,7 @@ fn color_field(ui: &mut egui::Ui, f: &mut Map) { let tri = vec![pos2(x, sy), pos2(x - 6.0 * dir, sy - 4.0), pos2(x - 6.0 * dir, sy + 4.0)]; ui.painter().add(egui::Shape::convex_polygon(tri, t.text, Stroke::NONE)); } - if let Some(p) = sresp.interact_pointer_pos().filter(|_| sresp.is_pointer_button_down_on() || sresp.clicked()) { + if let Some(p) = widgets::pick_pos(&sresp) { let z = 1.0 - ((p.y - strip.top()) / strip.height()).clamp(0.0, 1.0); let h = match mode.as_str() { "h" => Some([z * 360.0, hsv[1], hsv[2]]), @@ -863,6 +863,95 @@ mod tests { h.state().get("color").and_then(Value::as_str).unwrap().to_string() } + /// Press at `at` and hold, with no release and no time passing, then run one frame. + fn press(h: &mut Harness<'static, S>, at: egui::Pos2) { + press_button(h, at, egui::PointerButton::Primary, true); + } + + fn press_button(h: &mut Harness<'static, S>, at: egui::Pos2, button: egui::PointerButton, pressed: bool) { + h.event(egui::Event::PointerMoved(at)); + h.event(egui::Event::PointerButton { pos: at, button, pressed, modifiers: egui::Modifiers::NONE }); + h.run_steps(1); + } + + /// #7793: the field and the slider pick on the frame the button goes down, not after the + /// pointer has travelled past egui's click threshold or been held for a while, and follow + /// moves of a few points. + #[test] + fn field_and_slider_pick_on_press_and_follow_small_moves() { + let mut f = Map::new(); + f.insert("color".into(), json!("#336699")); + let mut h = Harness::builder().with_size(vec2(400.0, 300.0)).build_ui_state( + |ui, f: &mut Map| { + ui.horizontal(|ui| { + let o = ui.cursor().min; + let gap = ui.spacing().item_spacing.x; + ui.data_mut(|d| d.insert_temp(egui::Id::new("test-origin"), (o, gap))); + color_field(ui, f); + }); + }, + f, + ); + h.run_steps(2); + let (o, gap) = h.ctx.data(|d| d.get_temp::<(egui::Pos2, f32)>(egui::Id::new("test-origin"))).unwrap(); + // Field: saturation across, brightness down (hue mode). + let at = o + vec2(FIELD * 0.5, FIELD * 0.5); + press(&mut h, at); + let picked = color(&h); + assert_ne!(picked, "#336699", "the press itself picks"); + let hsv = rgb_to_hsb(parse_hex(&picked).unwrap()); + assert!((hsv[1] - 0.5).abs() < 0.02 && (hsv[2] - 0.5).abs() < 0.02, "{picked} {hsv:?}"); + // A 3-point nudge (under the 6-point click threshold) moves the pick. + h.event(egui::Event::PointerMoved(at + vec2(3.0, 0.0))); + h.run_steps(1); + assert_ne!(color(&h), picked, "a small drag adjusts the pick"); + h.event(egui::Event::PointerButton { + pos: at + vec2(3.0, 0.0), + button: egui::PointerButton::Primary, + pressed: false, + modifiers: egui::Modifiers::NONE, + }); + h.run_steps(2); + // Hue slider: a press near the bottom picks a hue near 0 at once. + let before = color(&h); + press(&mut h, o + vec2(FIELD + gap + 9.0, FIELD * 0.95)); + assert_ne!(color(&h), before, "the slider picks on press"); + let hue = rgb_to_hsb(parse_hex(&color(&h)).unwrap())[0]; + assert!(hue < 30.0 || hue > 330.0, "hue {hue}"); + } + + /// Only the primary button picks: a right or middle press (and its release) on the field or + /// the slider leaves the colour alone. + #[test] + fn field_and_slider_ignore_secondary_and_middle_presses() { + let mut f = Map::new(); + f.insert("color".into(), json!("#336699")); + let mut h = Harness::builder().with_size(vec2(400.0, 300.0)).build_ui_state( + |ui, f: &mut Map| { + ui.horizontal(|ui| { + let o = ui.cursor().min; + let gap = ui.spacing().item_spacing.x; + ui.data_mut(|d| d.insert_temp(egui::Id::new("test-origin"), (o, gap))); + color_field(ui, f); + }); + }, + f, + ); + h.run_steps(2); + let (o, gap) = h.ctx.data(|d| d.get_temp::<(egui::Pos2, f32)>(egui::Id::new("test-origin"))).unwrap(); + for at in [o + vec2(FIELD * 0.5, FIELD * 0.5), o + vec2(FIELD + gap + 9.0, FIELD * 0.95)] { + for button in [egui::PointerButton::Secondary, egui::PointerButton::Middle] { + press_button(&mut h, at, button, true); + assert_eq!(color(&h), "#336699", "{button:?} press at {at:?}"); + h.event(egui::Event::PointerMoved(at + vec2(3.0, 0.0))); + h.run_steps(1); + press_button(&mut h, at + vec2(3.0, 0.0), button, false); + h.run_steps(1); + assert_eq!(color(&h), "#336699", "{button:?} drag and release at {at:?}"); + } + } + } + #[test] fn hex_field_shows_uppercase_without_rewriting_the_selected_color() { let h = picker("#12abef"); diff --git a/crates/ui-egui/src/crop_size.rs b/crates/ui-egui/src/crop_size.rs index dc91ec681b..71fc57f495 100644 --- a/crates/ui-egui/src/crop_size.rs +++ b/crates/ui-egui/src/crop_size.rs @@ -226,15 +226,21 @@ pub fn clear(o: &mut ToolOptions) { o.crop_resolution.clear(); } -/// Swaps W and H (the ⇄ button, X). False outside W x H x Resolution mode. +/// Swaps W and H for the Crop tool's X. False outside W x H x Resolution mode. pub fn swap(o: &mut ToolOptions) -> bool { if o.crop_ratio != WHR { return false; } - std::mem::swap(&mut o.crop_width, &mut o.crop_height); + swap_fields(o); true } +/// The ⇄ button between the W and H fields: always swaps, as Perspective Crop shows the fields +/// without the Crop tool's W x H x Resolution mode. +fn swap_fields(o: &mut ToolOptions) { + std::mem::swap(&mut o.crop_width, &mut o.crop_height); +} + /// After the preset dropdown chose `crop_ratio`: a size preset fills the fields and Front Image /// reads the document, both leaving W x H x Resolution mode on. pub fn chosen(app: &mut PhotocraftApp) { @@ -298,7 +304,7 @@ fn text_field(ui: &mut egui::Ui, id: &str, value: &mut String, hint: &str, width pub fn fields(o: &mut ToolOptions, ui: &mut egui::Ui) { text_field(ui, "crop-width", &mut o.crop_width, tl!("Width"), 64.0, normalize_length); if crate::icons::button(ui, "arrow-left-right", 22.0, false, tl!("Swaps height and width")).clicked() { - swap(o); + swap_fields(o); } text_field(ui, "crop-height", &mut o.crop_height, tl!("Height"), 64.0, normalize_length); text_field(ui, "crop-resolution", &mut o.crop_resolution, tl!("Resolution"), 64.0, normalize_resolution); diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index eadffa17ae..bc658783cd 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -181,16 +181,47 @@ fn resizable_dialog(fields: &serde_json::Map) -> Option) -> bool { + fields.get("__adjust").and_then(Value::as_str) == Some("curves") +} + +/// The size the Curves dialog first opens at: the 300-point graph it had before it could be +/// resized (#5113); it grows to fit its controls below the graph. +const CURVES_SIZE: egui::Vec2 = egui::vec2(300.0, 420.0); +/// The smallest a resizable dialog is dragged to: Curves keeps a usable graph (#8984), the others +/// their two-column layouts. +fn resizable_min(fields: &serde_json::Map) -> egui::Vec2 { + if is_curves(fields) { egui::vec2(300.0, 420.0) } else { egui::vec2(560.0, 400.0) } +} + +/// Where the size the user last gave a resizable dialog is kept (#8984), beside its place. +fn size_id(place: egui::Id) -> egui::Id { + place.with("size") +} + +/// The largest a resizable dialog of `fields` with smallest size `min` is dragged to: the window +/// less a margin, and for Curves no wider than its graph can grow (#8984), so no blank column. +fn resizable_max(ctx: &egui::Context, fields: &serde_json::Map, min: egui::Vec2) -> egui::Vec2 { + let mut max = (ctx.content_rect().size() - egui::vec2(40.0, 40.0)).max(min); + if is_curves(fields) { + max.x = max.x.min(crate::adjust_editors::curve_side_max(ctx)).max(min.x); + } + max +} + fn show_modal( modal: egui::Modal, ctx: &egui::Context, id: egui::Id, - resizable: Option, + resizable: Option<(egui::Vec2, egui::Vec2, egui::Vec2)>, interactive: bool, contents: impl FnOnce(&mut egui::Ui) -> T, ) -> egui::ModalResponse { @@ -199,17 +230,10 @@ fn show_modal( if !interactive { ui.disable(); } - if let Some(size) = resizable { - let max = (ctx.content_rect().size() - egui::vec2(40.0, 40.0)).max(egui::vec2(560.0, 400.0)); + if let Some((size, min, max)) = resizable { // No stroke: Resize outlines its contents 2 points out, an inner box inside the // dialog's frame that left the title and buttons flush against it (#8199). - egui::Resize::default() - .id(id.with("resize")) - .with_stroke(false) - .default_size(size.min(max)) - .min_size(egui::vec2(560.0, 400.0)) - .max_size(max) - .show(ui, contents) + egui::Resize::default().id(id.with("resize")).with_stroke(false).default_size(size.min(max)).min_size(min).max_size(max).show(ui, contents) } else { contents(ui) } @@ -289,14 +313,22 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) { let web = fields.contains_key("__web"); let web_popup_open = web && egui::Popup::is_any_open(ctx); let resizable = resizable_dialog(&fields); - let modal = show_modal(modal, ctx, id, resizable, interactive, |ui| { + // Curves reopens at the size it was left at (#8984). + let remembered = if is_curves(&fields) { ctx.data_mut(|m| m.get_persisted::(size_id(place))) } else { None }; + let resize = resizable.map(|size| { + let min = resizable_min(&fields); + (remembered.unwrap_or(size), min, resizable_max(ctx, &fields, min)) + }); + let mut given = None; + let modal = show_modal(modal, ctx, id, resize, interactive, |ui| { sizing = ui.is_sizing_pass(); let available = ui.available_size(); + given = Some(available); // A dialog of a few short fields (`__compact`, Fit Image) is narrower (#4673): the title // rule and the button row fill the width they are given, so it needs its own maximum too. let compact = d.fields.contains_key("__compact"); - // The curve editor caps its graph allocation at 300 points (#5113). - let curves = fields.get("__adjust").and_then(Value::as_str) == Some("curves"); + // The curve editor's graph is as wide as the dialog lets it be (#5113, #8984). + let curves = is_curves(&fields); if !compact { ui.set_min_width(if curves { 300.0 } else { 380.0 }); } @@ -323,7 +355,7 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) { } else if compact { 260.0 } else if curves { - 300.0 + available.x.max(300.0) } else { 440.0 })); @@ -582,6 +614,17 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) { }); // The picker has a larger frame margin; include that whole frame in the canvas hit guard. shown.push(if color_picker { modal.response.rect } else { modal.inner }); + // The size Curves was given (dragged or grown to fit) is the one it reopens at (#8984). + // A size at the window's limit is the window's, not the user's: a large remembered size + // squeezed into a small window is kept for a larger one. + if let (false, true, Some(size), Some((opened, _, max))) = (sizing, is_curves(&fields), given.filter(|g| g.is_finite()), resize) + && size != opened.min(max) + && size.x < max.x - 0.5 + && size.y < max.y - 0.5 + && remembered != Some(size) + { + ctx.data_mut(|m| m.insert_persisted(size_id(place), size)); + } // Pin once laid out at its real size (the first frame is an invisible sizing pass). if !sizing && (web || pinned.is_none() || drag != egui::Vec2::ZERO) { let screen = ctx.content_rect(); diff --git a/crates/ui-egui/src/distort_ui.rs b/crates/ui-egui/src/distort_ui.rs index 5441a971ee..be1c0ddbc8 100644 --- a/crates/ui-egui/src/distort_ui.rs +++ b/crates/ui-egui/src/distort_ui.rs @@ -10,7 +10,7 @@ use std::sync::Arc; use egui::{Color32, ColorImage}; -use photocraft_doc::{Document, LayerContent, LayerId}; +use photocraft_doc::{DocId, Document, LayerContent, LayerId}; use photocraft_geom::Rect; use photocraft_raster::Surface; use serde_json::{Value, json}; @@ -91,15 +91,50 @@ pub(crate) fn surface_image(surf: &Surface, r: Rect, max_side: usize) -> ColorIm ColorImage::new([tw, th], px) } +fn active_doc(app: &PhotocraftApp) -> Option { + app.session.active().map(|st| st.doc.id) +} + +fn on_doc(app: &PhotocraftApp, document: DocId) -> bool { + active_doc(app) == Some(document) +} + +/// Drops Liquify / Puppet / Perspective Warp / Vanishing Point / Filter Gallery when the +/// document they were opened on is no longer active, so they cannot steal undo, block layer +/// delete, or draw another document's pixels (#5115). +pub fn cancel_stale(app: &mut PhotocraftApp) { + let active = active_doc(app); + if app.distort.liquify.as_ref().is_some_and(|d| Some(d.document) != active) { + crate::liquify_ui::cancel(app); + } + if app.distort.puppet.as_ref().is_some_and(|p| Some(p.document) != active) { + app.distort.puppet = None; + } + if app.distort.perspective.as_ref().is_some_and(|p| Some(p.document) != active) { + app.distort.perspective = None; + } + if app.distort.vanishing.as_ref().is_some_and(|v| Some(v.document) != active) { + app.distort.vanishing = None; + } + if app.distort.gallery.as_ref().is_some_and(|g| Some(g.document) != active) { + app.distort.gallery = None; + } +} + /// The document shown on the canvas while Puppet or Perspective Warp previews a layer. pub fn display_doc(app: &PhotocraftApp, idx: usize) -> Option<(Arc, u64)> { if app.session.active_index() != Some(idx) { return None; } - if let Some(p) = &app.distort.puppet { + let id = app.session.documents().get(idx).map(|st| st.doc.id)?; + if let Some(p) = &app.distort.puppet + && p.document == id + { return Some((p.preview_doc.clone(), (1 << 42) + p.key)); } - if let Some(p) = &app.distort.perspective { + if let Some(p) = &app.distort.perspective + && p.document == id + { return Some((p.preview_doc.clone(), (1 << 42) + p.key)); } None @@ -137,23 +172,24 @@ pub fn menu(app: &mut PhotocraftApp, ctx: &egui::Context, id: &str, params: &Val /// Pointer events (document coordinates) while a mode is active. True when consumed. pub fn pointer(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui::Modifiers) -> bool { + cancel_stale(app); if app.distort.caf.is_some() { crate::caf_ui::pointer(app, ev, mods); return true; } - if app.distort.liquify.is_some() { + if app.distort.liquify.as_ref().is_some_and(|d| on_doc(app, d.document)) { crate::liquify_ui::pointer(app, ev, mods); return true; } - if app.distort.puppet.is_some() { + if app.distort.puppet.as_ref().is_some_and(|p| on_doc(app, p.document)) { crate::puppet_ui::pointer(app, ev, mods); return true; } - if app.distort.perspective.is_some() { + if app.distort.perspective.as_ref().is_some_and(|p| on_doc(app, p.document)) { crate::perspective_ui::pointer(app, ev, mods); return true; } - if app.distort.vanishing.is_some() { + if app.distort.vanishing.as_ref().is_some_and(|v| on_doc(app, v.document)) { crate::vp_ui::pointer(app, ev, mods); return true; } @@ -164,6 +200,7 @@ pub fn pointer(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui::Modifiers) -> /// of the rest of the app should be skipped. pub fn keys(app: &mut PhotocraftApp, ctx: &egui::Context) -> bool { use egui::{Key, Modifiers}; + cancel_stale(app); let enter = |ctx: &egui::Context| ctx.input_mut(|i| i.consume_key(Modifiers::NONE, Key::Enter)); let esc = |ctx: &egui::Context| ctx.input_mut(|i| i.consume_key(Modifiers::NONE, Key::Escape)); if app.distort.gallery.is_some() { @@ -245,19 +282,26 @@ pub fn draw_content(app: &PhotocraftApp, painter: &egui::Painter, xf: &ViewXform /// Canvas overlay for the on-canvas modes: pins, planes and meshes over the guides. pub fn draw_overlay(app: &PhotocraftApp, painter: &egui::Painter, xf: &ViewXform) { - if let Some(p) = &app.distort.puppet { + if let Some(p) = &app.distort.puppet + && on_doc(app, p.document) + { crate::puppet_ui::draw(p, painter, xf); } - if let Some(p) = &app.distort.perspective { + if let Some(p) = &app.distort.perspective + && on_doc(app, p.document) + { crate::perspective_ui::draw(p, painter, xf); } - if let Some(v) = &app.distort.vanishing { + if let Some(v) = &app.distort.vanishing + && on_doc(app, v.document) + { crate::vp_ui::draw(v, painter, xf); } } /// Options bar for the on-canvas modes. True when drawn. pub fn options_bar(app: &mut PhotocraftApp, ui: &mut egui::Ui) -> bool { + cancel_stale(app); if app.distort.puppet.is_some() { crate::puppet_ui::options_bar(app, ui); return true; @@ -295,6 +339,7 @@ pub fn drop_foreign(app: &mut PhotocraftApp) { /// Full-window dialogs (Liquify, Content-Aware Fill, Filter Gallery). pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) { + cancel_stale(app); drop_foreign(app); if app.distort.caf.is_some() { crate::caf_ui::show(app, ctx); @@ -312,3 +357,93 @@ pub(crate) fn tolerance(app: &PhotocraftApp) -> f64 { let z = app.point_zoom().max(0.01); 8.0 / f64::from(z) } + +#[cfg(test)] +mod tests { + use super::*; + use photocraft_geom::Rect; + use serde_json::json; + + fn paint_bar(app: &mut PhotocraftApp) { + app.session + .edit("paint", |doc, active| { + doc.layer_mut(active.unwrap()).unwrap().surface_mut().unwrap().fill_rect(Rect::new(20, 30, 80, 50), &[1.0, 0.0, 0.0, 1.0]); + Ok(()) + }) + .unwrap(); + } + + /// #5115: Puppet Warp is per-document. Switching tabs drops the session so Ctrl+Z and + /// layer delete apply to the new document, and the warp mesh does not leak onto it. + #[test] + fn switching_documents_drops_puppet_and_restores_undo_and_delete() { + let ctx = egui::Context::default(); + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.session.execute("file.new", json!({"width": 100, "height": 80, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + paint_bar(&mut app); + app.sync_views(); + menu(&mut app, &ctx, "edit.puppetWarp", &json!({})).unwrap().unwrap(); + pointer(&mut app, ToolEvent::Down { x: 50.0, y: 40.0, pressure: 1.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Up { x: 50.0, y: 40.0 }, egui::Modifiers::NONE); + assert!(app.distort.puppet.is_some()); + let first = app.session.active().unwrap().doc.id; + app.session.execute("file.new", json!({"width": 60, "height": 40, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + app.sync_views(); + assert!(app.distort.puppet.is_none(), "puppet does not follow the new document"); + assert!(display_doc(&app, app.session.active_index().unwrap()).is_none()); + let layers_before = app.session.active().unwrap().doc.layers.len(); + app.session.execute("layer.delete", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().doc.layers.len(), layers_before - 1); + assert!(app.session.set_active(0)); + app.sync_views(); + assert_eq!(app.session.active().unwrap().doc.id, first); + let past = app.session.active().unwrap().history.past_len(); + app.session.execute("edit.undo", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().history.past_len(), past - 1); + } + + #[test] + fn committed_puppet_is_one_undo_step_and_the_layer_stays_deletable() { + let ctx = egui::Context::default(); + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.session.execute("file.new", json!({"width": 100, "height": 80, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + paint_bar(&mut app); + app.sync_views(); + menu(&mut app, &ctx, "edit.puppetWarp", &json!({})).unwrap().unwrap(); + pointer(&mut app, ToolEvent::Down { x: 25.0, y: 40.0, pressure: 1.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Up { x: 25.0, y: 40.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Down { x: 75.0, y: 40.0, pressure: 1.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Move { x: 75.0, y: 30.0, pressure: 1.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Up { x: 75.0, y: 20.0 }, egui::Modifiers::NONE); + let before = app.session.active().unwrap().history.past_len(); + crate::puppet_ui::commit(&mut app); + assert!(app.distort.puppet.is_none()); + assert_eq!(app.session.active().unwrap().history.past_len(), before + 1); + app.session.execute("edit.undo", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().history.past_len(), before); + let layers = app.session.active().unwrap().doc.layers.len(); + app.session.execute("layer.delete", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().doc.layers.len(), layers - 1); + } + + #[test] + fn switching_documents_drops_liquify() { + let ctx = egui::Context::default(); + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.session.execute("file.new", json!({"width": 100, "height": 80, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + paint_bar(&mut app); + crate::liquify_ui::open(&mut app, &ctx).unwrap(); + assert!(app.distort.liquify.is_some()); + app.session.execute("file.new", json!({"width": 40, "height": 40, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + app.sync_views(); + assert!(app.distort.liquify.is_none()); + let past = app.session.active().unwrap().history.past_len(); + app.session.execute("edit.undo", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().history.past_len(), past - 1); + } +} diff --git a/crates/ui-egui/src/gallery_ui.rs b/crates/ui-egui/src/gallery_ui.rs index 89eac953e9..b5e50734f9 100644 --- a/crates/ui-egui/src/gallery_ui.rs +++ b/crates/ui-egui/src/gallery_ui.rs @@ -15,7 +15,7 @@ use std::hash::{Hash, Hasher}; use egui::{Align2, Color32, FontId, Rect as ERect, Sense, Stroke, TextureHandle, pos2, vec2}; use photocraft_algo::{FilterParams, GALLERY_CATEGORIES, GalleryFilter}; -use photocraft_doc::LayerId; +use photocraft_doc::{DocId, LayerId}; use photocraft_geom::Rect; use photocraft_raster::{Interrupt, Surface}; use serde_json::{Map, Value, json}; @@ -66,6 +66,8 @@ fn default_params(f: GalleryFilter) -> Map { } pub struct GalleryDialog { + /// Document this dialog belongs to (#5115). + pub document: DocId, pub layer: LayerId, layer_name: String, canvas: Rect, @@ -100,6 +102,7 @@ pub struct GalleryDialog { impl GalleryDialog { pub fn describe(&self) -> Value { json!({ + "document": self.document.0, "layer": self.layer.0, "effects": self.effects.iter().map(EffectLayer::to_json).collect::>(), "selected": self.selected, @@ -351,6 +354,7 @@ fn open_on( let open_cats = std::array::from_fn(|i| effects[sel].filter.category() == GALLERY_CATEGORIES[i]); let history = EditHistory::new(&effects); app.distort.gallery = Some(GalleryDialog { + document: st.doc.id, layer, layer_name, canvas, diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 1116f4c842..cf2b581436 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1499,6 +1499,7 @@ impl PhotocraftApp { // ... and to the document it was started on. crate::canvas::polygon_cancel_stale(self); crate::crop_ui::cancel_stale(self); + crate::distort_ui::cancel_stale(self); crate::perspective_crop_ui::cancel_stale(self); // A pen path under construction belongs to the document it was started on too. vector_ui::cancel_stale(self); @@ -3633,10 +3634,11 @@ mod clipboard_tests { app } + /// Paste is enabled from the clipboard service alone (no read); the explicit paste imports + /// the OS image, once, and centres it in the view like Photoshop does for foreign clipboard + /// content. #[test] fn external_clipboard_enables_and_centres_paste() { - // Paste is enabled from the clipboard service alone (no read); the explicit paste imports - // the OS image, once, and centres it like Photoshop does for foreign clipboard content. let (os, reads) = (OsClip::default(), Arc::default()); let mut app = app_with_os_clipboard(&os, &reads); *os.lock().unwrap() = Some((4, 4, [255u8, 0, 0, 255].repeat(16))); @@ -3644,8 +3646,9 @@ mod clipboard_tests { assert!(crate::menus::is_enabled(&app, "edit.paste")); assert!(crate::menus::is_enabled(&app, "edit.pasteSpecial.pasteInPlace")); assert_eq!(reads.load(std::sync::atomic::Ordering::SeqCst), 0, "enablement never reads the clipboard"); + app.ui.views[0].center = [40.0, 20.0]; let r = crate::menus::invoke(&mut app, &egui::Context::default(), "edit.paste", serde_json::json!({})).unwrap(); - assert_ne!(r["offset"], serde_json::json!([0, 0]), "external image is centred: {r}"); + assert_eq!(r["offset"], serde_json::json!([38, 18]), "external image is centred in the view: {r}"); assert_eq!(reads.load(std::sync::atomic::Ordering::SeqCst), 1, "one read per explicit paste"); // Without a clipboard service Paste greys until something is copied in the app. let mut plain = PhotocraftApp::new(Session::new(), Services::default()); diff --git a/crates/ui-egui/src/liquify_ui.rs b/crates/ui-egui/src/liquify_ui.rs index cf1c932518..43a792d3ff 100644 --- a/crates/ui-egui/src/liquify_ui.rs +++ b/crates/ui-egui/src/liquify_ui.rs @@ -10,7 +10,7 @@ use egui::{Align2, Color32, FontId, Pos2, Rect as ERect, Sense, Stroke, TextureHandle, pos2, vec2}; use photocraft_algo::liquify::{LiquifyField, LiquifyStroke, LiquifyTool, ProxyImage, auto_cell}; -use photocraft_doc::{Document, LayerId}; +use photocraft_doc::{DocId, Document, LayerId}; use photocraft_geom::Rect; use photocraft_raster::Surface; use serde_json::{Value, json}; @@ -153,6 +153,8 @@ impl LiquifyOpts { } pub struct LiquifyDialog { + /// Document this dialog belongs to (#5115). + pub document: DocId, pub layer: LayerId, layer_name: String, canvas: Rect, @@ -219,6 +221,7 @@ fn union_rect(a: Option, b: Rect) -> Option { impl LiquifyDialog { pub fn describe(&self) -> Value { json!({ + "document": self.document.0, "layer": self.layer.0, "tool": self.opts.tool, "size": self.opts.size, @@ -529,6 +532,7 @@ fn open_on( let out = vec![[0u8; 4]; proxy.w * proxy.h]; let center = [f64::from(canvas.x0 + canvas.x1) / 2.0, f64::from(canvas.y0 + canvas.y1) / 2.0]; let mut d = LiquifyDialog { + document: st.doc.id, layer, layer_name: name, canvas, diff --git a/crates/ui-egui/src/new_doc_ui.rs b/crates/ui-egui/src/new_doc_ui.rs index 563a9a5fca..0fb7b7b21f 100644 --- a/crates/ui-egui/src/new_doc_ui.rs +++ b/crates/ui-egui/src/new_doc_ui.rs @@ -583,6 +583,7 @@ pub fn body(app: &mut crate::PhotocraftApp, ui: &mut egui::Ui, f: &mut Map chosen == p.0, diff --git a/crates/ui-egui/src/panels.rs b/crates/ui-egui/src/panels.rs index 2d8dc5a8cd..b6bd34d9d2 100644 --- a/crates/ui-egui/src/panels.rs +++ b/crates/ui-egui/src/panels.rs @@ -4177,9 +4177,8 @@ pub(crate) fn color_field(app: &mut PhotocraftApp, ui: &mut egui::Ui) { }; ui.painter().add(color_grid(field, 16, |x, y| hsva_color32(at(x, y)))); ui.painter().rect_stroke(field, 0.0, Stroke::new(1.0, t.separator), StrokeKind::Outside); - if (fresp.dragged() || fresp.clicked()) - && let Some(p) = fresp.interact_pointer_pos() - { + let field_pick = widgets::pick_pos(&fresp); + if let Some(p) = field_pick { let x = ((p.x - field.left()) / field.width()).clamp(0.0, 1.0); let y = ((p.y - field.top()) / field.height()).clamp(0.0, 1.0); if brightness { @@ -4200,9 +4199,8 @@ pub(crate) fn color_field(app: &mut PhotocraftApp, ui: &mut egui::Ui) { if brightness { egui::ecolor::Hsva::new(hsva.h, hsva.s, 1.0 - f, 1.0) } else { egui::ecolor::Hsva::new(1.0 - f, 1.0, 1.0, 1.0) } }; ui.painter().add(color_grid(strip, 24, |_, y| hsva_color32(strip_at(y)))); - if (sresp.dragged() || sresp.clicked()) - && let Some(p) = sresp.interact_pointer_pos() - { + let strip_pick = widgets::pick_pos(&sresp); + if let Some(p) = strip_pick { let f = ((p.y - strip.top()) / strip.height()).clamp(0.0, 1.0); if brightness { hsva.v = 1.0 - f; @@ -4213,10 +4211,11 @@ pub(crate) fn color_field(app: &mut PhotocraftApp, ui: &mut egui::Ui) { let y = strip.top() + (1.0 - if brightness { hsva.v } else { hsva.h }) * strip.height(); let tri = vec![pos2(strip.right() + 1.0, y), pos2(strip.right() + 6.0, y - 4.0), pos2(strip.right() + 6.0, y + 4.0)]; ui.painter().add(egui::Shape::convex_polygon(tri, t.text, Stroke::NONE)); - if fresp.dragged() || fresp.clicked() || sresp.dragged() || sresp.clicked() { + if field_pick.is_some() || strip_pick.is_some() { if bg_active { app.session.tools.background = hsva_srgb(hsva); - } else { + } else if app.session.tools.foreground != hsva_srgb(hsva) { + // A held, unmoved press repeats the pick: restyle selected type only on a change. app.session.tools.foreground = hsva_srgb(hsva); crate::apply_color::foreground_changed(app, crate::apply_color::gesture(ui.ctx())); } @@ -4856,6 +4855,80 @@ mod color_tests { } } + /// #7793: the Color panel's field and hue strip pick on the frame the button goes down (no + /// release, no time passing) and follow moves smaller than egui's 6-point click threshold. + #[test] + fn color_panel_field_and_hue_strip_pick_on_press() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); + app.session.tools.foreground = [0.2, 0.4, 0.6, 1.0]; + let mut h = egui_kittest::Harness::builder().with_size(vec2(300.0, 300.0)).build_ui_state( + |ui, app: &mut PhotocraftApp| { + let at = (ui.cursor().min, ui.available_width()); + ui.data_mut(|d| d.insert_temp(egui::Id::new("test-origin"), at)); + color_field(app, ui); + }, + app, + ); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::Studio); + h.run_steps(2); + let (o, w) = h.ctx.data(|d| d.get_temp::<(Pos2, f32)>(egui::Id::new("test-origin"))).unwrap(); + let field_w = w - 38.0 - 14.0 - 16.0; + let press = |h: &mut egui_kittest::Harness<'static, PhotocraftApp>, at: Pos2, pressed: bool| { + h.event(egui::Event::PointerMoved(at)); + h.event(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed, modifiers: egui::Modifiers::NONE }); + h.run_steps(1); + }; + let at = o + vec2(38.0 + 8.0 + field_w * 0.5, 60.0); + press(&mut h, at, true); + let picked = h.state().session.tools.foreground; + assert_ne!(picked, [0.2, 0.4, 0.6, 1.0], "the press itself picks"); + let hsva = srgb_hsva(picked); + assert!((hsva.s - 0.5).abs() < 0.03 && (hsva.v - 0.5).abs() < 0.03, "{hsva:?}"); + h.event(egui::Event::PointerMoved(at + vec2(3.0, 0.0))); + h.run_steps(1); + assert_ne!(h.state().session.tools.foreground, picked, "a small drag adjusts the pick"); + press(&mut h, at + vec2(3.0, 0.0), false); + h.run_steps(1); + // Hue strip: a press a third of the way down is a hue of about 240° at once. + let before = h.state().session.tools.foreground; + press(&mut h, o + vec2(38.0 + 8.0 + field_w + 8.0 + 7.0, 40.0), true); + let now = h.state().session.tools.foreground; + assert_ne!(now, before, "the strip picks on press"); + assert!((srgb_hsva(now).h * 360.0 - 240.0).abs() < 8.0, "{:?}", srgb_hsva(now)); + } + + /// Only the primary button picks: a right or middle press (and its release) on the field or + /// the hue strip leaves the foreground colour alone. + #[test] + fn color_panel_ignores_secondary_and_middle_presses() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); + app.session.tools.foreground = [0.2, 0.4, 0.6, 1.0]; + let mut h = egui_kittest::Harness::builder().with_size(vec2(300.0, 300.0)).build_ui_state( + |ui, app: &mut PhotocraftApp| { + let at = (ui.cursor().min, ui.available_width()); + ui.data_mut(|d| d.insert_temp(egui::Id::new("test-origin"), at)); + color_field(app, ui); + }, + app, + ); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::Studio); + h.run_steps(2); + let (o, w) = h.ctx.data(|d| d.get_temp::<(Pos2, f32)>(egui::Id::new("test-origin"))).unwrap(); + let field_w = w - 38.0 - 14.0 - 16.0; + let field = o + vec2(38.0 + 8.0 + field_w * 0.5, 60.0); + let strip = o + vec2(38.0 + 8.0 + field_w + 8.0 + 7.0, 40.0); + for at in [field, strip] { + for button in [egui::PointerButton::Secondary, egui::PointerButton::Middle] { + for pressed in [true, false] { + h.event(egui::Event::PointerMoved(at)); + h.event(egui::Event::PointerButton { pos: at, button, pressed, modifiers: egui::Modifiers::NONE }); + h.run_steps(1); + assert_eq!(h.state().session.tools.foreground, [0.2, 0.4, 0.6, 1.0], "{button:?} pressed={pressed} at {at:?}"); + } + } + } + } + /// Click the hex field, select its text and type `text` one key per frame. fn type_hex(h: &mut egui_kittest::Harness<'static, PhotocraftApp>, text: &str) { use egui_kittest::kittest::Queryable; diff --git a/crates/ui-egui/src/perspective_crop_ui.rs b/crates/ui-egui/src/perspective_crop_ui.rs index 396fb87997..2a696bd5ed 100644 --- a/crates/ui-egui/src/perspective_crop_ui.rs +++ b/crates/ui-egui/src/perspective_crop_ui.rs @@ -352,6 +352,36 @@ mod tests { assert_eq!(ordered([[10.0, 90.0], [100.0, 5.0], [0.0, 0.0], [90.0, 100.0]]), [[0.0, 0.0], [100.0, 5.0], [90.0, 100.0], [10.0, 90.0]]); } + /// Clicking ⇄ in the rendered options bar swaps W and H, although Perspective Crop never sets + /// the Crop tool's W x H x Resolution mode (#7059). + #[test] + fn the_options_bar_swap_button_swaps_w_and_h() { + use egui_kittest::kittest::Queryable; + let doc = Document::with_background("p", Size::new(200, 150), ColorMode::Rgb, SampleType::U8, Color::WHITE); + let mut s = photocraft_engine::Session::new(); + s.add_document(doc, None); + let mut app = PhotocraftApp::new(s, crate::Services::default()); + app.ui.tool = Tool::PerspectiveCrop; + app.ui.tool_options.crop_width = "4 in".into(); + app.ui.tool_options.crop_height = "6 in".into(); + assert_ne!(app.ui.tool_options.crop_ratio, crate::crop_size::WHR); + let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1400.0, 60.0)).build_ui_state( + |ui, app: &mut PhotocraftApp| { + if !ui.ctx().fonts(|f| f.families().contains(&egui::FontFamily::Name("medium".into()))) { + return; + } + crate::panels::options_bar(app, ui); + }, + app, + ); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ProMedium); + h.run_steps(4); + h.get_by_label("Swaps height and width").click(); + h.run_steps(2); + let o = &h.state().ui.tool_options; + assert_eq!((o.crop_width.as_str(), o.crop_height.as_str()), ("6 in", "4 in")); + } + /// The pending quad belongs to its document (#1918): switching documents drops it, so it is /// never drawn on the other one; on its own document it stays. #[test] diff --git a/crates/ui-egui/src/perspective_ui.rs b/crates/ui-egui/src/perspective_ui.rs index e39a791d4f..b383184975 100644 --- a/crates/ui-egui/src/perspective_ui.rs +++ b/crates/ui-egui/src/perspective_ui.rs @@ -11,7 +11,7 @@ use std::sync::Arc; use egui::{Color32, Pos2, Stroke, TextureHandle, pos2}; use photocraft_algo::perspective::{PerspectiveMap, Plane, Straighten, linked_corners, plane_point, straighten}; -use photocraft_doc::{Document, LayerId}; +use photocraft_doc::{DocId, Document, LayerId}; use photocraft_geom::Rect; use serde_json::{Value, json}; @@ -39,6 +39,8 @@ enum Drag { pub struct PerspSession { pub key: u64, + /// Document this session belongs to (#5115). + pub document: DocId, pub layer: LayerId, command: String, bounds: Rect, @@ -56,7 +58,7 @@ const GRID: usize = 40; impl PerspSession { pub fn describe(&self) -> Value { - json!({"layer": self.layer.0, "mode": self.mode, "planes": self.planes}) + json!({"document": self.document.0, "layer": self.layer.0, "mode": self.mode, "planes": self.planes}) } fn update_grid(&mut self) { @@ -134,6 +136,7 @@ pub fn begin(app: &mut PhotocraftApp, ctx: &egui::Context, command: &str) -> Res let texture = ctx.load_texture(format!("persp-{key}"), crate::distort_ui::surface_image(&surface, bounds, 2048), egui::TextureOptions::LINEAR); let mut s = PerspSession { key, + document: st.doc.id, layer, command: command.to_string(), bounds, diff --git a/crates/ui-egui/src/puppet_ui.rs b/crates/ui-egui/src/puppet_ui.rs index 3045ca1ca6..831c8bad37 100644 --- a/crates/ui-egui/src/puppet_ui.rs +++ b/crates/ui-egui/src/puppet_ui.rs @@ -13,7 +13,7 @@ use std::sync::Arc; use egui::{Color32, Stroke, TextureHandle, pos2, vec2}; use photocraft_algo::puppet::{ITERATIONS, PuppetDensity, PuppetMesh, PuppetMode, PuppetPin, PuppetSolver, PuppetWarp, build_mesh}; -use photocraft_doc::{Document, LayerId}; +use photocraft_doc::{DocId, Document, LayerId}; use photocraft_geom::Rect; use photocraft_raster::Surface; use serde_json::{Value, json}; @@ -23,6 +23,8 @@ use crate::canvas::{ToolEvent, ViewXform}; pub struct PuppetSession { pub key: u64, + /// Document this session belongs to (#5115). + pub document: DocId, pub layer: LayerId, /// Command to commit with (`edit.puppetWarp` or `layer.smartObjects.puppetWarp`). command: String, @@ -98,6 +100,7 @@ impl PuppetSession { pub fn describe(&self) -> Value { json!({ + "document": self.document.0, "layer": self.layer.0, "pins": self.warp.pins, "mode": self.warp.mode, @@ -202,6 +205,7 @@ pub fn begin(app: &mut PhotocraftApp, ctx: &egui::Context, command: &str) -> Res let texture = ctx.load_texture(format!("puppet-{key}"), img, egui::TextureOptions::LINEAR); let mut s = PuppetSession { key, + document: st.doc.id, layer, command: command.to_string(), surface, @@ -760,10 +764,13 @@ mod tests { crate::distort_ui::drop_foreign(&mut app); assert!(app.distort.puppet.is_some()); app.session.execute("file.new", json!({"width": 32, "height": 32, "depth": 8})).unwrap(); - assert!(crate::distort_ui::display_doc(&app, 1).is_some()); + let active = app.session.active_index().unwrap(); + // The session stays until drop_foreign; the preview must not follow this document (#5115). + assert!(app.distort.puppet.is_some()); + assert!(crate::distort_ui::display_doc(&app, active).is_none()); crate::distort_ui::drop_foreign(&mut app); assert!(app.distort.puppet.is_none()); - assert!(crate::distort_ui::display_doc(&app, 1).is_none()); + assert!(crate::distort_ui::display_doc(&app, active).is_none()); // Closing the document it was started on drops it too. assert!(app.session.set_active(0)); crate::distort_ui::menu(&mut app, &ctx, "edit.puppetWarp", &json!({})).unwrap().unwrap(); diff --git a/crates/ui-egui/src/shortcuts.rs b/crates/ui-egui/src/shortcuts.rs index 3300b9d92e..023bbb4d79 100644 --- a/crates/ui-egui/src/shortcuts.rs +++ b/crates/ui-egui/src/shortcuts.rs @@ -459,6 +459,7 @@ pub fn handle(app: &mut PhotocraftApp, ctx: &egui::Context) { // Liquify is a full-window custom dialog with focusable sliders. egui can therefore claim // keyboard input before the distortion-mode handler below runs. Give Liquify's local // shortcuts first refusal (Undo, brush size, tool keys), but never steal keys from text edits. + crate::distort_ui::cancel_stale(app); if app.distort.liquify.is_some() { // Undo and redo still apply while a numeric field (Size, Pressure, …) has focus. // Other Liquify keys must not steal characters from that field (#4027). diff --git a/crates/ui-egui/src/vp_ui.rs b/crates/ui-egui/src/vp_ui.rs index 3034c1c5f2..61ad0a4425 100644 --- a/crates/ui-egui/src/vp_ui.rs +++ b/crates/ui-egui/src/vp_ui.rs @@ -12,7 +12,7 @@ use egui::{Color32, Pos2, Stroke, TextureHandle, pos2}; use photocraft_algo::transform::Homography; use photocraft_algo::vanishing::{PlaneCheck, Scene, VpPlane}; -use photocraft_doc::{LayerContent, LayerId}; +use photocraft_doc::{DocId, LayerContent, LayerId}; use photocraft_engine::vp_cmds::VP; use photocraft_geom::Rect; use serde_json::{Value, json}; @@ -46,6 +46,8 @@ struct Clip { } pub struct VpSession { + /// Document this session belongs to (#5115). + pub document: DocId, pub layer: LayerId, canvas: Rect, pub planes: Vec, @@ -80,6 +82,7 @@ impl VpSession { let center = self.center(); let planes: Vec = self.planes.iter().map(|p| json!({"corners": p.corners, "check": p.check(center)})).collect(); json!({ + "document": self.document.0, "layer": self.layer.0, "tool": self.tool, "planes": planes, @@ -310,6 +313,7 @@ pub fn begin(app: &mut PhotocraftApp, ctx: &egui::Context) -> Result<(), String> }) }); app.distort.vanishing = Some(VpSession { + document: st.doc.id, layer, canvas, planes: Vec::new(), diff --git a/crates/ui-egui/src/widgets.rs b/crates/ui-egui/src/widgets.rs index ad5cba2105..1210c6c804 100644 --- a/crates/ui-egui/src/widgets.rs +++ b/crates/ui-egui/src/widgets.rs @@ -3001,6 +3001,16 @@ pub(crate) fn color_swatch(ui: &mut Ui, color: Color32) -> Response { ui.painter().rect_stroke(rect, t.radius_sm, Stroke::new(1.0, t.field_border), StrokeKind::Inside); response } +/// Where a press or drag on a picking surface (a colour field, a hue strip) points, from the +/// frame the button goes down and on every move after it. `dragged()` waits for egui's click +/// threshold (a few points of travel, or a long hold), so a press picked nothing until the +/// pointer had moved away and fine adjustments jumped (#7793). Only the primary button picks: +/// `is_pointer_button_down_on` is true for any button, so a right or middle press is ignored. +pub fn pick_pos(resp: &Response) -> Option { + let held = resp.is_pointer_button_down_on() + && resp.ctx.input(|i| i.pointer.primary_down() && !i.pointer.button_down(egui::PointerButton::Secondary) && !i.pointer.middle_down()); + if held || resp.clicked() { resp.interact_pointer_pos() } else { None } +} /// A colour swatch that only reports clicks (no popup): for colours edited in PhotoCraft's own /// Color Picker dialog. `label` names it for hover text and accessibility. pub fn color_swatch_button(ui: &mut Ui, color: Color32, label: &str) -> Response { diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 6d8b00a805..77edb5e4d0 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -376,6 +376,11 @@ capability interface. Interactive desktop file pickers retain normal user-select ## Headless server +An unused TCP headless server exits after 30 minutes without a client connection, releasing +its listening port so a later session can reuse it (#3682). An active connection prevents +idle shutdown, including during a long command. Clients should reconnect by restarting +`photocraft-cli serve` if they need a server after the idle deadline. + `photocraft-cli serve` keeps one headless engine session (no window, no GPU) and answers the same JSON-lines envelope on stdio, or on `127.0.0.1:` with `--port ` (loopback only; each authenticated connection shares the session). TCP uses the same first-frame `auth` exchange and