Merge pull request #5194 from dotCooCoo/fix/shortcuts-commands-that-need-params

Keyboard Shortcuts: commands a key cannot run without a value are not listed

crates/ui-egui/src/shortcut_editor.rs

@@ -392,36 +392,85 @@ fn listed(overrides: &Overrides, e: &Entry) -> bool {
     !needs_a_value(&e.key) || overrides.get(&crate::keymap::stored_key(&e.key)).is_some_and(|t| !t.is_empty())
 }
 
-/// Is `key` a command with a menu path and no default shortcut that a key press can't run: no
-/// item of the menu bar runs it without params, it opens no dialog when run without them (as the
-/// Variables panel's commands do), and its params doc names a field a call must give (File ›
-/// Document Setup › Units, `{units}`)?
-fn needs_a_value(key: &str) -> bool {
-    fn run_bare(items: &[menus::Item], out: &mut HashSet<&'static str>) {
+/// Commands whose params are all optional that do nothing, or fail, when a key press runs them
+/// with none, and that no item of the menu bar runs: Transparency and Opacity Mask Options need
+/// a value to set, Align and Distribute a direction, Delete Swatch and Move Swatch a swatch name,
+/// and the library and data set loaders a file, which their panel menu items ask for.
+const NEEDS_A_PARAM: [&str; 16] = [
+    "transparency.set",
+    "transparency.setOpacityMask",
+    "stroke.set",
+    "layer.setProps",
+    "object.align",
+    "object.distribute",
+    "swatch.delete",
+    "swatch.move",
+    "swatch.spotOptions",
+    "edit.colors.adjustBalance",
+    "swatch.library.load",
+    "graphicStyle.loadLibrary",
+    "symbol.loadLibrary",
+    "brush.loadLibrary",
+    "dataset.import",
+    "variable.loadLibrary",
+];
+
+/// The (label, params) of each menu bar item, by command id.
+fn menu_bar_items() -> &'static 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(_, id, p) if p.as_object().is_none_or(|o| o.is_empty()) => {
-                    out.insert(id);
-                }
-                menus::Item::Sub(_, children) => run_bare(children, out),
+                menus::Item::Cmd(label, 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
+    })
+}
+
+fn is_bare(p: &Value) -> bool {
+    p.as_object().is_none_or(|o| o.is_empty())
+}
+
+/// What a shortcut without params for command `id` runs when no menu bar item runs `id` without
+/// params and every item that runs it gives the same params: what a click on that item runs
+/// ([`menus::click_target`]), so Print Ink Fixes runs with its menu item's params and Clean Up…
+/// opens its dialog.
+pub(crate) fn menu_target(id: &str) -> Option<(String, Value)> {
+    let items = menu_bar_items().get(id)?;
+    let (label, p) = items.first().filter(|(_, p)| !is_bare(p))?;
+    items.iter().all(|(_, q)| q == p).then(|| menus::click_target(label, id, p))
+}
+
+/// 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
+/// press drops (Select All Unused), a command of [`NEEDS_A_PARAM`], or a menu command whose params
+/// doc names a field a call must give (File › Document Setup › Units, `{units}`)?
+fn needs_a_value(key: &str) -> bool {
     static KEYS: OnceLock<HashSet<&'static str>> = OnceLock::new();
     KEYS.get_or_init(|| {
-        let mut bare = HashSet::new();
-        for (_, items) in menus::menu_tree() {
-            run_bare(&items, &mut bare);
-        }
+        // A menu item that runs the command itself with params of its own counts only when the
+        // params doc names no required field: Edit › Find Next gives `find` empty, which it rejects.
+        let runs = |c: &vectorcraft_engine::CommandSpec| {
+            menu_bar_items().get(c.id).is_some_and(|ps| ps.iter().any(|(_, p)| is_bare(p)))
+                || menu_target(c.id).is_some_and(|(id, _)| id != c.id || !names_a_required_field(c.params))
+        };
         vectorcraft_engine::command_specs()
             .iter()
             .filter(|c| {
-                !c.menu.is_empty()
-                    && c.shortcut.is_none()
-                    && !bare.contains(c.id)
+                c.shortcut.is_none()
+                    && !runs(c)
                     && !crate::dialogs::variables::opens(c.id)
-                    && names_a_required_field(c.params)
+                    && (!c.journal || NEEDS_A_PARAM.contains(&c.id) || (!c.menu.is_empty() && names_a_required_field(c.params)))
             })
             .map(|c| c.id)
             .collect()
@@ -1465,6 +1514,77 @@ mod tests {
         }
     }
 
+    /// Commands whose params are all optional but that a key press can't run are not listed while
+    /// they have no shortcut: Transparency and Opacity Mask Options set nothing without a value,
+    /// Align needs a direction, Other Library… a file, and a query's answer (Select All Unused)
+    /// is dropped. A key saved for one before keeps it listed, where it can be cleared.
+    #[test]
+    fn commands_a_key_press_cannot_run_are_not_listed() {
+        let none = Overrides::new();
+        // Edit › Find Next's menu item gives an empty `find`, which it rejects.
+        let others = ["swatch.unused", "graphicStyle.unused", "document.info", "view.proofSetup", "magicWand.set", "edit.findNext"];
+        for id in NEEDS_A_PARAM.iter().chain(&others) {
+            let e = entry(id).unwrap_or_else(|| panic!("{id} has an entry"));
+            assert!(!listed(&none, e), "{id}");
+            assert_eq!(default_of(id), None, "{id}");
+        }
+        let mut ov = Overrides::new();
+        crate::keymap::set_override(&mut ov, "transparency.set", "Cmd+Alt+Shift+F10");
+        assert!(entry("transparency.set").is_some_and(|e| listed(&ov, e)));
+        let mut app = VectorcraftApp::new(vectorcraft_engine::Session::new(), Default::default());
+        for id in ["transparency.set", "transparency.setOpacityMask"] {
+            let list = run_command(&mut app, "shortcuts.list", &json!({ "query": id })).unwrap().unwrap();
+            assert_eq!(list, json!([]), "{id}");
+        }
+        // Commands that work without params stay listed: panel menu commands that act on the
+        // selection, toggles, and a menu item that runs with params of its own (Clean Up…).
+        for id in [
+            "transparency.releaseOpacityMask",
+            "transparency.toggleNewMasksClipping",
+            "object.path.cleanUp",
+            "addon.color.printFixes",
+            "edit.findReplace",
+        ] {
+            assert!(entry(id).is_some_and(|e| listed(&none, e)), "{id}");
+        }
+        assert_eq!(menu_target("object.path.cleanUp").map(|(id, _)| id).as_deref(), Some("ui.paramDialog"));
+        assert_eq!(
+            menu_target("addon.color.printFixes").map(|(id, p)| (id, p["command"].clone(), p["params"]["inkLimit"].clone())),
+            Some(("ui.paramDialog".into(), json!("addon.color.printFixes"), json!(300)))
+        );
+        assert_eq!(menu_target("edit.findReplace").map(|(id, _)| id).as_deref(), Some("ui.paramDialog"));
+        assert_eq!(menu_target("object.align"), None, "seven items, each with its own params");
+        assert_eq!(menu_target("edit.undo"), None, "runs without params");
+    }
+
+    /// 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]
+    fn listed_commands_run_from_a_key_without_a_missing_param() {
+        use vectorcraft_testkit::fixtures::Fixture;
+        // These need a state the fixtures lack: text being edited, a pattern being edited, an
+        // object linked to a graphic style, a plug-ins folder.
+        let context = ["text.insertInline", "pattern.options", "graphicStyle.redefine", "plugin.reload"];
+        let none = Overrides::new();
+        let mut failures = vec![];
+        for e in entries().iter().filter(|e| !e.is_tool && !e.key.starts_with("panel:") && listed(&none, e)) {
+            if ["file.", "app.", "help."].iter().any(|p| e.key.starts_with(p)) || context.contains(&e.key.as_str()) {
+                continue;
+            }
+            for fx in [Fixture::Single, Fixture::Multi] {
+                let mut app = VectorcraftApp::new(fx.session(), Default::default());
+                app.ui.status.clear();
+                let (id, p) = menu_target(&e.key).unwrap_or_else(|| (e.key.clone(), json!({})));
+                menus::invoke(&mut app, &id, p);
+                let s = &app.ui.status;
+                if s.starts_with("invalid parameters for") && (s.contains(": give ") || s.contains(": missing ")) {
+                    failures.push(format!("{} [{fx:?}]: {s}", e.key));
+                }
+            }
+        }
+        assert!(failures.is_empty(), "{failures:#?}");
+    }
+
     /// #2991: Enter Isolation Mode has no default key but runs on the selection, so it belongs in
     /// Edit › Keyboard Shortcuts.
     #[test]

crates/ui-egui/src/shortcuts.rs

@@ -400,6 +400,7 @@ fn single_key_shortcuts(app: &mut VectorcraftApp, ctx: &egui::Context) {
         let Some(hit) = hits.iter().find(|c| !c.id.starts_with("tool:")).or(hits.first()) else { continue };
         match hit.id.strip_prefix("tool:") {
             Some(tool) => app.select_tool(tool),
+            None if let Some((id, p)) = menu_target(hit) => crate::menus::invoke(app, &id, p),
             None if hit.params.is_null() => {
                 let _ = app.run(&hit.id, json!({}));
             }
@@ -778,6 +779,12 @@ 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()
+}
+
 /// Params used when a shortcut invokes its command. The named panel function keys retain their
 /// toggle action when their bindings are reassigned.
 fn shortcut_params(c: &CommandRef) -> serde_json::Value {
@@ -818,7 +825,8 @@ fn run_hit(app: &mut VectorcraftApp, ctx: &egui::Context, c: &CommandRef) {
         // every artboard instead of the art.
         crate::panels::artboards::select_all(app);
     } else {
-        crate::menus::invoke(app, &c.id, shortcut_params(c));
+        let (id, p) = menu_target(c).unwrap_or_else(|| (c.id.clone(), shortcut_params(c)));
+        crate::menus::invoke(app, &id, p);
     }
 }
 

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

@@ -225,6 +225,26 @@ fn presses_follow_the_users_shortcuts() {
     assert!(runs_a_shortcut("=", "Equal", cmd()), "the defaults again");
 }
 
+/// A user's shortcut for a command whose menu bar item gives params runs what a click on that item
+/// runs: Clean Up…, Print Ink Fixes… and Find and Replace… open their dialogs instead of running
+/// without the values the dialog asks for.
+#[test]
+fn a_shortcut_runs_what_its_menu_item_runs() {
+    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 ["object.path.cleanUp", "addon.color.printFixes", "edit.findReplace"] {
+        let mut app = VectorcraftApp::new(vectorcraft_testkit::fixtures::Fixture::Single.session(), Default::default());
+        crate::keymap::set_override(&mut app.ui.shortcut_overrides, id, "Cmd+Alt+Shift+F9");
+        app.ui.status.clear();
+        frame(&mut app, chord.clone());
+        crate::shortcut_editor::sync(&crate::state::UiState::default());
+        let d = app.ui.dialog.as_ref().unwrap_or_else(|| panic!("{id} opens its dialog: {}", app.ui.status));
+        assert_eq!(d.fields["__command"], json!(id), "{:?}", d.fields);
+    }
+}
+
 /// #2094: Cmd+Shift+O is Type › Create Outlines (not File › Open's Cmd+O, not the Artboard
 /// tool's Shift+O), with the Selection tool or while the Type tool edits the text.
 #[test]

docs/mcp.md

@@ -3081,11 +3081,21 @@ the Align To choice, and while there is a key object or a key guide they send th
 `spacing`. Edit › Keyboard Shortcuts lists them under Align, with no default shortcut, and
 `shortcuts.set {id, shortcut}` gives one a shortcut.
 
-Edit › Keyboard Shortcuts and `shortcuts.list` leave out a command with a menu path and no default shortcut when its
-params doc has a required field (one with no `?` and no `=` default), no item of the menu bar runs it without params,
-and it opens no dialog when run without them, such as `document.setUnits {units}` (File › Document Setup › Units).
+Edit › Keyboard Shortcuts and `shortcuts.list` leave out a command with no default shortcut when a key press cannot
+run it: no item of the menu bar runs it, it opens no dialog when run without params, and it is one of these:
+
+- a query with a menu path, such as `swatch.unused`;
+- a command with a menu path whose params doc has a required field (one with no `?` and no `=` default), such as
+  `document.setUnits {units}` (File › Document Setup › Units);
+- a command that sets nothing or fails without a param: `transparency.set`, `transparency.setOpacityMask`,
+  `stroke.set`, `layer.setProps`, `object.align`, `object.distribute`, `swatch.delete`, `swatch.move`,
+  `swatch.spotOptions`, `edit.colors.adjustBalance`, `swatch.library.load`, `graphicStyle.loadLibrary`,
+  `symbol.loadLibrary`, `brush.loadLibrary`, `dataset.import` and `variable.loadLibrary`.
+
 They list such a command while the user has changed its shortcut (`overridden: true` in `shortcuts.list`), and
-`shortcuts.set` can change or clear it.
+`shortcuts.set` can change or clear it. When every menu bar item that runs a command gives it the same params, a
+shortcut for that command runs what a click on the item runs: Object › Path › Clean Up…, Print Ink Fixes… and Edit ›
+Find and Replace… open their dialogs.
 
 The desktop app adds its active artboard, the one the Artboards panel highlights, to the `object.align`,
 `object.distribute`, `object.distributeSpacing` and `select.allOnArtboard` calls it runs without `artboard`, as it does