Merge pull request #6951 from dotCooCoo/fix/proof-view-unchanged

Color management: proof view commands redraw only when the view changes

crates/engine/src/cmd/colormgmt.rs

@@ -210,8 +210,12 @@ fn load_profile(s: &mut Session, p: &Value) -> Result<Value> {
     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<Value> {
         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<Value> {
     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<Value> {
     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<bool>)) -> Result<Value> {
     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<Value> {
     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::<Result<Vec<_>>>()?);
+            let mut vis: Vec<String> = 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<Value> {
             vis.push(n);
         }
     }
-    proof::set_view(v);
-    touch_all(s);
+    store_view(s, v);
     Ok(plates_json(s))
 }
 

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",

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();

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());

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::<Vec<u8>>();
+        let mut bytes = cms::icc_bytes(builtin).unwrap().to_vec();
+        let (from, to) = (utf16(builtin), utf16(&format!("{name:<width$}", width = builtin.len())));
+        let at = bytes.windows(from.len()).position(|w| w == from).unwrap();
+        bytes[at..at + to.len()].copy_from_slice(&to);
+        bytes
+    };
+    let dir = fresh_dir("reload-proof-cmyk");
+    std::fs::create_dir_all(&dir).unwrap();
+    let _reset = ColorSettingsReset { profile: None, paths: vec![dir.clone()] };
+    let load = |s: &mut Session, name: &str| {
+        let path = dir.join(format!("{}.icc", name.len()));
+        std::fs::write(&path, renamed(cms::GENERIC_CMYK, name)).unwrap();
+        let r = s.execute("color.loadProfile", &json!({"path": path.to_str().unwrap()})).unwrap();
+        assert_eq!(r["name"], name);
+        s.doc().unwrap().revision
+    };
+    let (proofed, other) = ("Reload Proof CMYK", "Reload Other CMYK 2");
+    let _view = ProofReset([proofed, other]);
+    let mut s = session();
+    load(&mut s, proofed);
+    load(&mut s, other);
+    exec(&mut s, "view.proofSetup", json!({"target": format!("cmyk:{proofed}"), "proof": true}));
+    let revision = s.doc().unwrap().revision;
+    assert_eq!(load(&mut s, other), revision, "another profile's file");
+    let reloaded = load(&mut s, proofed);
+    assert!(reloaded > 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}"));

crates/ui-egui/src/control/tests.rs

@@ -1073,8 +1073,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 = [