From ede008a65cabaef15695da0a6646ec65a2b48f30 Mon Sep 17 00:00:00 2001 From: Joe Polny Date: Thu, 8 May 2025 16:34:25 -0400 Subject: [PATCH 1/7] fix: do not considering NoAction bindings to be pending --- crates/gpui/src/keymap.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 4f2b57fc52e71b..bb1e3ee8118402 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -153,7 +153,7 @@ impl Keymap { 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); + is_pending = Some(pending && !is_no_action(&*binding.action)) } if !pending { bindings.push((binding.clone(), depth)); From 0326338c027fcafce0648d251f051ea290ef30fb Mon Sep 17 00:00:00 2001 From: Joe Polny Date: Thu, 8 May 2025 20:58:28 -0400 Subject: [PATCH 2/7] test: add test for NoAction multi-key bindings in a lower context --- crates/gpui/src/keymap.rs | 53 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index bb1e3ee8118402..3278cf27ff4d1b 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -307,6 +307,59 @@ mod tests { ); } + #[test] + 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); + } + #[test] fn test_bindings_for_action() { let bindings = [ From e3985d6bf9dc5fabb7723c6d0c7c759f1bfb7f62 Mon Sep 17 00:00:00 2001 From: Joe Polny Date: Fri, 9 May 2025 06:24:34 -0400 Subject: [PATCH 3/7] test: add test for both NoAction and action binings This reveals a problem with how is_pending is set upon the first keybind match. --- crates/gpui/src/keymap.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 3278cf27ff4d1b..712c6f2d7acd9c 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -358,6 +358,34 @@ mod tests { 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); } #[test] From 95a1a8632dc102eea6afe1c3a15be8df26e5bcbc Mon Sep 17 00:00:00 2001 From: Joe Polny Date: Fri, 9 May 2025 06:56:54 -0400 Subject: [PATCH 4/7] fix: proper set pending if there is both a NoAction and action binding at the same depth --- crates/gpui/src/keymap.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 712c6f2d7acd9c..5186549ce9048a 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -147,14 +147,19 @@ impl Keymap { }); let mut bindings: SmallVec<[(KeyBinding, usize); 1]> = SmallVec::new(); - let mut is_pending = None; + let mut is_pending_opt: Option<(bool, usize)> = 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 && !is_no_action(&*binding.action)) + if let Some(is_pending) = is_pending_opt.as_mut() { + if !is_pending.0 && pending && is_pending.1 == depth { + is_pending.0 = !is_no_action(&*binding.action); + } + } else { + is_pending_opt = Some((pending && !is_no_action(&*binding.action), depth)); } + if !pending { bindings.push((binding.clone(), depth)); continue 'outer; @@ -174,7 +179,7 @@ impl Keymap { }) .collect(); - (bindings, is_pending.unwrap_or_default()) + (bindings, is_pending_opt.unwrap_or_default().0) } /// Check if the given binding is enabled, given a certain key context. From 59ee67ec8d9e3d7a7dd652c7e01c894aabe89ccf Mon Sep 17 00:00:00 2001 From: Joe Polny Date: Fri, 9 May 2025 07:25:15 -0400 Subject: [PATCH 5/7] chore: add comments --- crates/gpui/src/keymap.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 5186549ce9048a..17b050c8f498ee 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -152,6 +152,9 @@ impl Keymap { 'outer: for (binding, pending) in possibilities { for depth in (0..=context_stack.len()).rev() { if self.binding_enabled(binding, &context_stack[0..depth]) { + // 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(is_pending) = is_pending_opt.as_mut() { if !is_pending.0 && pending && is_pending.1 == depth { is_pending.0 = !is_no_action(&*binding.action); @@ -313,6 +316,7 @@ 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")), From d93b9dc4aa6740b2e5667af80f3e3d21c6cd4a01 Mon Sep 17 00:00:00 2001 From: Joe Polny Date: Fri, 9 May 2025 07:49:01 -0400 Subject: [PATCH 6/7] fix: prevent NoAction binding override pending status from action binding --- crates/gpui/src/keymap.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 17b050c8f498ee..5e26a2695f128b 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -147,20 +147,33 @@ impl Keymap { }); let mut bindings: SmallVec<[(KeyBinding, usize); 1]> = SmallVec::new(); - let mut is_pending_opt: Option<(bool, usize)> = None; + + // (pending, is_no_action, depth) + let mut pending_info_opt: Option<(bool, bool, usize)> = None; 'outer: for (binding, pending) in possibilities { for depth in (0..=context_stack.len()).rev() { if self.binding_enabled(binding, &context_stack[0..depth]) { + 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(is_pending) = is_pending_opt.as_mut() { - if !is_pending.0 && pending && is_pending.1 == depth { - is_pending.0 = !is_no_action(&*binding.action); + if let Some(pending_info) = pending_info_opt.as_mut() { + let (already_pending, pending_is_no_action, pending_depth) = *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 + if pending + && !already_pending + && pending_is_no_action + && pending_depth == depth + { + pending_info.0 = !is_no_action; } } else { - is_pending_opt = Some((pending && !is_no_action(&*binding.action), depth)); + pending_info_opt = Some((pending && !is_no_action, is_no_action, depth)); } if !pending { @@ -182,7 +195,7 @@ impl Keymap { }) .collect(); - (bindings, is_pending_opt.unwrap_or_default().0) + (bindings, pending_info_opt.unwrap_or_default().0) } /// Check if the given binding is enabled, given a certain key context. From e0c118e95d10253c52f607fa1d7f349dad411502 Mon Sep 17 00:00:00 2001 From: Joe Polny Date: Fri, 9 May 2025 08:05:24 -0400 Subject: [PATCH 7/7] fix: fix higher-context-level bindings not being properly set as pending --- crates/gpui/src/keymap.rs | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 5e26a2695f128b..e53889cb58e0eb 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -148,8 +148,8 @@ impl Keymap { let mut bindings: SmallVec<[(KeyBinding, usize); 1]> = SmallVec::new(); - // (pending, is_no_action, depth) - let mut pending_info_opt: Option<(bool, bool, usize)> = 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() { @@ -159,21 +159,35 @@ impl Keymap { // 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_info; + 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_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)); + pending_info_opt = Some(( + pending && !is_no_action, + is_no_action, + depth, + binding.keystrokes(), + )); } if !pending { @@ -408,6 +422,20 @@ mod tests { 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]