Make app notifications appear in new workspaces + dismiss on all workspaces (#23432)

The keymap error notifications got convoluted to support displaying the
notification on startup. This change addresses it systemically for all
future app notifications.

Reverts most of #20531, while keeping the fix to handle keyboard layout
switching. This is a better fix for #20531

Release Notes:

- N/A

crates/workspace/src/notifications.rs

@@ -2,13 +2,21 @@ use crate::{Toast, Workspace};
 use anyhow::Context;
 use anyhow::{anyhow, Result};
 use gpui::{
-    svg, AppContext, AsyncWindowContext, ClipboardItem, DismissEvent, EventEmitter, PromptLevel,
-    Render, ScrollHandle, Task, View, ViewContext, VisualContext, WindowContext,
+    svg, AnyView, AppContext, AsyncWindowContext, ClipboardItem, DismissEvent, EventEmitter,
+    Global, PromptLevel, Render, ScrollHandle, Task, View, ViewContext, VisualContext,
+    WindowContext,
 };
+use std::rc::Rc;
 use std::{any::TypeId, time::Duration};
 use ui::{prelude::*, Tooltip};
 use util::ResultExt;
 
+pub fn init(cx: &mut AppContext) {
+    cx.set_global(GlobalAppNotifications {
+        app_notifications: Vec::new(),
+    })
+}
+
 #[derive(Debug, PartialEq, Clone)]
 pub enum NotificationId {
     Unique(TypeId),
@@ -54,17 +62,33 @@ impl Workspace {
         cx: &mut ViewContext<Self>,
         build_notification: impl FnOnce(&mut ViewContext<Self>) -> View<V>,
     ) {
-        self.dismiss_notification_internal(&id, cx);
+        self.show_notification_without_handling_dismiss_events(&id, cx, |cx| {
+            let notification = build_notification(cx);
+            cx.subscribe(&notification, {
+                let id = id.clone();
+                move |this, _, _: &DismissEvent, cx| {
+                    this.dismiss_notification(&id, cx);
+                }
+            })
+            .detach();
+            notification.into()
+        });
+    }
 
-        let notification = build_notification(cx);
-        cx.subscribe(&notification, {
-            let id = id.clone();
-            move |this, _, _: &DismissEvent, cx| {
-                this.dismiss_notification_internal(&id, cx);
-            }
-        })
-        .detach();
-        self.notifications.push((id, notification.into()));
+    /// Shows a notification in this workspace's window. Caller must handle dismiss.
+    ///
+    /// This exists so that the `build_notification` closures stored for app notifications can
+    /// return `AnyView`. Subscribing to events from an `AnyView` is not supported, so instead that
+    /// responsibility is pushed to the caller where the `V` type is known.
+    pub(crate) fn show_notification_without_handling_dismiss_events(
+        &mut self,
+        id: &NotificationId,
+        cx: &mut ViewContext<Self>,
+        build_notification: impl FnOnce(&mut ViewContext<Self>) -> AnyView,
+    ) {
+        self.dismiss_notification(id, cx);
+        self.notifications
+            .push((id.clone(), build_notification(cx)));
         cx.notify();
     }
 
@@ -91,7 +115,14 @@ impl Workspace {
     }
 
     pub fn dismiss_notification(&mut self, id: &NotificationId, cx: &mut ViewContext<Self>) {
-        self.dismiss_notification_internal(id, cx)
+        self.notifications.retain(|(existing_id, _)| {
+            if existing_id == id {
+                cx.notify();
+                false
+            } else {
+                true
+            }
+        });
     }
 
     pub fn show_toast(&mut self, toast: Toast, cx: &mut ViewContext<Self>) {
@@ -131,15 +162,18 @@ impl Workspace {
         cx.notify();
     }
 
-    fn dismiss_notification_internal(&mut self, id: &NotificationId, cx: &mut ViewContext<Self>) {
-        self.notifications.retain(|(existing_id, _)| {
-            if existing_id == id {
-                cx.notify();
-                false
-            } else {
-                true
-            }
-        });
+    pub fn show_initial_notifications(&mut self, cx: &mut ViewContext<Self>) {
+        // Allow absence of the global so that tests don't need to initialize it.
+        let app_notifications = cx
+            .try_global::<GlobalAppNotifications>()
+            .iter()
+            .flat_map(|global| global.app_notifications.iter().cloned())
+            .collect::<Vec<_>>();
+        for (id, build_notification) in app_notifications {
+            self.show_notification_without_handling_dismiss_events(&id, cx, |cx| {
+                build_notification(cx)
+            });
+        }
     }
 }
 
@@ -467,66 +501,103 @@ pub mod simple_message_notification {
     }
 }
 
-/// Shows a notification in the active workspace if there is one, otherwise shows it in all workspaces.
-pub fn show_app_notification<V: Notification>(
+/// Stores app notifications so that they can be shown in new workspaces.
+struct GlobalAppNotifications {
+    app_notifications: Vec<(
+        NotificationId,
+        Rc<dyn Fn(&mut ViewContext<Workspace>) -> AnyView>,
+    )>,
+}
+
+impl Global for GlobalAppNotifications {}
+
+impl GlobalAppNotifications {
+    pub fn insert(
+        &mut self,
+        id: NotificationId,
+        build_notification: Rc<dyn Fn(&mut ViewContext<Workspace>) -> AnyView>,
+    ) {
+        self.remove(&id);
+        self.app_notifications.push((id, build_notification))
+    }
+
+    pub fn remove(&mut self, id: &NotificationId) {
+        self.app_notifications
+            .retain(|(existing_id, _)| existing_id != id);
+    }
+}
+
+/// Shows a notification in all workspaces. New workspaces will also receive the notification - this
+/// is particularly to handle notifications that occur on initialization before any workspaces
+/// exist. If the notification is dismissed within any workspace, it will be removed from all.
+pub fn show_app_notification<V: Notification + 'static>(
     id: NotificationId,
     cx: &mut AppContext,
-    build_notification: impl Fn(&mut ViewContext<Workspace>) -> View<V>,
+    build_notification: impl Fn(&mut ViewContext<Workspace>) -> View<V> + 'static,
 ) -> Result<()> {
-    let workspaces_to_notify = if let Some(active_workspace_window) = cx
-        .active_window()
-        .and_then(|window| window.downcast::<Workspace>())
-    {
-        vec![active_workspace_window]
-    } else {
-        let mut workspaces_to_notify = Vec::new();
-        for window in cx.windows() {
-            if let Some(workspace_window) = window.downcast::<Workspace>() {
-                workspaces_to_notify.push(workspace_window);
-            }
+    // Handle dismiss events by removing the notification from all workspaces.
+    let build_notification: Rc<dyn Fn(&mut ViewContext<Workspace>) -> AnyView> = Rc::new({
+        let id = id.clone();
+        move |cx| {
+            let notification = build_notification(cx);
+            cx.subscribe(&notification, {
+                let id = id.clone();
+                move |_, _, _: &DismissEvent, cx| {
+                    dismiss_app_notification(&id, cx);
+                }
+            })
+            .detach();
+            notification.into()
         }
-        workspaces_to_notify
-    };
+    });
+
+    // Store the notification so that new workspaces also receive it.
+    cx.global_mut::<GlobalAppNotifications>()
+        .insert(id.clone(), build_notification.clone());
 
-    let mut notified = false;
     let mut notify_errors = Vec::new();
 
-    for workspace_window in workspaces_to_notify {
-        let notify_result = workspace_window.update(cx, |workspace, cx| {
-            workspace.show_notification(id.clone(), cx, &build_notification);
-        });
-        match notify_result {
-            Ok(()) => notified = true,
-            Err(notify_err) => notify_errors.push(notify_err),
+    for window in cx.windows() {
+        if let Some(workspace_window) = window.downcast::<Workspace>() {
+            let notify_result = workspace_window.update(cx, |workspace, cx| {
+                workspace.show_notification_without_handling_dismiss_events(&id, cx, |cx| {
+                    build_notification(cx)
+                });
+            });
+            match notify_result {
+                Ok(()) => {}
+                Err(notify_err) => notify_errors.push(notify_err),
+            }
         }
     }
 
-    if notified {
+    if notify_errors.is_empty() {
         Ok(())
     } else {
-        if notify_errors.is_empty() {
-            Err(anyhow!("Found no workspaces to show notification."))
-        } else {
-            Err(anyhow!(
-                "No workspaces were able to show notification. Errors:\n\n{}",
-                notify_errors
-                    .iter()
-                    .map(|e| e.to_string())
-                    .collect::<Vec<_>>()
-                    .join("\n\n")
-            ))
-        }
+        Err(anyhow!(
+            "No workspaces were able to show notification. Errors:\n\n{}",
+            notify_errors
+                .iter()
+                .map(|e| e.to_string())
+                .collect::<Vec<_>>()
+                .join("\n\n")
+        ))
     }
 }
 
 pub fn dismiss_app_notification(id: &NotificationId, cx: &mut AppContext) {
+    cx.global_mut::<GlobalAppNotifications>().remove(id);
     for window in cx.windows() {
         if let Some(workspace_window) = window.downcast::<Workspace>() {
-            workspace_window
-                .update(cx, |workspace, cx| {
-                    workspace.dismiss_notification(&id, cx);
+            let id = id.clone();
+            // This spawn is necessary in order to dismiss the notification on which the click
+            // occurred, because in that case we're already in the middle of an update.
+            cx.spawn(move |mut cx| async move {
+                workspace_window.update(&mut cx, |workspace, cx| {
+                    workspace.dismiss_notification(&id, cx)
                 })
-                .ok();
+            })
+            .detach_and_log_err(cx);
         }
     }
 }
@@ -556,7 +627,7 @@ where
         match self {
             Ok(value) => Some(value),
             Err(err) => {
-                log::error!("TODO {err:?}");
+                log::error!("Showing error notification in workspace: {err:?}");
                 workspace.show_error(&err, cx);
                 None
             }
@@ -584,10 +655,17 @@ where
             Ok(value) => Some(value),
             Err(err) => {
                 let message: SharedString = format!("Error: {err}").into();
-                show_app_notification(workspace_error_notification_id(), cx, |cx| {
-                    cx.new_view(|_cx| ErrorMessagePrompt::new(message.clone()))
+                log::error!("Showing error notification in app: {message}");
+                show_app_notification(workspace_error_notification_id(), cx, {
+                    let message = message.clone();
+                    move |cx| {
+                        cx.new_view({
+                            let message = message.clone();
+                            move |_cx| ErrorMessagePrompt::new(message)
+                        })
+                    }
                 })
-                .with_context(|| format!("Showing error notification: {message}"))
+                .with_context(|| format!("Error while showing error notification: {message}"))
                 .log_err();
                 None
             }

crates/workspace/src/workspace.rs

@@ -368,6 +368,7 @@ fn prompt_and_open_paths(
 
 pub fn init(app_state: Arc<AppState>, cx: &mut AppContext) {
     init_settings(cx);
+    notifications::init(cx);
     theme_preview::init(cx);
 
     cx.on_action(Workspace::close_global);
@@ -1056,6 +1057,7 @@ impl Workspace {
 
         cx.defer(|this, cx| {
             this.update_window_title(cx);
+            this.show_initial_notifications(cx);
         });
         Workspace {
             weak_self: weak_handle.clone(),

crates/zed/src/zed.rs

@@ -23,10 +23,10 @@ use feature_flags::FeatureFlagAppExt;
 use futures::FutureExt;
 use futures::{channel::mpsc, select_biased, StreamExt};
 use gpui::{
-    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,
+    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;
@@ -1053,16 +1053,6 @@ 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());
@@ -1080,65 +1070,34 @@ pub fn handle_keymap_file_changes(
 
     cx.spawn(move |cx| async move {
         let mut user_keymap_content = String::new();
-        enum LastError {
-            None,
-            JsonError(anyhow::Error),
-            LoadError(MarkdownString),
-        }
-        let mut last_load_error = LastError::None;
         loop {
-            let new_workspace_window = select_biased! {
-                _ = base_keymap_rx.next() => None,
-                _ = keyboard_layout_rx.next() => None,
-                workspace = new_workspace_window_rx.next() => workspace,
+            select_biased! {
+                _ = base_keymap_rx.next() => {},
+                _ = keyboard_layout_rx.next() => {},
                 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 } => {
+                let load_result = KeymapFile::load(&user_keymap_content, cx);
+                match load_result {
+                    KeymapFileLoadResult::Success { key_bindings } => {
+                        reload_keymaps(cx, key_bindings);
+                        dismiss_app_notification(&notification_id, cx);
+                    }
+                    KeymapFileLoadResult::SomeFailedToLoad {
+                        key_bindings,
+                        error_message,
+                    } => {
+                        if !key_bindings.is_empty() {
                             reload_keymaps(cx, key_bindings);
-                            dismiss_app_notification(&notification_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);
                         }
+                        show_keymap_file_load_error(notification_id.clone(), error_message, cx)
                     }
-                }
-                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,
-                        );
+                    KeymapFileLoadResult::JsonParseFailure { error } => {
+                        show_keymap_file_json_error(notification_id.clone(), &error, cx)
                     }
                 }
             })
@@ -1149,32 +1108,26 @@ pub fn handle_keymap_file_changes(
 }
 
 fn show_keymap_file_json_error(
-    new_workspace_window: Option<AnyWindowHandle>,
     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_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);
-                    })
-            })
-        },
-    );
+    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(
-    new_workspace_window: Option<AnyWindowHandle>,
     notification_id: NotificationId,
     markdown_error_message: MarkdownString,
     cx: &mut AppContext,
@@ -1193,57 +1146,34 @@ fn show_keymap_file_load_error(
     cx.spawn(move |cx| async move {
         let parsed_markdown = Rc::new(parsed_markdown.await);
         cx.update(|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| {
-                        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);
-                        })
+            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();
         })
         .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);