Show settings file errors on startup (#23817)

Required using a global `LazyLock<Mutex<AppNotifications>>` instead of a context global because settings errors first occur before initialization of the notifications global. Release Notes: - Errors in settings file are now reported in UI on startup.
2025-01-29 00:05:33 -07:00 · 2025-01-29 00:05:33 -07:00 · dbdf140ca1
commit dbdf140ca1
parent 06936c69f6
5 changed files with 74 additions and 89 deletions
--- a/crates/settings/src/settings_file.rs
+++ b/crates/settings/src/settings_file.rs
@ -3,7 +3,6 @@ use fs::Fs;
 use futures::{channel::mpsc, StreamExt};
 use gpui::{App, BackgroundExecutor, ReadGlobal, UpdateGlobal};
 use std::{path::PathBuf, sync::Arc, time::Duration};
-use util::ResultExt;

 pub const EMPTY_THEME_NAME: &str = "empty-theme";

@ -73,9 +72,11 @@ pub fn handle_settings_file_changes(
        .block(user_settings_file_rx.next())
        .unwrap();
    SettingsStore::update_global(cx, |store, cx| {
-        store
-            .set_user_settings(&user_settings_content, cx)
-            .log_err();
+        let result = store.set_user_settings(&user_settings_content, cx);
+        if let Err(err) = &result {
+            log::error!("Failed to load user settings: {err}");
+        }
+        settings_changed(result.err(), cx);
    });
    cx.spawn(move |cx| async move {
        while let Some(user_settings_content) = user_settings_file_rx.next().await {
--- a/crates/workspace/src/notifications.rs
+++ b/crates/workspace/src/notifications.rs
@ -3,17 +3,12 @@ use gpui::{
    svg, AnyView, App, AppContext as _, AsyncWindowContext, ClipboardItem, Context, DismissEvent,
    Entity, EventEmitter, Global, PromptLevel, Render, ScrollHandle, Task,
 };
-use std::rc::Rc;
+use parking_lot::Mutex;
+use std::sync::{Arc, LazyLock};
 use std::{any::TypeId, time::Duration};
 use ui::{prelude::*, Tooltip};
 use util::ResultExt;

-pub fn init(cx: &mut App) {
-    cx.set_global(GlobalAppNotifications {
-        app_notifications: Vec::new(),
-    })
-}
-
 #[derive(Debug, PartialEq, Clone)]
 pub enum NotificationId {
    Unique(TypeId),
@ -162,7 +157,7 @@ impl Workspace {
    pub fn show_initial_notifications(&mut self, cx: &mut Context<Self>) {
        // Allow absence of the global so that tests don't need to initialize it.
        let app_notifications = cx
-            .try_global::<GlobalAppNotifications>()
+            .try_global::<AppNotifications>()
            .iter()
            .flat_map(|global| global.app_notifications.iter().cloned())
            .collect::<Vec<_>>();
@ -500,21 +495,27 @@ pub mod simple_message_notification {
    }
 }

+static GLOBAL_APP_NOTIFICATIONS: LazyLock<Mutex<AppNotifications>> = LazyLock::new(|| {
+    Mutex::new(AppNotifications {
+        app_notifications: Vec::new(),
+    })
+});
+
 /// Stores app notifications so that they can be shown in new workspaces.
-struct GlobalAppNotifications {
+struct AppNotifications {
    app_notifications: Vec<(
        NotificationId,
-        Rc<dyn Fn(&mut Context<Workspace>) -> AnyView>,
+        Arc<dyn Fn(&mut Context<Workspace>) -> AnyView + Send + Sync>,
    )>,
 }

-impl Global for GlobalAppNotifications {}
+impl Global for AppNotifications {}

-impl GlobalAppNotifications {
+impl AppNotifications {
    pub fn insert(
        &mut self,
        id: NotificationId,
-        build_notification: Rc<dyn Fn(&mut Context<Workspace>) -> AnyView>,
+        build_notification: Arc<dyn Fn(&mut Context<Workspace>) -> AnyView + Send + Sync>,
    ) {
        self.remove(&id);
        self.app_notifications.push((id, build_notification))
@ -532,28 +533,30 @@ impl GlobalAppNotifications {
 pub fn show_app_notification<V: Notification + 'static>(
    id: NotificationId,
    cx: &mut App,
-    build_notification: impl Fn(&mut Context<Workspace>) -> Entity<V> + 'static,
+    build_notification: impl Fn(&mut Context<Workspace>) -> Entity<V> + 'static + Send + Sync,
 ) {
    // Defer notification creation so that windows on the stack can be returned to GPUI
    cx.defer(move |cx| {
        // Handle dismiss events by removing the notification from all workspaces.
-        let build_notification: Rc<dyn Fn(&mut Context<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()
-            }
-        });
+        let build_notification: Arc<dyn Fn(&mut Context<Workspace>) -> AnyView + Send + Sync> =
+            Arc::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()
+                }
+            });

        // Store the notification so that new workspaces also receive it.
-        cx.global_mut::<GlobalAppNotifications>()
+        GLOBAL_APP_NOTIFICATIONS
+            .lock()
            .insert(id.clone(), build_notification.clone());

        for window in cx.windows() {
@ -576,7 +579,7 @@ pub fn dismiss_app_notification(id: &NotificationId, cx: &mut App) {
    let id = id.clone();
    // Defer notification dismissal so that windows on the stack can be returned to GPUI
    cx.defer(move |cx| {
-        cx.global_mut::<GlobalAppNotifications>().remove(&id);
+        GLOBAL_APP_NOTIFICATIONS.lock().remove(&id);
        for window in cx.windows() {
            if let Some(workspace_window) = window.downcast::<Workspace>() {
                let id = id.clone();
--- a/crates/workspace/src/workspace.rs
+++ b/crates/workspace/src/workspace.rs
@ -365,7 +365,6 @@ fn prompt_and_open_paths(app_state: Arc<AppState>, options: PathPromptOptions, c

 pub fn init(app_state: Arc<AppState>, cx: &mut App) {
    init_settings(cx);
-    notifications::init(cx);
    theme_preview::init(cx);

    cx.on_action(Workspace::close_global);
--- a/crates/zed/src/main.rs
+++ b/crates/zed/src/main.rs
@ -18,7 +18,7 @@ use extension::ExtensionHostProxy;
 use fs::{Fs, RealFs};
 use futures::{future, StreamExt};
 use git::GitHostingProviderRegistry;
-use gpui::{Action, App, AppContext as _, Application, AsyncApp, DismissEvent, UpdateGlobal as _};
+use gpui::{App, AppContext as _, Application, AsyncApp, UpdateGlobal as _};

 use http_client::{read_proxy_from_env, Uri};
 use language::LanguageRegistry;
@ -33,9 +33,7 @@ use project::project_settings::ProjectSettings;
 use recent_projects::{open_ssh_project, SshSettings};
 use release_channel::{AppCommitSha, AppVersion, ReleaseChannel};
 use session::{AppSession, Session};
-use settings::{
-    handle_settings_file_changes, watch_config_file, InvalidSettingsError, Settings, SettingsStore,
-};
+use settings::{handle_settings_file_changes, watch_config_file, Settings, SettingsStore};
 use simplelog::ConfigBuilder;
 use std::{
    env,
@ -50,18 +48,13 @@ use time::UtcOffset;
 use util::{maybe, ResultExt, TryFutureExt};
 use uuid::Uuid;
 use welcome::{show_welcome_view, BaseKeymap, FIRST_OPEN};
-use workspace::{
-    notifications::{simple_message_notification::MessageNotification, NotificationId},
-    AppState, SerializedWorkspaceLocation, WorkspaceSettings, WorkspaceStore,
-};
+use workspace::{AppState, SerializedWorkspaceLocation, WorkspaceSettings, WorkspaceStore};
 use zed::{
    app_menus, build_window_options, derive_paths_with_position, handle_cli_connection,
-    handle_keymap_file_changes, initialize_workspace, open_paths_with_positions, OpenListener,
-    OpenRequest,
+    handle_keymap_file_changes, handle_settings_changed, initialize_workspace,
+    inline_completion_registry, open_paths_with_positions, OpenListener, OpenRequest,
 };

-use crate::zed::inline_completion_registry;
-
 #[cfg(unix)]
 use util::{load_login_shell_environment, load_shell_from_passwd};

@ -614,44 +607,6 @@ fn main() {
    });
 }

-fn handle_settings_changed(error: Option<anyhow::Error>, cx: &mut App) {
-    struct SettingsParseErrorNotification;
-    let id = NotificationId::unique::<SettingsParseErrorNotification>();
-
-    for workspace in workspace::local_workspace_windows(cx) {
-        workspace
-            .update(cx, |workspace, _, cx| {
-                match error.as_ref() {
-                    Some(error) => {
-                        if let Some(InvalidSettingsError::LocalSettings { .. }) =
-                            error.downcast_ref::<InvalidSettingsError>()
-                        {
-                            // Local settings will be displayed by the projects
-                        } else {
-                            workspace.show_notification(id.clone(), cx, |cx| {
-                                cx.new(|_cx| {
-                                    MessageNotification::new(format!(
-                                        "Invalid user settings file\n{error}"
-                                    ))
-                                    .with_click_message("Open settings file")
-                                    .on_click(|window, cx| {
-                                        window.dispatch_action(
-                                            zed_actions::OpenSettings.boxed_clone(),
-                                            cx,
-                                        );
-                                        cx.emit(DismissEvent);
-                                    })
-                                })
-                            });
-                        }
-                    }
-                    None => workspace.dismiss_notification(&id, cx),
-                }
-            })
-            .log_err();
-    }
-}
-
 fn handle_open_request(request: OpenRequest, app_state: Arc<AppState>, cx: &mut App) {
    if let Some(connection) = request.cli_connection {
        let app_state = app_state.clone();
--- a/crates/zed/src/zed.rs
+++ b/crates/zed/src/zed.rs
@ -39,12 +39,12 @@ use release_channel::{AppCommitSha, ReleaseChannel};
 use rope::Rope;
 use search::project_search::ProjectSearchBar;
 use settings::{
-    initial_project_settings_content, initial_tasks_content, update_settings_file, KeymapFile,
-    KeymapFileLoadResult, Settings, SettingsStore, DEFAULT_KEYMAP_PATH, VIM_KEYMAP_PATH,
+    initial_project_settings_content, initial_tasks_content, update_settings_file,
+    InvalidSettingsError, KeymapFile, 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};
@ -1220,7 +1220,7 @@ fn show_keymap_file_load_error(
    });

    cx.spawn(move |cx| async move {
-        let parsed_markdown = Rc::new(parsed_markdown.await);
+        let parsed_markdown = Arc::new(parsed_markdown.await);
        cx.update(|cx| {
            show_app_notification(notification_id, cx, move |cx| {
                let workspace_handle = cx.entity().downgrade();
@ -1274,6 +1274,33 @@ pub fn load_default_keymap(cx: &mut App) {
    }
 }

+pub fn handle_settings_changed(error: Option<anyhow::Error>, cx: &mut App) {
+    struct SettingsParseErrorNotification;
+    let id = NotificationId::unique::<SettingsParseErrorNotification>();
+
+    match error {
+        Some(error) => {
+            if let Some(InvalidSettingsError::LocalSettings { .. }) =
+                error.downcast_ref::<InvalidSettingsError>()
+            {
+                // Local settings errors are displayed by the projects
+                return;
+            }
+            show_app_notification(id, cx, move |cx| {
+                cx.new(|_cx| {
+                    MessageNotification::new(format!("Invalid user settings file\n{error}"))
+                        .with_click_message("Open settings file")
+                        .on_click(|window, cx| {
+                            window.dispatch_action(zed_actions::OpenSettings.boxed_clone(), cx);
+                            cx.emit(DismissEvent);
+                        })
+                })
+            });
+        }
+        None => dismiss_app_notification(&id, cx),
+    }
+}
+
 pub fn open_new_ssh_project_from_project(
    workspace: &mut Workspace,
    paths: Vec<PathBuf>,