From 711dc21eb271fc81eb8b675ec444e1a29e90cf3e Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Sat, 18 Jan 2025 15:27:08 -0700 Subject: [PATCH] Load all key bindings that parse and use markdown in error notifications (#23113) * 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. --- Cargo.lock | 2 + crates/collab/src/tests/test_server.rs | 4 +- crates/command_palette/src/command_palette.rs | 9 +- .../src/docs_preprocessor.rs | 4 +- crates/gpui/src/action.rs | 64 +++- crates/gpui/src/app.rs | 12 +- crates/gpui/src/keymap/binding.rs | 34 +- crates/gpui/src/keymap/context.rs | 12 +- crates/gpui/src/platform/keystroke.rs | 45 ++- crates/language_tools/src/key_context_view.rs | 2 +- .../markdown_preview/src/markdown_renderer.rs | 14 +- crates/settings/src/keymap_file.rs | 362 ++++++++++++++---- crates/settings/src/settings.rs | 2 +- crates/storybook/src/storybook.rs | 2 +- crates/vim/Cargo.toml | 2 + crates/vim/src/test/vim_test_context.rs | 13 +- crates/zed/src/main.rs | 27 +- crates/zed/src/zed.rs | 136 +++++-- 18 files changed, 551 insertions(+), 195 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2a21a0fdbdb705..d001c3371b58a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14333,6 +14333,7 @@ name = "vim" version = "0.1.0" dependencies = [ "anyhow", + "assets", "async-compat", "async-trait", "collections", @@ -14350,6 +14351,7 @@ dependencies = [ "multi_buffer", "nvim-rs", "parking_lot", + "project_panel", "regex", "release_channel", "schemars", diff --git a/crates/collab/src/tests/test_server.rs b/crates/collab/src/tests/test_server.rs index 2e246b1408e291..5d0231153d0a17 100644 --- a/crates/collab/src/tests/test_server.rs +++ b/crates/collab/src/tests/test_server.rs @@ -304,7 +304,9 @@ impl TestServer { collab_ui::init(&app_state, cx); file_finder::init(cx); menu::init(); - settings::KeymapFile::load_asset(os_keymap, cx).unwrap(); + cx.bind_keys( + settings::KeymapFile::load_asset_allow_partial_failure(os_keymap, cx).unwrap(), + ); language_model::LanguageModelRegistry::test(cx); assistant::context_store::init(&client.clone().into()); }); diff --git a/crates/command_palette/src/command_palette.rs b/crates/command_palette/src/command_palette.rs index f065a986a15bde..ce0c36300e51f9 100644 --- a/crates/command_palette/src/command_palette.rs +++ b/crates/command_palette/src/command_palette.rs @@ -15,7 +15,6 @@ use gpui::{ ParentElement, Render, Styled, Task, UpdateGlobal, View, ViewContext, VisualContext, WeakView, }; use picker::{Picker, PickerDelegate}; - use postage::{sink::Sink, stream::Stream}; use settings::Settings; use ui::{h_flex, prelude::*, v_flex, HighlightedLabel, KeyBinding, ListItem, ListItemSpacing}; @@ -635,7 +634,7 @@ mod tests { workspace::init(app_state.clone(), cx); init(cx); Project::init_settings(cx); - KeymapFile::parse( + cx.bind_keys(KeymapFile::load_panic_on_failure( r#"[ { "bindings": { @@ -645,10 +644,8 @@ mod tests { } } ]"#, - ) - .unwrap() - .add_to_cx(cx) - .unwrap(); + cx, + )); app_state }) } diff --git a/crates/docs_preprocessor/src/docs_preprocessor.rs b/crates/docs_preprocessor/src/docs_preprocessor.rs index dcb08738795cc4..cd019c2c97caf9 100644 --- a/crates/docs_preprocessor/src/docs_preprocessor.rs +++ b/crates/docs_preprocessor/src/docs_preprocessor.rs @@ -32,8 +32,8 @@ impl PreprocessorContext { _ => return None, }; - keymap.blocks().iter().find_map(|block| { - block.bindings().iter().find_map(|(keystroke, a)| { + keymap.sections().find_map(|section| { + section.bindings().find_map(|(keystroke, a)| { if a.to_string() == action { Some(keystroke.to_string()) } else { diff --git a/crates/gpui/src/action.rs b/crates/gpui/src/action.rs index ee285b896bf040..993f9d152f36b6 100644 --- a/crates/gpui/src/action.rs +++ b/crates/gpui/src/action.rs @@ -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>; 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, - ) -> Result> { + ) -> std::result::Result, 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] { diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index 772447eb6f8042..770299ab00ee58 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -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, - ) -> Result> { + ) -> std::result::Result, ActionBuildError> { self.actions.build_action(name, data) } diff --git a/crates/gpui/src/keymap/binding.rs b/crates/gpui/src/keymap/binding.rs index ecf97658662932..cbe934212ffaf3 100644 --- a/crates/gpui/src/keymap/binding.rs +++ b/crates/gpui/src/keymap/binding.rs @@ -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, pub(crate) keystrokes: SmallVec<[Keystroke; 2]>, - pub(crate) context_predicate: Option, + pub(crate) context_predicate: Option>, } 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(keystrokes: &str, action: A, context_predicate: Option<&str>) -> Self { + /// Construct a new keybinding from the given data. Panics on parse error. + pub fn new(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, - context: Option<&str>, + context_predicate: Option>, key_equivalents: Option<&HashMap>, - ) -> Result { - let context = if let Some(context) = context { - Some(KeyBindingContextPredicate::parse(context)?) - } else { - None - }; - + ) -> std::result::Result { let mut keystrokes: SmallVec<[Keystroke; 2]> = keystrokes .split_whitespace() .map(Keystroke::parse) - .collect::>()?; + .collect::>()?; 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> { + self.context_predicate.as_ref().map(|rc| rc.clone()) } } diff --git a/crates/gpui/src/keymap/context.rs b/crates/gpui/src/keymap/context.rs index dbc3f50f320c31..3dc8292a5f61fe 100644 --- a/crates/gpui/src/keymap/context.rs +++ b/crates/gpui/src/keymap/context.rs @@ -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" ); } diff --git a/crates/gpui/src/platform/keystroke.rs b/crates/gpui/src/platform/keystroke.rs index af1e5179db7b4e..f96d3d1721fa86 100644 --- a/crates/gpui/src/platform/keystroke.rs +++ b/crates/gpui/src/platform/keystroke.rs @@ -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, } +/// 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 { + pub fn parse(source: &str) -> std::result::Result { 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 { diff --git a/crates/language_tools/src/key_context_view.rs b/crates/language_tools/src/key_context_view.rs index 856660f9af5784..4ce19245cfc681 100644 --- a/crates/language_tools/src/key_context_view.rs +++ b/crates/language_tools/src/key_context_view.rs @@ -52,7 +52,7 @@ impl KeyContextView { .into_iter() .map(|binding| { let match_state = if let Some(predicate) = binding.predicate() { - if this.matches(predicate) { + if this.matches(&predicate) { if this.action_matches(&e.action, binding.action()) { Some(true) } else { diff --git a/crates/markdown_preview/src/markdown_renderer.rs b/crates/markdown_preview/src/markdown_renderer.rs index b6b19fab917300..f7ddea32629ab5 100644 --- a/crates/markdown_preview/src/markdown_renderer.rs +++ b/crates/markdown_preview/src/markdown_renderer.rs @@ -110,15 +110,15 @@ pub fn render_parsed_markdown( parsed: &ParsedMarkdown, workspace: Option>, cx: &WindowContext, -) -> Vec { +) -> Div { let mut cx = RenderContext::new(workspace, cx); - let mut elements = Vec::new(); - for child in &parsed.children { - elements.push(render_markdown_block(child, &mut cx)); - } - - elements + v_flex().gap_3().children( + parsed + .children + .iter() + .map(|block| render_markdown_block(block, &mut cx)), + ) } pub fn render_markdown_block(block: &ParsedMarkdownElement, cx: &mut RenderContext) -> AnyElement { diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index b8c9cff21f3930..c06ba10516e3b3 100644 --- a/crates/settings/src/keymap_file.rs +++ b/crates/settings/src/keymap_file.rs @@ -1,7 +1,12 @@ +use std::rc::Rc; + use crate::{settings_store::parse_json_with_comments, SettingsAssets}; -use anyhow::{anyhow, Context, Result}; -use collections::{BTreeMap, HashMap}; -use gpui::{Action, AppContext, KeyBinding, NoAction, SharedString}; +use anyhow::anyhow; +use collections::{BTreeMap, HashMap, IndexMap}; +use gpui::{ + Action, ActionBuildError, AppContext, InvalidKeystrokeError, KeyBinding, + KeyBindingContextPredicate, NoAction, SharedString, KEYSTROKE_PARSE_EXPECTED_MESSAGE, +}; use schemars::{ gen::{SchemaGenerator, SchemaSettings}, schema::{ArrayValidation, InstanceType, Schema, SchemaObject, SubschemaValidation}, @@ -9,31 +14,37 @@ use schemars::{ }; use serde::Deserialize; use serde_json::Value; -use util::{asset_str, ResultExt}; +use std::fmt::Write; +use util::{asset_str, markdown::MarkdownString}; #[derive(Debug, Deserialize, Default, Clone, JsonSchema)] #[serde(transparent)] -pub struct KeymapFile(Vec); +pub struct KeymapFile(Vec); #[derive(Debug, Deserialize, Default, Clone, JsonSchema)] -pub struct KeymapBlock { +pub struct KeymapSection { #[serde(default)] - context: Option, + context: String, #[serde(default)] - use_key_equivalents: Option, - bindings: BTreeMap, + use_key_equivalents: bool, + #[serde(default)] + bindings: Option>, + #[serde(flatten)] + unrecognized_fields: IndexMap, } -impl KeymapBlock { - pub fn context(&self) -> Option<&str> { - self.context.as_deref() - } - - pub fn bindings(&self) -> &BTreeMap { - &self.bindings +impl KeymapSection { + pub fn bindings(&self) -> impl Iterator { + self.bindings.iter().flatten() } } +/// Keymap action as a JSON value, since it can either be null for no action, or the name of the +/// action, or an array of the name of the action and the action input. +/// +/// Unlike the other deserializable types here, this doc-comment will not be included in the +/// generated JSON schema, as it manually defines its `JsonSchema` impl. The actual schema used for +/// it is automatically generated in `KeymapFile::generate_json_schema`. #[derive(Debug, Deserialize, Default, Clone)] #[serde(transparent)] pub struct KeymapAction(Value); @@ -52,91 +63,278 @@ impl std::fmt::Display for KeymapAction { } impl JsonSchema for KeymapAction { + /// This is used when generating the JSON schema for the `KeymapAction` type, so that it can + /// reference the keymap action schema. fn schema_name() -> String { "KeymapAction".into() } + /// This schema will be replaced with the full action schema in + /// `KeymapFile::generate_json_schema`. fn json_schema(_: &mut SchemaGenerator) -> Schema { Schema::Bool(true) } } +#[derive(Debug)] +#[must_use] +pub enum KeymapFileLoadResult { + Success { + key_bindings: Vec, + }, + SomeFailedToLoad { + key_bindings: Vec, + error_message: MarkdownString, + }, + AllFailedToLoad { + error_message: MarkdownString, + }, + JsonParseFailure { + error: anyhow::Error, + }, +} + impl KeymapFile { - pub fn load_asset(asset_path: &str, cx: &mut AppContext) -> Result<()> { - let content = asset_str::(asset_path); + pub fn parse(content: &str) -> anyhow::Result { + parse_json_with_comments::(content) + } - Self::parse(content.as_ref())?.add_to_cx(cx) + pub fn load_asset(asset_path: &str, cx: &AppContext) -> anyhow::Result> { + match Self::load(asset_str::(asset_path).as_ref(), cx) { + KeymapFileLoadResult::Success { key_bindings, .. } => Ok(key_bindings), + KeymapFileLoadResult::SomeFailedToLoad { error_message, .. } + | KeymapFileLoadResult::AllFailedToLoad { error_message } => Err(anyhow!( + "Error loading built-in keymap \"{asset_path}\": {error_message}" + )), + KeymapFileLoadResult::JsonParseFailure { error } => Err(anyhow!( + "JSON parse error in built-in keymap \"{asset_path}\": {error}" + )), + } } - pub fn parse(content: &str) -> Result { - if content.is_empty() { - return Ok(Self::default()); + #[cfg(feature = "test-support")] + pub fn load_asset_allow_partial_failure( + asset_path: &str, + cx: &AppContext, + ) -> anyhow::Result> { + match Self::load(asset_str::(asset_path).as_ref(), cx) { + KeymapFileLoadResult::Success { key_bindings, .. } + | KeymapFileLoadResult::SomeFailedToLoad { key_bindings, .. } => Ok(key_bindings), + KeymapFileLoadResult::AllFailedToLoad { error_message } => Err(anyhow!( + "Error loading built-in keymap \"{asset_path}\": {error_message}" + )), + KeymapFileLoadResult::JsonParseFailure { error } => Err(anyhow!( + "JSON parse error in built-in keymap \"{asset_path}\": {error}" + )), + } + } + + #[cfg(feature = "test-support")] + pub fn load_panic_on_failure(content: &str, cx: &AppContext) -> Vec { + match Self::load(content, cx) { + KeymapFileLoadResult::Success { key_bindings } => key_bindings, + KeymapFileLoadResult::SomeFailedToLoad { error_message, .. } + | KeymapFileLoadResult::AllFailedToLoad { error_message, .. } => { + panic!("{error_message}"); + } + KeymapFileLoadResult::JsonParseFailure { error } => { + panic!("JSON parse error: {error}"); + } } - parse_json_with_comments::(content) } - pub fn add_to_cx(self, cx: &mut AppContext) -> Result<()> { + pub fn load(content: &str, cx: &AppContext) -> KeymapFileLoadResult { let key_equivalents = crate::key_equivalents::get_key_equivalents(&cx.keyboard_layout()); - for KeymapBlock { + if content.is_empty() { + return KeymapFileLoadResult::Success { + key_bindings: Vec::new(), + }; + } + let keymap_file = match parse_json_with_comments::(content) { + Ok(keymap_file) => keymap_file, + Err(error) => { + return KeymapFileLoadResult::JsonParseFailure { error }; + } + }; + + // Accumulate errors in order to support partial load of user keymap in the presence of + // errors in context and binding parsing. + let mut errors = Vec::new(); + let mut key_bindings = Vec::new(); + + for KeymapSection { context, use_key_equivalents, bindings, - } in self.0 + unrecognized_fields, + } in keymap_file.0.iter() { - let bindings = bindings - .into_iter() - .filter_map(|(keystroke, action)| { - let action = action.0; - - // This is a workaround for a limitation in serde: serde-rs/json#497 - // We want to deserialize the action data as a `RawValue` so that we can - // deserialize the action itself dynamically directly from the JSON - // string. But `RawValue` currently does not work inside of an untagged enum. - match action { - Value::Array(items) => { - let Ok([name, data]): Result<[serde_json::Value; 2], _> = - items.try_into() - else { - return Some(Err(anyhow!("Expected array of length 2"))); - }; - let serde_json::Value::String(name) = name else { - return Some(Err(anyhow!( - "Expected first item in array to be a string." - ))); - }; - cx.build_action(&name, Some(data)) + let context_predicate: Option> = if context.is_empty() { + None + } else { + match KeyBindingContextPredicate::parse(context) { + Ok(context_predicate) => Some(context_predicate.into()), + Err(err) => { + // Leading space is to separate from the message indicating which section + // the error occurred in. + errors.push(( + context, + format!(" Parse error in section `context` field: {}", err), + )); + continue; + } + } + }; + + let key_equivalents = if *use_key_equivalents { + key_equivalents.as_ref() + } else { + None + }; + + let mut section_errors = String::new(); + + if !unrecognized_fields.is_empty() { + write!( + section_errors, + "\n\n - Unrecognized fields: {}", + MarkdownString::inline_code(&format!("{:?}", unrecognized_fields.keys())) + ) + .unwrap(); + } + + if let Some(bindings) = bindings { + for binding in bindings { + let (keystrokes, action) = binding; + let result = Self::load_keybinding( + keystrokes, + action, + context_predicate.clone(), + key_equivalents, + cx, + ); + match result { + Ok(key_binding) => { + key_bindings.push(key_binding); } - Value::String(name) => cx.build_action(&name, None), - Value::Null => Ok(no_action()), - _ => { - return Some(Err(anyhow!("Expected two-element array, got {action:?}"))) + Err(err) => { + write!( + section_errors, + "\n\n - In binding {}, {err}", + inline_code_string(keystrokes), + ) + .unwrap(); } } - .with_context(|| { - format!( - "invalid binding value for keystroke {keystroke}, context {context:?}" - ) - }) - .log_err() - .map(|action| { - KeyBinding::load( - &keystroke, - action, - context.as_deref(), - if use_key_equivalents.unwrap_or_default() { - key_equivalents.as_ref() - } else { - None - }, - ) - }) - }) - .collect::>>()?; - - cx.bind_keys(bindings); + } + } + + if !section_errors.is_empty() { + errors.push((context, section_errors)) + } + } + + if errors.is_empty() { + KeymapFileLoadResult::Success { key_bindings } + } else { + let mut error_message = "Errors in user keymap file.\n".to_owned(); + for (context, section_errors) in errors { + if context.is_empty() { + write!(error_message, "\n\nIn section without context predicate:").unwrap() + } else { + write!( + error_message, + "\n\nIn section with {}:", + MarkdownString::inline_code(&format!("context = \"{}\"", context)) + ) + .unwrap() + } + write!(error_message, "{section_errors}").unwrap(); + } + KeymapFileLoadResult::SomeFailedToLoad { + key_bindings, + error_message: MarkdownString(error_message), + } + } + } + + fn load_keybinding( + keystrokes: &str, + action: &KeymapAction, + context: Option>, + key_equivalents: Option<&HashMap>, + cx: &AppContext, + ) -> std::result::Result { + let (build_result, action_input_string) = match &action.0 { + Value::Array(items) => { + if items.len() != 2 { + return Err(format!( + "expected two-element array of `[name, input]`. \ + Instead found {}.", + MarkdownString::inline_code(&action.0.to_string()) + )); + } + let serde_json::Value::String(ref name) = items[0] else { + return Err(format!( + "expected two-element array of `[name, input]`, \ + but the first element is not a string in {}.", + MarkdownString::inline_code(&action.0.to_string()) + )); + }; + let action_input = items[1].clone(); + let action_input_string = action_input.to_string(); + ( + cx.build_action(&name, Some(action_input)), + Some(action_input_string), + ) + } + Value::String(name) => (cx.build_action(&name, None), None), + Value::Null => (Ok(NoAction.boxed_clone()), None), + _ => { + return Err(format!( + "expected two-element array of `[name, input]`. \ + Instead found {}.", + MarkdownString::inline_code(&action.0.to_string()) + )); + } + }; + + let action = match build_result { + Ok(action) => action, + Err(ActionBuildError::NotFound { name }) => { + return Err(format!( + "didn't find an action named {}.", + inline_code_string(&name) + )) + } + Err(ActionBuildError::BuildError { name, error }) => match action_input_string { + Some(action_input_string) => { + return Err(format!( + "can't build {} action from input value {}: {}", + inline_code_string(&name), + MarkdownString::inline_code(&action_input_string), + MarkdownString::escape(&error.to_string()) + )) + } + None => { + return Err(format!( + "can't build {} action - it requires input data via [name, input]: {}", + inline_code_string(&name), + MarkdownString::escape(&error.to_string()) + )) + } + }, + }; + + match KeyBinding::load(keystrokes, action, context, key_equivalents) { + Ok(binding) => Ok(binding), + Err(InvalidKeystrokeError { keystroke }) => Err(format!( + "invalid keystroke {}. {}", + inline_code_string(&keystroke), + KEYSTROKE_PARSE_EXPECTED_MESSAGE + )), } - Ok(()) } pub fn generate_json_schema_for_registered_actions(cx: &mut AppContext) -> Value { @@ -308,23 +506,26 @@ impl KeymapFile { } .into(); + // The `KeymapSection` schema will reference the `KeymapAction` schema by name, so replacing + // the definition of `KeymapAction` results in the full action schema being used. let mut root_schema = generator.into_root_schema_for::(); root_schema .definitions - .insert("KeymapAction".to_owned(), action_schema); + .insert(KeymapAction::schema_name(), action_schema); // This and other json schemas can be viewed via `debug: open language server logs` -> // `json-language-server` -> `Server Info`. serde_json::to_value(root_schema).unwrap() } - pub fn blocks(&self) -> &[KeymapBlock] { - &self.0 + pub fn sections(&self) -> impl Iterator { + self.0.iter() } } -fn no_action() -> Box { - gpui::NoAction.boxed_clone() +// Double quotes a string and wraps it in backticks for markdown inline code.. +fn inline_code_string(text: &str) -> MarkdownString { + MarkdownString::inline_code(&format!("\"{}\"", text)) } #[cfg(test)] @@ -342,7 +543,6 @@ mod tests { }, ] " - }; KeymapFile::parse(json).unwrap(); } diff --git a/crates/settings/src/settings.rs b/crates/settings/src/settings.rs index 359171fff6b712..e347dcddddf2b0 100644 --- a/crates/settings/src/settings.rs +++ b/crates/settings/src/settings.rs @@ -13,7 +13,7 @@ use util::asset_str; pub use editable_setting_control::*; pub use json_schema::*; pub use key_equivalents::*; -pub use keymap_file::KeymapFile; +pub use keymap_file::{KeymapFile, KeymapFileLoadResult}; pub use settings_file::*; pub use settings_store::{ parse_json_with_comments, InvalidSettingsError, LocalSettingsKind, Settings, SettingsLocation, diff --git a/crates/storybook/src/storybook.rs b/crates/storybook/src/storybook.rs index 9fe61a70c4d818..7f66deef5f1f6b 100644 --- a/crates/storybook/src/storybook.rs +++ b/crates/storybook/src/storybook.rs @@ -145,7 +145,7 @@ fn load_embedded_fonts(cx: &AppContext) -> gpui::Result<()> { } fn load_storybook_keymap(cx: &mut AppContext) { - KeymapFile::load_asset("keymaps/storybook.json", cx).unwrap(); + cx.bind_keys(KeymapFile::load_asset("keymaps/storybook.json", cx).unwrap()); } pub fn init(cx: &mut AppContext) { diff --git a/crates/vim/Cargo.toml b/crates/vim/Cargo.toml index b84d2b65d329fc..46915c9f6eb879 100644 --- a/crates/vim/Cargo.toml +++ b/crates/vim/Cargo.toml @@ -48,6 +48,7 @@ workspace.workspace = true zed_actions.workspace = true [dev-dependencies] +assets.workspace = true command_palette.workspace = true editor = { workspace = true, features = ["test-support"] } gpui = { workspace = true, features = ["test-support"] } @@ -55,6 +56,7 @@ indoc.workspace = true language = { workspace = true, features = ["test-support"] } lsp = { workspace = true, features = ["test-support"] } parking_lot.workspace = true +project_panel.workspace = true release_channel.workspace = true settings.workspace = true util = { workspace = true, features = ["test-support"] } diff --git a/crates/vim/src/test/vim_test_context.rs b/crates/vim/src/test/vim_test_context.rs index c985f68e701eb1..2d28f67f5eb050 100644 --- a/crates/vim/src/test/vim_test_context.rs +++ b/crates/vim/src/test/vim_test_context.rs @@ -1,5 +1,6 @@ use std::ops::{Deref, DerefMut}; +use assets::Assets; use editor::test::editor_lsp_test_context::EditorLspTestContext; use gpui::{Context, SemanticVersion, UpdateGlobal, View, VisualContext}; use search::{project_search::ProjectSearchBar, BufferSearchBar}; @@ -20,6 +21,7 @@ impl VimTestContext { cx.set_global(settings); release_channel::init(SemanticVersion::default(), cx); command_palette::init(cx); + project_panel::init(Assets, cx); crate::init(cx); search::init(cx); }); @@ -59,9 +61,16 @@ impl VimTestContext { SettingsStore::update_global(cx, |store, cx| { store.update_user_settings::(cx, |s| *s = Some(enabled)); }); - settings::KeymapFile::load_asset("keymaps/default-macos.json", cx).unwrap(); + let default_key_bindings = settings::KeymapFile::load_asset_allow_partial_failure( + "keymaps/default-macos.json", + cx, + ) + .unwrap(); + cx.bind_keys(default_key_bindings); if enabled { - settings::KeymapFile::load_asset("keymaps/vim.json", cx).unwrap(); + let vim_key_bindings = + settings::KeymapFile::load_asset("keymaps/vim.json", cx).unwrap(); + cx.bind_keys(vim_key_bindings); } }); diff --git a/crates/zed/src/main.rs b/crates/zed/src/main.rs index 4283c8de049a66..52d8808d1ae54a 100644 --- a/crates/zed/src/main.rs +++ b/crates/zed/src/main.rs @@ -291,7 +291,7 @@ fn main() { } settings::init(cx); handle_settings_file_changes(user_settings_file_rx, cx, handle_settings_changed); - handle_keymap_file_changes(user_keymap_file_rx, cx, handle_keymap_changed); + handle_keymap_file_changes(user_keymap_file_rx, cx); client::init_settings(cx); let user_agent = format!( "Zed/{} ({}; {})", @@ -609,31 +609,6 @@ fn main() { }); } -fn handle_keymap_changed(error: Option, cx: &mut AppContext) { - struct KeymapParseErrorNotification; - let id = NotificationId::unique::(); - - for workspace in workspace::local_workspace_windows(cx) { - workspace - .update(cx, |workspace, cx| match &error { - Some(error) => { - workspace.show_notification(id.clone(), cx, |cx| { - cx.new_view(|_| { - MessageNotification::new(format!("Invalid keymap file\n{error}")) - .with_click_message("Open keymap file") - .on_click(|cx| { - cx.dispatch_action(zed_actions::OpenKeymap.boxed_clone()); - cx.emit(DismissEvent); - }) - }) - }); - } - None => workspace.dismiss_notification(&id, cx), - }) - .log_err(); - } -} - fn handle_settings_changed(error: Option, cx: &mut AppContext) { struct SettingsParseErrorNotification; let id = NotificationId::unique::(); diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 11bff8bdbaf81e..bef0da0a9b2804 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -22,9 +22,10 @@ use feature_flags::FeatureFlagAppExt; use futures::FutureExt; use futures::{channel::mpsc, select_biased, StreamExt}; use gpui::{ - actions, point, px, AppContext, AsyncAppContext, Context, FocusableView, MenuItem, - PathPromptOptions, PromptLevel, ReadGlobal, Task, TitlebarOptions, View, ViewContext, - VisualContext, WindowKind, WindowOptions, + actions, point, px, Action, AppContext, AsyncAppContext, Context, DismissEvent, Element, + FocusableView, KeyBinding, MenuItem, ParentElement, PathPromptOptions, PromptLevel, ReadGlobal, + SharedString, Styled, Task, TitlebarOptions, View, ViewContext, VisualContext, WindowKind, + WindowOptions, }; pub use open_listener::*; use outline_panel::OutlinePanel; @@ -39,18 +40,20 @@ use rope::Rope; use search::project_search::ProjectSearchBar; use settings::{ initial_project_settings_content, initial_tasks_content, update_settings_file, KeymapFile, - Settings, SettingsStore, DEFAULT_KEYMAP_PATH, VIM_KEYMAP_PATH, + KeymapFileLoadResult, Settings, SettingsStore, DEFAULT_KEYMAP_PATH, VIM_KEYMAP_PATH, }; use std::any::TypeId; use std::path::PathBuf; +use std::rc::Rc; use std::{borrow::Cow, ops::Deref, path::Path, sync::Arc}; use terminal_view::terminal_panel::{self, TerminalPanel}; use theme::{ActiveTheme, ThemeSettings}; +use util::markdown::MarkdownString; use util::{asset_str, ResultExt}; use uuid::Uuid; use vim_mode_setting::VimModeSetting; use welcome::{BaseKeymap, MultibufferHint}; -use workspace::notifications::NotificationId; +use workspace::notifications::{dismiss_app_notification, show_app_notification, NotificationId}; use workspace::CloseIntent; use workspace::{ create_and_open_local_file, notifications::simple_message_notification::MessageNotification, @@ -983,7 +986,6 @@ fn open_log_file(workspace: &mut Workspace, cx: &mut ViewContext) { pub fn handle_keymap_file_changes( mut user_keymap_file_rx: mpsc::UnboundedReceiver, cx: &mut AppContext, - keymap_changed: impl Fn(Option, &mut AppContext) + 'static, ) { BaseKeymap::register(cx); VimModeSetting::register(cx); @@ -1016,36 +1018,122 @@ pub fn handle_keymap_file_changes( load_default_keymap(cx); + struct KeymapParseErrorNotification; + let notification_id = NotificationId::unique::(); + cx.spawn(move |cx| async move { - let mut user_keymap = KeymapFile::default(); + let mut user_key_bindings = Vec::new(); loop { select_biased! { _ = base_keymap_rx.next() => {} _ = keyboard_layout_rx.next() => {} user_keymap_content = user_keymap_file_rx.next() => { if let Some(user_keymap_content) = user_keymap_content { - match KeymapFile::parse(&user_keymap_content) { - Ok(keymap_content) => { - cx.update(|cx| keymap_changed(None, cx)).log_err(); - user_keymap = keymap_content; - } - Err(error) => { - cx.update(|cx| keymap_changed(Some(error), cx)).log_err(); - } - } + cx.update(|cx| { + let load_result = KeymapFile::load(&user_keymap_content, cx); + match load_result { + KeymapFileLoadResult::Success { key_bindings } => { + user_key_bindings = key_bindings; + dismiss_app_notification(¬ification_id, cx); + } + KeymapFileLoadResult::SomeFailedToLoad { + key_bindings, + error_message + } => { + user_key_bindings = key_bindings; + show_keymap_file_load_error(notification_id.clone(), error_message, cx); + } + KeymapFileLoadResult::AllFailedToLoad { + error_message + } => { + show_keymap_file_load_error(notification_id.clone(), error_message, cx); + } + KeymapFileLoadResult::JsonParseFailure { error } => { + show_keymap_file_json_error(notification_id.clone(), error, cx); + } + }; + }).ok(); } } } - cx.update(|cx| reload_keymaps(cx, &user_keymap)).ok(); + cx.update(|cx| reload_keymaps(cx, user_key_bindings.clone())) + .ok(); } }) .detach(); } -fn reload_keymaps(cx: &mut AppContext, keymap_content: &KeymapFile) { +fn show_keymap_file_json_error( + notification_id: NotificationId, + error: anyhow::Error, + cx: &mut AppContext, +) { + let message: SharedString = + format!("JSON parse error in keymap file. Bindings not reloaded.\n\n{error}").into(); + show_app_notification(notification_id, cx, move |cx| { + cx.new_view(|_cx| { + MessageNotification::new(message.clone()) + .with_click_message("Open keymap file") + .on_click(|cx| { + cx.dispatch_action(zed_actions::OpenKeymap.boxed_clone()); + cx.emit(DismissEvent); + }) + }) + }) + .log_err(); +} + +fn show_keymap_file_load_error( + notification_id: NotificationId, + markdown_error_message: MarkdownString, + cx: &mut AppContext, +) { + let parsed_markdown = cx.background_executor().spawn(async move { + let file_location_directory = None; + let language_registry = None; + markdown_preview::markdown_parser::parse_markdown( + &markdown_error_message.0, + file_location_directory, + language_registry, + ) + .await + }); + + cx.spawn(move |cx| async move { + let parsed_markdown = Rc::new(parsed_markdown.await); + cx.update(|cx| { + show_app_notification(notification_id, cx, move |cx| { + let workspace_handle = cx.view().downgrade(); + let parsed_markdown = parsed_markdown.clone(); + cx.new_view(move |_cx| { + MessageNotification::new_from_builder(move |cx| { + gpui::div() + .text_xs() + .child(markdown_preview::markdown_renderer::render_parsed_markdown( + &parsed_markdown.clone(), + Some(workspace_handle.clone()), + cx, + )) + .into_any() + }) + .with_click_message("Open keymap file") + .on_click(|cx| { + cx.dispatch_action(zed_actions::OpenKeymap.boxed_clone()); + cx.emit(DismissEvent); + }) + }) + }) + .log_err(); + }) + .log_err(); + }) + .detach(); +} + +fn reload_keymaps(cx: &mut AppContext, user_key_bindings: Vec) { cx.clear_key_bindings(); load_default_keymap(cx); - keymap_content.clone().add_to_cx(cx).log_err(); + cx.bind_keys(user_key_bindings); cx.set_menus(app_menus()); cx.set_dock_menu(vec![MenuItem::action("New Window", workspace::NewWindow)]); } @@ -1056,13 +1144,13 @@ pub fn load_default_keymap(cx: &mut AppContext) { return; } - KeymapFile::load_asset(DEFAULT_KEYMAP_PATH, cx).unwrap(); + cx.bind_keys(KeymapFile::load_asset(DEFAULT_KEYMAP_PATH, cx).unwrap()); if VimModeSetting::get_global(cx).0 { - KeymapFile::load_asset(VIM_KEYMAP_PATH, cx).unwrap(); + cx.bind_keys(KeymapFile::load_asset(VIM_KEYMAP_PATH, cx).unwrap()); } if let Some(asset_path) = base_keymap.asset_path() { - KeymapFile::load_asset(asset_path, cx).unwrap(); + cx.bind_keys(KeymapFile::load_asset(asset_path, cx).unwrap()); } } @@ -3375,7 +3463,7 @@ mod tests { PathBuf::from("/keymap.json"), ); handle_settings_file_changes(settings_rx, cx, |_, _| {}); - handle_keymap_file_changes(keymap_rx, cx, |_, _| {}); + handle_keymap_file_changes(keymap_rx, cx); }); workspace .update(cx, |workspace, cx| { @@ -3488,7 +3576,7 @@ mod tests { ); handle_settings_file_changes(settings_rx, cx, |_, _| {}); - handle_keymap_file_changes(keymap_rx, cx, |_, _| {}); + handle_keymap_file_changes(keymap_rx, cx); }); cx.background_executor.run_until_parked();