diff --git a/crates/engine/src/cmd/appearance.rs b/crates/engine/src/cmd/appearance.rs index 4531f22cd..27f5aee81 100644 --- a/crates/engine/src/cmd/appearance.rs +++ b/crates/engine/src/cmd/appearance.rs @@ -45,7 +45,7 @@ pub fn specs() -> Vec { "Add New Fill", ["Window", "Appearance"], Some("Cmd+/"), - "{ids?|id?, target?: \"object\"|\"contents\"} add a fill on top of each selected object's own stack, on an image right below its Contents row (a copy of its top fill; type without one: its characters' fill; else the default fill). With a fill row active (appearance.setActiveItem), no `ids` and no `target: \"contents\"`, the new fill is a copy of that row, right above it, and becomes the active row", + "{ids?|id?, target?: \"object\"|\"contents\"} add a fill on top of each selected object's own stack, on an image right below its Contents row, with its top fill's paint (type without one: its characters' fill; else the default fill). With a fill row active (appearance.setActiveItem), no `ids` and no `target: \"contents\"`, the new fill takes that row's paint, goes right above it, and becomes the active row; appearance.duplicateItem copies a whole row", has_doc, |s, p| add_item(s, p, true) ), @@ -54,7 +54,7 @@ pub fn specs() -> Vec { "Add New Stroke", ["Window", "Appearance"], Some("Cmd+Alt+/"), - "{ids?|id?, target?} add a stroke on top of each selected object's own stack, on an image right below its Contents row (a copy of its top stroke, else the default stroke)", + "{ids?|id?, target?} add a stroke on top of each selected object's own stack, on an image right below its Contents row, with its top stroke's paint and weight (else the default stroke)", has_doc, |s, p| add_item(s, p, false) ), @@ -519,8 +519,8 @@ fn set_active_item(s: &mut Session, p: &Value) -> Result { fn add_item(s: &mut Session, p: &Value, fill: bool) -> Result { let ids = require_targets(s, p, if fill { "appearance.addFill" } else { "appearance.addStroke" })?; let defaults = s.paint.clone(); - // The panel's active Fill row, when the new fill goes on the selection: the new fill is a copy - // of that row, right above it in each target that has it (others get a copy of their top fill). + // The panel's active Fill row, when the new fill goes on the selection: the new fill takes that + // row's paint, right above it in each target that has it (others take their top fill's paint). // Where the new item went in on the active item's object, when it is an image. let active = s.active_appearance_item.as_ref().and_then(|a| a.objects.first().copied()); // The active row is a row of the active object's stack, which need not come first in `ids` @@ -551,9 +551,9 @@ fn add_item(s: &mut Session, p: &Value, fill: bool) -> Result { for (id, row) in ids.iter().zip(&per) { if let Some(n) = d.node_mut(*id) { if let Some(i) = n.appearance.item_of_kind(*row, true) - && let Some(row) = n.appearance.fill_at(Some(i)).cloned() + && let Some(paint) = n.appearance.paint_at(Some(i), true).cloned() { - n.appearance.insert_item(i + 1, AppearanceItem::Fill(row)); + n.appearance.insert_item(i + 1, AppearanceItem::Fill(FillLayer::new(paint))); if active == Some(*id) { above = Some(i + 1); } diff --git a/crates/engine/src/tests/appearance.rs b/crates/engine/src/tests/appearance.rs index c07fc8eb7..4fea39bdf 100644 --- a/crates/engine/src/tests/appearance.rs +++ b/crates/engine/src/tests/appearance.rs @@ -3,7 +3,7 @@ use serde_json::{Value, json}; use vectorcraft_color::{BlendMode, Color, GradientKind, Paint}; -use vectorcraft_doc::{AppearanceItem, Node}; +use vectorcraft_doc::{AppearanceItem, FillLayer, Node}; use crate::*; @@ -288,8 +288,8 @@ fn add_fill_and_stroke_copy_the_top_ones() { assert_eq!(node(&s, id).appearance.items.len(), 3); } -/// With a fill row active, Add New Fill puts a copy of that row right above it and makes the copy -/// the active row; the rows above it stay where they are. +/// With a fill row active, Add New Fill puts a fill with that row's paint right above it and makes +/// the new fill the active row; the rows above it stay where they are. #[test] fn add_fill_copies_the_active_fill_row_right_above_it() { let mut s = session(); @@ -323,10 +323,10 @@ fn add_fill_copies_the_active_fill_row_right_above_it() { assert_eq!(s.appearance_item(), Some(0)); } -/// The copy of the active fill row keeps the row's opacity, blend mode, visibility, overprint and -/// effects as well as its paint. +/// The new fill above the active fill row takes only the row's paint: the row's opacity, blend +/// mode, visibility, overprint and effects are not copied (Duplicate Item copies those). #[test] -fn add_fill_copies_the_whole_active_fill_row() { +fn add_fill_takes_only_the_active_fill_rows_paint() { let mut s = session(); let id = rect(&mut s, 100.0); run(&mut s, "paint.setFill", json!({"color": "#ff0000"})); @@ -340,13 +340,13 @@ fn add_fill_copies_the_whole_active_fill_row() { let (AppearanceItem::Fill(row), AppearanceItem::Fill(copy)) = (&n.appearance.items[0], &n.appearance.items[1]) else { panic!("the first two rows are not fills") }; - assert_eq!((copy.opacity, copy.blend, copy.visible, copy.overprint), (0.4, BlendMode::Multiply, false, true)); - assert_eq!(copy.effects.len(), 1); - assert_eq!(copy, row); + assert_eq!((row.opacity, row.blend, row.visible, row.overprint), (0.4, BlendMode::Multiply, false, true)); + assert_eq!(row.effects.len(), 1); + assert_eq!(copy, &FillLayer::new(row.paint.clone())); } /// In a mixed selection the active fill row goes above itself in the first object; an object that -/// doesn't share that row gets a copy of its top fill on top, as without an active row. +/// doesn't share that row gets a fill with its top fill's paint on top, as without an active row. #[test] fn add_fill_above_an_unshared_active_row_tops_the_other_objects() { let mut s = session();