From 344d756bf610acecc8984a2bf664fd0326c4e32b Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Thu, 12 Nov 2026 09:10:51 -0800 Subject: [PATCH] Color management: proof view commands redraw only when the view changes view.proofSetup, view.proofColors, view.overprintPreview and view.separationsPreview stored the proof view and raised every open document's revision on each run. They did this also when the request asked for the view that was already showing, for example Proof Colors on while it is on, or Proof Setup with no params. Each such run redrew every canvas. When the active document's revision changes, the canvas also sends a tool that asks for it a new hover event, although the pointer has not moved. - The four commands compare the new proof view with the current one. When the two are equal, the commands leave the view and the revisions as they are. Separations Preview's visible plates are compared as a set, so the same plates in another order do not redraw. - view.separationsPreview with `plates` naming a plate twice shows it once, so one `toggle` of that plate hides it. - color.loadProfile with a new file of the CMYK profile that Proof Setup targets gives the documents new revisions, as it does for a working space. Before, running Proof Setup again redrew the documents through the new file; it now leaves an unchanged view alone. - color.removeProfile gives the documents new revisions only when Proof Setup targets the removed profile. --- crates/engine/src/cmd/colormgmt.rs | 52 ++++++++++++--- crates/engine/src/command_sets.rs | 2 +- crates/engine/src/tests/build.rs | 12 +++- crates/engine/src/tests/colormgmt.rs | 96 +++++++++++++++++++++++++++ crates/engine/tests/color_settings.rs | 54 +++++++++++++++ crates/ui-egui/src/control/tests.rs | 4 +- 6 files changed, 206 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/cmd/colormgmt.rs b/crates/engine/src/cmd/colormgmt.rs index 9ec21b6541..1046c4a948 100644 --- a/crates/engine/src/cmd/colormgmt.rs +++ b/crates/engine/src/cmd/colormgmt.rs @@ -210,8 +210,12 @@ fn load_profile(s: &mut Session, p: &Value) -> Result { let remembered = s.store_profile(&info.name, &bytes); // A new file of a working space: the active settings still hold the old one. let working = cms::active_settings(); - if working.rgb == info.name || working.cmyk == info.name { + let is_working = working.rgb == info.name || working.cmyk == info.name; + if is_working { cms::set_active(&working).map_err(|e| bad(C, e.to_string()))?; + } + // A new file of a working space, or of the CMYK profile View → Proof Setup targets: redraw. + if is_working || proof_targets(&info.name) { touch_all(s); } Ok(json!({"name": info.name, "kind": info.kind, "remembered": remembered})) @@ -233,10 +237,17 @@ fn remove_profile(s: &mut Session, p: &Value) -> Result { return Err(bad(C, format!("no profile named `{name}`"))); } s.unstore_profile(&info.name); - touch_all(s); + if proof_targets(&info.name) { + touch_all(s); + } Ok(json!({"removed": info.name})) } +/// Whether View → Proof Setup targets the CMYK profile `name`. +fn proof_targets(name: &str) -> bool { + matches!(proof::view().setup.target, ProofTarget::Cmyk(t) if t == name) +} + #[cfg(target_arch = "wasm32")] fn load_profile(_: &mut Session, _: &Value) -> Result { Err(bad("color.loadProfile", "loading profiles from disk isn't available in the browser")) @@ -763,17 +774,33 @@ fn proof_setup(s: &mut Session, p: &Value) -> Result { if let Some(on) = on_param(p, "proof", C)? { v.proof_colors = on; } - proof::set_view(v.clone()); - touch_all(s); + store_view(s, v.clone()); Ok(view_json(&v)) } +/// Make `v` the proof view and redraw every document. When `v` equals the current view, the view +/// and the revisions stay as they are; Separations Preview's visible plates are compared as a set. +fn store_view(s: &mut Session, v: proof::ProofView) { + let shown = |v: &proof::ProofView| proof::ProofView { + separations: v.separations.as_ref().map(|vis| { + let mut vis = vis.clone(); + vis.sort(); + vis.dedup(); + vis + }), + ..v.clone() + }; + if shown(&v) != shown(&proof::view()) { + proof::set_view(v); + touch_all(s); + } +} + fn toggle_view(s: &mut Session, p: &Value, cmd: &str, f: impl FnOnce(&mut proof::ProofView, Option)) -> Result { let on = on_param(p, "on", cmd)?; let mut v = proof::view(); f(&mut v, on); - proof::set_view(v.clone()); - touch_all(s); + store_view(s, v.clone()); Ok(view_json(&v)) } @@ -805,7 +832,15 @@ fn separations(s: &mut Session, p: &Value) -> Result { match p.get("plates") { None | Some(Value::Null) => {} Some(Value::Array(a)) if a.iter().all(Value::is_string) => { - v.separations = Some(a.iter().filter_map(Value::as_str).map(check).collect::>>()?); + let mut vis: Vec = Vec::new(); + for n in a.iter().filter_map(Value::as_str) { + let n = check(n)?; + // A plate named twice is shown once. + if !vis.contains(&n) { + vis.push(n); + } + } + v.separations = Some(vis); } Some(_) => { let key = "plates"; @@ -824,8 +859,7 @@ fn separations(s: &mut Session, p: &Value) -> Result { vis.push(n); } } - proof::set_view(v); - touch_all(s); + store_view(s, v); Ok(plates_json(s)) } diff --git a/crates/engine/src/command_sets.rs b/crates/engine/src/command_sets.rs index 95b44776f0..c9de286514 100644 --- a/crates/engine/src/command_sets.rs +++ b/crates/engine/src/command_sets.rs @@ -84,7 +84,7 @@ pub(crate) fn strip_replay_only(id: &str, p: &mut Value) { /// Queries that apply the edit the active tool is computing before they run, as edits do /// ([`Session::finish_tool_edit`]): a batch runs edits, Find Objects with `select`, Select All /// Instances and Lock Guides change the selection, and the proof views, the preferences and added -/// fonts give the documents new revisions (they draw again, and type lays out again). +/// fonts can give the documents new revisions (they draw again, and type lays out again). const WAIT_FOR_TOOL_EDIT: &[&str] = &[ "script.run", "command.batch", diff --git a/crates/engine/src/tests/build.rs b/crates/engine/src/tests/build.rs index 0fc2bcb476..49a806b258 100644 --- a/crates/engine/src/tests/build.rs +++ b/crates/engine/src/tests/build.rs @@ -793,8 +793,9 @@ fn shape_builder_applies_a_merge_before_what_comes_next() { let labels: Vec<&str> = s.doc().unwrap().history.undo.iter().rev().take(2).map(|e| e.label.as_str()).collect(); assert_eq!(labels, ["Move", "Shape Builder"]); }), - // The proof views redraw every document (a new revision); they are left off, as the - // other tests of the process-wide proof view expect. + // The proof views apply the merge first by command id (`WAIT_FOR_TOOL_EDIT`), also when + // the view does not change and no document gets a new revision. They are left off, as + // the other tests of the process-wide proof view expect. ("Separations Preview", |s, _, _| { s.execute("view.separationsPreview", &json!({"on": false})).unwrap(); }), @@ -807,6 +808,13 @@ fn shape_builder_applies_a_merge_before_what_comes_next() { ("Proof Setup", |s, _, _| { s.execute("view.proofSetup", &json!({})).unwrap(); }), + ("a proof view that changes", |s, _, _| { + let _v = crate::tests_colormgmt::DefaultView::new(); + s.execute("view.proofColors", &json!({"on": true})).unwrap(); + let on = s.doc().unwrap().revision; + s.execute("view.proofColors", &json!({"on": false})).unwrap(); + assert!(s.doc().unwrap().revision > on, "the view changes and the documents get new revisions"); + }), ("a preference", |s, _, _| { let every = s.prefs.gridline_every; s.execute("prefs.set", &json!({"key": "gridlineEvery", "value": every + 10.0})).unwrap(); diff --git a/crates/engine/src/tests/colormgmt.rs b/crates/engine/src/tests/colormgmt.rs index ae8ac0c4f0..493f569805 100644 --- a/crates/engine/src/tests/colormgmt.rs +++ b/crates/engine/src/tests/colormgmt.rs @@ -455,6 +455,7 @@ fn overprint_black_multiplies_in_preview() { #[test] fn view_toggles_redraw_without_dirtying() { let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); + let _v = DefaultView::new(); let mut s = session(); let rev = s.doc().unwrap().revision; let r = s.execute("view.proofColors", &json!({"on": true})).unwrap(); @@ -475,6 +476,101 @@ fn view_toggles_redraw_without_dirtying() { s.execute("view.overprintPreview", &json!({"on": false})).unwrap(); } +/// Sets the default proof view when made and again when dropped, also after a failed assertion, +/// so the next test starts from the default view. +pub(crate) struct DefaultView; + +impl DefaultView { + pub(crate) fn new() -> Self { + vectorcraft_render::proof::set_view(Default::default()); + DefaultView + } +} + +impl Drop for DefaultView { + fn drop(&mut self) { + vectorcraft_render::proof::set_view(Default::default()); + } +} + +/// Runs `cmd` with `p` twice from the default proof view: the first run changes the view and +/// redraws, and the second asks for the view that is already showing and leaves the revision alone. +fn repeat_keeps_revision(cmd: &str, p: Value) { + let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); + let _v = DefaultView::new(); + let mut s = session(); + let rev = s.doc().unwrap().revision; + s.execute(cmd, &p).unwrap(); + let changed = s.doc().unwrap().revision; + assert!(changed > rev, "{cmd} {p}: the first run changes the view"); + s.execute(cmd, &p).unwrap(); + assert_eq!(s.doc().unwrap().revision, changed, "{cmd} {p}: the same view again"); +} + +/// Runs `cmd` with `p` once from the default proof view, which `p` leaves as it is. +fn default_view_keeps_revision(cmd: &str, p: Value) { + let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); + let _v = DefaultView::new(); + let mut s = session(); + let rev = s.doc().unwrap().revision; + s.execute(cmd, &p).unwrap(); + assert_eq!(s.doc().unwrap().revision, rev, "{cmd} {p}"); + assert_eq!(vectorcraft_render::proof::view(), Default::default(), "{cmd} {p}"); +} + +#[test] +fn proof_setup_with_nothing_changed_keeps_the_revision() { + repeat_keeps_revision("view.proofSetup", json!({"target": "deuteranopia"})); + repeat_keeps_revision("view.proofSetup", json!({"simulatePaper": true, "proof": true})); + default_view_keeps_revision("view.proofSetup", json!({})); + default_view_keeps_revision("view.proofSetup", json!({"target": "workingCmyk", "proof": false})); +} + +#[test] +fn proof_colors_with_nothing_changed_keeps_the_revision() { + repeat_keeps_revision("view.proofColors", json!({"on": true})); + default_view_keeps_revision("view.proofColors", json!({"on": false})); +} + +#[test] +fn overprint_preview_with_nothing_changed_keeps_the_revision() { + repeat_keeps_revision("view.overprintPreview", json!({"on": true})); + default_view_keeps_revision("view.overprintPreview", json!({"on": false})); +} + +#[test] +fn separations_preview_with_nothing_changed_keeps_the_revision() { + repeat_keeps_revision("view.separationsPreview", json!({"on": true})); + repeat_keeps_revision("view.separationsPreview", json!({"only": "Cyan"})); + repeat_keeps_revision("view.separationsPreview", json!({"plates": ["Magenta", "Black"]})); + default_view_keeps_revision("view.separationsPreview", json!({"on": false})); +} + +/// Separations Preview's visible plates are a set: the same plates in another order, or with one +/// named twice, leave the revision alone, and a plate named twice hides with one `toggle`. +#[test] +fn separations_preview_compares_the_visible_plates_as_a_set() { + let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); + let _v = DefaultView::new(); + let mut s = session(); + let sep = |s: &mut Session, p: Value| { + s.execute("view.separationsPreview", &p).unwrap(); + s.doc().unwrap().revision + }; + let shown = sep(&mut s, json!({"plates": ["Magenta", "Black"]})); + assert_eq!(sep(&mut s, json!({"plates": ["Black", "Magenta"]})), shown, "another order"); + assert_eq!(sep(&mut s, json!({"plates": ["Magenta", "Black", "Black"]})), shown, "a plate named twice"); + // Cyan hidden and shown again goes to the end of the list; every plate is shown. + sep(&mut s, json!({"on": false})); + sep(&mut s, json!({"on": true})); + sep(&mut s, json!({"toggle": "Cyan"})); + let all = sep(&mut s, json!({"toggle": "Cyan"})); + assert_eq!(sep(&mut s, json!({"plates": ["Cyan", "Magenta", "Yellow", "Black"]})), all, "every plate in plate order"); + sep(&mut s, json!({"plates": ["Cyan", "Cyan"]})); + let r = s.execute("view.separationsPreview", &json!({"toggle": "Cyan"})).unwrap(); + assert!(r["plates"].as_array().unwrap().iter().all(|p| p["visible"] == false), "{r}"); +} + #[test] fn proof_setup_simulates_tritanopia() { let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); diff --git a/crates/engine/tests/color_settings.rs b/crates/engine/tests/color_settings.rs index c84921f6de..c3d6df771b 100644 --- a/crates/engine/tests/color_settings.rs +++ b/crates/engine/tests/color_settings.rs @@ -446,6 +446,60 @@ fn loading_the_working_rgb_profile_again_takes_effect_at_once() { assert!(redrawn, "the canvas redraws"); } +/// Loading a new file of the CMYK profile View → Proof Setup targets, or removing that profile, +/// gives the documents new revisions, as a working space's new file does; loading or removing +/// another profile does not. +#[test] +fn loading_or_removing_the_proof_profile_redraws() { + let _g = settings(); + // A built-in CMYK profile's file renamed `name`: padded with spaces, which names drop. + let renamed = |builtin: &str, name: &str| { + let utf16 = |s: &str| s.encode_utf16().flat_map(u16::to_be_bytes).collect::>(); + let mut bytes = cms::icc_bytes(builtin).unwrap().to_vec(); + let (from, to) = (utf16(builtin), utf16(&format!("{name: revision, "the proof profile's file"); + exec(&mut s, "color.removeProfile", json!({"name": other})); + assert_eq!(s.doc().unwrap().revision, reloaded, "another profile removed"); + exec(&mut s, "color.removeProfile", json!({"name": proofed})); + assert!(s.doc().unwrap().revision > reloaded, "the proof profile removed"); +} + +/// Sets the default proof view and unregisters the profiles when dropped, also after a failed +/// assertion. +struct ProofReset([&'static str; 2]); + +impl Drop for ProofReset { + fn drop(&mut self) { + vectorcraft_render::proof::set_view(Default::default()); + for name in self.0 { + cms::unregister_icc(name); + } + } +} + /// A folder of its own for the test `tag`, empty. fn fresh_dir(tag: &str) -> std::path::PathBuf { let dir = vectorcraft_testkit::temp_dir(&format!("color-settings-{tag}")); diff --git a/crates/ui-egui/src/control/tests.rs b/crates/ui-egui/src/control/tests.rs index 1763fd679a..ac524e0b65 100644 --- a/crates/ui-egui/src/control/tests.rs +++ b/crates/ui-egui/src/control/tests.rs @@ -679,8 +679,8 @@ fn menu_dialogs_open_after_a_merge_on_another_thread() { } /// The Separations panel's checkboxes and a preference set while a Shape Builder merge is -/// computed on another thread give the documents new revisions: the merge is applied first, -/// and the document keeps it. +/// computed on another thread apply the merge first, and the document keeps it. The checkboxes +/// leave the view off, so they apply it by command id without giving the documents new revisions. #[test] fn panels_that_redraw_apply_a_merge_on_another_thread_first() { let cases = [