Palette: a pick opens the dialog its menu item opens

A pick in Search Commands ran its command with no params, while the
menu bar item gives the command params and opens a dialog for them.
Clean Up, Create Gradient Mesh, Smart Punctuation, Release Type on a
Path and Deep Ungroup ran at once with their default values, Find and
Replace reported "missing `find`", and Print Ink Fixes reported that it
needs a value. A pick now runs what a click on the menu item runs, so
these open their dialogs and nothing changes until the dialog is
confirmed.

While the menu item is dimmed (Remove Background or Object Mosaic with
no image selected, Find and Replace with no document open), a pick and
a shortcut open no dialog, and the status line says why the command
can't run. Before this, a shortcut of such a command opened its dialog,
and confirming it failed. Both routes use one helper,
shortcut_editor::menu_target_now.

The menu items that helper reads are read once. Items that list an
installed plug-in (File › Open With importers, Object › Plug-ins
filters) are now left out of them, so a plug-in installed or removed
later does not leave an item there that the menu no longer shows.

crates/ui-egui/src/menus.rs

@@ -71,7 +71,6 @@ pub(crate) use invoke::graph_dialog;
 #[cfg(test)]
 pub(crate) use invoke::with_held_keys;
 pub use invoke::{click_target, invoke, invoke_from_system_menu};
-#[cfg(test)]
 pub(crate) use labels::lists_plugin;
 #[cfg(test)]
 use labels::shows_a_name;

crates/ui-egui/src/palette.rs

@@ -91,7 +91,10 @@ fn build(lang: crate::i18n::Lang) -> Vec<Entry> {
 }
 
 /// Runs the palette result `id`: selects a tool, or invokes a command with the params its entry
-/// stands for.
+/// stands for. A command whose menu bar item gives params runs what a click on that item runs
+/// ([`crate::shortcut_editor::menu_target_now`]), so Clean Up… and Find and Replace… open their
+/// dialogs. While that item is dimmed, no dialog opens and the status line says why the command
+/// can't run.
 pub(crate) fn run_pick(app: &mut VectorcraftApp, id: &str) {
     if let Some(tool) = id.strip_prefix("tool:") {
         app.select_tool(tool);
@@ -99,7 +102,11 @@ pub(crate) fn run_pick(app: &mut VectorcraftApp, id: &str) {
         let command = crate::keymap::command_of(id);
         menus::invoke(app, &command.id, command.params);
     } else {
-        menus::invoke(app, id, json!({}));
+        match crate::shortcut_editor::menu_target_now(app, id) {
+            Some(Ok((id, p))) => menus::invoke(app, &id, p),
+            Some(Err(why)) => app.status(why),
+            None => menus::invoke(app, id, json!({})),
+        }
     }
 }
 
@@ -217,7 +224,7 @@ mod tests {
         let mut app = VectorcraftApp::new(vectorcraft_engine::Session::new(), crate::Services::default());
         let before = app.session.prefs.use_precise_cursors;
         let first = matches.first().expect("query has a result");
-        menus::invoke(&mut app, &first.id, json!({}));
+        run_pick(&mut app, &first.id);
         assert_ne!(app.session.prefs.use_precise_cursors, before, "first result toggles the preference");
     }
 
@@ -238,8 +245,7 @@ mod tests {
         app.run("select.set", json!({"ids": [a, b]})).unwrap();
         app.run("select.key", json!({"id": b})).unwrap();
         assert!(app.session.active().unwrap().selection.key.is_some());
-        // Run a pick the way `show` runs it.
-        menus::invoke(&mut app, ids[0], json!({}));
+        run_pick(&mut app, ids[0]);
         let st = app.session.active().unwrap();
         assert_eq!((st.selection.len(), st.selection.key), (2, None), "both objects stay selected, without a key");
     }
@@ -255,20 +261,162 @@ mod tests {
             app.run("file.new", json!({"width": 200, "height": 200})).unwrap();
             app.ui.status.clear();
             assert!(!crate::panel_docking::showing(&app.ui, crate::panels::asset_export::ID), "{id}");
-            // Run a pick the way `show` runs it.
-            menus::invoke(&mut app, id, json!({}));
+            run_pick(&mut app, id);
             assert!(crate::panel_docking::showing(&app.ui, crate::panels::asset_export::ID), "{id}: {}", app.ui.status);
             assert!(!app.ui.status.contains("invalid parameters"), "{id}: {}", app.ui.status);
             // A second pick leaves the open panel open, docked or floating.
-            menus::invoke(&mut app, id, json!({}));
+            run_pick(&mut app, id);
             assert!(crate::panel_docking::showing(&app.ui, crate::panels::asset_export::ID), "{id}: picked twice");
             app.run("window.panel.float", json!({"panel": crate::panels::asset_export::ID})).unwrap();
             assert!(crate::panel_docking::showing(&app.ui, crate::panels::asset_export::ID), "{id}: floating");
-            menus::invoke(&mut app, id, json!({}));
+            run_pick(&mut app, id);
             assert!(crate::panel_docking::showing(&app.ui, crate::panels::asset_export::ID), "{id}: floating, picked again");
         }
     }
 
+    /// The open dialog's kind and fields, the active document's undo depth and the status line.
+    type Outcome = (Option<(String, serde_json::Map<String, serde_json::Value>)>, Option<usize>, String);
+
+    /// The [`Outcome`] `app` shows.
+    fn outcome(app: &VectorcraftApp) -> Outcome {
+        (
+            app.ui.dialog.as_ref().map(|d| (d.kind.clone(), d.fields.clone())),
+            app.session.active().map(|st| st.history.undo.len()),
+            app.ui.status.clone(),
+        )
+    }
+
+    /// A pick of a "…" command whose menu bar item gives params opens that item's dialog, and Smooth
+    /// opens its slider; nothing changes until the dialog is confirmed.
+    #[test]
+    fn a_pick_opens_the_dialog_its_menu_item_opens() {
+        let expected = [
+            ("object.path.cleanUp", "command"),
+            ("object.mesh.create", "command"),
+            ("type.smartPunctuation", "command"),
+            ("edit.findReplace", "command"),
+            ("object.path.smooth", crate::dialogs::smooth_slider::KIND),
+        ];
+        for (id, kind) in expected {
+            assert!(items().iter().any(|(_, item, _)| item == id), "{id} is in the palette");
+            let mut app = VectorcraftApp::new(vectorcraft_testkit::fixtures::Fixture::Single.session(), crate::Services::default());
+            app.ui.status.clear();
+            let before = app.session.active().unwrap().history.undo.len();
+            run_pick(&mut app, id);
+            let d = app.ui.dialog.as_ref().unwrap_or_else(|| panic!("{id} opens its dialog: {}", app.ui.status));
+            assert_eq!(d.kind, kind, "{id}");
+            if kind == "command" {
+                assert_eq!(d.fields["__command"], json!(id), "{id}: {:?}", d.fields);
+            }
+            assert_eq!(app.session.active().unwrap().history.undo.len(), before, "{id} changes nothing yet");
+            assert!(app.ui.status.is_empty(), "{id}: {}", app.ui.status);
+        }
+    }
+
+    /// Every palette command whose menu bar items all give it the same params does what a click on
+    /// such an item does, as the menu row shows it ([`menus::entry`]): with no document open, in an
+    /// empty document and with one or several objects selected, the pick opens the same dialog with
+    /// the same fields and leaves the same undo steps and status. While the row is dimmed, the pick
+    /// opens no dialog, changes nothing and says why the command can't run.
+    #[test]
+    fn every_pick_runs_what_its_menu_item_runs() {
+        use vectorcraft_testkit::fixtures::Fixture;
+        fn walk<'a>(items: &'a [menus::Item], out: &mut Vec<&'a menus::Item>) {
+            for it in items {
+                match it {
+                    menus::Item::Sub(_, children) => walk(children, out),
+                    // Items that list an installed plug-in come and go as tests beside this one
+                    // install plug-ins, and no shortcut or pick reads them.
+                    menus::Item::Cmd(_, id, p) if !menus::lists_plugin(id, p) => out.push(it),
+                    _ => {}
+                }
+            }
+        }
+        let tree = menus::menu_tree();
+        let mut rows = vec![];
+        for (_, items) in &tree {
+            walk(items, &mut rows);
+        }
+        let mut checked: Vec<(String, &menus::Item)> = vec![];
+        for (_, id, _) in items() {
+            let of_id: Vec<&menus::Item> = rows.iter().copied().filter(|it| matches!(it, menus::Item::Cmd(_, c, _) if *c == id)).collect();
+            let params: Vec<&serde_json::Value> =
+                of_id.iter().filter_map(|it| if let menus::Item::Cmd(_, _, p) = it { Some(p) } else { None }).collect();
+            let Some(first) = params.first() else { continue };
+            if first.as_object().is_none_or(serde_json::Map::is_empty) || params.iter().any(|p| p != first) {
+                continue;
+            }
+            checked.push((id, of_id[0]));
+        }
+        assert!(checked.iter().any(|(id, _)| id == "edit.findReplace"), "{:?}", checked.iter().map(|c| &c.0).collect::<Vec<_>>());
+        let state = |n: usize| match n.checked_sub(1) {
+            None => vectorcraft_engine::Session::new(),
+            Some(k) => Fixture::ALL[k].session(),
+        };
+        let (mut compared, mut dimmed) = (0, 0);
+        for (id, item) in &checked {
+            for n in 0..=Fixture::ALL.len() {
+                let mut picked = VectorcraftApp::new(state(n), crate::Services::default());
+                let mut clicked = VectorcraftApp::new(state(n), crate::Services::default());
+                picked.ui.status.clear();
+                clicked.ui.status.clear();
+                // A row the menu leaves out in this state has no click to compare with.
+                let Some(menus::Entry::Item(row)) = menus::entry(&clicked, item) else { continue };
+                let (target, enabled) = (row.target().expect("the item is implemented"), row.enabled);
+                let before = outcome(&picked);
+                run_pick(&mut picked, id);
+                if enabled {
+                    menus::invoke(&mut clicked, &target.0, target.1);
+                    assert_eq!(outcome(&picked), outcome(&clicked), "{id} [state {n}]");
+                    compared += 1;
+                } else {
+                    let after = outcome(&picked);
+                    assert_eq!((after.0, after.1), (None, before.1), "{id} [state {n}] is dimmed");
+                    assert!(after.2.contains("not available"), "{id} [state {n}]: {}", after.2);
+                    dimmed += 1;
+                }
+            }
+        }
+        assert!(compared > 0 && dimmed > 0, "{compared} compared, {dimmed} dimmed");
+    }
+
+    /// A pick of a "…" command whose menu bar item is dimmed (Remove Background and Object Mosaic
+    /// with no image selected, Find and Replace with no document open) opens no dialog, changes
+    /// nothing and reports why the command can't run.
+    #[test]
+    fn a_pick_of_a_dimmed_command_says_why_it_cannot_run() {
+        let cases = [
+            ("addon.image.removeBackground", vectorcraft_testkit::fixtures::Fixture::Multi.session()),
+            ("object.createObjectMosaic", vectorcraft_testkit::fixtures::Fixture::Multi.session()),
+            ("edit.findReplace", vectorcraft_engine::Session::new()),
+        ];
+        for (id, session) in cases {
+            assert!(crate::shortcut_editor::menu_target(id).is_some(), "{id} has a dialog");
+            let mut app = VectorcraftApp::new(session, crate::Services::default());
+            assert!(!menus::enabled(&app, id), "{id}");
+            app.ui.status.clear();
+            let before = app.session.active().map(|st| st.history.undo.len());
+            run_pick(&mut app, id);
+            assert!(app.ui.dialog.is_none(), "{id}");
+            assert_eq!(app.session.active().map(|st| st.history.undo.len()), before, "{id}");
+            assert!(app.ui.status.contains("not available"), "{id}: {}", app.ui.status);
+        }
+    }
+
+    /// A pick of a command that opens no dialog runs it at once.
+    #[test]
+    fn a_pick_of_a_command_without_a_dialog_runs_at_once() {
+        for id in ["object.group", "paint.swap"] {
+            assert!(items().iter().any(|(_, item, _)| item == id), "{id} is in the palette");
+            assert_eq!(crate::shortcut_editor::menu_target(id), None, "{id}");
+            let mut app = VectorcraftApp::new(vectorcraft_testkit::fixtures::Fixture::Multi.session(), crate::Services::default());
+            let before = app.session.active().unwrap().history.undo.len();
+            run_pick(&mut app, id);
+            assert!(app.ui.dialog.is_none(), "{id}");
+            assert_eq!(app.session.active().unwrap().history.undo.len(), before + 1, "{id}: {}", app.ui.status);
+        }
+    }
+
     /// #1757: Up and Down move the highlighted result, wrapping at the ends; a new query starts
     /// from the first again.
     #[test]

crates/ui-egui/src/shortcut_editor.rs

@@ -653,23 +653,28 @@ const NEEDS_A_PARAM: [&str; 20] = [
 
 /// The (label, params) of each menu bar item, by command id.
 fn menu_bar_items() -> &'static HashMap<&'static str, Vec<(&'static str, Value)>> {
+    static ITEMS: OnceLock<HashMap<&'static str, Vec<(&'static str, Value)>>> = OnceLock::new();
+    ITEMS.get_or_init(|| bar_items(&menus::menu_tree()))
+}
+
+/// The (label, params) of each item of the menus `tree`, by command id. Items that list an
+/// installed plug-in ([`menus::lists_plugin`]) are left out: [`menu_bar_items`] is built once,
+/// and a plug-in installed or removed later would leave it with a row the menu no longer shows.
+fn bar_items(tree: &[(&'static str, Vec<menus::Item>)]) -> HashMap<&'static str, Vec<(&'static str, Value)>> {
     fn walk(items: &[menus::Item], out: &mut HashMap<&'static str, Vec<(&'static str, Value)>>) {
         for it in items {
             match it {
-                menus::Item::Cmd(label, id, p) => out.entry(*id).or_default().push((*label, p.clone())),
+                menus::Item::Cmd(label, id, p) if !menus::lists_plugin(id, p) => out.entry(*id).or_default().push((*label, p.clone())),
                 menus::Item::Sub(_, children) => walk(children, out),
                 _ => {}
             }
         }
     }
-    static ITEMS: OnceLock<HashMap<&'static str, Vec<(&'static str, Value)>>> = OnceLock::new();
-    ITEMS.get_or_init(|| {
-        let mut out = HashMap::new();
-        for (_, items) in menus::menu_tree() {
-            walk(&items, &mut out);
-        }
-        out
-    })
+    let mut out = HashMap::new();
+    for (_, items) in tree {
+        walk(items, &mut out);
+    }
+    out
 }
 
 fn is_bare(p: &Value) -> bool {
@@ -686,6 +691,19 @@ pub(crate) fn menu_target(id: &str) -> Option<(String, Value)> {
     items.iter().all(|(_, q)| q == p).then(|| menus::click_target(label, id, p))
 }
 
+/// What a command search pick, or a shortcut without params, of command `id` runs now: `Ok` with
+/// what a click on its menu bar item runs ([`menu_target`]), or `None` when no such item gives it
+/// params. While that item is dimmed, no dialog opens for it: `Err` holds the status line that says
+/// why the command can't run (Remove Background… with no image selected).
+pub(crate) fn menu_target_now(app: &VectorcraftApp, id: &str) -> Option<Result<(String, Value), String>> {
+    let target = menu_target(id)?;
+    if menus::enabled(app, id) {
+        return Some(Ok(target));
+    }
+    let why = (vectorcraft_engine::find_command(id)?.enabled)(&app.session).err()?;
+    Some(Err(vectorcraft_engine::EngineError::Disabled(id.to_string(), why).to_string()))
+}
+
 /// Is `key` a command with no default shortcut that a key press can't run: no item of the menu
 /// bar runs it without params or with one set of params ([`menu_target`]), it opens no dialog when
 /// run without them (as the Variables panel's commands do), and it is a query, whose answer a key
@@ -2270,6 +2288,25 @@ mod tests {
         assert_eq!(menu_target("edit.undo"), None, "runs without params");
     }
 
+    /// The menu bar items a shortcut or a command search pick reads leave out items that list an
+    /// installed plug-in (a File › Open With importer, an Object › Plug-ins filter), which come and
+    /// go after the items are read; the other items stay.
+    #[test]
+    fn menu_bar_items_leave_out_installed_plug_ins() {
+        let tree = vec![(
+            "File",
+            vec![
+                menus::Item::Sub("Open With", vec![menus::Item::Cmd("Spins…", "file.openWith", json!({"importer": "plugin.test.spins"}))]),
+                menus::Item::Cmd("Filter…", "plugin.dialog", json!({"id": "test.filter"})),
+                menus::Item::Cmd("Clean Up…", "object.path.cleanUp", json!({"strayPoints": true})),
+            ],
+        )];
+        let items = bar_items(&tree);
+        assert!(!items.contains_key("file.openWith"), "{items:?}");
+        assert!(!items.contains_key("plugin.dialog"), "{items:?}");
+        assert_eq!(items.get("object.path.cleanUp"), Some(&vec![("Clean Up…", json!({"strayPoints": true}))]));
+    }
+
     /// Every command the editor lists, run as a key press runs it on a document with one or with
     /// several objects selected, reports no missing param.
     #[test]

crates/ui-egui/src/shortcuts.rs

@@ -985,9 +985,10 @@ fn dispatch(app: &mut VectorcraftApp, ctx: &egui::Context, accept: impl FnMut(&H
 }
 
 /// What a shortcut without params runs when its command's menu bar item gives params
-/// ([`crate::shortcut_editor::menu_target`]): what a click on that item runs.
-fn menu_target(c: &CommandRef) -> Option<(String, serde_json::Value)> {
-    c.params.as_object().is_none_or(serde_json::Map::is_empty).then(|| crate::shortcut_editor::menu_target(&c.id)).flatten()
+/// ([`crate::shortcut_editor::menu_target_now`]): what a click on that item runs, or while the item
+/// is dimmed, why the command can't run.
+fn menu_target(app: &VectorcraftApp, c: &CommandRef) -> Option<Result<(String, serde_json::Value), String>> {
+    c.params.as_object().is_none_or(serde_json::Map::is_empty).then(|| crate::shortcut_editor::menu_target_now(app, &c.id)).flatten()
 }
 
 /// Params used when a shortcut invokes its command. The named panel function keys retain their
@@ -1030,8 +1031,11 @@ fn run_hit(app: &mut VectorcraftApp, ctx: &egui::Context, c: &CommandRef) {
         // every artboard instead of the art.
         crate::panels::artboards::select_all(app);
     } else {
-        let (id, p) = menu_target(c).unwrap_or_else(|| (c.id.clone(), shortcut_params(c)));
-        crate::menus::invoke(app, &id, p);
+        match menu_target(app, c) {
+            Some(Ok((id, p))) => crate::menus::invoke(app, &id, p),
+            Some(Err(why)) => app.status(why),
+            None => crate::menus::invoke(app, &c.id, shortcut_params(c)),
+        }
     }
 }
 

crates/ui-egui/src/shortcuts/tests.rs

@@ -412,6 +412,28 @@ fn a_single_key_shortcut_without_a_dialog_runs_at_once() {
     assert_eq!((open_dialog(&app), undo_len(&app)), (None, before + 1), "Shift+X swaps fill and stroke: {}", app.ui.status);
 }
 
+/// A key or a chord of a command whose menu item is dimmed opens no dialog: Remove Background… and
+/// Object Mosaic… with a rectangle selected change nothing and say why they can't run.
+#[test]
+fn a_shortcut_of_a_dimmed_command_opens_no_dialog_and_says_why() {
+    let chord = vec![
+        egui::Event::ModifiersChanged(cmd() | Modifiers::ALT | Modifiers::SHIFT),
+        egui::Event::Key { key: Key::F9, physical_key: None, pressed: true, repeat: false, modifiers: cmd() | Modifiers::ALT | Modifiers::SHIFT },
+    ];
+    for id in ["addon.image.removeBackground", "object.createObjectMosaic"] {
+        assert!(crate::shortcut_editor::menu_target(id).is_some(), "{id} has a dialog");
+        for (shortcut, events) in [("F", press_f()), ("Cmd+Alt+Shift+F9", chord.clone())] {
+            let mut app = app_with_shortcut(id, shortcut);
+            assert!(!crate::menus::enabled(&app, id), "{id}");
+            let before = undo_len(&app);
+            frame(&mut app, events);
+            crate::shortcut_editor::sync(&crate::state::UiState::default());
+            assert_eq!((open_dialog(&app), undo_len(&app)), (None, before), "{id} on {shortcut}: {}", app.ui.status);
+            assert!(app.ui.status.contains("not available"), "{id} on {shortcut}: {}", app.ui.status);
+        }
+    }
+}
+
 /// With no document open, a command's key without a modifier reports what the same command
 /// reports on a chord.
 #[test]
@@ -1616,6 +1638,29 @@ fn the_command_search_that_opens_while_enter_is_down_waits_for_a_new_press() {
     assert!(!app.ui.palette_open, "a new press runs the highlighted result");
 }
 
+/// Enter on a command search result whose menu item opens a dialog (Create Gradient Mesh…) opens
+/// that dialog: the press and its repeats do not confirm it, and a new press does.
+#[test]
+fn enter_on_a_command_search_result_opens_its_dialog_and_a_new_press_confirms_it() {
+    let mut app = app_with_a_selected_rectangle();
+    let held = HeldKey::new(Key::Enter, Modifiers::NONE);
+    app.select_tool("selection");
+    app.ui.palette_open = true;
+    app.ui.palette_query = "object.mesh.create".into();
+    let before = undo_len(&app);
+    for repeat in 0..4 {
+        held.press(&mut app);
+        let command = app.ui.dialog.as_ref().and_then(|d| d.fields.get("__command").cloned());
+        assert_eq!(command, Some(json!("object.mesh.create")), "repeat {repeat}: its dialog stays open: {}", app.ui.status);
+        assert_eq!(undo_len(&app), before, "repeat {repeat}: nothing changed");
+    }
+    assert!(!app.ui.palette_open);
+    held.release(&mut app);
+    held.press(&mut app);
+    assert_eq!(open_dialog(&app), None, "a new press confirms");
+    assert_eq!(undo_len(&app), before + 1, "the mesh is one undo step: {}", app.ui.status);
+}
+
 /// A field that rejects its text on Enter keeps the dialog open, while the key is held too.
 #[test]
 fn holding_enter_on_a_rejected_value_leaves_the_dialog_open() {