Keep the colour picker and open settings on their item after a removal

Fixes #442

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/ui-egui/src/lib.rs

@@ -118,6 +118,8 @@ mod online_tests;
 #[cfg(test)]
 mod open_issues_tests;
 #[cfg(test)]
+mod panel_remove_tests;
+#[cfg(test)]
 mod panel_reorder_tests;
 #[cfg(test)]
 mod panel_width_tests;

crates/ui-egui/src/list_drag.rs

@@ -181,6 +181,22 @@ pub fn move_flags(ui: &Ui, salt: &'static str, len: usize, from: usize, to: usiz
     });
 }
 
+/// Keep per-row flags (`Id::new((salt, index))`) with their rows when item `removed` of a
+/// `len`-long list goes; its own flag goes with it.
+pub fn remove_flags(ui: &Ui, salt: &'static str, len: usize, removed: usize) {
+    ui.data_mut(|d| {
+        let flags: Vec<Option<bool>> = (0..len).map(|i| d.get_temp::<bool>(Id::new((salt, i)))).collect();
+        for i in 0..len {
+            d.remove::<bool>(Id::new((salt, i)));
+        }
+        for (i, f) in flags.into_iter().enumerate() {
+            if let (Some(j), Some(v)) = (figcraft_engine::cmd::lists::removed_index(i, removed), f) {
+                d.insert_temp(Id::new((salt, j)), v);
+            }
+        }
+    });
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/ui-egui/src/panel_remove_tests.rs

@@ -0,0 +1,125 @@
+//! Removing fill, effect and layout grid rows in the Design panel, driven through egui headless: an
+//! open colour picker and open settings stay with their items, and go when their item goes.
+
+use egui::{Event, Id, Pos2};
+use figcraft_doc::NodeId;
+use figcraft_engine::Session;
+use serde_json::json;
+
+use crate::colorpicker::PickerTarget;
+use crate::layers_drag_tests::Ui;
+use crate::list_drag::grip_id;
+
+fn id(v: serde_json::Value) -> NodeId {
+    NodeId(v["id"].as_u64().unwrap())
+}
+
+fn click(u: &mut Ui, p: Pos2) {
+    u.button(p, true);
+    u.button(p, false);
+    u.frame(vec![Event::PointerMoved(Pos2::new(1430.0, 890.0))]);
+}
+
+/// The Remove button in item `index`'s row of list `list` (found in the AccessKit tree).
+fn remove_button(u: &mut Ui, list: &str, index: usize) -> Pos2 {
+    // Let the layout settle (settings just opened).
+    for _ in 0..5 {
+        u.frame(vec![]);
+    }
+    // Scroll the panel until the row is in view.
+    for _ in 0..20 {
+        let row = u.ctx.read_response(grip_id(list, index)).unwrap().rect;
+        if row.max.y < 860.0 {
+            break;
+        }
+        let wheel = Event::MouseWheel {
+            unit: egui::MouseWheelUnit::Point,
+            delta: egui::vec2(0.0, -100.0),
+            phase: egui::TouchPhase::Move,
+            modifiers: Default::default(),
+        };
+        u.frame(vec![Event::PointerMoved(Pos2::new(1300.0, 500.0)), wheel]);
+        // Let the scroll settle.
+        for _ in 0..30 {
+            u.frame(vec![]);
+        }
+    }
+    let out = u.frame_output(vec![]);
+    let row = u.ctx.read_response(grip_id(list, index)).unwrap().rect;
+    let all: Vec<Pos2> = out
+        .platform_output
+        .accesskit_update
+        .unwrap()
+        .nodes
+        .into_iter()
+        .filter(|(_, n)| n.label() == Some("Remove"))
+        .filter_map(|(_, n)| n.bounds())
+        .map(|b| Pos2::new(((b.x0 + b.x1) / 2.0) as f32, ((b.y0 + b.y1) / 2.0) as f32))
+        .collect();
+    all.iter()
+        .copied()
+        .find(|p| p.y > row.min.y && p.y < row.min.y + 32.0)
+        .unwrap_or_else(|| panic!("no Remove button in {list} row {index} {row:?} {all:?}"))
+}
+
+fn flag(u: &Ui, salt: &'static str, i: usize) -> bool {
+    u.ctx.data(|d| d.get_temp::<bool>(Id::new((salt, i)))).unwrap_or(false)
+}
+
+#[test]
+fn removing_a_fill_keeps_the_picker_on_its_paint() {
+    let mut s = Session::new();
+    let r = id(s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 50, "height": 50})).unwrap());
+    s.execute("fill.set", &json!({"ids": [r], "color": "#FF0000"})).unwrap();
+    s.execute("fill.add", &json!({"ids": [r], "color": "#00FF00"})).unwrap();
+    s.execute("fill.add", &json!({"ids": [r], "color": "#0000FF"})).unwrap();
+    s.execute("select.set", &json!({"ids": [r]})).unwrap();
+    let mut u = Ui::new(s);
+    u.ctx.enable_accesskit();
+    // Green's swatch, then red's Remove: the picker follows green to index 0.
+    let sw = u.ctx.read_response(Id::new(("swatch", true, 1usize))).unwrap().rect.center();
+    click(&mut u, sw);
+    assert_eq!(u.app.ui.picker, Some(PickerTarget::Paint { fills: true, index: 1, ids: vec![r] }));
+    let rm = remove_button(&mut u, "fills", 0);
+    click(&mut u, rm);
+    let fills: Vec<String> = u.app.session.doc().unwrap().node(r).unwrap().fills.iter().filter_map(|p| p.solid_color()).map(|c| c.to_hex()).collect();
+    assert_eq!(fills, ["00FF00", "0000FF"]);
+    assert_eq!(u.app.ui.picker, Some(PickerTarget::Paint { fills: true, index: 0, ids: vec![r] }));
+    // Removing the paint being edited closes the picker.
+    let rm = remove_button(&mut u, "fills", 0);
+    click(&mut u, rm);
+    assert_eq!(u.app.ui.picker, None);
+}
+
+#[test]
+fn removing_effects_and_grids_keeps_open_settings_with_their_rows() {
+    let mut s = Session::new();
+    let f = id(s.execute("frame.create", &json!({"x": 0, "y": 0, "width": 100, "height": 100})).unwrap());
+    for _ in 0..3 {
+        s.execute("effect.add", &json!({"ids": [f], "type": "dropShadow"})).unwrap();
+    }
+    s.execute("grid.add", &json!({"ids": [f], "grid": {"kind": "columns"}})).unwrap();
+    s.execute("grid.add", &json!({"ids": [f], "grid": {"kind": "rows"}})).unwrap();
+    s.execute("select.set", &json!({"ids": [f]})).unwrap();
+    let mut u = Ui::new(s);
+    u.ctx.enable_accesskit();
+    // Effect 2's settings open and its colour in the picker; effect 0 goes.
+    u.ctx.data_mut(|d| d.insert_temp(Id::new(("fx_open", 2usize)), true));
+    u.app.ui.picker = Some(PickerTarget::Effect { ids: vec![f], index: 2, secondary: false });
+    let rm = remove_button(&mut u, "effects", 0);
+    click(&mut u, rm);
+    assert_eq!(u.app.session.doc().unwrap().node(f).unwrap().effects.len(), 2);
+    assert!(flag(&u, "fx_open", 1) && !flag(&u, "fx_open", 2));
+    assert_eq!(u.app.ui.picker, Some(PickerTarget::Effect { ids: vec![f], index: 1, secondary: false }));
+    // Removing the open effect closes its settings and the picker.
+    let rm = remove_button(&mut u, "effects", 1);
+    click(&mut u, rm);
+    assert!(!flag(&u, "fx_open", 0) && !flag(&u, "fx_open", 1));
+    assert_eq!(u.app.ui.picker, None);
+    // Grid 1's settings open; grid 0 goes.
+    u.ctx.data_mut(|d| d.insert_temp(Id::new(("grid_open", 1usize)), true));
+    let rm = remove_button(&mut u, "grids", 0);
+    click(&mut u, rm);
+    assert_eq!(u.app.session.doc().unwrap().node(f).unwrap().grids.len(), 1);
+    assert!(flag(&u, "grid_open", 0) && !flag(&u, "grid_open", 1));
+}

crates/ui-egui/src/right.rs

@@ -3,13 +3,13 @@
 use egui::{Align2, Color32, CornerRadius, Id, Pos2, Rect, Sense, Stroke, Ui, pos2, vec2};
 use figcraft_color::{BlendMode, Paint, PaintKind};
 use figcraft_doc::*;
-use figcraft_engine::cmd::lists::moved_index;
+use figcraft_engine::cmd::lists::{moved_index, removed_index};
 use serde_json::{Value, json};
 
 use crate::a11y::{self, Tip};
 use crate::i18n::{tr, trf, trn};
 use crate::icons::{self, Icon};
-use crate::list_drag::{ListDrag, move_flags};
+use crate::list_drag::{ListDrag, move_flags, remove_flags};
 use crate::style_picker::{self, StyleSlot};
 use crate::theme::{ACCENT, Tokens, regular, semibold};
 use crate::widgets::{self, Edit, FIELD_H, FieldEdit, Lead, PAD};
@@ -1596,8 +1596,16 @@ fn paint_row(app: &mut FigApp, ui: &mut Ui, ids: &[NodeId], i: usize, p: &Paint,
         if icons::button(ui, if p.visible { Icon::Eye } else { Icon::EyeOff }, 24.0, false, tr("Toggle visibility")).clicked() {
             app.run(cmd, json!({"ids": ids, "index": i, "visible": !p.visible}));
         }
-        if icons::button(ui, Icon::Minus, 24.0, false, tr("Remove")).clicked() {
-            app.run(if fills { "fill.remove" } else { "stroke.remove" }, json!({"ids": ids, "index": i}));
+        if icons::button(ui, Icon::Minus, 24.0, false, tr("Remove")).clicked()
+            && app.run(if fills { "fill.remove" } else { "stroke.remove" }, json!({"ids": ids, "index": i})).is_some()
+            && let Some(crate::colorpicker::PickerTarget::Paint { fills: f, index, .. }) = &mut app.ui.picker
+            && *f == fills
+        {
+            // An open colour picker keeps editing the paint it was on, or closes with it.
+            match removed_index(*index, i) {
+                Some(k) => *index = k,
+                None => app.ui.picker = None,
+            }
         }
     })
 }
@@ -1627,6 +1635,7 @@ fn effects(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
     style_picker::chip(app, ui, doc, &ids, StyleSlot::Effect);
     let Some(list) = list else { return };
     let mut drag = ListDrag::new(ui, "effects");
+    let mut removed = None;
     for (i, e) in list.iter().enumerate().rev() {
         let mut open = effect_open(ui.ctx(), i);
         let header = row_rect(ui, 32.0, |ui| {
@@ -1651,8 +1660,10 @@ fn effects(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
                 e2.visible = !e2.visible;
                 app.run("effect.set", json!({"ids": ids, "index": i, "effect": e2}));
             }
-            if icons::button(ui, Icon::Minus, 24.0, false, tr("Remove")).clicked() {
-                app.run("effect.remove", json!({"ids": ids, "index": i}));
+            if icons::button(ui, Icon::Minus, 24.0, false, tr("Remove")).clicked()
+                && app.run("effect.remove", json!({"ids": ids, "index": i})).is_some()
+            {
+                removed = Some(i);
             }
         });
         if open {
@@ -1667,6 +1678,16 @@ fn effects(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
             *index = moved_index(*index, from, to);
         }
     }
+    // Open settings and an open colour picker stay with their effects, or go with theirs.
+    if let Some(i) = removed {
+        remove_flags(ui, FX_OPEN, list.len(), i);
+        if let Some(crate::colorpicker::PickerTarget::Effect { index, .. }) = &mut app.ui.picker {
+            match removed_index(*index, i) {
+                Some(k) => *index = k,
+                None => app.ui.picker = None,
+            }
+        }
+    }
 }
 
 /// Salt of the flags that keep effect rows' settings open.
@@ -1925,6 +1946,7 @@ fn grids(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
     let Some(list) = list else { return };
     // The last grid first, like paints and effects; rows drag to reorder.
     let mut drag = ListDrag::new(ui, "grids");
+    let mut removed = None;
     for (i, g) in list.iter().enumerate().rev() {
         let open_id = Id::new(("grid_open", i));
         let mut open = ui.data(|d| d.get_temp::<bool>(open_id)).unwrap_or(false);
@@ -1951,8 +1973,10 @@ fn grids(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
             if icons::button(ui, if g.visible { Icon::Eye } else { Icon::EyeOff }, 24.0, false, tr("Toggle")).clicked() {
                 app.set_props(&ids, json!({"grids": grids_with(&list, i, |x| x.visible = !x.visible)}));
             }
-            if icons::button(ui, Icon::Minus, 24.0, false, tr("Remove")).clicked() {
-                app.run("grid.remove", json!({"ids": ids, "index": i}));
+            if icons::button(ui, Icon::Minus, 24.0, false, tr("Remove")).clicked()
+                && app.run("grid.remove", json!({"ids": ids, "index": i})).is_some()
+            {
+                removed = Some(i);
             }
         });
         if open {
@@ -1964,6 +1988,10 @@ fn grids(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
         app.run("grid.move", json!({"ids": ids, "from": from, "to": to}));
         move_flags(ui, "grid_open", list.len(), from, to);
     }
+    // Open settings stay with their grids, or go with theirs.
+    if let Some(i) = removed {
+        remove_flags(ui, "grid_open", list.len(), i);
+    }
 }
 
 /// A layout grid's name in its row ("Grid (10)", "5 columns").