From cc1af7d96bc00d45dfad790873e2b8d77027b6c7 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Tue, 21 Jan 2025 00:14:46 -0700 Subject: [PATCH] Display keymap errors on initial load (#23394) Also fixes issue introduced in #23113 where changes to keyboard layout would not cause reload of keymap configuration. Closes #20531 Release Notes: - N/A --- crates/settings/src/keymap_file.rs | 18 ++- crates/util/src/markdown.rs | 2 +- crates/zed/src/zed.rs | 209 +++++++++++++++++++---------- 3 files changed, 147 insertions(+), 82 deletions(-) diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index 57db095bee..27e67a7c9f 100644 --- a/crates/settings/src/keymap_file.rs +++ b/crates/settings/src/keymap_file.rs @@ -119,9 +119,6 @@ pub enum KeymapFileLoadResult { key_bindings: Vec, error_message: MarkdownString, }, - AllFailedToLoad { - error_message: MarkdownString, - }, JsonParseFailure { error: anyhow::Error, }, @@ -135,8 +132,7 @@ impl KeymapFile { 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!( + KeymapFileLoadResult::SomeFailedToLoad { error_message, .. } => Err(anyhow!( "Error loading built-in keymap \"{asset_path}\": {error_message}" )), KeymapFileLoadResult::JsonParseFailure { error } => Err(anyhow!( @@ -151,11 +147,14 @@ impl KeymapFile { 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!( + KeymapFileLoadResult::SomeFailedToLoad { + key_bindings, + error_message, + } if key_bindings.is_empty() => Err(anyhow!( "Error loading built-in keymap \"{asset_path}\": {error_message}" )), + KeymapFileLoadResult::Success { key_bindings, .. } + | KeymapFileLoadResult::SomeFailedToLoad { key_bindings, .. } => Ok(key_bindings), KeymapFileLoadResult::JsonParseFailure { error } => Err(anyhow!( "JSON parse error in built-in keymap \"{asset_path}\": {error}" )), @@ -166,8 +165,7 @@ impl KeymapFile { 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, .. } => { + KeymapFileLoadResult::SomeFailedToLoad { error_message, .. } => { panic!("{error_message}"); } KeymapFileLoadResult::JsonParseFailure { error } => { diff --git a/crates/util/src/markdown.rs b/crates/util/src/markdown.rs index a95b486f49..333887a9b1 100644 --- a/crates/util/src/markdown.rs +++ b/crates/util/src/markdown.rs @@ -1,7 +1,7 @@ use std::fmt::{Display, Formatter}; /// Markdown text. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct MarkdownString(pub String); impl Display for MarkdownString { diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 79f62e7f9d..aa4f08ba69 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -22,10 +22,10 @@ use feature_flags::FeatureFlagAppExt; use futures::FutureExt; use futures::{channel::mpsc, select_biased, StreamExt}; use gpui::{ - 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, + actions, point, px, Action, AnyWindowHandle, 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; @@ -1017,6 +1017,16 @@ pub fn handle_keymap_file_changes( }) .detach(); + // Need to notify about keymap load errors when new workspaces are created, so that initial + // keymap load errors are shown to the user. + let (new_workspace_window_tx, mut new_workspace_window_rx) = mpsc::unbounded(); + cx.observe_new_views(move |_: &mut Workspace, cx| { + new_workspace_window_tx + .unbounded_send(cx.window_handle()) + .unwrap(); + }) + .detach(); + let mut current_mapping = settings::get_key_equivalents(cx.keyboard_layout()); cx.on_keyboard_layout_change(move |cx| { let next_mapping = settings::get_key_equivalents(cx.keyboard_layout()); @@ -1033,68 +1043,102 @@ pub fn handle_keymap_file_changes( let notification_id = NotificationId::unique::(); cx.spawn(move |cx| async move { - let mut user_key_bindings = Vec::new(); + let mut user_keymap_content = String::new(); + enum LastError { + None, + JsonError(anyhow::Error), + LoadError(MarkdownString), + } + let mut last_load_error = LastError::None; 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 { - 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(); + let new_workspace_window = select_biased! { + _ = base_keymap_rx.next() => None, + _ = keyboard_layout_rx.next() => None, + workspace = new_workspace_window_rx.next() => workspace, + content = user_keymap_file_rx.next() => { + if let Some(content) = content { + user_keymap_content = content; + } + None + } + }; + cx.update(|cx| { + // No need to reload keymaps when a new workspace is added, just need to send the notification to it. + if new_workspace_window.is_none() { + let load_result = KeymapFile::load(&user_keymap_content, cx); + match load_result { + KeymapFileLoadResult::Success { key_bindings } => { + reload_keymaps(cx, key_bindings); + dismiss_app_notification(¬ification_id, cx); + last_load_error = LastError::None; + } + KeymapFileLoadResult::SomeFailedToLoad { + key_bindings, + error_message, + } => { + if !key_bindings.is_empty() { + reload_keymaps(cx, key_bindings); + } + last_load_error = LastError::LoadError(error_message); + } + KeymapFileLoadResult::JsonParseFailure { error } => { + last_load_error = LastError::JsonError(error); + } } } - } - cx.update(|cx| reload_keymaps(cx, user_key_bindings.clone())) - .ok(); + match &last_load_error { + LastError::None => {} + LastError::JsonError(err) => { + show_keymap_file_json_error( + new_workspace_window, + notification_id.clone(), + err, + cx, + ); + } + LastError::LoadError(message) => { + show_keymap_file_load_error( + new_workspace_window, + notification_id.clone(), + message.clone(), + cx, + ); + } + } + }) + .ok(); } }) .detach(); } fn show_keymap_file_json_error( + new_workspace_window: Option, notification_id: NotificationId, - error: anyhow::Error, + 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(); + show_notification_to_specific_workspace_or_all_workspaces( + new_workspace_window, + 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); + }) + }) + }, + ); } fn show_keymap_file_load_error( + new_workspace_window: Option, notification_id: NotificationId, markdown_error_message: MarkdownString, cx: &mut AppContext, @@ -1113,34 +1157,57 @@ fn show_keymap_file_load_error( 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() + show_notification_to_specific_workspace_or_all_workspaces( + new_workspace_window, + 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); + }) }) - .with_click_message("Open keymap file") - .on_click(|cx| { - cx.dispatch_action(zed_actions::OpenKeymap.boxed_clone()); - cx.emit(DismissEvent); - }) - }) - }) - .log_err(); + }, + ) }) - .log_err(); + .ok(); }) .detach(); } +fn show_notification_to_specific_workspace_or_all_workspaces( + new_workspace_window: Option, + notification_id: NotificationId, + cx: &mut AppContext, + build_notification: impl Fn(&mut ViewContext) -> View, +) where + V: workspace::notifications::Notification, +{ + if let Some(workspace_window) = new_workspace_window.and_then(|w| w.downcast::()) { + workspace_window + .update(cx, |workspace, cx| { + workspace.show_notification(notification_id, cx, build_notification); + }) + .ok(); + } else { + show_app_notification(notification_id, cx, build_notification).ok(); + } +} + fn reload_keymaps(cx: &mut AppContext, user_key_bindings: Vec) { cx.clear_key_bindings(); load_default_keymap(cx);