Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Load all key bindings that parse and use markdown in error notifications (#23113)

Browse source 
* Collects and reports all parse errors

* Shares parsed `KeyBindingContextPredicate` among the actions.

* Updates gpui keybinding and action parsing to return structured
errors.

* Renames "block" to "section" to match the docs, as types like
`KeymapSection` are shown in `json-language-server` hovers.

* Removes wrapping of `context` and `use_key_equivalents` fields so that
`json-language-server` auto-inserts `""` and `false` instead of `null`.

* Updates `add_to_cx` to take `&self`, so that the user keymap doesn't
get unnecessarily cloned.

In retrospect I wish I'd just switched to using TreeSitter to do the
parsing and provide proper diagnostics. This is tracked in #23333

Release Notes:

- Improved handling of errors within the user keymap file. Parse errors
within context, keystrokes, or actions no longer prevent loading the key
bindings that do parse.
This commit is contained in:
Michael Sloan 2025-01-18 15:27:08 -07:00 • committed by GitHub
parent c929533e00
commit 711dc21eb2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
18 changed files with 552 additions and 196 deletions
Download patch file Download diff file Expand all files Collapse all files

64
crates/gpui/src/action.rs
View file

@ -1,9 +1,12 @@
use crate::SharedString;
use anyhow::{anyhow, Context, Result};
use anyhow::{anyhow, Result};
use collections::HashMap;
pub use no_action::{is_no_action, NoAction};
use serde_json::json;
use std::any::{Any, TypeId};
use std::{
any::{Any, TypeId},
fmt::Display,
};
/// Actions are used to implement keyboard-driven UI.
/// When you declare an action, you can bind keys to the action in the keymap and
@ -97,6 +100,47 @@ impl dyn Action {
}
}
/// Error type for `Keystroke::parse`. This is used instead of `anyhow::Error` so that Zed can use
/// markdown to display it.
#[derive(Debug)]
pub enum ActionBuildError {
/// Indicates that an action with this name has not been registered.
NotFound {
/// Name of the action that was not found.
name: String,
},
/// Indicates that an error occurred while building the action, typically a JSON deserialization
/// error.
BuildError {
/// Name of the action that was attempting to be built.
name: String,
/// Error that occurred while building the action.
error: anyhow::Error,
},
}
impl std::error::Error for ActionBuildError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self {
ActionBuildError::NotFound { .. } => None,
ActionBuildError::BuildError { error, .. } => error.source(),
}
}
}
impl Display for ActionBuildError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ActionBuildError::NotFound { name } => {
write!(f, "Didn't find an action named \"{name}\"")
}
ActionBuildError::BuildError { name, error } => {
write!(f, "Error while building action \"{name}\": {error}")
}
}
}
}
type ActionBuilder = fn(json: serde_json::Value) -> anyhow::Result<Box<dyn Action>>;
pub(crate) struct ActionRegistry {
@ -201,7 +245,7 @@ impl ActionRegistry {
.ok_or_else(|| anyhow!("no action type registered for {:?}", type_id))?
.clone();
self.build_action(&name, None)
Ok(self.build_action(&name, None)?)
}
/// Construct an action based on its name and optional JSON parameters sourced from the keymap.
@ -209,14 +253,20 @@ impl ActionRegistry {
&self,
name: &str,
params: Option<serde_json::Value>,
) -> Result<Box<dyn Action>> {
) -> std::result::Result<Box<dyn Action>, ActionBuildError> {
let build_action = self
.by_name
.get(name)
.ok_or_else(|| anyhow!("No action type registered for {}", name))?
.ok_or_else(|| ActionBuildError::NotFound {
name: name.to_owned(),
})?
.build;
(build_action)(params.unwrap_or_else(|| json!({})))
.with_context(|| format!("Attempting to build action {}", name))
(build_action)(params.unwrap_or_else(|| json!({}))).map_err(|e| {
ActionBuildError::BuildError {
name: name.to_owned(),
error: e,
}
})
}
pub fn all_action_names(&self) -> &[SharedString] {

12
crates/gpui/src/app.rs
View file

@ -32,11 +32,11 @@ pub use test_context::*;
use util::ResultExt;
use crate::{
current_platform, hash, init_app_menus, Action, ActionRegistry, Any, AnyView, AnyWindowHandle,
Asset, AssetSource, BackgroundExecutor, Bounds, ClipboardItem, Context, DispatchPhase,
DisplayId, Entity, EventEmitter, FocusHandle, FocusId, ForegroundExecutor, Global, KeyBinding,
Keymap, Keystroke, LayoutId, Menu, MenuItem, OwnedMenu, PathPromptOptions, Pixels, Platform,
PlatformDisplay, Point, PromptBuilder, PromptHandle, PromptLevel, Render,
current_platform, hash, init_app_menus, Action, ActionBuildError, ActionRegistry, Any, AnyView,
AnyWindowHandle, Asset, AssetSource, BackgroundExecutor, Bounds, ClipboardItem, Context,
DispatchPhase, DisplayId, Entity, EventEmitter, FocusHandle, FocusId, ForegroundExecutor,
Global, KeyBinding, Keymap, Keystroke, LayoutId, Menu, MenuItem, OwnedMenu, PathPromptOptions,
Pixels, Platform, PlatformDisplay, Point, PromptBuilder, PromptHandle, PromptLevel, Render,
RenderablePromptHandle, Reservation, ScreenCaptureSource, SharedString, SubscriberSet,
Subscription, SvgRenderer, Task, TextSystem, View, ViewContext, Window, WindowAppearance,
WindowContext, WindowHandle, WindowId,
@ -1220,7 +1220,7 @@ impl AppContext {
&self,
name: &str,
data: Option<serde_json::Value>,
) -> Result<Box<dyn Action>> {
) -> std::result::Result<Box<dyn Action>, ActionBuildError> {
self.actions.build_action(name, data)
}

34
crates/gpui/src/keymap/binding.rs
View file

@ -1,14 +1,15 @@
use std::rc::Rc;
use collections::HashMap;
use crate::{Action, KeyBindingContextPredicate, Keystroke};
use anyhow::Result;
use crate::{Action, InvalidKeystrokeError, KeyBindingContextPredicate, Keystroke};
use smallvec::SmallVec;
/// A keybinding and its associated metadata, from the keymap.
pub struct KeyBinding {
pub(crate) action: Box<dyn Action>,
pub(crate) keystrokes: SmallVec<[Keystroke; 2]>,
pub(crate) context_predicate: Option<KeyBindingContextPredicate>,
pub(crate) context_predicate: Option<Rc<KeyBindingContextPredicate>>,
}
impl Clone for KeyBinding {
@ -22,8 +23,13 @@ impl Clone for KeyBinding {
}
impl KeyBinding {
/// Construct a new keybinding from the given data.
pub fn new<A: Action>(keystrokes: &str, action: A, context_predicate: Option<&str>) -> Self {
/// Construct a new keybinding from the given data. Panics on parse error.
pub fn new<A: Action>(keystrokes: &str, action: A, context: Option<&str>) -> Self {
let context_predicate = if let Some(context) = context {
Some(KeyBindingContextPredicate::parse(context).unwrap().into())
} else {
None
};
Self::load(keystrokes, Box::new(action), context_predicate, None).unwrap()
}
@ -31,19 +37,13 @@ impl KeyBinding {
pub fn load(
keystrokes: &str,
action: Box<dyn Action>,
context: Option<&str>,
context_predicate: Option<Rc<KeyBindingContextPredicate>>,
key_equivalents: Option<&HashMap<char, char>>,
) -> Result<Self> {
let context = if let Some(context) = context {
Some(KeyBindingContextPredicate::parse(context)?)
} else {
None
};
) -> std::result::Result<Self, InvalidKeystrokeError> {
let mut keystrokes: SmallVec<[Keystroke; 2]> = keystrokes
.split_whitespace()
.map(Keystroke::parse)
.collect::<Result<_>>()?;
.collect::<std::result::Result<_, _>>()?;
if let Some(equivalents) = key_equivalents {
for keystroke in keystrokes.iter_mut() {
@ -58,7 +58,7 @@ impl KeyBinding {
Ok(Self {
keystrokes,
action,
context_predicate: context,
context_predicate,
})
}
@ -88,8 +88,8 @@ impl KeyBinding {
}
/// Get the predicate used to match this binding
pub fn predicate(&self) -> Option<&KeyBindingContextPredicate> {
self.context_predicate.as_ref()
pub fn predicate(&self) -> Option<Rc<KeyBindingContextPredicate>> {
self.context_predicate.as_ref().map(|rc| rc.clone())
}
}

12
crates/gpui/src/keymap/context.rs
View file

@ -244,7 +244,7 @@ impl KeyBindingContextPredicate {
let source = skip_whitespace(source);
let (predicate, rest) = Self::parse_expr(source, 0)?;
if let Some(next) = rest.chars().next() {
Err(anyhow!("unexpected character {next:?}"))
Err(anyhow!("unexpected character '{next:?}'"))
} else {
Ok(predicate)
}
@ -333,7 +333,7 @@ impl KeyBindingContextPredicate {
let next = source
.chars()
.next()
.ok_or_else(|| anyhow!("unexpected eof"))?;
.ok_or_else(|| anyhow!("unexpected end"))?;
match next {
'(' => {
source = skip_whitespace(&source[1..]);
@ -369,7 +369,7 @@ impl KeyBindingContextPredicate {
source,
))
}
_ => Err(anyhow!("unexpected character {next:?}")),
_ => Err(anyhow!("unexpected character '{next:?}'")),
}
}
@ -389,7 +389,7 @@ impl KeyBindingContextPredicate {
if let (Self::Identifier(left), Self::Identifier(right)) = (self, other) {
Ok(Self::Equal(left, right))
} else {
Err(anyhow!("operands must be identifiers"))
Err(anyhow!("operands of == must be identifiers"))
}
}
@ -397,7 +397,7 @@ impl KeyBindingContextPredicate {
if let (Self::Identifier(left), Self::Identifier(right)) = (self, other) {
Ok(Self::NotEqual(left, right))
} else {
Err(anyhow!("operands must be identifiers"))
Err(anyhow!("operands of != must be identifiers"))
}
}
}
@ -504,7 +504,7 @@ mod tests {
KeyBindingContextPredicate::parse("c == !d")
.unwrap_err()
.to_string(),
"operands must be identifiers"
"operands of == must be identifiers"
);
}

45
crates/gpui/src/platform/keystroke.rs
View file

@ -1,6 +1,8 @@
use anyhow::anyhow;
use serde::Deserialize;
use std::fmt::Write;
use std::{
error::Error,
fmt::{Display, Write},
};
/// A keystroke and associated metadata generated by the platform
#[derive(Clone, Debug, Eq, PartialEq, Default, Deserialize, Hash)]
@ -18,6 +20,31 @@ pub struct Keystroke {
pub key_char: Option<String>,
}
/// Error type for `Keystroke::parse`. This is used instead of `anyhow::Error` so that Zed can use
/// markdown to display it.
#[derive(Debug)]
pub struct InvalidKeystrokeError {
/// The invalid keystroke.
pub keystroke: String,
}
impl Error for InvalidKeystrokeError {}
impl Display for InvalidKeystrokeError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(
f,
"Invalid keystroke \"{}\". {}",
self.keystroke, KEYSTROKE_PARSE_EXPECTED_MESSAGE
)
}
}
/// Sentence explaining what keystroke parser expects, starting with "Expected ..."
pub const KEYSTROKE_PARSE_EXPECTED_MESSAGE: &str = "Expected a sequence of modifiers \
(`ctrl`, `alt`, `shift`, `fn`, `cmd`, `super`, or `win`) \
followed by a key, separated by `-`.";
impl Keystroke {
/// When matching a key we cannot know whether the user intended to type
/// the key_char or the key itself. On some non-US keyboards keys we use in our
@ -51,7 +78,7 @@ impl Keystroke {
/// [ctrl-][alt-][shift-][cmd-][fn-]key[->key_char]
/// key_char syntax is only used for generating test events,
/// when matching a key with an key_char set will be matched without it.
pub fn parse(source: &str) -> anyhow::Result<Self> {
pub fn parse(source: &str) -> std::result::Result<Self, InvalidKeystrokeError> {
let mut control = false;
let mut alt = false;
let mut shift = false;
@ -78,7 +105,9 @@ impl Keystroke {
key_char = Some(String::from(&next[1..]));
components.next();
} else {
return Err(anyhow!("Invalid keystroke `{}`", source));
return Err(InvalidKeystrokeError {
keystroke: source.to_owned(),
});
}
} else {
key = Some(String::from(component));
@ -87,8 +116,8 @@ impl Keystroke {
}
}
//Allow for the user to specify a keystroke modifier as the key itself
//This sets the `key` to the modifier, and disables the modifier
// Allow for the user to specify a keystroke modifier as the key itself
// This sets the `key` to the modifier, and disables the modifier
if key.is_none() {
if shift {
key = Some("shift".to_string());
@ -108,7 +137,9 @@ impl Keystroke {
}
}
let key = key.ok_or_else(|| anyhow!("Invalid keystroke `{}`", source))?;
let key = key.ok_or_else(|| InvalidKeystrokeError {
keystroke: source.to_owned(),
})?;
Ok(Keystroke {
modifiers: Modifiers {