diff --git a/crates/app/src/workspace/chrome.rs b/crates/app/src/workspace/chrome.rs index eaf24b37..737bb46c 100644 --- a/crates/app/src/workspace/chrome.rs +++ b/crates/app/src/workspace/chrome.rs @@ -205,6 +205,7 @@ impl Workspace { /// Jump in history: negative = undo n steps, positive = redo n steps. pub fn history_jump(&mut self, steps: i32, cx: &mut Context) { + let profile_before = self.doc.as_ref().and_then(|doc| doc.icc_profile.clone()); if let Some(doc) = &mut self.doc { if steps < 0 { for _ in 0..(-steps) { @@ -220,6 +221,9 @@ impl Workspace { } } } + if self.doc.as_ref().and_then(|doc| doc.icc_profile.clone()) != profile_before { + self.rebuild_color_transforms(); + } self.after_change(cx); } diff --git a/crates/app/src/workspace/colormgmt.rs b/crates/app/src/workspace/colormgmt.rs index c6496906..f5d86007 100644 --- a/crates/app/src/workspace/colormgmt.rs +++ b/crates/app/src/workspace/colormgmt.rs @@ -66,8 +66,9 @@ impl Workspace { /// Assign a profile: same numbers, new interpretation. pub fn assign_profile(&mut self, profile: schist_colormgmt::Profile, cx: &mut Context) { if let Some(doc) = self.doc.as_mut() { - doc.icc_profile = profile.icc_bytes().map(|b| b.to_vec()); - doc.dirty = true; + let mut edit = doc.begin_edit(format!("Assign {}", profile.name())); + edit.set_icc_profile(profile.icc_bytes().map(|b| b.to_vec())); + edit.commit(); doc.damage_all(); } self.status = format!("Assigned {}", profile.name()).into(); @@ -112,8 +113,8 @@ impl Workspace { tile.encode_f32(&buf); } } + edit.set_icc_profile(profile.icc_bytes().map(|b| b.to_vec())); edit.commit(); - doc.icc_profile = profile.icc_bytes().map(|b| b.to_vec()); self.status = format!("Converted to {}", profile.name()).into(); self.rebuild_color_transforms(); self.after_change(cx); @@ -121,10 +122,13 @@ impl Workspace { /// Toggle soft proofing against a device profile. pub fn toggle_proof(&mut self, profile: schist_colormgmt::Profile, cx: &mut Context) { - self.color.proof = match &self.color.proof { - Some(_) => None, - None => Some(profile), - }; + // Picking a different proof profile while one is active switches to + // it; picking the active profile turns proofing off. Names are not + // identities: loaded device profiles use a generic display name. + let already_proofing_this = self.color.proof.as_ref().is_some_and(|current| { + current.icc_bytes() == profile.icc_bytes() && current.name() == profile.name() + }); + self.color.proof = (!already_proofing_this).then_some(profile); self.status = if self.color.proof.is_some() { "Proof colors on".into() } else { diff --git a/crates/app/src/workspace/commands.rs b/crates/app/src/workspace/commands.rs index e022bfe3..64026c5b 100644 --- a/crates/app/src/workspace/commands.rs +++ b/crates/app/src/workspace/commands.rs @@ -59,6 +59,7 @@ impl Workspace { if id.starts_with("edit.paste") { self.sync_clipboard_in(cx); } + let profile_before = self.doc.as_ref().and_then(|doc| doc.icc_profile.clone()); let Some(doc) = self.doc.as_mut() else { return }; if let Some(command) = self.registry.command(id) { let mut ctx = CommandCtx { @@ -80,6 +81,12 @@ impl Workspace { } else { log::warn!("unknown command {id}"); } + // Undo and redo are plugin commands, so a profile restored by + // their history operation must rebuild the cached display hop. + // Damage alone only drops pixels rendered through that cache. + if self.doc.as_ref().and_then(|doc| doc.icc_profile.clone()) != profile_before { + self.rebuild_color_transforms(); + } self.after_change(cx); } diff --git a/crates/codec-psd/src/writer/mod.rs b/crates/codec-psd/src/writer/mod.rs index e3e15edb..a5154096 100644 --- a/crates/codec-psd/src/writer/mod.rs +++ b/crates/codec-psd/src/writer/mod.rs @@ -139,7 +139,26 @@ fn write_image_resources(b: &mut Buf, doc: &Document) { continue; // written as its own section above } seen_resolution |= res.id == RES_RESOLUTION_INFO; - seen_icc |= res.id == RES_ICC_PROFILE; + // `doc.icc_profile` is the authority when it is set. Convert and + // Assign Profile rewrite it, while the preserved resource still + // describes the space the file arrived in; re-emitting that one + // tagged converted pixels with the profile they were converted + // away from. + // + // It goes in the preserved block's own place rather than being + // appended at the end. The reader always mirrors ICC into + // `doc.icc_profile`, so the substitution fires on every round + // trip, and moving the tag rewrote the resource section's layout + // for a file nothing had actually changed. + let mut data: &[u8] = &res.data; + if res.id == RES_ICC_PROFILE { + seen_icc = true; + // With no profile of its own the document keeps whatever the + // file arrived with, rather than losing its colour space. + if let Some(icc) = &doc.icc_profile { + data = icc; + } + } b.bytes(b"8BIM"); b.u16(res.id); // `name` holds the raw pascal bytes (length byte + content + pad) @@ -152,8 +171,8 @@ fn write_image_resources(b: &mut Buf, doc: &Document) { b.u8(0); } } - b.u32(res.data.len() as u32); - b.bytes(&res.data); + b.u32(data.len() as u32); + b.bytes(data); b.pad_to(2); } diff --git a/crates/codec-psd/tests/writer.rs b/crates/codec-psd/tests/writer.rs index 25d42897..b5fd2c50 100644 --- a/crates/codec-psd/tests/writer.rs +++ b/crates/codec-psd/tests/writer.rs @@ -276,6 +276,108 @@ fn psd_keeps_u32_lengths_for_the_same_keys() { assert_eq!(block.data, b"preserved payload"); } +#[test] +fn a_converted_profile_replaces_the_preserved_one() { + // Convert/Assign Profile rewrite `doc.icc_profile` while the resource + // preserved from the source file still describes the old space. The + // writer re-emitted that one and suppressed the new tag, so converted + // pixels came back tagged with the profile they were converted away + // from. + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "l", + IntRect::from_xywh(0, 0, 8, 8), + [1, 2, 3, 255], + Depth::Eight, + )); + // A file that arrived carrying one profile... + doc.preserved_resources.push(PreservedResource { + id: 0x040F, + name: Vec::new(), + data: b"OLD-PROFILE-BYTES".to_vec(), + }); + // ...and was then converted to another. + doc.icc_profile = Some(b"NEW-PROFILE-BYTES".to_vec()); + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + assert_eq!( + back.icc_profile.as_deref(), + Some(b"NEW-PROFILE-BYTES".as_slice()), + "the document's own profile must win" + ); + let iccs = back + .preserved_resources + .iter() + .filter(|r| r.id == 0x040F) + .count(); + assert!(iccs <= 1, "the stale profile must not be written alongside"); +} + +#[test] +fn an_untagged_document_keeps_a_preserved_profile() { + // The other direction: with no profile of its own, dropping the + // preserved resource would lose the file's colour space outright. + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "l", + IntRect::from_xywh(0, 0, 8, 8), + [1, 2, 3, 255], + Depth::Eight, + )); + doc.preserved_resources.push(PreservedResource { + id: 0x040F, + name: Vec::new(), + data: b"ONLY-PROFILE".to_vec(), + }); + doc.icc_profile = None; + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + assert_eq!( + back.icc_profile.as_deref(), + Some(b"ONLY-PROFILE".as_slice()), + "the preserved profile must survive" + ); +} + +#[test] +fn a_round_trip_leaves_the_resource_section_where_it_was() { + // The reader always mirrors ICC into `doc.icc_profile`, so the + // substitution above fires on every round trip. Appending the tag at + // the end instead of writing it in the preserved block's own place + // rewrote the resource layout of a file nothing had changed. + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "l", + IntRect::from_xywh(0, 0, 8, 8), + [1, 2, 3, 255], + Depth::Eight, + )); + doc.preserved_resources.push(PreservedResource { + id: 0x040F, + name: Vec::new(), + data: b"THE-PROFILE".to_vec(), + }); + doc.preserved_resources.push(PreservedResource { + id: 0x0421, + name: Vec::new(), + data: b"after-the-profile".to_vec(), + }); + doc.icc_profile = Some(b"THE-PROFILE".to_vec()); + + let once = write_psd(&doc).unwrap(); + let twice = write_psd(&read_psd(&once).unwrap()).unwrap(); + assert_eq!(once, twice, "saving again moved bytes around"); + + let back = read_psd(&once).unwrap(); + let ids: Vec = back.preserved_resources.iter().map(|r| r.id).collect(); + let icc = ids.iter().position(|id| *id == 0x040F).expect("icc kept"); + let after = ids + .iter() + .position(|id| *id == 0x0421) + .expect("the block after it kept"); + assert!(icc < after, "the profile moved to the end: {ids:?}"); +} + #[test] fn an_adjustment_layer_made_in_schist_survives_a_save() { // The critical one: adjustment layers created in the app carry their diff --git a/crates/colormgmt/src/lib.rs b/crates/colormgmt/src/lib.rs index cda5e758..308a9524 100644 --- a/crates/colormgmt/src/lib.rs +++ b/crates/colormgmt/src/lib.rs @@ -94,18 +94,29 @@ impl Profile { } pub fn srgb() -> Profile { - Profile { - profile: Arc::new(ColorProfile::new_srgb()), - bytes: None, - name: "sRGB".into(), - } + Profile::builtin(ColorProfile::new_srgb(), "sRGB") } pub fn display_p3() -> Profile { + Profile::builtin(ColorProfile::new_display_p3(), "Display P3") + } + + /// A built-in profile, serialized so it can be embedded on save. + /// + /// These used to carry `bytes: None`, which made them unusable as + /// assignment targets: `icc_bytes()` returned `None`, so assigning + /// either of the two profiles the UI offers *untagged* the document + /// instead of tagging it, and the next open reinterpreted the pixels + /// against whatever the working space happened to be. + fn builtin(profile: ColorProfile, name: &str) -> Profile { + let bytes = profile.encode().ok().map(Arc::new); + if bytes.is_none() { + log::warn!("could not serialize the built-in {name} profile"); + } Profile { - profile: Arc::new(ColorProfile::new_display_p3()), - bytes: None, - name: "Display P3".into(), + profile: Arc::new(profile), + bytes, + name: name.into(), } } @@ -184,6 +195,13 @@ impl ColorTransform { /// Convert a straight-alpha f32 RGBA buffer in place. /// /// Alpha is carried through untouched: it is coverage, not colour. + /// + /// Note this cannot preserve extended range even on the editing path: + /// moxcms clamps to 0..1 while evaluating the transfer curves + /// (`gamma.rs`), so a 32-bit document's out-of-range highlights are + /// clipped by the CMS before we see the output. Removing the clamp + /// below would not change that; it needs either CMS support or a + /// matrix-only path that skips the curves. pub fn apply(&self, pixels: &mut [f32]) { if self.identity || pixels.is_empty() { return; @@ -194,8 +212,8 @@ impl ColorTransform { pixels.copy_from_slice(&src); return; } - // Extended-range output can exceed 0..1; clamp for display and - // restore alpha, which a matrix transform may have touched. + // Clamp for display and restore alpha, which a matrix transform + // may have touched. for (out, inp) in pixels .as_chunks_mut::<4>() .0 @@ -234,20 +252,19 @@ impl Default for ColorSettings { } impl ColorSettings { - /// Build the transform for a document with the given embedded profile. + /// Build the display hop for a document with the given embedded + /// profile. /// - /// Soft proofing runs document→proof→display; the two hops are baked - /// into one executor chain by applying them in sequence. + /// Soft proofing runs document → proof → display, applied in + /// sequence. The second hop therefore starts at the *proof* profile: + /// building it from the document profile, as this used to, converts + /// from a space the pixels already left, so Proof Colors was doubly + /// wrong whenever the display profile differed from the document's -- + /// and people make colour decisions against that view. pub fn transform_for(&self, document_icc: Option<&[u8]>) -> ColorTransform { - let source = match document_icc { - Some(bytes) => match Profile::from_bytes(bytes) { - Ok(p) => p, - Err(err) => { - log::warn!("{err:#}; falling back to the working space"); - self.working.clone() - } - }, - None => self.working.clone(), + let source = match &self.proof { + Some(proof) => proof.clone(), + None => self.document_profile(document_icc), }; match ColorTransform::new(&source, &self.display, self.intent) { Ok(t) => t, @@ -261,14 +278,26 @@ impl ColorSettings { /// The proofing hop, if soft proofing is on. pub fn proof_transform(&self, document_icc: Option<&[u8]>) -> Option { let proof = self.proof.as_ref()?; - let source = match document_icc { - Some(bytes) => Profile::from_bytes(bytes).unwrap_or_else(|_| self.working.clone()), - None => self.working.clone(), - }; + let source = self.document_profile(document_icc); // Proofing is colorimetric by definition: it must show the target's // gamut clipping rather than re-map it pleasingly. ColorTransform::new(&source, proof, Intent::RelativeColorimetric).ok() } + + /// The document's own profile, or the working space when it has none + /// or carries one we cannot read. + fn document_profile(&self, document_icc: Option<&[u8]>) -> Profile { + match document_icc { + Some(bytes) => match Profile::from_bytes(bytes) { + Ok(p) => p, + Err(err) => { + log::warn!("{err:#}; falling back to the working space"); + self.working.clone() + } + }, + None => self.working.clone(), + } + } } /// Bake BT.2100 HDR pixels (PQ or HLG signal, straight-alpha RGBA f32) @@ -611,4 +640,90 @@ mod tests { .apply(&mut b); assert_eq!(a, b); } + #[test] + fn builtin_profiles_can_be_embedded() { + // `bytes: None` made these unusable as assignment targets: + // `assign_profile` writes `icc_bytes()` onto the document, so + // assigning either of the two profiles the UI offers untagged the + // document rather than tagging it. + for p in [Profile::srgb(), Profile::display_p3()] { + let bytes = p + .icc_bytes() + .unwrap_or_else(|| panic!("{} has no bytes", p.name)); + assert!(bytes.len() > 128, "{} icc is implausibly small", p.name); + assert_eq!(&bytes[36..40], b"acsp", "{} is not an icc profile", p.name); + // And it must round-trip back through the parser. + assert!( + Profile::from_bytes(bytes).is_ok(), + "{} did not parse back", + p.name + ); + } + } + + /// Proofing to the very profile the display uses must show exactly + /// what an unproofed document→display conversion shows: the proof hop + /// takes the pixels to P3 and the display hop then has nothing left + /// to do. Building the display hop from the *document* profile + /// instead -- as it used to -- runs sRGB→P3 a second time over pixels + /// that are already P3, so Proof Colors was doubly wrong whenever the + /// display profile differed from the document's, and people make + /// colour decisions against that view. + #[test] + fn the_display_hop_starts_where_the_proof_hop_ended() { + let mut proofed = [0.8f32, 0.2, 0.1, 1.0]; + let mut direct = proofed; + + let proofing = ColorSettings { + working: Profile::srgb(), + display: Profile::display_p3(), + intent: Intent::Perceptual, + proof: Some(Profile::display_p3()), + }; + proofing.proof_transform(None).unwrap().apply(&mut proofed); + proofing.transform_for(None).apply(&mut proofed); + + let plain = ColorSettings { + proof: None, + ..proofing + }; + plain.transform_for(None).apply(&mut direct); + + for (got, want) in proofed.iter().zip(&direct) { + assert!( + (got - want).abs() < 1e-3, + "proof + display applied a second conversion: {proofed:?} vs {direct:?}" + ); + } + } + + #[test] + fn conversion_still_leaves_alpha_alone() { + let mut px = vec![0.4f32, 0.2, 0.7, 0.33]; + convert_pixels( + &mut px, + &Profile::srgb(), + &Profile::display_p3(), + Intent::Perceptual, + ) + .unwrap(); + assert!((px[3] - 0.33).abs() < 1e-6, "alpha changed: {}", px[3]); + } + + /// With proofing off, the display hop is still document → display. + #[test] + fn without_proofing_the_display_hop_is_unchanged() { + let settings = ColorSettings { + working: Profile::srgb(), + display: Profile::display_p3(), + intent: Intent::Perceptual, + proof: None, + }; + let mut pixels = [0.8f32, 0.2, 0.1, 1.0]; + settings.transform_for(None).apply(&mut pixels); + assert!( + (pixels[0] - 0.8).abs() > 1e-3 || (pixels[1] - 0.2).abs() > 1e-3, + "sRGB to Display P3 should have moved the pixel" + ); + } } diff --git a/crates/compositor-gpu/src/composite.wgsl b/crates/compositor-gpu/src/composite.wgsl index 64913d69..7ae82207 100644 --- a/crates/compositor-gpu/src/composite.wgsl +++ b/crates/compositor-gpu/src/composite.wgsl @@ -652,7 +652,17 @@ fn composite(@builtin(global_invocation_id) gid: vec3) { d.rgb, ); } - stack[sp - 1u] = vec4(d.rgb + (adjusted - d.rgb) * weight, d.a); + // Mirrors the CPU compositor: the adjustment's + // own blend mode applies, with the adjusted colour + // as the source and `weight` as its alpha. It used + // to be uploaded and then ignored, so every + // adjustment rendered as Normal. + if (op.mode == M_NORMAL) { + stack[sp - 1u] = vec4(d.rgb + (adjusted - d.rgb) * weight, d.a); + } else { + let blended = blend_px(op.mode, vec4(adjusted, weight), d, x, y); + stack[sp - 1u] = vec4(blended.rgb, d.a); + } } } } diff --git a/crates/compositor-gpu/src/plan.rs b/crates/compositor-gpu/src/plan.rs index 22a14fee..925c8c78 100644 --- a/crates/compositor-gpu/src/plan.rs +++ b/crates/compositor-gpu/src/plan.rs @@ -280,8 +280,12 @@ fn emit_layers<'a>( op.opacity = layer.opacity * content_alpha(layer); } LayerKind::Group(g) => { + // Mirrors the CPU compositor: fill opacity is part of + // the pass-through test, or a group at fill 50% would + // render its children at full strength. let pass_through = layer.blend == BlendMode::PassThrough && layer.opacity >= 1.0 + && content_alpha(layer) >= 1.0 && layer.mask.is_none(); if pass_through { emit_layers(&g.children, plan, depth)?; @@ -297,7 +301,7 @@ fn emit_layers<'a>( let mask = plan.mask_ref(layer); let op = plan.op(OP_BLEND); op.mode = mode_id(mode); - op.opacity = layer.opacity; + op.opacity = layer.opacity * content_alpha(layer); op.mask = mask; } } diff --git a/crates/compositor/src/lib.rs b/crates/compositor/src/lib.rs index e4b0f5e4..af648c8b 100644 --- a/crates/compositor/src/lib.rs +++ b/crates/compositor/src/lib.rs @@ -489,8 +489,12 @@ fn composite_layers( scratch.give(src); } LayerKind::Group(g) => { + // Fill opacity has to be part of this: a pass-through + // group renders its children straight into `dst`, so + // a group at fill 50% was drawn at full strength. let pass_through = layer.blend == BlendMode::PassThrough && layer.opacity >= 1.0 + && content_alpha(layer) >= 1.0 && layer.mask.is_none(); if pass_through { composite_layers(doc, &g.children, coord, dst, scratch); @@ -502,7 +506,20 @@ fn composite_layers( } else { layer.blend }; - blend_buf_onto(mode, &group_buf, dst, coord, layer.opacity, layer, doc); + // `content_alpha` is fill opacity. Every other + // blend in this file multiplies it in; the + // isolated-group path did not, so the Fill slider + // did nothing on a group unless it happened to be + // a clip base, where a different path applied it. + blend_buf_onto( + mode, + &group_buf, + dst, + coord, + layer.opacity * content_alpha(layer), + layer, + doc, + ); scratch.give(group_buf); } } @@ -594,11 +611,34 @@ fn apply_adjustment( blend_pixel(layer.blend, Rgba { a: weight, ..src }, base, x, y) } else { let adjusted = params.apply(base); - Rgba { - r: base.r + (adjusted.r - base.r) * weight, - g: base.g + (adjusted.g - base.g) * weight, - b: base.b + (adjusted.b - base.b) * weight, - a: base.a, + // The blend mode was read from the file, stored on the layer + // and uploaded to the shader, then dropped here: every + // "Curves set to Luminosity" or "Levels set to Multiply" + // rendered as Normal, with nothing to say it had been ignored. + if layer.blend == BlendMode::Normal { + Rgba { + r: base.r + (adjusted.r - base.r) * weight, + g: base.g + (adjusted.g - base.g) * weight, + b: base.b + (adjusted.b - base.b) * weight, + a: base.a, + } + } else { + // The adjusted colour is the source, `weight` its alpha, + // exactly as the fill path above treats its own colour. + let blended = blend_pixel( + layer.blend, + Rgba { + a: weight, + ..adjusted + }, + base, + x, + y, + ); + Rgba { + a: base.a, + ..blended + } } }; d[0] = out.r; @@ -1027,6 +1067,74 @@ mod tests { .select_rect(IntRect::from_xywh(0, 0, 8, 8), SelectOp::Replace); assert_eq!(px(&doc, 20, 20), [5, 6, 7, 255]); } + + #[test] + fn a_groups_fill_opacity_is_honoured() { + // Every blend in this file multiplies in `content_alpha` (fill + // opacity) except the isolated-group one, so the Fill slider did + // nothing on a group unless it happened to be a clip base, where + // a different path applied it. + let build = |fill: f32| { + let mut doc = Document::new("t", 8, 8, Depth::Eight); + // An opaque backdrop, so the group's fill shows as tone + // rather than only as alpha. + doc.push_layer(solid_layer( + "black", + IntRect::from_xywh(0, 0, 8, 8), + [0, 0, 0, 255], + )); + let child = solid_layer( + "white", + IntRect::from_xywh(0, 0, 8, 8), + [255, 255, 255, 255], + ); + let mut group = Layer::new_group("g"); + if let LayerKind::Group(g) = &mut group.kind { + g.children.push(child); + } + group.fill_opacity = fill; + doc.push_layer(group); + px(&doc, 4, 4) + }; + let full = build(1.0); + let half = build(0.5); + assert_ne!(full, half, "fill opacity must change the group"); + assert!(half[0] < full[0], "half fill must be dimmer: {half:?}"); + } + + #[test] + fn an_adjustment_layers_blend_mode_is_used() { + // Parsed, stored, round-tripped to PSD and uploaded to the shader, + // then dropped here: every "Curves set to Luminosity" rendered as + // Normal, with nothing to say it had been ignored. + let build = |mode: BlendMode| { + let mut doc = Document::new("t", 8, 8, Depth::Eight); + doc.push_layer(solid_layer( + "grey", + IntRect::from_xywh(0, 0, 8, 8), + [128, 128, 128, 255], + )); + let mut adj = Layer::new_raster("invert"); + adj.kind = LayerKind::Adjustment(schist_core::AdjustmentData { + kind: schist_core::AdjustmentKind::Invert, + raw: Vec::new(), + params_json: Some("\"Invert\"".into()), + }); + adj.blend = mode; + doc.push_layer(adj); + px(&doc, 4, 4) + }; + let normal = build(BlendMode::Normal); + let multiply = build(BlendMode::Multiply); + assert_ne!( + normal, multiply, + "the blend mode must reach the result (both {normal:?})" + ); + assert!( + multiply[0] < normal[0], + "multiply must darken: {multiply:?} vs {normal:?}" + ); + } } #[cfg(test)] diff --git a/crates/core/src/document.rs b/crates/core/src/document.rs index b2057e84..95dcfc07 100644 --- a/crates/core/src/document.rs +++ b/crates/core/src/document.rs @@ -490,6 +490,19 @@ impl Document { self.selection = (**target).clone(); self.revision += 1; } + EditOp::IccProfileSet { before, after } => { + let target = if dir == Direction::Undo { + before + } else { + after + }; + self.icc_profile = target.clone(); + // The profile decides what every pixel means, so the whole + // canvas has to repaint. Without this the restored pixels + // kept rendering through the transform built for the other + // profile, and a pure Assign did not repaint at all. + self.damage_all(); + } EditOp::ColorModeSet { before, after } => { self.mode = if dir == Direction::Undo { *before @@ -916,6 +929,24 @@ impl<'a> EditBuilder<'a> { } } + /// Set the document's embedded ICC profile as part of this edit. + /// + /// Assign and Convert to Profile set it outside the edit, so undo + /// restored the pixels but left the new tag in place: the old numbers + /// were then interpreted under the wrong profile, which is worse than + /// either state on its own. + pub fn set_icc_profile(&mut self, profile: Option>) { + let before = self.doc.icc_profile.clone(); + if before == profile { + return; + } + self.doc.icc_profile = profile.clone(); + self.ops.push(EditOp::IccProfileSet { + before, + after: profile, + }); + } + /// Change the document's colour mode as part of this edit. pub fn set_color_mode(&mut self, mode: schist_color::ColorMode) { if self.doc.mode == mode { @@ -1404,6 +1435,64 @@ mod tests { assert!(!doc.dirty, "but nothing was actually changed"); } + #[test] + fn the_icc_profile_undoes_with_the_pixels() { + // Convert to Profile rewrites the pixels *and* the tag. Setting + // the tag outside the edit meant undo restored the old numbers + // while leaving the new profile on them, which reads as a + // different colour than either state. + let mut doc = Document::new("t", 32, 32, Depth::Eight); + doc.icc_profile = Some(b"OLD".to_vec()); + + let mut edit = doc.begin_edit("Convert"); + edit.set_icc_profile(Some(b"NEW".to_vec())); + edit.commit(); + assert_eq!(doc.icc_profile.as_deref(), Some(b"NEW".as_slice())); + + doc.undo(); + assert_eq!( + doc.icc_profile.as_deref(), + Some(b"OLD".as_slice()), + "undo must put the original tag back" + ); + doc.redo(); + assert_eq!(doc.icc_profile.as_deref(), Some(b"NEW".as_slice())); + } + + #[test] + fn setting_the_same_profile_records_nothing() { + let mut doc = Document::new("t", 32, 32, Depth::Eight); + doc.icc_profile = Some(b"SAME".to_vec()); + let mut edit = doc.begin_edit("Assign"); + edit.set_icc_profile(Some(b"SAME".to_vec())); + edit.commit(); + assert!(!doc.history.can_undo(), "a no-op must not enter history"); + } + /// Undoing a profile change has to repaint: the profile decides what + /// every pixel means, and without damage the canvas kept rendering + /// the restored pixels through the transform built for the profile + /// that had just been undone away. + #[test] + fn undoing_a_profile_change_damages_the_canvas() { + let mut doc = Document::new("t", 32, 32, Depth::Eight); + doc.push_layer(Layer::new_raster("bg")); + let mut edit = doc.begin_edit("Assign Profile"); + edit.set_icc_profile(Some(vec![1, 2, 3, 4])); + assert!(edit.commit()); + doc.take_damage(); + + doc.undo().unwrap(); + assert!(doc.icc_profile.is_none(), "the profile was restored"); + assert!( + !doc.take_damage().is_empty(), + "undo left nothing for the canvas to repaint" + ); + + doc.redo().unwrap(); + assert_eq!(doc.icc_profile.as_deref(), Some(&[1, 2, 3, 4][..])); + assert!(!doc.take_damage().is_empty(), "redo repaints too"); + } + #[test] fn the_colour_mode_undoes_with_the_pixels() { // Image > Mode set `doc.mode` outside the edit, so undo restored diff --git a/crates/core/src/history.rs b/crates/core/src/history.rs index aecc8428..7ad0dd38 100644 --- a/crates/core/src/history.rs +++ b/crates/core/src/history.rs @@ -134,6 +134,14 @@ pub enum EditOp { before: Box, after: Box, }, + /// The document's embedded ICC profile changed (Assign / Convert to + /// Profile). Rides in the same edit as the pixel rewrite so undo puts + /// the pixels and their tag back together; undoing only the pixels + /// left the old numbers interpreted under the new profile. + IccProfileSet { + before: Option>, + after: Option>, + }, /// The document's notes changed -- placed, moved, retyped, deleted or /// cleared. ///