Merge remote-tracking branch 'ao/craft-keymap' into keymap-layout-rule

.github/workflows/keymap.yml

@@ -22,10 +22,12 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
-      - uses: dtolnay/rust-toolchain@stable
+        with:
+          persist-credentials: false
+      - uses: dtolnay/rust-toolchain@b725eda525362e7cf34af9a7243cac1e3f01a7ec # stable
         with:
           components: rustfmt, clippy
-      - uses: Swatinem/rust-cache@v2
+      - uses: Swatinem/rust-cache@d1527c8f67ef9af357290475d4af4b0fab8f12e0 # v2.9.3
         with:
           key: craft-keymap-linux
       - run: cargo fmt -p craft-keymap --check
@@ -46,8 +48,10 @@ jobs:
     runs-on: ${{ matrix.os }}
     steps:
       - uses: actions/checkout@v4
-      - uses: dtolnay/rust-toolchain@stable
-      - uses: Swatinem/rust-cache@v2
+        with:
+          persist-credentials: false
+      - uses: dtolnay/rust-toolchain@b725eda525362e7cf34af9a7243cac1e3f01a7ec # stable
+      - uses: Swatinem/rust-cache@d1527c8f67ef9af357290475d4af4b0fab8f12e0 # v2.9.3
         with:
           key: craft-keymap-${{ matrix.os }}
       - run: cargo test -p craft-keymap --all-features --locked
@@ -57,10 +61,12 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
-      - uses: dtolnay/rust-toolchain@stable
+        with:
+          persist-credentials: false
+      - uses: dtolnay/rust-toolchain@b725eda525362e7cf34af9a7243cac1e3f01a7ec # stable
         with:
           targets: wasm32-unknown-unknown
-      - uses: Swatinem/rust-cache@v2
+      - uses: Swatinem/rust-cache@d1527c8f67ef9af357290475d4af4b0fab8f12e0 # v2.9.3
         with:
           key: craft-keymap-wasm
       - run: cargo check -p craft-keymap --all-features --locked --target wasm32-unknown-unknown

crates/keymap/src/import.rs

@@ -562,8 +562,9 @@ 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. That includes taking a chord from another command:
-    /// both commands keep the bindings they had. With [`Start::Defaults`] that cannot happen.
+    /// earlier entry stays with that entry. That includes taking a chord from another command,
+    /// or from a command whose own change failed: 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 catalog command takes at most one
@@ -582,11 +583,14 @@ impl ImportPlan {
             let home_of = |km: &Keymap, c: &CommandRef| km.bindable(c).map_or_else(Scope::app, |b| b.scope.clone());
             let label_of = |km: &Keymap, c: &CommandRef| km.bindable(c).map(|b| b.label.clone()).unwrap_or_default();
             let mut decided: HashSet<String> = HashSet::new();
+            // Commands whose change failed keep the bindings they had, whatever later changes free.
+            let mut failed_here: HashSet<String> = HashSet::new();
             for (c, (p, list)) in self.commands.iter().zip(&accepted).enumerate() {
                 let failed =
                     |km: &Keymap, change, shortcuts| Failed { command: p.command.clone(), label: label_of(km, &p.command), change, shortcuts };
                 if let Some(f) = dropped.get(&c) {
                     report.failed.push(f.clone());
+                    failed_here.insert(p.command.key());
                     continue;
                 }
                 if !list.is_empty() {
@@ -600,11 +604,13 @@ impl ImportPlan {
                             continue 'attempt;
                         }
                         report.failed.push(f);
+                        failed_here.insert(p.command.key());
                         continue;
                     }
                 } else if p.none_defined && self.none_defined == NoneDefined::Clear {
                     if !keymap.clear(&p.command) {
                         report.failed.push(failed(keymap, FailedChange::Clear, Vec::new()));
+                        failed_here.insert(p.command.key());
                         continue;
                     }
                 } else {
@@ -614,7 +620,8 @@ impl ImportPlan {
             }
 
             // A command that cannot take a chord from another command is dropped, so both keep
-            // the bindings they had.
+            // the bindings they had. That includes a command whose own change failed: freeing an
+            // entry later must not take its chord away.
             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()) {
@@ -627,7 +634,7 @@ impl ImportPlan {
                     let home = home_of(keymap, &h.command);
                     let label = label_of(keymap, &h.command);
                     let shortcut = h.binding.text(&home);
-                    if keymap.remove(&h.command, &h.binding) {
+                    if !failed_here.contains(&h.command.key()) && keymap.remove(&h.command, &h.binding) {
                         report.conflicts_cleared.push(Cleared { command: h.command, label, shortcut });
                     } else {
                         let home = home_of(keymap, &p.command);

crates/keymap/tests/import.rs

@@ -309,6 +309,38 @@ fn start_and_none_defined_options() {
     assert_eq!(r.imported, 2);
 }
 
+#[test]
+fn a_failed_change_keeps_its_shortcut_when_a_later_change_frees_room() {
+    // Save's change finds the overrides full. Close's change then frees an entry, and Zoom In
+    // takes Cmd+S. Removing Cmd+S from Save would fit now, but Save's import failed, so Save
+    // keeps it and Zoom In stays as it was.
+    let set = ForeignSet {
+        name: None,
+        entries: vec![
+            win("File", &["Save"], &[("Default", "Ctrl+Alt+S")]),
+            win("File", &["Close"], &[("Default", "Ctrl+W")]),
+            win("View", &["Zoom In"], &[("Default", "Ctrl+S")]),
+        ],
+    };
+    let mut o = Overrides::new();
+    o.insert("file.close", vec!["Cmd+Q".into()]);
+    o.insert("view.zoomIn", vec!["Cmd+Alt+Z".into()]);
+    for n in 2..MAX_COMMANDS {
+        o.insert(format!("junk.{n}"), vec!["Cmd+K".into()]);
+    }
+    assert_eq!(o.len(), MAX_COMMANDS);
+    let options = ImportOptions { contexts: vec![("Default".into(), ContextRule::Home)], ..ImportOptions::default() };
+    let mut km = keymap(Platform::Windows).with_overrides(o);
+    let r = plan(&set, &km, &options).apply(&mut km);
+    has(&km, "file.save", &["Cmd+S"]);
+    has(&km, "file.close", &["Cmd+W"]);
+    has(&km, "view.zoomIn", &["Cmd+Alt+Z"]);
+    let failed: Vec<(&str, FailedChange)> = r.failed.iter().map(|f| (f.command.id.as_str(), f.change)).collect();
+    assert_eq!(failed, [("file.save", FailedChange::Bind), ("view.zoomIn", FailedChange::Bind)]);
+    assert!(r.conflicts_cleared.is_empty(), "{:?}", r.conflicts_cleared);
+    assert_eq!(r.imported, 1);
+}
+
 #[test]
 fn changes_that_find_the_overrides_full_are_reported() {
     let set = ForeignSet {