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 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 {