From 8ce872938f12a25a536f33cbbac9b3bbb2f3da33 Mon Sep 17 00:00:00 2001 From: aiongg Date: Wed, 21 Oct 2026 18:14:21 +0000 Subject: [PATCH 1/4] keymap: a failed import change keeps its shortcuts when a later change frees room The cleanup after an import could remove a chord from a command whose own change had failed, once a later change freed an overrides entry. That command now keeps its bindings, and the change that would have taken its chord is dropped and reported, as when the removal itself fails. --- crates/keymap/src/import.rs | 15 +++++++++++---- crates/keymap/tests/import.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/crates/keymap/src/import.rs b/crates/keymap/src/import.rs index a387ad67..87d41048 100644 --- a/crates/keymap/src/import.rs +++ b/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> = 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 = HashSet::new(); + // Commands whose change failed keep the bindings they had, whatever later changes free. + let mut failed_here: HashSet = 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); diff --git a/crates/keymap/tests/import.rs b/crates/keymap/tests/import.rs index dd66bcb6..d2414b57 100644 --- a/crates/keymap/tests/import.rs +++ b/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 { From e6b54fa8aa256c12cb05e330f766a7bc3c86797f Mon Sep 17 00:00:00 2001 From: aiongg Date: Fri, 23 Oct 2026 05:03:07 +0000 Subject: [PATCH 2/4] keymap: leave the token out of .git/config after checkout in the keymap workflow --- .github/workflows/keymap.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/keymap.yml b/.github/workflows/keymap.yml index 29bddad6..e935a784 100644 --- a/.github/workflows/keymap.yml +++ b/.github/workflows/keymap.yml @@ -22,6 +22,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: dtolnay/rust-toolchain@stable with: components: rustfmt, clippy @@ -46,6 +48,8 @@ jobs: runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 with: @@ -57,6 +61,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: dtolnay/rust-toolchain@stable with: targets: wasm32-unknown-unknown From af6eb6ae1eef8868f34af385195dc34f7a83820c Mon Sep 17 00:00:00 2001 From: aiongg Date: Fri, 23 Oct 2026 05:16:16 +0000 Subject: [PATCH 3/4] Revert "keymap: leave the token out of .git/config after checkout in the keymap workflow" This reverts commit e6b54fa8aa256c12cb05e330f766a7bc3c86797f. --- .github/workflows/keymap.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.github/workflows/keymap.yml b/.github/workflows/keymap.yml index e935a784..29bddad6 100644 --- a/.github/workflows/keymap.yml +++ b/.github/workflows/keymap.yml @@ -22,8 +22,6 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - with: - persist-credentials: false - uses: dtolnay/rust-toolchain@stable with: components: rustfmt, clippy @@ -48,8 +46,6 @@ jobs: runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 - with: - persist-credentials: false - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 with: @@ -61,8 +57,6 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - with: - persist-credentials: false - uses: dtolnay/rust-toolchain@stable with: targets: wasm32-unknown-unknown From 457da42c5f133ae94efe94e160ee8ed20469fc93 Mon Sep 17 00:00:00 2001 From: aiongg Date: Fri, 23 Oct 2026 05:28:19 +0000 Subject: [PATCH 4/4] keymap: pin the workflow's toolchain and cache actions to main's commits, and keep the token out of .git/config after checkout --- .github/workflows/keymap.yml | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/.github/workflows/keymap.yml b/.github/workflows/keymap.yml index 29bddad6..2f0bdb9f 100644 --- a/.github/workflows/keymap.yml +++ b/.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