keymap ui: Assign takes a held shortcut in one step (#352)

Assign in the detail pane adds the recorded chord to the command and takes
it from the commands listed under "Currently assigned to", as one undo
step, without opening the conflict bar. ConflictMode::Keep keeps both.
A chord recorded in a row's cell still asks with the conflict bar.

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.

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: 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: 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()));
     }
 

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));