Merge pull request #2520 from vangel76/fix/2170-recolor-model

Recolor: recolor.apply takes a model option for the new colour's model (#2170)

crates/color/src/recolor.rs

@@ -264,6 +264,8 @@ pub struct RowMap {
     mean_l: f32,
     /// Hue turn from the key colour to the new colour (Hue Shift).
     turn: f32,
+    /// Whether colours take the new colour in its own model, not in the model they had.
+    own_model: bool,
 }
 
 impl RowMap {
@@ -274,7 +276,20 @@ impl RowMap {
             Method::TintsShades => mean_l(&row.iter().map(|c| (*c, c.to_lab())).collect::<Vec<_>>()),
             _ => 0.0,
         };
-        Self { method, to, key, mean_l, turn: to.to_hsb()[0] - key.to_hsb()[0] }
+        Self { method, to, key, mean_l, turn: to.to_hsb()[0] - key.to_hsb()[0], own_model: false }
+    }
+
+    /// With `own`, current colours take the new colour in the new colour's model (an RGB colour
+    /// replaced by a CMYK one becomes CMYK) instead of keeping the model they had.
+    pub fn with_own_model(mut self, own: bool) -> Self {
+        self.own_model = own;
+        self
+    }
+
+    /// True when current colour `c` keeps a colour model other than the new colour's (so the new
+    /// colour is converted into `c`'s model and may look different from what was asked for).
+    pub fn converts(&self, c: Color) -> bool {
+        !self.own_model && c.model() != self.to.model()
     }
 
     /// The colour of the row the new colour stands for.
@@ -301,7 +316,7 @@ impl RowMap {
 
     /// The new colour of current colour `c`, in `c`'s model.
     pub fn map(&self, c: Color) -> Color {
-        let to = keep_model(c, self.to);
+        let to = if self.own_model { self.to } else { keep_model(c, self.to) };
         match self.method {
             Method::Exact => to,
             Method::PreserveTints if is_tint(c, self.key) => to.tinted(tint_of(c, self.key)),
@@ -314,7 +329,13 @@ impl RowMap {
             Method::HueShift if c == self.key => to,
             Method::HueShift => {
                 let [h, s, v] = c.to_hsb();
-                if s <= 0.0 { c } else { keep_model(c, Color::from_hsb(h + self.turn, s, v)) }
+                if s <= 0.0 {
+                    if self.own_model { c.in_model(self.to.model()) } else { c }
+                } else if self.own_model {
+                    Color::from_hsb(h + self.turn, s, v).in_model(self.to.model())
+                } else {
+                    keep_model(c, Color::from_hsb(h + self.turn, s, v))
+                }
             }
         }
     }
@@ -576,6 +597,16 @@ mod tests {
         assert!(cluster(&[], &[], 3).is_empty());
     }
 
+    #[test]
+    fn hue_shift_with_own_model_puts_every_colour_of_the_row_in_the_new_model() {
+        let to = Color::cmyk(0.0, 1.0, 1.0, 0.0);
+        let row = [Color::rgb(0.2, 0.2, 0.9), Color::rgb(0.1, 0.1, 0.5), Color::rgb(0.3, 0.3, 0.3)];
+        let own = RowMap::new(Method::HueShift, &row, to).with_own_model(true);
+        assert!(row.iter().all(|c| own.map(*c).model() == to.model()), "key and other colours alike");
+        let kept = RowMap::new(Method::HueShift, &row, to);
+        assert!(row.iter().all(|c| kept.map(*c).model() == c.model()));
+    }
+
     #[test]
     fn palette_snaps_to_the_nearest_colour() {
         let p = Palette::new([Color::rgb(1.0, 0.0, 0.0), Color::cmyk(1.0, 0.0, 0.0, 0.0), Color::gray(0.5)]);

crates/engine/src/cmd/recolor.rs

@@ -30,7 +30,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Recolor Artwork",
             ["Edit", "Edit Colors"],
             None,
-            "{map: rows [{from: [colour keys (recolor.colors) or \"#rrggbb\" (every colour shown as that hex)], to: colour (hex, key, {c,m,y,k} or {l,a,b}) | null, exclude?: false (the row's colours are kept)}] | {\"key or #rrggbb\": colour, …} (one row per colour), method?: \"exact\" (default)|\"preserveTints\"|\"scaleTints\"|\"tintsShades\"|\"hueShift\" (how a row's colours take its new colour, relative to the row's darkest, average or most saturated colour), limitTo?: swatch library id or name; or \"document\" (the document's swatches): new colours snap to its nearest colour, group?: colour group to rewrite, groupColors?: [colour] (what `group` is rewritten with; default: the rows' new colours, in order, excluded rows left out), rename?: the group's new name, includeImages?: true, includePatterns?: true} recolour the selection (gradients, text, meshes, image pixels and pattern tiles included; each new colour keeps the model of the one it replaces, except a library swatch's with limitTo: art takes the swatch itself, linked to a global or spot colour at its tints, and the swatch joins the document's swatches) and the group, as one undo step → {changed, swatches: [names added to the document's swatches]}",
+            "{map: rows [{from: [colour keys (recolor.colors) or \"#rrggbb\" (every colour shown as that hex)], to: colour (hex, key, {c,m,y,k} or {l,a,b}) | null, exclude?: false (the row's colours are kept)}] | {\"key or #rrggbb\": colour, …} (one row per colour), method?: \"exact\" (default)|\"preserveTints\"|\"scaleTints\"|\"tintsShades\"|\"hueShift\" (how a row's colours take its new colour, relative to the row's darkest, average or most saturated colour), limitTo?: swatch library id or name; or \"document\" (the document's swatches): new colours snap to its nearest colour, group?: colour group to rewrite, groupColors?: [colour] (what `group` is rewritten with; default: the rows' new colours, in order, excluded rows left out), rename?: the group's new name, includeImages?: true, includePatterns?: true} recolour the selection (gradients, text, meshes, image pixels and pattern tiles included; each new colour keeps the model of the one it replaces (model?: \"keep\" default | \"new\" (the new colour's own model, so a CMYK colour replacing RGB art gives CMYK art) | \"document\" (the document's colour mode)), except a library swatch's with limitTo: art takes the swatch itself, linked to a global or spot colour at its tints, and the swatch joins the document's swatches) and the group, as one undo step → {changed, swatches: [names added to the document's swatches], note?: set when a colour kept a model other than its new colour's (see model)}",
             has_doc,
             apply
         ),
@@ -268,17 +268,19 @@ struct ColorMap {
     links: Vec<Option<Link>>,
     keys: HashMap<ColorKey, usize>,
     hexes: HashMap<String, usize>,
+    /// Set when a colour took a new colour of another model in its own model.
+    converted: std::cell::Cell<bool>,
 }
 
 impl ColorMap {
     /// `links`: the swatch of each of `rows`' new colours (as many as `rows`, or none).
-    fn new(rows: &[Row], method: Method, links: &[Option<Link>]) -> Self {
-        let mut out = Self { rows: vec![], links: vec![], keys: HashMap::new(), hexes: HashMap::new() };
+    fn new(rows: &[Row], method: Method, own_model: bool, links: &[Option<Link>]) -> Self {
+        let mut out = Self { rows: vec![], links: vec![], keys: HashMap::new(), hexes: HashMap::new(), converted: Default::default() };
         for (n, r) in rows.iter().enumerate().filter(|(_, r)| !r.from.is_empty()) {
             let Some(to) = r.new_color() else { continue };
             let i = out.rows.len();
             let colors: Vec<Color> = r.from.iter().map(Match::color).collect();
-            out.rows.push(RowMap::new(method, &colors, to));
+            out.rows.push(RowMap::new(method, &colors, to).with_own_model(own_model));
             out.links.push(links.get(n).cloned().flatten());
             for m in &r.from {
                 match m {
@@ -296,7 +298,11 @@ impl ColorMap {
     }
 
     fn map(&self, c: Color) -> Color {
-        self.row_of(c).and_then(|i| self.rows.get(i)).map_or(c, |r| r.map(c))
+        let Some(r) = self.row_of(c).and_then(|i| self.rows.get(i)) else { return c };
+        if r.converts(c) {
+            self.converted.set(true);
+        }
+        r.map(c)
     }
 
     /// Recolour a colour that can link to a swatch: a row whose new colour is a global or spot
@@ -315,7 +321,12 @@ impl ColorMap {
                 return changed;
             }
             (Some(l), Some(t)) => l.color.tinted(t),
-            _ => row.map(*c),
+            _ => {
+                if row.converts(*c) {
+                    self.converted.set(true);
+                }
+                row.map(*c)
+            }
         };
         if new == *c {
             return false;
@@ -351,17 +362,42 @@ fn apply(s: &mut Session, p: &Value) -> Result<Value> {
     }
     let scope = Scope { fill: true, stroke: true, ..Scope::of(p) };
     let label = if ids.is_empty() { "Edit Color Group" } else { "Recolor Artwork" };
+    // `model`: keep each colour's model (default), take the new colour's own, or the document's.
+    // A given `model` must be a string; only a missing one means "keep".
+    let model = match p.get("model") {
+        None => "keep",
+        Some(Value::String(m)) => m.as_str(),
+        Some(other) => return Err(bad(C, format!("bad model {other} (keep, new or document)"))),
+    };
+    let mut doc_model = None;
+    let own_model = match model {
+        "keep" => false,
+        "new" => true,
+        "document" => {
+            let m = s.doc()?.doc.color_mode.model();
+            doc_model = Some(m);
+            for r in &mut rows {
+                r.to = r.to.map(|c| c.in_model(m));
+            }
+            true
+        }
+        other => return Err(bad(C, format!("unknown model `{other}` (keep, new or document)"))),
+    };
+    // The colours a colour group takes follow the model too (`document` converts them).
+    let in_doc_model = |c: Color| doc_model.map_or(c, |m| c.in_model(m));
     let new: Vec<Color> = match p.get("groupColors").and_then(Value::as_array) {
         Some(cs) => cs
             .iter()
             .map(|v| {
-                color_value(v).map(|c| palette.as_ref().map_or(c, |pl| pl.nearest(c))).ok_or_else(|| bad(C, format!("bad colour {v} in groupColors")))
+                color_value(v)
+                    .map(|c| in_doc_model(palette.as_ref().map_or(c, |pl| pl.nearest(c))))
+                    .ok_or_else(|| bad(C, format!("bad colour {v} in groupColors")))
             })
             .collect::<Result<_>>()?,
         None => rows.iter().filter_map(Row::new_color).collect(),
     };
     let rename = str_param(p, "rename");
-    let (changed, edit, added) = s.edit(label, |d, _| {
+    let (changed, edit, added, converted) = s.edit(label, |d, _| {
         // The rows' library swatches join the document (one it has with that name and colour is
         // used as it is).
         let mut added = vec![];
@@ -382,17 +418,24 @@ fn apply(s: &mut Session, p: &Value) -> Result<Value> {
                 Some(Link { name, global: w.global, color })
             })
             .collect();
-        let map = ColorMap::new(&rows, method, &links);
+        let map = ColorMap::new(&rows, method, own_model, &links);
         let f = |c: Color| map.map(c);
         let lf = |c: &mut Color, link: &mut Option<String>, tint: &mut f32| map.map_link(c, link, tint);
         let changed = Recolor::with_links(scope, &f, &lf).run(d, &ids);
         let edit = group.map(|g| super::swatch::rewrite_group(d, g, &new, rename, C)).transpose()?;
-        Ok((changed, edit, added))
+        Ok((changed, edit, added, map.converted.get()))
     })?;
     if let Some(e) = edit {
         e.defaults(s);
     }
-    Ok(json!({ "changed": changed, "swatches": added }))
+    let mut out = json!({ "changed": changed, "swatches": added });
+    if converted {
+        out["note"] = json!(
+            "some colours kept their own colour model, so the new colour was converted into it and may look different; \
+             pass model \"new\" (the new colour's model) or \"document\" (the document's colour mode) to change the model"
+        );
+    }
+    Ok(out)
 }
 
 fn randomize(_: &mut Session, p: &Value) -> Result<Value> {

crates/engine/src/tests_recolor.rs

@@ -369,3 +369,54 @@ fn recolor_to_a_library_spot_colour_links_it_and_adds_the_swatch() {
     let r = run(&mut s, "recolor.apply", json!({"map": map, "method": "scaleTints", "limitTo": lib}));
     assert_eq!(r["swatches"], json!(["Leaf"]));
 }
+
+#[test]
+fn model_param_chooses_the_model_of_the_new_colour() {
+    // RGB art in a CMYK document: by default a CMYK replacement comes out RGB (#2170).
+    let mut s = session();
+    run(&mut s, "file.new", json!({"width": 400, "height": 200, "colorMode": "cmyk"}));
+    let art = NodeId(run(&mut s, "shape.rectangle", json!({"x": 10, "y": 10, "width": 15, "height": 15}))["id"].as_u64().unwrap());
+    run(&mut s, "paint.setFill", json!({"ids": [art.0], "color": "#1a1a1a", "keepModel": true}));
+    assert_eq!(fill(&s, art).model(), vectorcraft_color::cms::Model::Rgb);
+    select_all(&mut s);
+    let map = json!({"#1a1a1a": {"c": 0, "m": 0, "y": 0, "k": 100}});
+    let r = run(&mut s, "recolor.apply", json!({"map": map}));
+    assert_eq!(fill(&s, art).model(), vectorcraft_color::cms::Model::Rgb, "kept RGB");
+    assert!(r["note"].as_str().is_some_and(|n| n.contains("model")), "warns the model was kept: {r}");
+    run(&mut s, "edit.undo", json!({}));
+    let r = run(&mut s, "recolor.apply", json!({"map": map, "model": "new"}));
+    assert_eq!(fill(&s, art), Color::cmyk(0.0, 0.0, 0.0, 1.0));
+    assert!(r.get("note").is_none(), "no note when the model follows the new colour: {r}");
+    run(&mut s, "edit.undo", json!({}));
+    // "document" converts a new RGB colour into the document's CMYK.
+    run(&mut s, "recolor.apply", json!({"map": {"#1a1a1a": "#ff0000"}, "model": "document"}));
+    assert_eq!(fill(&s, art).model(), vectorcraft_color::cms::Model::Cmyk);
+    assert!(s.execute("recolor.apply", &json!({"map": map, "model": "nope"})).is_err());
+}
+
+#[test]
+fn model_param_must_be_a_string_when_given() {
+    let mut s = session();
+    let art = rect(&mut s, json!("#1a1a1a"));
+    select_all(&mut s);
+    for bad in [json!(42), json!(null), json!(true)] {
+        assert!(s.execute("recolor.apply", &json!({"map": {"#1a1a1a": "#ff0000"}, "model": bad})).is_err(), "{bad}");
+    }
+    assert_eq!(fill(&s, art).to_hex(), "#1a1a1a", "nothing changed");
+}
+
+#[test]
+fn document_model_converts_the_colours_of_a_group_too() {
+    let mut s = session();
+    run(&mut s, "file.new", json!({"width": 400, "height": 200, "colorMode": "cmyk"}));
+    run(&mut s, "swatch.newGroup", json!({"name": "Theme", "colors": ["cmyk 0 0 0 50"]}));
+    let group = |s: &Session| -> Vec<Color> {
+        let g = s.doc().unwrap().doc.swatch_groups.iter().find(|g| g.name == "Theme").unwrap();
+        g.swatches.iter().filter_map(|w| w.paint.color()).collect()
+    };
+    // Group-only edit from the rows, and with explicit group colours.
+    run(&mut s, "recolor.apply", json!({"map": {"cmyk 0 0 0 50": "#ff0000"}, "group": "Theme", "model": "document"}));
+    assert!(group(&s).iter().all(|c| c.model() == vectorcraft_color::cms::Model::Cmyk), "{:?}", group(&s));
+    run(&mut s, "recolor.apply", json!({"map": [], "group": "Theme", "groupColors": ["#00ff00"], "model": "document"}));
+    assert!(group(&s).iter().all(|c| c.model() == vectorcraft_color::cms::Model::Cmyk), "{:?}", group(&s));
+}

crates/text/src/tests_ruby.rs

@@ -108,14 +108,24 @@ fn a_reading_wider_than_its_base_overhangs_or_is_squeezed() {
 #[test]
 fn ruby_aligns_start_centre_and_end_over_its_base() {
     let mut t = ruby_text("漢字", 6, "かん", 20.0);
-    let base = union_rect(layout(db(), &TextObject::point(Point::ZERO, "漢字", style(20.0))).glyphs.iter()).expect("base ink");
+    // Alignment uses advances, not ink bearings, which differ between installed fallback fonts.
+    let base_lay = layout(db(), &TextObject::point(Point::ZERO, "漢字", style(20.0)));
+    let base_line = &base_lay.lines[0];
+    let base = Rect::new(base_line.x0, 0.0, base_line.x1, 0.0);
+    let reading = layout(db(), &TextObject::point(Point::ZERO, "かん", style(10.0)));
+    let reading_line = &reading.lines[0];
+    let width = reading_line.x1 - reading_line.x0;
     for (align, check) in [
-        (RubyAlign::Start, (|r: Rect, b: Rect| (r.x0 - b.x0).abs() < 0.5) as fn(Rect, Rect) -> bool),
-        (RubyAlign::Center, |r: Rect, b: Rect| (((r.x0 + r.x1) - (b.x0 + b.x1)) * 0.5).abs() < 0.5),
-        (RubyAlign::End, |r: Rect, b: Rect| (r.x1 - b.x1).abs() < 0.5),
+        (RubyAlign::Start, (|r: Rect, b: Rect| (r.x0 - b.x0).abs() < 1e-6) as fn(Rect, Rect) -> bool),
+        (RubyAlign::Center, |r: Rect, b: Rect| (((r.x0 + r.x1) - (b.x0 + b.x1)) * 0.5).abs() < 1e-6),
+        (RubyAlign::End, |r: Rect, b: Rect| (r.x1 - b.x1).abs() < 1e-6),
     ] {
         t.ruby[0].align = align;
-        let ruby = ruby_ink(&layout(db(), &t));
+        let lay = layout(db(), &t);
+        // Ruby glyphs are attachments with zero advance: use their pen origin and the reading's
+        // standalone advance width. A wrong Start/Center/End placement still fails this test.
+        let x0 = lay.ruby[0].origin.x - reading.glyphs[0].origin.x + reading_line.x0;
+        let ruby = Rect::new(x0, 0.0, x0 + width, 0.0);
         assert!(check(ruby, base), "{align:?}: {ruby:?} over {base:?}");
     }
 }

crates/ui-egui/src/dialogs/recolor.rs

@@ -633,6 +633,16 @@ mod tests {
         ]
     }
 
+    #[test]
+    fn recolor_dialog_keeps_the_default_model_without_an_extra_dropdown() {
+        let mut app = VectorcraftApp::new(vectorcraft_engine::Session::new(), Default::default());
+        app.run("file.new", json!({"width": 100, "height": 100})).unwrap();
+        app.run("ui.recolorDialog", json!({"colors": ["#ff8000"]})).unwrap();
+        let d = app.ui.dialog.as_ref().unwrap();
+        assert!(!d.fields.contains_key("model"));
+        assert!(apply_params(d).get("model").is_none());
+    }
+
     #[test]
     fn merge_and_separate_rows() {
         let mut r = rows();

crates/ui-egui/src/i18n/untranslated_messages.txt

@@ -329,6 +329,7 @@ bad edit
 bad fill colour
 bad fill {v}
 bad fit: {e}
+bad model {other} (keep, new or document)
 bad new colour {v}
 bad object
 bad object number

docs/mcp.md

@@ -1535,7 +1535,13 @@ Recolor Artwork keys colours by identity, not hex: `recolor.colors` lists each c
 Lab and RGB colours that look alike stay apart, and every colour parameter also accepts a key.
 `recolor.reduce {colors?, method?, preserve?, limitTo?}` groups the selection's colours into rows of similar colours (k-means in Lab, weighted by use;
 tints of a global swatch share its row; White and Black are preserved by default) and returns a map of rows
-`[{from: [keys], to: key}]` to edit and pass to `recolor.apply {map, method?, limitTo?, group?, groupColors?, rename?}`.
+`[{from: [keys], to: key}]` to edit and pass to `recolor.apply {map, method?, model?, limitTo?, group?, groupColors?, rename?}`.
+For scripts and MCP, `model` selects the replacement color model: `keep` (default) retains each
+original color's model, `new` takes the replacement's model, and `document` converts replacements
+and edited color groups to the document's model. Invalid values fail before editing. This parameter
+is not a control in the Recolor Artwork dialog. A returned `note` reports a model conversion in
+`keep` mode.
+
 `method` says how a row's colours take its new colour: `exact`, `preserveTints` (tints of the row's darkest colour
 stay tints), `scaleTints` (every colour becomes a tint as light, relative to the darkest), `tintsShades` (lighter and
 darker than the row's average become tints and shades) or `hueShift` (the most saturated colour takes the new colour,