keymap: an import change that cannot take a chord from another command leaves both commands as they were

crates/keymap/src/import.rs

@@ -279,8 +279,7 @@ pub struct Failed {
     pub command: CommandRef,
     pub label: String,
     pub change: FailedChange,
-    /// The set's shortcuts for [`FailedChange::Bind`], the one left on the command for
-    /// [`FailedChange::Unbind`]; empty for [`FailedChange::Clear`].
+    /// The set's shortcuts for [`FailedChange::Bind`]; empty for [`FailedChange::Clear`].
     pub shortcuts: Vec<String>,
 }
 
@@ -288,13 +287,11 @@ pub struct Failed {
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub enum FailedChange {
-    /// Giving the command the set's bindings.
+    /// Giving the command the set's bindings, or taking one of them from another command.
     Bind,
     /// Leaving the command without shortcuts: the set has it without keys and
     /// [`NoneDefined::Clear`] applies.
     Clear,
-    /// Taking a chord the set gives to another command; both commands keep it.
-    Unbind,
 }
 
 /// Why keys of an entry were not used.
@@ -548,7 +545,8 @@ impl ImportPlan {
     ///
     /// A change that finds the overrides full is left out and reported in
     /// [`ImportReport::failed`]; the others still apply, and a chord it would have taken from an
-    /// earlier entry stays with that entry. With [`Start::Defaults`] that cannot happen.
+    /// earlier entry stays with that entry. That includes taking a chord from another command:
+    /// both commands keep the bindings they had. With [`Start::Defaults`] that cannot happen.
     pub fn apply(&self, keymap: &mut Keymap) -> ImportReport {
         let before: Vec<Vec<Binding>> = self.commands.iter().map(|p| keymap.bindings(&p.command)).collect();
         // Dropping every entry leaves room for all of them: each catalogue command takes at most one
@@ -598,7 +596,10 @@ impl ImportPlan {
                 decided.insert(p.command.key());
             }
 
-            for (p, list) in self.commands.iter().zip(&accepted) {
+            // A command that cannot take a chord from another command is dropped, so both keep
+            // the bindings they had.
+            let mut unbind_failed = false;
+            for (c, (p, list)) in self.commands.iter().zip(&accepted).enumerate() {
                 if list.is_empty() || !decided.contains(&p.command.key()) {
                     continue;
                 }
@@ -612,10 +613,22 @@ impl ImportPlan {
                     if keymap.remove(&h.command, &h.binding) {
                         report.conflicts_cleared.push(Cleared { command: h.command, label, shortcut });
                     } else {
-                        report.failed.push(Failed { command: h.command, label, change: FailedChange::Unbind, shortcuts: vec![shortcut] });
+                        let home = home_of(keymap, &p.command);
+                        let list = in_default_order(keymap, &p.command, list);
+                        let shortcuts = list.iter().map(|b| b.text(&home)).collect();
+                        dropped.insert(
+                            c,
+                            Failed { command: p.command.clone(), label: label_of(keymap, &p.command), change: FailedChange::Bind, shortcuts },
+                        );
+                        unbind_failed = true;
+                        break;
                     }
                 }
             }
+            if unbind_failed {
+                *keymap = start.clone();
+                continue 'attempt;
+            }
 
             for (p, from) in self.commands.iter().zip(&before) {
                 if !decided.contains(&p.command.key()) {

crates/keymap/tests/import.rs

@@ -330,10 +330,11 @@ fn changes_that_find_the_overrides_full_are_reported() {
     };
     let options = |start| ImportOptions { contexts: vec![("Default".into(), ContextRule::Home)], start, ..ImportOptions::default() };
 
-    // file.close has an entry to rewrite; the other changes each need a new one.
+    // file.close has an entry to rewrite, but taking Cmd+J from view.goToPage needs a new one, so
+    // both stay as they were. The other changes each need a new entry too.
     let mut km = keymap(Platform::Windows).with_overrides(full());
     let r = plan(&set, &km, &options(Start::Current)).apply(&mut km);
-    has(&km, "file.close", &["Cmd+J"]);
+    has(&km, "file.close", &["Cmd+Q"]);
     has(&km, "view.goToPage", &["Cmd+J"]);
     has(&km, "file.save", &["Cmd+S"]);
     has(&km, "edit.duplicate", &["Cmd+Alt+Shift+D"]);
@@ -346,16 +347,16 @@ fn changes_that_find_the_overrides_full_are_reported() {
     assert_eq!(
         r.failed,
         [
+            row("file.close", "Close", FailedChange::Bind, &["Cmd+J"]),
             row("file.save", "Save", FailedChange::Bind, &["Cmd+Alt+S"]),
             row("edit.duplicate", "Duplicate", FailedChange::Clear, &[]),
-            row("view.goToPage", "Go to Page…", FailedChange::Unbind, &["Cmd+J"]),
         ]
     );
-    assert_eq!(r.imported, 1);
+    assert_eq!(r.imported, 0);
     assert!(r.conflicts_cleared.is_empty());
     let v = serde_json::to_value(&r).unwrap();
     assert_eq!(
-        v["failed"][0],
+        v["failed"][1],
         json!({"command": {"id": "file.save", "params": null}, "label": "Save", "change": "bind", "shortcuts": ["Cmd+Alt+S"]})
     );