From 90cd4797ea9c65b3a2ce902e033dda82f3128254 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 20:31:45 -0800 Subject: [PATCH] Graphs: an added or duplicated top row sets the series' stored paint appearance.addFill, appearance.addStroke and appearance.duplicateItem can put a new topmost row on a series mark with the paint the mark already shows. When another mark of the series had set a different stored paint, the mark's top paint did not change, so the graph kept the other mark's paint and drew it again. A new topmost visible row now stores its paint, as an Eyedropper append already does. --- crates/engine/src/cmd/appearance.rs | 29 ++++++++++++++++-- crates/engine/src/cmd/graph/tests.rs | 44 ++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/appearance.rs b/crates/engine/src/cmd/appearance.rs index 4b478e1a7..30e48d4fc 100644 --- a/crates/engine/src/cmd/appearance.rs +++ b/crates/engine/src/cmd/appearance.rs @@ -551,12 +551,17 @@ fn add_item(s: &mut Session, p: &Value, fill: bool) -> Result { // Where the new fill went in on the active item's object, when it went above the active row. let mut above = None; s.edit(if fill { "Add New Fill" } else { "Add New Stroke" }, |d, _| { + // The objects that got a new topmost row of the kind: the series stores its paint even when the object showed it. + let mut added = Vec::new(); 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(paint) = n.appearance.paint_at(Some(i), true).cloned() { n.appearance.insert_item(i + 1, AppearanceItem::Fill(FillLayer::new(paint))); + if let Some(kind) = top_row_kind(&n.appearance, i + 1) { + added.push((*id, kind)); + } if active == Some(*id) { above = Some(i + 1); } @@ -591,11 +596,14 @@ fn add_item(s: &mut Session, p: &Value, fill: bool) -> Result { } } else { n.appearance.items.push(item); + added.push((*id, fill)); } } } super::graph::capture_series_opacity(d, &top_opacity_changed(&before, d, &ids)); - super::graph::capture_series_top_paints(d, &top_paint_changed(&before, d, &ids)); + let mut rows = top_paint_changed(&before, d, &ids); + rows.extend(painted_rows(d, added)); + super::graph::capture_series_top_paints(d, &rows); Ok(()) })?; // A fill added above the active row becomes the active row; otherwise the rows above the new @@ -653,6 +661,12 @@ fn painted_rows(d: &vectorcraft_doc::Document, rows: impl IntoIterator Option { + let fill = ap.items.get(at)?.is_fill(); + (ap.items.iter().rposition(|it| it.is_fill() == fill && it.visible()) == Some(at)).then_some(fill) +} + /// Clear Appearance on `n`: no fill, no stroke, and the object's Opacity row back to Default. A /// group keeps no fill or stroke rows of its own. fn clear(n: &mut Node) { @@ -898,6 +912,9 @@ fn duplicate_item(s: &mut Session, p: &Value) -> Result { let per = items_per_target(&before, &ids, &idx, aligned(p, C)?); s.edit("Duplicate Item", |d, _| { let mut any = false; + // Copies that became the topmost visible row of their kind: the series stores their paint even when the object + // showed it. + let mut added = Vec::new(); for (id, idx) in ids.iter().zip(&per) { let Some(n) = d.node_mut(*id) else { continue }; let has_contents = n.contents_label().is_some(); @@ -913,9 +930,11 @@ fn duplicate_item(s: &mut Session, p: &Value) -> Result { if active == Some(*id) { active_at = Some(at); } + let count = copies.len(); for (k, item) in copies.into_iter().enumerate() { ap.insert_item(at + k, item); } + added.extend((at..at + count).filter_map(|k| top_row_kind(ap, k)).map(|kind| (*id, kind))); if let Some(c) = contents.filter(|_| has_contents) { ap.set_contents_at(c); } @@ -929,6 +948,10 @@ fn duplicate_item(s: &mut Session, p: &Value) -> Result { if let Some(item) = ap.items.get(i).cloned() { first_at.get_or_insert(i + 1); ap.insert_item(i + 1, item); + // Copies below this one go in lower down, so a copy that is the topmost row of its kind stays it. + if let Some(kind) = top_row_kind(ap, i + 1) { + added.push((*id, kind)); + } any = true; } } @@ -937,7 +960,9 @@ fn duplicate_item(s: &mut Session, p: &Value) -> Result { return Err(bad(C, format!("no item at index {}", idx[0]))); } super::graph::capture_series_opacity(d, &top_opacity_changed(&before, d, &ids)); - super::graph::capture_series_top_paints(d, &top_paint_changed(&before, d, &ids)); + let mut rows = top_paint_changed(&before, d, &ids); + rows.extend(painted_rows(d, added)); + super::graph::capture_series_top_paints(d, &rows); Ok(()) })?; // Rows at or above each copy move up by one (by the block, for copies placed at `to`). diff --git a/crates/engine/src/cmd/graph/tests.rs b/crates/engine/src/cmd/graph/tests.rs index a626d05e4..8b059c111 100644 --- a/crates/engine/src/cmd/graph/tests.rs +++ b/crates/engine/src/cmd/graph/tests.rs @@ -1211,6 +1211,50 @@ fn appending_a_visible_row_under_a_hidden_one_replaces_the_kept_stroke() { assert_eq!(kept.stroke_width, Some(2.0)); } +/// A stroke added on top of a mark replaces the stroke another mark of the series left on the graph, even when the new +/// row shows the paint the mark already showed. +#[test] +fn adding_a_stroke_replaces_the_kept_stroke_with_the_marks_own() { + let (mut s, id, _, marks) = column_marks(); + let stroke = top_row(&s, marks[0], false); + s.execute("appearance.setItem", &json!({"ids": [marks[1].0], "index": stroke, "color": "#ff0000", "weight": 2})).unwrap(); + s.execute("appearance.setItem", &json!({"ids": [marks[0].0], "index": stroke, "color": "#0000ff", "weight": 5})).unwrap(); + s.execute("appearance.addStroke", &json!({"ids": [marks[1].0]})).unwrap(); + let kept = &spec(&s, id).series_paints[0]; + assert_eq!(kept.stroke, Some(Paint::solid(vectorcraft_color::Color::rgb(1.0, 0.0, 0.0)))); + assert_eq!(kept.stroke_width, Some(2.0)); +} + +/// A fill added right above a mark's active top fill replaces the fill another mark of the series left on the graph. +#[test] +fn adding_a_fill_above_the_active_top_fill_replaces_the_kept_fill() { + let (mut s, id, was, marks) = column_marks(); + let fill = top_row(&s, marks[0], true); + s.execute("appearance.setItem", &json!({"ids": [marks[0].0], "index": fill, "color": "#0000ff"})).unwrap(); + s.execute("select.set", &json!({"ids": [marks[1].0]})).unwrap(); + s.execute("appearance.setActiveItem", &json!({"index": fill})).unwrap(); + s.execute("appearance.addFill", &json!({})).unwrap(); + assert_eq!(spec(&s, id).series_paints[0].fill, Some(was)); +} + +/// A copy of a mark's top stroke replaces the stroke another mark of the series left on the graph. +#[test] +fn duplicating_a_marks_top_stroke_replaces_the_kept_stroke() { + let (mut s, id, _, marks) = column_marks(); + let stroke = top_row(&s, marks[0], false); + s.execute("appearance.setItem", &json!({"ids": [marks[1].0], "index": stroke, "color": "#ff0000", "weight": 2})).unwrap(); + s.execute("appearance.setItem", &json!({"ids": [marks[0].0], "index": stroke, "color": "#0000ff", "weight": 5})).unwrap(); + for to in [false, true] { + s.execute("appearance.setItem", &json!({"ids": [marks[0].0], "index": stroke, "color": "#0000ff", "weight": 5})).unwrap(); + let top = top_row(&s, marks[1], false); + let p = if to { json!({"ids": [marks[1].0], "index": top, "to": top + 1}) } else { json!({"ids": [marks[1].0], "index": top}) }; + s.execute("appearance.duplicateItem", &p).unwrap(); + let kept = &spec(&s, id).series_paints[0]; + assert_eq!(kept.stroke, Some(Paint::solid(vectorcraft_color::Color::rgb(1.0, 0.0, 0.0))), "{p}"); + assert_eq!(kept.stroke_width, Some(2.0), "{p}"); + } +} + /// A weight set on a stroke without paint (a line graph's marker) leaves the paint the series keeps for its line. #[test] fn a_weight_on_a_stroke_without_paint_leaves_the_kept_stroke() {