From caae1c8a1f29560766a27ca20ef718dd2772cc79 Mon Sep 17 00:00:00 2001 From: David Mann Date: Sun, 8 Nov 2026 02:19:35 +0100 Subject: [PATCH] Address CodeRabbit review: close the sampler menu when its sampler changes The Color Sampler menu kept only the sampler's index. If a control command deleted an earlier sampler while the menu was open, the index named another sampler and Delete removed that one. The menu now also remembers the active document and the sampler's position; when either no longer matches, Delete greys out and the menu closes. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas/canvas_tool_menu.rs | 24 +++++++++++++++-- .../tools/analysis_ui/sampler_menu_tests.rs | 27 +++++++++++++++++++ crates/ui-egui/src/transform_type_tests.rs | 1 + 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/canvas/canvas_tool_menu.rs b/crates/ui-egui/src/canvas/canvas_tool_menu.rs index 098304d147..50059007ad 100644 --- a/crates/ui-egui/src/canvas/canvas_tool_menu.rs +++ b/crates/ui-egui/src/canvas/canvas_tool_menu.rs @@ -50,6 +50,11 @@ pub struct CanvasToolMenu { /// `None` off a sampler, where every row is greyed. #[serde(default)] pub sampler: Option, + /// Where that sampler was when the menu opened: the active document's index and the sampler's + /// position. Once a control command deletes, moves or renumbers samplers, or another document + /// becomes active, the index may name a different sampler, so the menu closes and Delete greys. + #[serde(default)] + pub sampler_at: Option<(usize, [f64; 2])>, } /// A misspelled word in the type layer being edited (`edit.checkSpelling`'s `list` entry). @@ -308,6 +313,7 @@ pub fn open_crop(app: &mut PhotocraftApp, ctx: &Context, tool: Tool, pos: [f32; spelling: None, crop: Some(users), sampler: None, + sampler_at: None, }); crate::canvas_popup::mark_open(ctx, menu_id()); true @@ -478,6 +484,14 @@ pub const SAMPLER_MENU: &[Row] = &[ Some(("Total Ink", "colorSampler.readout.totalInk")), ]; +/// Does the Color Sampler menu's index still name the sampler it opened on, in the same document? +/// `true` off a sampler, where there is nothing to go stale. +fn sampler_current(app: &PhotocraftApp, menu: &CanvasToolMenu) -> bool { + let Some(i) = menu.sampler else { return true }; + let now = app.session.active_index().zip(app.session.active().and_then(|d| d.color_samplers.get(i).copied())); + now.is_some() && now == menu.sampler_at +} + /// Is `menu` the Color Sampler's own menu (not the pasteboard, guide or transform menu opened /// with it)? fn sampler_menu(menu: &CanvasToolMenu) -> bool { @@ -693,6 +707,7 @@ pub fn open_transform(app: &mut PhotocraftApp, ctx: &Context, pos: [f32; 2]) -> spelling: None, crop: None, sampler: None, + sampler_at: None, }); crate::canvas_popup::mark_open(ctx, menu_id()); true @@ -775,6 +790,7 @@ pub fn open_pasteboard(app: &mut PhotocraftApp, ctx: &Context, tool: Tool, pos: spelling: None, crop: None, sampler: None, + sampler_at: None, }); crate::canvas_popup::mark_open(ctx, menu_id()); true @@ -807,6 +823,7 @@ pub fn open_guide(app: &mut PhotocraftApp, ctx: &Context, tool: Tool, mods: egui spelling: None, crop: None, sampler: None, + sampler_at: None, }); crate::canvas_popup::mark_open(ctx, menu_id()); true @@ -861,8 +878,7 @@ pub fn entry_enabled(app: &PhotocraftApp, menu: &CanvasToolMenu, command: &str) } // Off a sampler every row is greyed. The readout rows wait for per-sampler readout modes. if sampler_menu(menu) { - let count = app.session.active().map_or(0, |d| d.color_samplers.len()); - return command == SAMPLER_DELETE && menu.sampler.is_some_and(|i| i < count); + return command == SAMPLER_DELETE && menu.sampler.is_some() && sampler_current(app, menu); } if menu.transform || !path_menu(menu.tool) { return crate::menus::is_enabled(app, command); @@ -925,6 +941,7 @@ pub fn open_at(app: &mut PhotocraftApp, ctx: &Context, tool: Tool, pos: [f32; 2] let path_name = crate::vector_ui::active_path_name(app); let spelling = if text { doc.and_then(|d| spelling_at(app, d)) } else { None }; let sampler = if tool == Tool::ColorSampler { doc.and_then(|d| crate::analysis_ui::sampler_at(app, d)) } else { None }; + let sampler_at = sampler.and_then(|i| Some((app.session.active_index()?, *app.session.active()?.color_samplers.get(i)?))); app.ui.canvas_tool_menu = Some(CanvasToolMenu { pos, tool, @@ -940,6 +957,7 @@ pub fn open_at(app: &mut PhotocraftApp, ctx: &Context, tool: Tool, pos: [f32; 2] spelling, crop: None, sampler, + sampler_at, }); crate::canvas_popup::mark_open(ctx, menu_id()); true @@ -968,6 +986,7 @@ pub fn open_view(app: &mut PhotocraftApp, ctx: &Context, tool: Tool, pos: [f32; spelling: None, crop: None, sampler: None, + sampler_at: None, }); crate::canvas_popup::mark_open(ctx, menu_id()); true @@ -1213,6 +1232,7 @@ pub fn show(app: &mut PhotocraftApp, ctx: &Context) { || (menu.text && app.ui.text_edit.is_none()) || (menu.type_layer && app.ui.text_edit.is_some()) || (menu.crop.is_some() && !crate::crop_ui::pending(app)) + || (sampler_menu(&menu) && !sampler_current(app, &menu)) { app.ui.canvas_tool_menu = None; return; diff --git a/crates/ui-egui/src/tools/analysis_ui/sampler_menu_tests.rs b/crates/ui-egui/src/tools/analysis_ui/sampler_menu_tests.rs index feef455427..9388a31f68 100644 --- a/crates/ui-egui/src/tools/analysis_ui/sampler_menu_tests.rs +++ b/crates/ui-egui/src/tools/analysis_ui/sampler_menu_tests.rs @@ -112,6 +112,33 @@ fn delete_removes_the_sampler_under_the_pointer_and_renumbers_the_rest() { assert_eq!(listed["samplers"][1]["number"], json!(2)); } +#[test] +fn delete_never_removes_another_sampler_after_the_list_changes_under_the_menu() { + let (mut app, ctx) = app(); + app.run("view.colorSamplers.add", json!({"x": 180, "y": 90})).unwrap(); + right_click(&mut app, &ctx, 120.0, 60.0); + assert_eq!(app.ui.canvas_tool_menu.as_ref().and_then(|m| m.sampler), Some(1)); + // An agent deletes #1 while the menu is open: index 1 now names the old #3. + app.run("view.colorSamplers.delete", json!({"index": 0})).unwrap(); + let r = control(&mut app, &ctx, "ui.context.choose", json!({"id": SAMPLER_DELETE})); + assert_eq!(r["ok"], json!(false), "Delete greys out: {r}"); + assert_eq!(samplers(&app), [[120.0, 60.0], [180.0, 90.0]], "no other sampler is deleted"); + // The menu itself closes on the next frame. + right_click(&mut app, &ctx, 120.0, 60.0); + app.run("view.colorSamplers.move", json!({"index": 0, "x": 30, "y": 30})).unwrap(); + ctx.run_ui(Default::default(), |ui| crate::canvas_tool_menu::show(&mut app, ui.ctx())).textures_delta.clear(); + assert!(app.ui.canvas_tool_menu.is_none(), "a moved sampler closes the menu"); + // Another document with a sampler at the same index and spot: still not this menu's sampler. + right_click(&mut app, &ctx, 180.0, 90.0); + assert_eq!(app.ui.canvas_tool_menu.as_ref().and_then(|m| m.sampler), Some(1)); + app.run("file.new", json!({"width": 200, "height": 100})).unwrap(); + for (x, y) in [(20, 20), (180, 90)] { + app.run("view.colorSamplers.add", json!({"x": x, "y": y})).unwrap(); + } + let menu = app.ui.canvas_tool_menu.clone().expect("the menu is still open"); + assert!(!crate::canvas_tool_menu::entry_enabled(&app, &menu, SAMPLER_DELETE), "another document's sampler"); +} + #[test] fn readout_rows_do_nothing_until_samplers_have_readout_modes() { let (mut app, ctx) = app(); diff --git a/crates/ui-egui/src/transform_type_tests.rs b/crates/ui-egui/src/transform_type_tests.rs index c62e7cf87d..e179e4f6d5 100644 --- a/crates/ui-egui/src/transform_type_tests.rs +++ b/crates/ui-egui/src/transform_type_tests.rs @@ -57,6 +57,7 @@ fn type_layers_offer_free_transform_but_not_distort_or_perspective() { spelling: None, crop: None, sampler: None, + sampler_at: None, }; assert!(crate::canvas_tool_menu::available(&app, &menu, "edit.freeTransform")); assert!(!crate::canvas_tool_menu::available(&app, &menu, "edit.transform.distort"));