diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 1698e06ed4..5651a6ff34 100644 --- a/crates/ui-egui/src/menus.rs +++ b/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; diff --git a/crates/ui-egui/src/palette.rs b/crates/ui-egui/src/palette.rs index d5bf7b6098..1a58c496b2 100644 --- a/crates/ui-egui/src/palette.rs +++ b/crates/ui-egui/src/palette.rs @@ -91,7 +91,10 @@ fn build(lang: crate::i18n::Lang) -> Vec { } /// 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)>, Option, 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::>()); + 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] diff --git a/crates/ui-egui/src/shortcut_editor.rs b/crates/ui-egui/src/shortcut_editor.rs index 7ada99ee5c..a580b2255a 100644 --- a/crates/ui-egui/src/shortcut_editor.rs +++ b/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>> = 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)]) -> 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>> = 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> { + 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] diff --git a/crates/ui-egui/src/shortcuts.rs b/crates/ui-egui/src/shortcuts.rs index 83d53d700c..f0ff34ef64 100644 --- a/crates/ui-egui/src/shortcuts.rs +++ b/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> { + 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)), + } } } diff --git a/crates/ui-egui/src/shortcuts/tests.rs b/crates/ui-egui/src/shortcuts/tests.rs index 8d32a4de22..bdc6efbc52 100644 --- a/crates/ui-egui/src/shortcuts/tests.rs +++ b/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() {