Display case-sensitive keybindings for vim commands (#24322)

This Pull Request tackles the issue outline in #14287 by changing the
way `KeyBinding`s for vim mode are displayed in the command palette.
It's worth pointing out that this whole thing was pretty much
implemented by Conrad Irwin during a pairing session, I just tried to
clean up some other changes introduced for a different issue, while
improving some comments.

Here's a quick list of the changes introduced:

- Update `KeyBinding` with a new `vim_mode` field to determine whether
the keybinding should be displayed in vim mode.
- Update the way `KeyBinding` is rendered, so as to detect if the
keybinding is for vim mode, if it is, only display keys in uppercase if
they require the shift key.
- Introduce a new global state – `VimStyle(bool)` - use to determine
whether `vim_mode` should be enabled or disabled when creating a new
`KeyBinding` struct. This global state is automatically set by the `vim`
crate whenever vim mode is enabled or disabled.
- Since the app's context is now required when building a `KeyBinding` ,
update a lot of callers to correctly pass this context.

And before and after screenshots, for comparison:

| before | after |
|--------|-------|
| <img width="1050" alt="SCR-20250205-tyeq"
src="https://github.com/user-attachments/assets/e577206d-2a3d-4e06-a96f-a98899cc15c0"
/> | <img width="1050" alt="SCR-20250205-tylh"
src="https://github.com/user-attachments/assets/ebbf70a9-e838-4d32-aee5-0ffde94d65fb"
/> |

Closes #14287 

Release Notes:

- Fix rendering of vim commands to preserve case sensitivity

---------

Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>
This commit is contained in:
Dino 2025-02-15 05:03:59 +00:00 committed by GitHub
parent 14289b5a6e
commit e0fc767c11
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
30 changed files with 236 additions and 165 deletions

View file

@ -716,11 +716,12 @@ impl Render for ContextMenu {
KeyBinding::for_action_in(
&**action, focus,
window,
cx
)
})
.unwrap_or_else(|| {
KeyBinding::for_action(
&**action, window,
&**action, window, cx
)
})
.map(|binding| {

View file

@ -2,8 +2,10 @@
use crate::PlatformStyle;
use crate::{h_flex, prelude::*, Icon, IconName, IconSize};
use gpui::{
relative, Action, AnyElement, App, FocusHandle, IntoElement, Keystroke, Modifiers, Window,
relative, Action, AnyElement, App, FocusHandle, Global, IntoElement, Keystroke, Modifiers,
Window,
};
use itertools::Itertools;
#[derive(Debug, IntoElement, Clone)]
pub struct KeyBinding {
@ -16,18 +18,24 @@ pub struct KeyBinding {
/// The [`PlatformStyle`] to use when displaying this keybinding.
platform_style: PlatformStyle,
size: Option<AbsoluteLength>,
/// Determines whether the keybinding is meant for vim mode.
vim_mode: bool,
}
struct VimStyle(bool);
impl Global for VimStyle {}
impl KeyBinding {
/// Returns the highest precedence keybinding for an action. This is the last binding added to
/// the keymap. User bindings are added after built-in bindings so that they take precedence.
pub fn for_action(action: &dyn Action, window: &mut Window) -> Option<Self> {
pub fn for_action(action: &dyn Action, window: &mut Window, cx: &App) -> Option<Self> {
let key_binding = window
.bindings_for_action(action)
.into_iter()
.rev()
.next()?;
Some(Self::new(key_binding))
Some(Self::new(key_binding, cx))
}
/// Like `for_action`, but lets you specify the context from which keybindings are matched.
@ -35,20 +43,30 @@ impl KeyBinding {
action: &dyn Action,
focus: &FocusHandle,
window: &mut Window,
cx: &App,
) -> Option<Self> {
let key_binding = window
.bindings_for_action_in(action, focus)
.into_iter()
.rev()
.next()?;
Some(Self::new(key_binding))
Some(Self::new(key_binding, cx))
}
pub fn new(key_binding: gpui::KeyBinding) -> Self {
pub fn set_vim_mode(cx: &mut App, enabled: bool) {
cx.set_global(VimStyle(enabled));
}
fn is_vim_mode(cx: &App) -> bool {
cx.try_global::<VimStyle>().is_some_and(|g| g.0)
}
pub fn new(key_binding: gpui::KeyBinding, cx: &App) -> Self {
Self {
key_binding,
platform_style: PlatformStyle::platform(),
size: None,
vim_mode: KeyBinding::is_vim_mode(cx),
}
}
@ -63,6 +81,30 @@ impl KeyBinding {
self.size = Some(size.into());
self
}
pub fn vim_mode(mut self, enabled: bool) -> Self {
self.vim_mode = enabled;
self
}
fn render_key(&self, keystroke: &Keystroke, color: Option<Color>) -> AnyElement {
let key_icon = icon_for_key(keystroke, self.platform_style);
match key_icon {
Some(icon) => KeyIcon::new(icon, color).size(self.size).into_any_element(),
None => {
let key = if self.vim_mode {
if keystroke.modifiers.shift && keystroke.key.len() == 1 {
keystroke.key.to_ascii_uppercase().to_string()
} else {
keystroke.key.to_string()
}
} else {
util::capitalize(&keystroke.key)
};
Key::new(&key, color).size(self.size).into_any_element()
}
}
}
}
impl RenderOnce for KeyBinding {
@ -94,28 +136,11 @@ impl RenderOnce for KeyBinding {
self.size,
true,
))
.map(|el| {
el.child(render_key(&keystroke, self.platform_style, None, self.size))
})
.map(|el| el.child(self.render_key(&keystroke, None)))
}))
}
}
pub fn render_key(
keystroke: &Keystroke,
platform_style: PlatformStyle,
color: Option<Color>,
size: Option<AbsoluteLength>,
) -> AnyElement {
let key_icon = icon_for_key(keystroke, platform_style);
match key_icon {
Some(icon) => KeyIcon::new(icon, color).size(size).into_any_element(),
None => Key::new(util::capitalize(&keystroke.key), color)
.size(size)
.into_any_element(),
}
}
fn icon_for_key(keystroke: &Keystroke, platform_style: PlatformStyle) -> Option<IconName> {
match keystroke.key.as_str() {
"left" => Some(IconName::ArrowLeft),
@ -312,39 +337,33 @@ impl KeyIcon {
}
/// Returns a textual representation of the key binding for the given [`Action`].
pub fn text_for_action(action: &dyn Action, window: &Window) -> Option<String> {
pub fn text_for_action(action: &dyn Action, window: &Window, cx: &App) -> Option<String> {
let bindings = window.bindings_for_action(action);
let key_binding = bindings.last()?;
Some(text_for_key_binding(key_binding, PlatformStyle::platform()))
Some(text_for_keystrokes(key_binding.keystrokes(), cx))
}
/// Returns a textual representation of the key binding for the given [`Action`]
/// as if the provided [`FocusHandle`] was focused.
pub fn text_for_action_in(
action: &dyn Action,
focus: &FocusHandle,
window: &mut Window,
) -> Option<String> {
let bindings = window.bindings_for_action_in(action, focus);
let key_binding = bindings.last()?;
Some(text_for_key_binding(key_binding, PlatformStyle::platform()))
}
/// Returns a textual representation of the given key binding for the specified platform.
pub fn text_for_key_binding(
key_binding: &gpui::KeyBinding,
platform_style: PlatformStyle,
) -> String {
key_binding
.keystrokes()
pub fn text_for_keystrokes(keystrokes: &[Keystroke], cx: &App) -> String {
let platform_style = PlatformStyle::platform();
let vim_enabled = cx.try_global::<VimStyle>().is_some();
keystrokes
.iter()
.map(|keystroke| text_for_keystroke(keystroke, platform_style))
.collect::<Vec<_>>()
.map(|keystroke| keystroke_text(keystroke, platform_style, vim_enabled))
.join(" ")
}
pub fn text_for_keystroke(keystroke: &Keystroke, cx: &App) -> String {
let platform_style = PlatformStyle::platform();
let vim_enabled = cx.try_global::<VimStyle>().is_some();
keystroke_text(keystroke, platform_style, vim_enabled)
}
/// Returns a textual representation of the given [`Keystroke`].
pub fn text_for_keystroke(keystroke: &Keystroke, platform_style: PlatformStyle) -> String {
fn keystroke_text(
keystroke: &Keystroke,
platform_style: PlatformStyle,
vim_enabled: bool,
) -> String {
let mut text = String::new();
let delimiter = match platform_style {
@ -354,7 +373,7 @@ pub fn text_for_keystroke(keystroke: &Keystroke, platform_style: PlatformStyle)
if keystroke.modifiers.function {
match platform_style {
PlatformStyle::Mac => text.push_str("fn"),
PlatformStyle::Mac => text.push_str("Fn"),
PlatformStyle::Linux | PlatformStyle::Windows => text.push_str("Fn"),
}
@ -390,18 +409,26 @@ pub fn text_for_keystroke(keystroke: &Keystroke, platform_style: PlatformStyle)
}
if keystroke.modifiers.shift {
match platform_style {
PlatformStyle::Mac | PlatformStyle::Linux | PlatformStyle::Windows => {
text.push_str("Shift")
if !(vim_enabled && keystroke.key.len() == 1) {
match platform_style {
PlatformStyle::Mac | PlatformStyle::Linux | PlatformStyle::Windows => {
text.push_str("Shift")
}
}
text.push(delimiter);
}
text.push(delimiter);
}
let key = match keystroke.key.as_str() {
"pageup" => "PageUp",
"pagedown" => "PageDown",
key if vim_enabled => {
if !keystroke.modifiers.shift && key.len() == 1 {
key
} else {
&util::capitalize(key)
}
}
key => &util::capitalize(key),
};
@ -417,58 +444,76 @@ mod tests {
#[test]
fn test_text_for_keystroke() {
assert_eq!(
text_for_keystroke(&Keystroke::parse("cmd-c").unwrap(), PlatformStyle::Mac),
keystroke_text(
&Keystroke::parse("cmd-c").unwrap(),
PlatformStyle::Mac,
false
),
"Command-C".to_string()
);
assert_eq!(
text_for_keystroke(&Keystroke::parse("cmd-c").unwrap(), PlatformStyle::Linux),
keystroke_text(
&Keystroke::parse("cmd-c").unwrap(),
PlatformStyle::Linux,
false
),
"Super+C".to_string()
);
assert_eq!(
text_for_keystroke(&Keystroke::parse("cmd-c").unwrap(), PlatformStyle::Windows),
keystroke_text(
&Keystroke::parse("cmd-c").unwrap(),
PlatformStyle::Windows,
false
),
"Win+C".to_string()
);
assert_eq!(
text_for_keystroke(
keystroke_text(
&Keystroke::parse("ctrl-alt-delete").unwrap(),
PlatformStyle::Mac
PlatformStyle::Mac,
false
),
"Control-Option-Delete".to_string()
);
assert_eq!(
text_for_keystroke(
keystroke_text(
&Keystroke::parse("ctrl-alt-delete").unwrap(),
PlatformStyle::Linux
PlatformStyle::Linux,
false
),
"Ctrl+Alt+Delete".to_string()
);
assert_eq!(
text_for_keystroke(
keystroke_text(
&Keystroke::parse("ctrl-alt-delete").unwrap(),
PlatformStyle::Windows
PlatformStyle::Windows,
false
),
"Ctrl+Alt+Delete".to_string()
);
assert_eq!(
text_for_keystroke(
keystroke_text(
&Keystroke::parse("shift-pageup").unwrap(),
PlatformStyle::Mac
PlatformStyle::Mac,
false
),
"Shift-PageUp".to_string()
);
assert_eq!(
text_for_keystroke(
keystroke_text(
&Keystroke::parse("shift-pageup").unwrap(),
PlatformStyle::Linux
PlatformStyle::Linux,
false,
),
"Shift+PageUp".to_string()
);
assert_eq!(
text_for_keystroke(
keystroke_text(
&Keystroke::parse("shift-pageup").unwrap(),
PlatformStyle::Windows
PlatformStyle::Windows,
false
),
"Shift+PageUp".to_string()
);

View file

@ -207,10 +207,10 @@ impl RenderOnce for KeybindingHint {
// View this component preview using `workspace: open component-preview`
impl ComponentPreview for KeybindingHint {
fn preview(window: &mut Window, _cx: &App) -> AnyElement {
fn preview(window: &mut Window, cx: &App) -> AnyElement {
let enter_fallback = gpui::KeyBinding::new("enter", menu::Confirm, None);
let enter = KeyBinding::for_action(&menu::Confirm, window)
.unwrap_or(KeyBinding::new(enter_fallback));
let enter = KeyBinding::for_action(&menu::Confirm, window, cx)
.unwrap_or(KeyBinding::new(enter_fallback, cx));
v_flex()
.gap_6()

View file

@ -12,22 +12,22 @@ pub fn binding(key: &str) -> gpui::KeyBinding {
}
impl Render for KeybindingStory {
fn render(&mut self, _window: &mut Window, _cx: &mut Context<Self>) -> impl IntoElement {
fn render(&mut self, _window: &mut Window, cx: &mut Context<Self>) -> impl IntoElement {
let all_modifier_permutations = ["ctrl", "alt", "cmd", "shift"].into_iter().permutations(2);
Story::container()
.child(Story::title_for::<KeyBinding>())
.child(Story::label("Single Key"))
.child(KeyBinding::new(binding("Z")))
.child(KeyBinding::new(binding("Z"), cx))
.child(Story::label("Single Key with Modifier"))
.child(
div()
.flex()
.gap_3()
.child(KeyBinding::new(binding("ctrl-c")))
.child(KeyBinding::new(binding("alt-c")))
.child(KeyBinding::new(binding("cmd-c")))
.child(KeyBinding::new(binding("shift-c"))),
.child(KeyBinding::new(binding("ctrl-c"), cx))
.child(KeyBinding::new(binding("alt-c"), cx))
.child(KeyBinding::new(binding("cmd-c"), cx))
.child(KeyBinding::new(binding("shift-c"), cx)),
)
.child(Story::label("Single Key with Modifier (Permuted)"))
.child(
@ -41,39 +41,42 @@ impl Render for KeybindingStory {
.gap_4()
.py_3()
.children(chunk.map(|permutation| {
KeyBinding::new(binding(&(permutation.join("-") + "-x")))
KeyBinding::new(binding(&(permutation.join("-") + "-x")), cx)
}))
}),
),
)
.child(Story::label("Single Key with All Modifiers"))
.child(KeyBinding::new(binding("ctrl-alt-cmd-shift-z")))
.child(KeyBinding::new(binding("ctrl-alt-cmd-shift-z"), cx))
.child(Story::label("Chord"))
.child(KeyBinding::new(binding("a z")))
.child(KeyBinding::new(binding("a z"), cx))
.child(Story::label("Chord with Modifier"))
.child(KeyBinding::new(binding("ctrl-a shift-z")))
.child(KeyBinding::new(binding("fn-s")))
.child(KeyBinding::new(binding("ctrl-a shift-z"), cx))
.child(KeyBinding::new(binding("fn-s"), cx))
.child(Story::label("Single Key with All Modifiers (Linux)"))
.child(
KeyBinding::new(binding("ctrl-alt-cmd-shift-z"))
KeyBinding::new(binding("ctrl-alt-cmd-shift-z"), cx)
.platform_style(PlatformStyle::Linux),
)
.child(Story::label("Chord (Linux)"))
.child(KeyBinding::new(binding("a z")).platform_style(PlatformStyle::Linux))
.child(KeyBinding::new(binding("a z"), cx).platform_style(PlatformStyle::Linux))
.child(Story::label("Chord with Modifier (Linux)"))
.child(KeyBinding::new(binding("ctrl-a shift-z")).platform_style(PlatformStyle::Linux))
.child(KeyBinding::new(binding("fn-s")).platform_style(PlatformStyle::Linux))
.child(
KeyBinding::new(binding("ctrl-a shift-z"), cx).platform_style(PlatformStyle::Linux),
)
.child(KeyBinding::new(binding("fn-s"), cx).platform_style(PlatformStyle::Linux))
.child(Story::label("Single Key with All Modifiers (Windows)"))
.child(
KeyBinding::new(binding("ctrl-alt-cmd-shift-z"))
KeyBinding::new(binding("ctrl-alt-cmd-shift-z"), cx)
.platform_style(PlatformStyle::Windows),
)
.child(Story::label("Chord (Windows)"))
.child(KeyBinding::new(binding("a z")).platform_style(PlatformStyle::Windows))
.child(KeyBinding::new(binding("a z"), cx).platform_style(PlatformStyle::Windows))
.child(Story::label("Chord with Modifier (Windows)"))
.child(
KeyBinding::new(binding("ctrl-a shift-z")).platform_style(PlatformStyle::Windows),
KeyBinding::new(binding("ctrl-a shift-z"), cx)
.platform_style(PlatformStyle::Windows),
)
.child(KeyBinding::new(binding("fn-s")).platform_style(PlatformStyle::Windows))
.child(KeyBinding::new(binding("fn-s"), cx).platform_style(PlatformStyle::Windows))
}
}

View file

@ -43,10 +43,10 @@ impl Tooltip {
let title = title.into();
let action = action.boxed_clone();
move |window, cx| {
cx.new(|_| Self {
cx.new(|cx| Self {
title: title.clone(),
meta: None,
key_binding: KeyBinding::for_action(action.as_ref(), window),
key_binding: KeyBinding::for_action(action.as_ref(), window, cx),
})
.into()
}
@ -58,10 +58,10 @@ impl Tooltip {
window: &mut Window,
cx: &mut App,
) -> AnyView {
cx.new(|_| Self {
cx.new(|cx| Self {
title: title.into(),
meta: None,
key_binding: KeyBinding::for_action(action, window),
key_binding: KeyBinding::for_action(action, window, cx),
})
.into()
}
@ -73,10 +73,10 @@ impl Tooltip {
window: &mut Window,
cx: &mut App,
) -> AnyView {
cx.new(|_| Self {
cx.new(|cx| Self {
title: title.into(),
meta: None,
key_binding: KeyBinding::for_action_in(action, focus_handle, window),
key_binding: KeyBinding::for_action_in(action, focus_handle, window, cx),
})
.into()
}
@ -88,10 +88,10 @@ impl Tooltip {
window: &mut Window,
cx: &mut App,
) -> AnyView {
cx.new(|_| Self {
cx.new(|cx| Self {
title: title.into(),
meta: Some(meta.into()),
key_binding: action.and_then(|action| KeyBinding::for_action(action, window)),
key_binding: action.and_then(|action| KeyBinding::for_action(action, window, cx)),
})
.into()
}
@ -104,11 +104,11 @@ impl Tooltip {
window: &mut Window,
cx: &mut App,
) -> AnyView {
cx.new(|_| Self {
cx.new(|cx| Self {
title: title.into(),
meta: Some(meta.into()),
key_binding: action
.and_then(|action| KeyBinding::for_action_in(action, focus_handle, window)),
.and_then(|action| KeyBinding::for_action_in(action, focus_handle, window, cx)),
})
.into()
}