From 324489a66fccb8e20dd09727856854dc4bae962f Mon Sep 17 00:00:00 2001 From: Hector Villarreal Date: Sat, 7 Nov 2026 16:06:44 -0600 Subject: [PATCH 1/3] Notes: default colour 255,255,131, popup at icon + (8,10) 240x140, local-time date; the popup keeps its size under transforms (#11795) --- crates/engine/src/canvas_geom.rs | 7 +- crates/engine/src/cmd.rs | 2 + crates/engine/src/cmd/notes_cmds.rs | 42 ++++++--- .../src/cmd/notes_stored_values_tests.rs | 87 +++++++++++++++++++ crates/ui-egui/src/tools/analysis_ui.rs | 2 +- 5 files changed, 128 insertions(+), 12 deletions(-) create mode 100644 crates/engine/src/cmd/notes_stored_values_tests.rs diff --git a/crates/engine/src/canvas_geom.rs b/crates/engine/src/canvas_geom.rs index 93bc4654b9..38cddc6ef5 100644 --- a/crates/engine/src/canvas_geom.rs +++ b/crates/engine/src/canvas_geom.rs @@ -242,9 +242,14 @@ pub(crate) fn transform_doc_marks(doc: &mut Document, a: &Affine) { if let Some(wp) = &mut doc.work_path { *wp = wp.transform(a); } + // A note's icon point maps with the pixels; its popup moves with it and keeps its size and its + // offset from the icon, as Photoshop's does under Image Size, Canvas Size and turns (#11795). for n in &mut doc.notes { + let [px, py] = n.position; + let (dx, dy, w, h) = (n.popup[0] - px, n.popup[1] - py, n.popup[2] - n.popup[0], n.popup[3] - n.popup[1]); n.position = map_pt(a, n.position); - n.popup = map_box(a, n.popup); + let [x, y] = n.position; + n.popup = [x + dx, y + dy, x + dx + w, y + dy + h]; } for g in &mut doc.measurement.count_groups { for p in &mut g.points { diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index c4addf4c22..6a741c521c 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -114,6 +114,8 @@ pub(super) mod migrate_cmds; pub mod mode_cmds; pub mod multichannel_cmds; pub mod notes_cmds; +#[cfg(test)] +mod notes_stored_values_tests; mod ops; pub mod paint_cmds; mod params; diff --git a/crates/engine/src/cmd/notes_cmds.rs b/crates/engine/src/cmd/notes_cmds.rs index 2f589f3390..133417649d 100644 --- a/crates/engine/src/cmd/notes_cmds.rs +++ b/crates/engine/src/cmd/notes_cmds.rs @@ -22,14 +22,33 @@ fn has_notes(s: &Session) -> std::result::Result<(), String> { if s.active().is_some_and(|d| !d.doc.notes.is_empty()) { Ok(()) } else { Err("the document has no notes".into()) } } -/// PDF-style date (`D:YYYYMMDDhhmmssZ`) as Photoshop stores in notes; empty without a clock. +/// Photoshop's default note colour (#11795): RGB 255, 255, 131 (stored as 16-bit 0x8383 blue). +pub const DEFAULT_NOTE_COLOR: [f32; 3] = [1.0, 1.0, 131.0 / 255.0]; + +/// The PDF date (`D:YYYYMMDDHHMMSS+HH'mm'`) Photoshop stores in a note: local time with its +/// offset from UTC (#11795), through chrono. Empty without a clock (the web build). fn pdf_date() -> String { - let iso = crate::analysis_cmds::now_iso(); - if iso.len() < 19 { - return String::new(); + #[cfg(not(target_arch = "wasm32"))] + { + let now = chrono::Local::now(); + pdf_date_at(&now.with_timezone(now.offset())) } - let digits: String = iso[..19].chars().filter(char::is_ascii_digit).collect(); - format!("D:{digits}Z") + #[cfg(target_arch = "wasm32")] + { + String::new() + } +} + +/// [`pdf_date`] for the moment `at`: its own wall-clock time and UTC offset. +#[cfg_attr(target_arch = "wasm32", allow(dead_code))] +pub(crate) fn pdf_date_at(at: &chrono::DateTime) -> String +where + Tz::Offset: std::fmt::Display, +{ + use chrono::Offset; + let off = at.offset().fix().local_minus_utc(); + let (sign, off) = if off < 0 { ('-', -off) } else { ('+', off) }; + format!("D:{}{sign}{:02}'{:02}'", at.format("%Y%m%d%H%M%S"), off / 3600, off % 3600 / 60) } pub fn note_json(i: usize, n: &Note) -> Value { @@ -44,7 +63,8 @@ fn color_of(cmd: &str, p: &Value) -> Result> { if p.get("color").is_none() { return Ok(None); } - let c = crate::commands::color_param(cmd, p, "color", [1.0, 1.0, 0.51, 1.0])?; + let [r, g, b] = DEFAULT_NOTE_COLOR; + let c = crate::commands::color_param(cmd, p, "color", [r, g, b, 1.0])?; Ok(Some(Color::rgb(c[0], c[1], c[2]))) } @@ -65,10 +85,11 @@ fn add(s: &mut Session, p: &Value) -> Result { author: p.get("author").and_then(Value::as_str).unwrap_or("").to_string(), text: p.get("text").and_then(Value::as_str).unwrap_or("").to_string(), position: [x, y], - popup: [x + 20.0, y + 20.0, x + 260.0, y + 160.0], + // Photoshop's popup: 240 × 140, at the icon's top-left + (8, 10) (#11795). + popup: [x + 8.0, y + 10.0, x + 248.0, y + 150.0], + color: Color::rgb(DEFAULT_NOTE_COLOR[0], DEFAULT_NOTE_COLOR[1], DEFAULT_NOTE_COLOR[2]), open: p.get("open").and_then(Value::as_bool).unwrap_or(true), modified: pdf_date(), - ..Default::default() }; if let Some(c) = color_of(CMD, p)? { n.color = c; @@ -270,7 +291,8 @@ mod tests { let d = s.active().unwrap().doc.clone(); assert_eq!(d.notes[0].text, "fixed"); assert_eq!(d.notes[0].position, [20.0, 12.0]); - assert_eq!(d.notes[0].popup[0], 40.0); + // The popup travels with its icon: 20 + 8 (#11795). + assert_eq!(d.notes[0].popup[0], 28.0); // Unchanged edits don't add history. let before = s.active().unwrap().history.past_len(); assert_eq!(s.execute("notes.set", json!({"index": 0, "text": "fixed"})).unwrap()["changed"], false); diff --git a/crates/engine/src/cmd/notes_stored_values_tests.rs b/crates/engine/src/cmd/notes_stored_values_tests.rs new file mode 100644 index 0000000000..b0920139d6 --- /dev/null +++ b/crates/engine/src/cmd/notes_stored_values_tests.rs @@ -0,0 +1,87 @@ +//! #11795: what a new note stores, as in Photoshop: colour 255, 255, 131, a 240 × 140 popup at the +//! icon's top-left + (8, 10), and a local-time PDF date with its offset. Document operations move +//! the popup with its icon and keep its size. + +use chrono::{FixedOffset, TimeZone}; +use serde_json::json; + +use super::notes_cmds::{DEFAULT_NOTE_COLOR, pdf_date_at}; +use crate::Session; + +fn session(w: u32, h: u32) -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": w, "height": h, "background": "white"})).unwrap(); + s +} + +fn note(s: &Session, i: usize) -> photocraft_doc::Note { + s.active().unwrap().doc.notes[i].clone() +} + +#[test] +fn a_new_note_has_the_default_colour_and_popup() { + let mut s = session(600, 400); + s.execute("notes.add", json!({"x": 100, "y": 100})).unwrap(); + let n = note(&s, 0); + let rgb = n.color.to_rgb(); + assert_eq!(rgb.map(|v| (v * 255.0).round() as i32), [255, 255, 131], "{rgb:?}"); + assert_eq!(DEFAULT_NOTE_COLOR.map(|v| (v * 65535.0).round() as u32), [0xffff, 0xffff, 0x8383], "the 16-bit components Photoshop saves"); + assert_eq!(n.popup, [108.0, 110.0, 348.0, 250.0], "icon + (8, 10), 240 × 140"); + // A note on the pasteboard keeps its popup there too. + s.execute("notes.add", json!({"x": -178, "y": 300})).unwrap(); + assert_eq!(note(&s, 1).popup, [-170.0, 310.0, 70.0, 450.0]); + // An explicit colour still wins. + s.execute("notes.add", json!({"x": 10, "y": 10, "color": [0.25, 0.5, 1.0]})).unwrap(); + assert_eq!(note(&s, 2).color.to_rgb(), [0.25, 0.5, 1.0]); +} + +#[test] +fn the_date_is_local_time_with_its_offset() { + let at = |offset: i32| FixedOffset::east_opt(offset).unwrap().with_ymd_and_hms(2026, 11, 6, 20, 14, 55).unwrap(); + assert_eq!(pdf_date_at(&at(3600)), "D:20261106201455+01'00'"); + assert_eq!(pdf_date_at(&at(0)), "D:20261106201455+00'00'"); + assert_eq!(pdf_date_at(&at(-(5 * 3600 + 30 * 60))), "D:20261106201455-05'30'"); + // A new note's own date has that shape. + let mut s = session(100, 100); + s.execute("notes.add", json!({"x": 10, "y": 10})).unwrap(); + let d = note(&s, 0).modified; + let b = d.as_bytes(); + assert!(d.len() == 23 && d.starts_with("D:") && matches!(b[16], b'+' | b'-') && b[19] == b'\'' && b[22] == b'\'', "{d}"); + assert!(d[2..16].chars().all(|c| c.is_ascii_digit()), "{d}"); +} + +#[test] +fn image_size_moves_the_icon_and_keeps_the_popup_size() { + let mut s = session(600, 400); + s.execute("notes.add", json!({"x": 100, "y": 99})).unwrap(); + let before = note(&s, 0); + s.execute("image.imageSize", json!({"width": 300, "height": 200})).unwrap(); + let n = note(&s, 0); + assert_eq!(n.position, [50.0, 49.5], "the icon point scales as today"); + let size = |p: [f64; 4]| [p[2] - p[0], p[3] - p[1]]; + assert_eq!(size(n.popup), size(before.popup), "240 × 140 still"); + assert_eq!([n.popup[0] - n.position[0], n.popup[1] - n.position[1]], [8.0, 10.0], "still at icon + (8, 10)"); +} + +#[test] +fn canvas_size_and_a_quarter_turn_keep_the_popup_size() { + let mut s = session(600, 400); + s.execute("notes.add", json!({"x": 100, "y": 100})).unwrap(); + s.execute("image.canvasSize", json!({"width": 400, "height": 300, "anchor": "center"})).unwrap(); + let n = note(&s, 0); + assert_eq!(n.position, [0.0, 50.0], "moved by the canvas offset"); + assert_eq!(n.popup, [8.0, 60.0, 248.0, 200.0]); + s.execute("image.imageRotation.90cw", json!({})).unwrap(); + let r = note(&s, 0); + assert_eq!([r.popup[2] - r.popup[0], r.popup[3] - r.popup[1]], [240.0, 140.0], "a turn doesn't swap the popup's sides"); + assert_eq!([r.popup[0] - r.position[0], r.popup[1] - r.position[1]], [8.0, 10.0], "the popup moves with its icon"); +} + +#[test] +fn a_mode_change_leaves_notes_alone() { + let mut s = session(100, 100); + s.execute("notes.add", json!({"x": 10, "y": 20})).unwrap(); + let before = note(&s, 0); + s.execute("image.mode.cmyk", json!({})).unwrap(); + assert_eq!(note(&s, 0), before); +} diff --git a/crates/ui-egui/src/tools/analysis_ui.rs b/crates/ui-egui/src/tools/analysis_ui.rs index ed113909a6..a0c1feecde 100644 --- a/crates/ui-egui/src/tools/analysis_ui.rs +++ b/crates/ui-egui/src/tools/analysis_ui.rs @@ -64,7 +64,7 @@ impl Default for AnalysisUi { log_selected: Vec::new(), note_selected: None, note_author: String::new(), - note_color: [1.0, 1.0, 0.51], + note_color: photocraft_engine::notes_cmds::DEFAULT_NOTE_COLOR, use_measurement_scale: false, dialog: None, drag: None, From b7e71b9ed525a1d2eee68a0a5407ee8744d4f275 Mon Sep 17 00:00:00 2001 From: Hector Villarreal Date: Sat, 7 Nov 2026 18:23:00 -0600 Subject: [PATCH 2/3] Notes: an unset (all-zero) popup stays unset under Image Size, Canvas Size and turns (CodeRabbit review on crates/engine/src/canvas_geom.rs:252) PSD export writes its default popup for an all-zero one; moving it with the icon turned it into a 0 x 0 rectangle elsewhere, which export then wrote as is. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/canvas_geom.rs | 5 +++++ crates/engine/src/cmd/notes_stored_values_tests.rs | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/crates/engine/src/canvas_geom.rs b/crates/engine/src/canvas_geom.rs index 38cddc6ef5..43fecf84de 100644 --- a/crates/engine/src/canvas_geom.rs +++ b/crates/engine/src/canvas_geom.rs @@ -245,6 +245,11 @@ pub(crate) fn transform_doc_marks(doc: &mut Document, a: &Affine) { // A note's icon point maps with the pixels; its popup moves with it and keeps its size and its // offset from the icon, as Photoshop's does under Image Size, Canvas Size and turns (#11795). for n in &mut doc.notes { + // An all-zero popup means none is stored (PSD export writes its default one): keep it. + if n.popup == [0.0; 4] { + n.position = map_pt(a, n.position); + continue; + } let [px, py] = n.position; let (dx, dy, w, h) = (n.popup[0] - px, n.popup[1] - py, n.popup[2] - n.popup[0], n.popup[3] - n.popup[1]); n.position = map_pt(a, n.position); diff --git a/crates/engine/src/cmd/notes_stored_values_tests.rs b/crates/engine/src/cmd/notes_stored_values_tests.rs index b0920139d6..cb1897f121 100644 --- a/crates/engine/src/cmd/notes_stored_values_tests.rs +++ b/crates/engine/src/cmd/notes_stored_values_tests.rs @@ -85,3 +85,16 @@ fn a_mode_change_leaves_notes_alone() { s.execute("image.mode.cmyk", json!({})).unwrap(); assert_eq!(note(&s, 0), before); } + +#[test] +fn an_unset_popup_stays_unset_under_image_size() { + // An all-zero popup means "none stored": PSD export writes its default popup for it, so Image + // Size must not turn it into a 0 × 0 rectangle somewhere else. + let mut s = session(600, 400); + let doc = std::sync::Arc::make_mut(&mut s.active_mut().unwrap().doc); + doc.notes.push(photocraft_doc::Note { position: [100.0, 100.0], ..Default::default() }); + s.execute("image.imageSize", json!({"width": 300, "height": 200})).unwrap(); + let n = note(&s, 0); + assert_eq!(n.position, [50.0, 50.0]); + assert_eq!(n.popup, [0.0; 4]); +} From 4a8e26a11a79843c31cb4cf31b5931e4e5d75cb4 Mon Sep 17 00:00:00 2001 From: Hector Villarreal Date: Sat, 7 Nov 2026 18:25:41 -0600 Subject: [PATCH 3/3] Notes: after a turn or flip the popup keeps today's origin, with its size kept (CodeRabbit review on crates/engine/src/canvas_geom.rs:252) #11795 didn't measure where the popup sits after a rotation, so that case keeps today's placement (the top-left of the mapped rectangle) and only the size rule applies. Image Size and Canvas Size still keep the popup's offset from the icon. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/canvas_geom.rs | 16 ++++++++++++---- .../src/cmd/notes_stored_values_tests.rs | 18 +++++++++++++++++- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/canvas_geom.rs b/crates/engine/src/canvas_geom.rs index 43fecf84de..ff40ea8a8b 100644 --- a/crates/engine/src/canvas_geom.rs +++ b/crates/engine/src/canvas_geom.rs @@ -242,8 +242,10 @@ pub(crate) fn transform_doc_marks(doc: &mut Document, a: &Affine) { if let Some(wp) = &mut doc.work_path { *wp = wp.transform(a); } - // A note's icon point maps with the pixels; its popup moves with it and keeps its size and its - // offset from the icon, as Photoshop's does under Image Size, Canvas Size and turns (#11795). + // A note's icon point maps with the pixels; its popup keeps its size (#11795). Under Image Size + // and Canvas Size it also keeps its offset from the icon, as Photoshop's does. Where it sits + // after a turn or flip wasn't measured, so it keeps today's origin there: the top-left of the + // mapped rectangle. for n in &mut doc.notes { // An all-zero popup means none is stored (PSD export writes its default one): keep it. if n.popup == [0.0; 4] { @@ -253,8 +255,14 @@ pub(crate) fn transform_doc_marks(doc: &mut Document, a: &Affine) { let [px, py] = n.position; let (dx, dy, w, h) = (n.popup[0] - px, n.popup[1] - py, n.popup[2] - n.popup[0], n.popup[3] - n.popup[1]); n.position = map_pt(a, n.position); - let [x, y] = n.position; - n.popup = [x + dx, y + dy, x + dx + w, y + dy + h]; + let [m0, m1, m2, m3, ..] = a.m; + let [x, y] = if m1.abs() < EPS && m2.abs() < EPS && m0 > 0.0 && m3 > 0.0 { + [n.position[0] + dx, n.position[1] + dy] + } else { + let [l, t, ..] = map_box(a, n.popup); + [l, t] + }; + n.popup = [x, y, x + w, y + h]; } for g in &mut doc.measurement.count_groups { for p in &mut g.points { diff --git a/crates/engine/src/cmd/notes_stored_values_tests.rs b/crates/engine/src/cmd/notes_stored_values_tests.rs index cb1897f121..1db8e5681b 100644 --- a/crates/engine/src/cmd/notes_stored_values_tests.rs +++ b/crates/engine/src/cmd/notes_stored_values_tests.rs @@ -74,7 +74,6 @@ fn canvas_size_and_a_quarter_turn_keep_the_popup_size() { s.execute("image.imageRotation.90cw", json!({})).unwrap(); let r = note(&s, 0); assert_eq!([r.popup[2] - r.popup[0], r.popup[3] - r.popup[1]], [240.0, 140.0], "a turn doesn't swap the popup's sides"); - assert_eq!([r.popup[0] - r.position[0], r.popup[1] - r.position[1]], [8.0, 10.0], "the popup moves with its icon"); } #[test] @@ -86,6 +85,23 @@ fn a_mode_change_leaves_notes_alone() { assert_eq!(note(&s, 0), before); } +#[test] +fn a_turn_keeps_todays_popup_origin_and_the_popup_size() { + // Where the popup sits after a turn wasn't measured (#11795), so it keeps today's origin (the + // top-left of the turned rectangle) and only the size rule applies. + let mut s = session(600, 400); + s.execute("notes.add", json!({"x": 100, "y": 100})).unwrap(); + // The popup [108, 110, 348, 250] turned 90° clockwise on a 400 px tall canvas spans x 150..290, + // y 108..348; it starts there and stays 240 × 140. + s.execute("image.imageRotation.90cw", json!({})).unwrap(); + assert_eq!(note(&s, 0).popup, [150.0, 108.0, 390.0, 248.0]); + // A flip keeps today's mirrored popup, which already has its size. + let mut s = session(600, 400); + s.execute("notes.add", json!({"x": 100, "y": 100})).unwrap(); + s.execute("image.imageRotation.flipCanvasHorizontal", json!({})).unwrap(); + assert_eq!(note(&s, 0).popup, [252.0, 110.0, 492.0, 250.0]); +} + #[test] fn an_unset_popup_stays_unset_under_image_size() { // An all-zero popup means "none stored": PSD export writes its default popup for it, so Image