diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 4f2b57fc52e71b..e53889cb58e0eb 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -147,14 +147,49 @@ impl Keymap { }); let mut bindings: SmallVec<[(KeyBinding, usize); 1]> = SmallVec::new(); - let mut is_pending = None; + + // (pending, is_no_action, depth, keystrokes) + let mut pending_info_opt: Option<(bool, bool, usize, &[Keystroke])> = None; 'outer: for (binding, pending) in possibilities { for depth in (0..=context_stack.len()).rev() { if self.binding_enabled(binding, &context_stack[0..depth]) { - if is_pending.is_none() { - is_pending = Some(pending); + let is_no_action = is_no_action(&*binding.action); + // We only want to consider a binding pending if it has an action + // This, however, means that if we have both a NoAction binding and a binding + // with an action at the same depth, we should still set is_pending to true. + if let Some(pending_info) = pending_info_opt.as_mut() { + let ( + already_pending, + pending_is_no_action, + pending_depth, + pending_keystrokes, + ) = *pending_info; + + // We only want to change the pending status if it's not already pending AND if + // the existing pending status was set by a NoAction binding. This avoids a NoAction + // binding erroneously setting the pending status to true when a binding with an action + // already set it to false + // + // We also want to change the pending status if the keystrokes don't match, + // meaning it's different keystrokes than the NoAction that set pending to false + if pending + && !already_pending + && pending_is_no_action + && (pending_depth == depth + || pending_keystrokes != binding.keystrokes()) + { + pending_info.0 = !is_no_action; + } + } else { + pending_info_opt = Some(( + pending && !is_no_action, + is_no_action, + depth, + binding.keystrokes(), + )); } + if !pending { bindings.push((binding.clone(), depth)); continue 'outer; @@ -174,7 +209,7 @@ impl Keymap { }) .collect(); - (bindings, is_pending.unwrap_or_default()) + (bindings, pending_info_opt.unwrap_or_default().0) } /// Check if the given binding is enabled, given a certain key context. @@ -307,6 +342,102 @@ mod tests { ); } + #[test] + /// Tests for https://github.com/zed-industries/zed/issues/30259 + fn test_multiple_keystroke_binding_disabled() { + let bindings = [ + KeyBinding::new("space w w", ActionAlpha {}, Some("workspace")), + KeyBinding::new("space w w", NoAction {}, Some("editor")), + ]; + + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings.clone()); + + let space = || Keystroke::parse("space").unwrap(); + let w = || Keystroke::parse("w").unwrap(); + + let space_w = [space(), w()]; + let space_w_w = [space(), w(), w()]; + + let workspace_context = || [KeyContext::parse("workspace").unwrap()]; + + let editor_workspace_context = || { + [ + KeyContext::parse("workspace").unwrap(), + KeyContext::parse("editor").unwrap(), + ] + }; + + // Ensure `space` results in pending input on the workspace, but not editor + let space_workspace = keymap.bindings_for_input(&[space()], &workspace_context()); + assert!(space_workspace.0.is_empty()); + assert_eq!(space_workspace.1, true); + + let space_editor = keymap.bindings_for_input(&[space()], &editor_workspace_context()); + assert!(space_editor.0.is_empty()); + assert_eq!(space_editor.1, false); + + // Ensure `space w` results in pending input on the workspace, but not editor + let space_w_workspace = keymap.bindings_for_input(&space_w, &workspace_context()); + assert!(space_w_workspace.0.is_empty()); + assert_eq!(space_w_workspace.1, true); + + let space_w_editor = keymap.bindings_for_input(&space_w, &editor_workspace_context()); + assert!(space_w_editor.0.is_empty()); + assert_eq!(space_w_editor.1, false); + + // Ensure `space w w` results in the binding in the workspace, but not in the editor + let space_w_w_workspace = keymap.bindings_for_input(&space_w_w, &workspace_context()); + assert!(!space_w_w_workspace.0.is_empty()); + assert_eq!(space_w_w_workspace.1, false); + + let space_w_w_editor = keymap.bindings_for_input(&space_w_w, &editor_workspace_context()); + assert!(space_w_w_editor.0.is_empty()); + assert_eq!(space_w_w_editor.1, false); + + // Now test what happens if we have another binding defined AFTER the NoAction + // that should result in pending + let bindings = [ + KeyBinding::new("space w w", ActionAlpha {}, Some("workspace")), + KeyBinding::new("space w w", NoAction {}, Some("editor")), + KeyBinding::new("space w x", ActionAlpha {}, Some("editor")), + ]; + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings.clone()); + + let space_editor = keymap.bindings_for_input(&[space()], &editor_workspace_context()); + assert!(space_editor.0.is_empty()); + assert_eq!(space_editor.1, true); + + // Now test what happens if we have another binding defined BEFORE the NoAction + // that should result in pending + let bindings = [ + KeyBinding::new("space w w", ActionAlpha {}, Some("workspace")), + KeyBinding::new("space w x", ActionAlpha {}, Some("editor")), + KeyBinding::new("space w w", NoAction {}, Some("editor")), + ]; + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings.clone()); + + let space_editor = keymap.bindings_for_input(&[space()], &editor_workspace_context()); + assert!(space_editor.0.is_empty()); + assert_eq!(space_editor.1, true); + + // Now test what happens if we have another binding defined at a higher context + // that should result in pending + let bindings = [ + KeyBinding::new("space w w", ActionAlpha {}, Some("workspace")), + KeyBinding::new("space w x", ActionAlpha {}, Some("workspace")), + KeyBinding::new("space w w", NoAction {}, Some("editor")), + ]; + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings.clone()); + + let space_editor = keymap.bindings_for_input(&[space()], &editor_workspace_context()); + assert!(space_editor.0.is_empty()); + assert_eq!(space_editor.1, true); + } + #[test] fn test_bindings_for_action() { let bindings = [