From 39d134fb549b3dafb84faa0882340b96e4b9b2b0 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Sun, 25 Oct 2026 23:25:35 -0400 Subject: [PATCH] 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 | 4 +++- crates/keymap/src/ui/state.rs | 23 +++++++++++++------ crates/keymap/tests/ui_dialog.rs | 38 +++++++++++++++++++++++++++----- 3 files changed, 52 insertions(+), 13 deletions(-) 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));