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

@@ -119,9 +119,6 @@ pub enum KeymapFileLoadResult {
         key_bindings: Vec<KeyBinding>,
         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<Vec<KeyBinding>> {
         match Self::load(asset_str::<SettingsAssets>(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<Vec<KeyBinding>> {
         match Self::load(asset_str::<SettingsAssets>(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<KeyBinding> {
         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 } => {

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 {

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,41 +1043,69 @@ pub fn handle_keymap_file_changes(
     let notification_id = NotificationId::unique::<KeymapParseErrorNotification>();
 
     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 {
+            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 } => {
-                                    user_key_bindings = key_bindings;
+                            reload_keymaps(cx, key_bindings);
                             dismiss_app_notification(&notification_id, cx);
+                            last_load_error = LastError::None;
                         }
                         KeymapFileLoadResult::SomeFailedToLoad {
                             key_bindings,
-                                    error_message
+                            error_message,
                         } => {
-                                    user_key_bindings = key_bindings;
-                                    show_keymap_file_load_error(notification_id.clone(), error_message, cx);
+                            if !key_bindings.is_empty() {
+                                reload_keymaps(cx, key_bindings);
                             }
-                                KeymapFileLoadResult::AllFailedToLoad {
-                                    error_message
-                                } => {
-                                    show_keymap_file_load_error(notification_id.clone(), error_message, cx);
+                            last_load_error = LastError::LoadError(error_message);
                         }
                         KeymapFileLoadResult::JsonParseFailure { error } => {
-                                    show_keymap_file_json_error(notification_id.clone(), error, cx);
-                                }
-                            };
-                        }).ok();
+                            last_load_error = LastError::JsonError(error);
                         }
                     }
                 }
-            cx.update(|cx| reload_keymaps(cx, user_key_bindings.clone()))
+                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();
         }
     })
@@ -1075,13 +1113,18 @@ pub fn handle_keymap_file_changes(
 }
 
 fn show_keymap_file_json_error(
+    new_workspace_window: Option<AnyWindowHandle>,
     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| {
+    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")
@@ -1090,11 +1133,12 @@ fn show_keymap_file_json_error(
                         cx.emit(DismissEvent);
                     })
             })
-    })
-    .log_err();
+        },
+    );
 }
 
 fn show_keymap_file_load_error(
+    new_workspace_window: Option<AnyWindowHandle>,
     notification_id: NotificationId,
     markdown_error_message: MarkdownString,
     cx: &mut AppContext,
@@ -1113,7 +1157,11 @@ 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| {
+            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| {
@@ -1133,14 +1181,33 @@ fn show_keymap_file_load_error(
                             cx.emit(DismissEvent);
                         })
                     })
+                },
+            )
         })
-            .log_err();
-        })
-        .log_err();
+        .ok();
     })
     .detach();
 }
 
+fn show_notification_to_specific_workspace_or_all_workspaces<V>(
+    new_workspace_window: Option<AnyWindowHandle>,
+    notification_id: NotificationId,
+    cx: &mut AppContext,
+    build_notification: impl Fn(&mut ViewContext<Workspace>) -> View<V>,
+) where
+    V: workspace::notifications::Notification,
+{
+    if let Some(workspace_window) = new_workspace_window.and_then(|w| w.downcast::<Workspace>()) {
+        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<KeyBinding>) {
     cx.clear_key_bindings();
     load_default_keymap(cx);