diff --git a/crates/keymap/src/ui/mod.rs b/crates/keymap/src/ui/mod.rs index 8567eb29..e32cb077 100644 --- a/crates/keymap/src/ui/mod.rs +++ b/crates/keymap/src/ui/mod.rs @@ -58,7 +58,9 @@ pub enum Outcome { #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] #[non_exhaustive] pub enum ConflictMode { - /// Ask with the conflict bar: Take It, Take It and Go to Conflict, Keep Both, Cancel. + /// Ask with the conflict bar: Take It, Take It and Go to Conflict, Keep Both, Cancel. Assign + /// in the detail pane takes the chord without asking, as Steal does: the pane has already + /// listed its holders under "Currently assigned to". #[default] Ask, /// Take the chord from the other commands, then say so. diff --git a/crates/keymap/src/ui/state.rs b/crates/keymap/src/ui/state.rs index f83b4392..b06db1b2 100644 --- a/crates/keymap/src/ui/state.rs +++ b/crates/keymap/src/ui/state.rs @@ -500,10 +500,13 @@ impl DialogState { self.field = Some(FieldChord { command, binding, holders, shadows }); } - /// Assign in the detail pane: commits the field's chord. + /// Assign in the detail pane: adds the field's chord to the command. The pane already lists + /// its holders under "Currently assigned to", so Assign takes it from them without the + /// conflict bar (one undo step), or keeps both with [`ConflictMode::Keep`]. pub(crate) fn assign_field(&mut self, env: &mut Env) { if let Some(f) = self.field.take() { - self.commit(env, &f.command, None, f.binding); + let mode = if env.options.conflicts == ConflictMode::Keep { ConflictMode::Keep } else { ConflictMode::Steal }; + self.commit_with(env, &f.command, None, f.binding, mode); } } @@ -524,6 +527,11 @@ impl DialogState { /// Reserved chord, then the app's rule, then holders: assign at once, or by /// [`Options::conflicts`]. pub(crate) fn commit(&mut self, env: &mut Env, command: &CommandRef, old: Option, binding: Binding) { + let mode = env.options.conflicts; + self.commit_with(env, command, old, binding, mode); + } + + fn commit_with(&mut self, env: &mut Env, command: &CommandRef, old: Option, binding: Binding, mode: ConflictMode) { self.conflict = None; self.message = None; let chord = env.chord_text(&binding.trigger); @@ -542,7 +550,7 @@ impl DialogState { self.apply(env, command, old, binding, Policy::Steal); return; } - match env.options.conflicts { + match mode { ConflictMode::Ask => { let text = env.ui("⚠ {chord} is used by {label}", &[("chord", &chord), ("label", &env.holder_labels(&holders))]); self.conflict = Some(ConflictBar { command: command.clone(), old, binding, holders, text }); @@ -1335,7 +1343,7 @@ mod tests { } #[test] - fn field_waits_for_assign_and_lists_holders() { + fn field_waits_for_assign_lists_holders_and_takes() { let mut fx = Fx::new(Options::default()); let mut st = open(&mut fx); record(&mut st, &mut fx, Target::Field { command: "file.save".into() }, &["Cmd+B"]); @@ -1344,13 +1352,14 @@ mod tests { st.set_context(&fx.env(), Scope::new("Text")); assert_eq!(st.field.as_ref().map(|f| f.holders.len()), Some(1), "in Text it clashes"); st.assign_field(&mut fx.env()); - assert!(st.conflict.is_some()); - assert!(fx.km.overrides().is_empty()); + assert!(st.conflict.is_none(), "Assign takes the chord without asking"); + assert_eq!(fx.keys("file.save"), ["Cmd+S", "Text: Cmd+B"]); + assert!(fx.keys("type.bold").is_empty()); st.set_context(&fx.env(), None); assert_eq!(st.type_binding(&mut fx.env(), &"file.save".into(), "F7 F7"), Ok(())); st.assign_field(&mut fx.env()); - assert_eq!(fx.keys("file.save"), ["Cmd+S", "F7 F7"]); + assert_eq!(fx.keys("file.save"), ["Cmd+S", "Text: Cmd+B", "F7 F7"]); assert_eq!(st.type_binding(&mut fx.env(), &"file.save".into(), "Cmd+Nope"), Err("unknown key Nope".into())); } diff --git a/crates/keymap/tests/ui_dialog.rs b/crates/keymap/tests/ui_dialog.rs index 77330d58..1187b613 100644 --- a/crates/keymap/tests/ui_dialog.rs +++ b/crates/keymap/tests/ui_dialog.rs @@ -6,8 +6,8 @@ use std::borrow::Cow; use craft_keymap::ui::{ConflictMode, DialogState, English, FixedRow, Options, Outcome, ShortcutsDialog, Text}; use craft_keymap::{ - Bindable, Chord, CommandRef, ConflictKind, ContextRule, Flavor, ForeignEntry, ForeignKey, ForeignSet, ImportOptions, Keymap, Kind, Overrides, - Platform, Profiles, Scope, Scopes, Trigger, + Active, Bindable, Binding, Chord, CommandRef, ConflictKind, ContextRule, Flavor, ForeignEntry, ForeignKey, ForeignSet, ImportOptions, Keymap, + Kind, Lookup, Overrides, Platform, Profiles, Scope, Scopes, Trigger, }; use egui::accesskit::Role; use egui::{Event, Key as EguiKey, Modifiers, Vec2, vec2}; @@ -206,7 +206,9 @@ fn double_press_records_k_k() { #[test] fn the_new_shortcut_field_waits_for_assign() { - let mut h = mac(); + let mut fx = Fx::new(fixture(Platform::Mac)); + fx.options.conflicts = ConflictMode::Keep; + let mut h = open(fx); h.get_by_label("Save").click(); h.run(); click(&mut h, "New shortcut: None"); @@ -214,9 +216,9 @@ fn the_new_shortcut_field_waits_for_assign() { assert!(has(&h, "Currently assigned to:")); assert_eq!(h.state().bindings("file.save"), ["Cmd+S"]); click(&mut h, "Assign"); - assert!(has(&h, "⚠ ⌘E is used by Export…")); - click(&mut h, "Keep Both"); + assert!(has(&h, "Also used by Export…"), "Keep mode keeps both"); assert_eq!(h.state().bindings("file.save"), ["Cmd+S", "Cmd+E"]); + assert_eq!(h.state().bindings("file.export"), ["Cmd+E"]); } /// Presses Tab until the control labeled `label` has focus; false after `max` presses. @@ -294,6 +296,32 @@ fn conflict_keep_both_lists_the_clash() { assert!(!has(&h, "Save As…: ⇧⌘S"), "the Conflicts filter shows only the clash"); } +#[test] +fn assign_takes_a_held_shortcut_and_keeps_the_others() { + let key = |text: &str| Binding::parse(text).unwrap_or_else(|e| panic!("{e}")); + let catalog = vec![ + Bindable::new("file.print", "Print…", Scope::app()).with_default(key("Cmd+P")), + Bindable::new("type.paragraphStyles", "Paragraph Styles", Scope::app()).with_default(key("Cmd+F11")), + ]; + let mut h = open(Fx::new(Keymap::new(catalog, Scopes::new(), Platform::Mac))); + h.get_by_label("Paragraph Styles").click(); + h.run(); + click(&mut h, "New shortcut: None"); + press(&mut h, Modifiers::COMMAND, EguiKey::P); + assert!(has(&h, "Currently assigned to:")); + assert!(has(&h, "⚠ Print…"), "the holder is listed under the field"); + click(&mut h, "Assign"); + assert!(has(&h, "⌘P was removed from Print…")); + click(&mut h, "OK"); + assert!(h.state().outcomes.iter().any(|o| matches!(o, Outcome::Ok))); + + let km = &h.state().km; + let run = |text: &str| km.lookup(&Chord::parse(text).unwrap_or_else(|e| panic!("{e}")), false, &Active::NONE); + assert_eq!(run("Cmd+P"), Lookup::Run("type.paragraphStyles".into())); + assert_eq!(run("Cmd+F11"), Lookup::Run("type.paragraphStyles".into())); + assert!(h.state().bindings("file.print").is_empty()); +} + #[test] fn steal_mode_says_what_it_took() { let mut fx = Fx::new(fixture(Platform::Mac));