From 8cbdd9e0fa7b1fdc9453caefd353468d1df4c1b2 Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Wed, 10 Apr 2024 17:21:23 -0400 Subject: [PATCH] Refactor workspace notifications to use explicit `NotificationId` type (#10342) This PR reworks the way workspace notifications are identified to use a new `NotificationId` type. A `NotificationId` is bound to a given type that is used as a unique identifier. Generally this will be a unit struct that can be used to uniquely identify this notification. A `NotificationId` can also accept an optional `ElementId` in order to distinguish between different notifications of the same type. This system avoids the issue we had previously of selecting `usize` IDs somewhat arbitrarily and running the risk of having two independent notifications collide (and thus interfere with each other). This also fixes a bug where multiple suggestion notifications for the same extension could be live at once Fixes https://github.com/zed-industries/zed/issues/10320. Release Notes: - Fixed a bug where multiple extension suggestions for the same extension could be shown at once ([#10320](https://github.com/zed-industries/zed/issues/10320)). --------- Co-authored-by: Max --- crates/assistant/src/assistant_panel.rs | 13 ++- crates/auto_update/src/auto_update.rs | 9 +- crates/collab_ui/src/channel_view.rs | 11 +- crates/collab_ui/src/notification_panel.rs | 10 +- crates/copilot_ui/src/copilot_button.rs | 24 +++- crates/editor/src/editor.rs | 15 ++- crates/extensions_ui/src/extension_suggest.rs | 13 ++- crates/feedback/src/feedback_modal.rs | 5 +- crates/vcs_menu/src/lib.rs | 7 +- crates/workspace/src/notifications.rs | 109 +++++++++++------- crates/workspace/src/workspace.rs | 59 ++++++---- crates/zed/src/zed.rs | 35 ++++-- 12 files changed, 212 insertions(+), 98 deletions(-) diff --git a/crates/assistant/src/assistant_panel.rs b/crates/assistant/src/assistant_panel.rs index 041bd2f62d..1a9284725d 100644 --- a/crates/assistant/src/assistant_panel.rs +++ b/crates/assistant/src/assistant_panel.rs @@ -46,6 +46,7 @@ use ui::{ }; use util::{paths::CONVERSATIONS_DIR, post_inc, ResultExt, TryFutureExt}; use uuid::Uuid; +use workspace::notifications::NotificationId; use workspace::{ dock::{DockPosition, Panel, PanelEvent}, searchable::Direction, @@ -418,10 +419,14 @@ impl AssistantPanel { if pending_assist.inline_assistant.is_none() { if let Some(workspace) = this.workspace.upgrade() { workspace.update(cx, |workspace, cx| { - workspace.show_toast( - Toast::new(inline_assist_id, error), - cx, - ); + struct InlineAssistantError; + + let id = + NotificationId::identified::( + inline_assist_id, + ); + + workspace.show_toast(Toast::new(id, error), cx); }) } diff --git a/crates/auto_update/src/auto_update.rs b/crates/auto_update/src/auto_update.rs index 1715be8b6a..5725efe39a 100644 --- a/crates/auto_update/src/auto_update.rs +++ b/crates/auto_update/src/auto_update.rs @@ -32,6 +32,7 @@ use util::{ http::{HttpClient, HttpClientWithUrl}, ResultExt, }; +use workspace::notifications::NotificationId; use workspace::Workspace; const SHOULD_SHOW_UPDATE_NOTIFICATION_KEY: &str = "auto-updater-should-show-updated-notification"; @@ -262,9 +263,11 @@ pub fn notify_of_any_new_update(cx: &mut ViewContext) -> Option<()> { let should_show_notification = should_show_notification.await?; if should_show_notification { workspace.update(&mut cx, |workspace, cx| { - workspace.show_notification(0, cx, |cx| { - cx.new_view(|_| UpdateNotification::new(version)) - }); + workspace.show_notification( + NotificationId::unique::(), + cx, + |cx| cx.new_view(|_| UpdateNotification::new(version)), + ); updater .read(cx) .set_should_show_update_notification(false, cx) diff --git a/crates/collab_ui/src/channel_view.rs b/crates/collab_ui/src/channel_view.rs index ac0793715f..bae647b826 100644 --- a/crates/collab_ui/src/channel_view.rs +++ b/crates/collab_ui/src/channel_view.rs @@ -22,6 +22,7 @@ use std::{ }; use ui::{prelude::*, Label}; use util::ResultExt; +use workspace::notifications::NotificationId; use workspace::{ item::{FollowableItem, Item, ItemEvent, ItemHandle}, register_followable_item, @@ -269,7 +270,15 @@ impl ChannelView { cx.write_to_clipboard(ClipboardItem::new(link)); self.workspace .update(cx, |workspace, cx| { - workspace.show_toast(Toast::new(0, "Link copied to clipboard"), cx); + struct CopyLinkForPositionToast; + + workspace.show_toast( + Toast::new( + NotificationId::unique::(), + "Link copied to clipboard", + ), + cx, + ); }) .ok(); } diff --git a/crates/collab_ui/src/notification_panel.rs b/crates/collab_ui/src/notification_panel.rs index d5a591d845..c8e58b1e85 100644 --- a/crates/collab_ui/src/notification_panel.rs +++ b/crates/collab_ui/src/notification_panel.rs @@ -21,6 +21,7 @@ use std::{sync::Arc, time::Duration}; use time::{OffsetDateTime, UtcOffset}; use ui::{h_flex, prelude::*, v_flex, Avatar, Button, Icon, IconButton, IconName, Label, Tooltip}; use util::{ResultExt, TryFutureExt}; +use workspace::notifications::NotificationId; use workspace::{ dock::{DockPosition, Panel, PanelEvent}, Workspace, @@ -534,8 +535,10 @@ impl NotificationPanel { self.workspace .update(cx, |workspace, cx| { - workspace.dismiss_notification::(0, cx); - workspace.show_notification(0, cx, |cx| { + let id = NotificationId::unique::(); + + workspace.dismiss_notification(&id, cx); + workspace.show_notification(id, cx, |cx| { let workspace = cx.view().downgrade(); cx.new_view(|_| NotificationToast { notification_id, @@ -554,7 +557,8 @@ impl NotificationPanel { self.current_notification_toast.take(); self.workspace .update(cx, |workspace, cx| { - workspace.dismiss_notification::(0, cx) + let id = NotificationId::unique::(); + workspace.dismiss_notification(&id, cx) }) .ok(); } diff --git a/crates/copilot_ui/src/copilot_button.rs b/crates/copilot_ui/src/copilot_button.rs index 93e99dd919..b228a10839 100644 --- a/crates/copilot_ui/src/copilot_button.rs +++ b/crates/copilot_ui/src/copilot_button.rs @@ -14,6 +14,7 @@ use language::{ use settings::{update_settings_file, Settings, SettingsStore}; use std::{path::Path, sync::Arc}; use util::{paths, ResultExt}; +use workspace::notifications::NotificationId; use workspace::{ create_and_open_local_file, item::ItemHandle, @@ -25,8 +26,10 @@ use workspace::{ use zed_actions::OpenBrowser; const COPILOT_SETTINGS_URL: &str = "https://github.com/settings/copilot"; -const COPILOT_STARTING_TOAST_ID: usize = 1337; -const COPILOT_ERROR_TOAST_ID: usize = 1338; + +struct CopilotStartingToast; + +struct CopilotErrorToast; pub struct CopilotButton { editor_subscription: Option<(Subscription, usize)>, @@ -74,7 +77,7 @@ impl Render for CopilotButton { .update(cx, |workspace, cx| { workspace.show_toast( Toast::new( - COPILOT_ERROR_TOAST_ID, + NotificationId::unique::(), format!("Copilot can't be started: {}", e), ) .on_click( @@ -350,7 +353,10 @@ pub fn initiate_sign_in(cx: &mut WindowContext) { let Ok(workspace) = workspace.update(cx, |workspace, cx| { workspace.show_toast( - Toast::new(COPILOT_STARTING_TOAST_ID, "Copilot is starting..."), + Toast::new( + NotificationId::unique::(), + "Copilot is starting...", + ), cx, ); workspace.weak_handle() @@ -364,11 +370,17 @@ pub fn initiate_sign_in(cx: &mut WindowContext) { workspace .update(&mut cx, |workspace, cx| match copilot.read(cx).status() { Status::Authorized => workspace.show_toast( - Toast::new(COPILOT_STARTING_TOAST_ID, "Copilot has started!"), + Toast::new( + NotificationId::unique::(), + "Copilot has started!", + ), cx, ), _ => { - workspace.dismiss_toast(COPILOT_STARTING_TOAST_ID, cx); + workspace.dismiss_toast( + &NotificationId::unique::(), + cx, + ); copilot .update(cx, |copilot, cx| copilot.sign_in(cx)) .detach_and_log_err(cx); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 710e040e96..9879874147 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -129,6 +129,7 @@ use ui::{ Tooltip, }; use util::{defer, maybe, post_inc, RangeExt, ResultExt, TryFutureExt}; +use workspace::notifications::NotificationId; use workspace::Toast; use workspace::{ searchable::SearchEvent, ItemNavHistory, SplitDirection, ViewId, Workspace, WorkspaceId, @@ -8922,7 +8923,12 @@ impl Editor { if let Some(workspace) = self.workspace() { workspace.update(cx, |workspace, cx| { - workspace.show_toast(Toast::new(0x156a5f9ee, message), cx) + struct CopyPermalinkToLine; + + workspace.show_toast( + Toast::new(NotificationId::unique::(), message), + cx, + ) }) } } @@ -8943,7 +8949,12 @@ impl Editor { if let Some(workspace) = self.workspace() { workspace.update(cx, |workspace, cx| { - workspace.show_toast(Toast::new(0x45a8978, message), cx) + struct OpenPermalinkToLine; + + workspace.show_toast( + Toast::new(NotificationId::unique::(), message), + cx, + ) }) } } diff --git a/crates/extensions_ui/src/extension_suggest.rs b/crates/extensions_ui/src/extension_suggest.rs index 99dee33dd8..f2d4ab14e4 100644 --- a/crates/extensions_ui/src/extension_suggest.rs +++ b/crates/extensions_ui/src/extension_suggest.rs @@ -5,9 +5,10 @@ use std::sync::{Arc, OnceLock}; use db::kvp::KEY_VALUE_STORE; use editor::Editor; use extension::ExtensionStore; -use gpui::{Entity, Model, VisualContext}; +use gpui::{Model, VisualContext}; use language::Buffer; -use ui::ViewContext; +use ui::{SharedString, ViewContext}; +use workspace::notifications::NotificationId; use workspace::{notifications::simple_message_notification, Workspace}; fn suggested_extensions() -> &'static HashMap<&'static str, Arc> { @@ -140,7 +141,13 @@ pub(crate) fn suggest(buffer: Model, cx: &mut ViewContext) { return; } - workspace.show_notification(buffer.entity_id().as_u64() as usize, cx, |cx| { + struct ExtensionSuggestionNotification; + + let notification_id = NotificationId::identified::( + SharedString::from(extension_id.clone()), + ); + + workspace.show_notification(notification_id, cx, |cx| { cx.new_view(move |_cx| { simple_message_notification::MessageNotification::new(format!( "Do you want to install the recommended '{}' extension for '{}' files?", diff --git a/crates/feedback/src/feedback_modal.rs b/crates/feedback/src/feedback_modal.rs index 04fa874012..bb109ef6ef 100644 --- a/crates/feedback/src/feedback_modal.rs +++ b/crates/feedback/src/feedback_modal.rs @@ -17,6 +17,7 @@ use regex::Regex; use serde_derive::Serialize; use ui::{prelude::*, Button, ButtonStyle, IconPosition, Tooltip}; use util::{http::HttpClient, ResultExt}; +use workspace::notifications::NotificationId; use workspace::{DismissDecision, ModalView, Toast, Workspace}; use crate::{system_specs::SystemSpecs, GiveFeedback, OpenZedRepo}; @@ -127,11 +128,11 @@ impl FeedbackModal { let is_local_project = project.read(cx).is_local(); if !is_local_project { - const TOAST_ID: usize = 0xdeadbeef; + struct FeedbackInRemoteProject; workspace.show_toast( Toast::new( - TOAST_ID, + NotificationId::unique::(), "You can only submit feedback in your own project.", ), cx, diff --git a/crates/vcs_menu/src/lib.rs b/crates/vcs_menu/src/lib.rs index 2f88328084..affd27de58 100644 --- a/crates/vcs_menu/src/lib.rs +++ b/crates/vcs_menu/src/lib.rs @@ -13,6 +13,7 @@ use ui::{ LabelSize, ListItem, ListItemSpacing, Selectable, }; use util::ResultExt; +use workspace::notifications::NotificationId; use workspace::{ModalView, Toast, Workspace}; actions!(branches, [OpenRecent]); @@ -125,9 +126,11 @@ impl BranchListDelegate { } fn display_error_toast(&self, message: String, cx: &mut WindowContext<'_>) { - const GIT_CHECKOUT_FAILURE_ID: usize = 2048; self.workspace.update(cx, |model, ctx| { - model.show_toast(Toast::new(GIT_CHECKOUT_FAILURE_ID, message), ctx) + struct GitCheckoutFailure; + let id = NotificationId::unique::(); + + model.show_toast(Toast::new(id, message), ctx) }); } } diff --git a/crates/workspace/src/notifications.rs b/crates/workspace/src/notifications.rs index 9576845399..c7ea762e15 100644 --- a/crates/workspace/src/notifications.rs +++ b/crates/workspace/src/notifications.rs @@ -15,6 +15,34 @@ pub fn init(cx: &mut AppContext) { cx.set_global(NotificationTracker::new()); } +#[derive(Debug, PartialEq, Clone)] +pub struct NotificationId { + /// A [`TypeId`] used to uniquely identify this notification. + type_id: TypeId, + /// A supplementary ID used to distinguish between multiple + /// notifications that have the same [`type_id`](Self::type_id); + id: Option, +} + +impl NotificationId { + /// Returns a unique [`NotificationId`] for the given type. + pub fn unique() -> Self { + Self { + type_id: TypeId::of::(), + id: None, + } + } + + /// Returns a [`NotificationId`] for the given type that is also identified + /// by the provided ID. + pub fn identified(id: impl Into) -> Self { + Self { + type_id: TypeId::of::(), + id: Some(id.into()), + } + } +} + pub trait Notification: EventEmitter + Render {} impl + Render> Notification for V {} @@ -41,13 +69,13 @@ impl From<&dyn NotificationHandle> for AnyView { } pub(crate) struct NotificationTracker { - notifications_sent: HashMap>, + notifications_sent: HashMap>, } impl Global for NotificationTracker {} impl std::ops::Deref for NotificationTracker { - type Target = HashMap>; + type Target = HashMap>; fn deref(&self) -> &Self::Target { &self.notifications_sent @@ -71,45 +99,46 @@ impl NotificationTracker { impl Workspace { pub fn has_shown_notification_once( &self, - id: usize, + id: &NotificationId, cx: &ViewContext, ) -> bool { cx.global::() .get(&TypeId::of::()) - .map(|ids| ids.contains(&id)) + .map(|ids| ids.contains(id)) .unwrap_or(false) } pub fn show_notification_once( &mut self, - id: usize, + id: NotificationId, cx: &mut ViewContext, build_notification: impl FnOnce(&mut ViewContext) -> View, ) { - if !self.has_shown_notification_once::(id, cx) { + if !self.has_shown_notification_once::(&id, cx) { let tracker = cx.global_mut::(); let entry = tracker.entry(TypeId::of::()).or_default(); - entry.push(id); + entry.push(id.clone()); self.show_notification::(id, cx, build_notification) } } pub fn show_notification( &mut self, - id: usize, + id: NotificationId, cx: &mut ViewContext, build_notification: impl FnOnce(&mut ViewContext) -> View, ) { - let type_id = TypeId::of::(); - self.dismiss_notification_internal(type_id, id, cx); + self.dismiss_notification_internal(&id, cx); let notification = build_notification(cx); - cx.subscribe(¬ification, move |this, _, _: &DismissEvent, cx| { - this.dismiss_notification_internal(type_id, id, cx); + cx.subscribe(¬ification, { + let id = id.clone(); + move |this, _, _: &DismissEvent, cx| { + this.dismiss_notification_internal(&id, cx); + } }) .detach(); - self.notifications - .push((type_id, id, Box::new(notification))); + self.notifications.push((id, Box::new(notification))); cx.notify(); } @@ -117,21 +146,25 @@ impl Workspace { where E: std::fmt::Debug, { - self.show_notification(0, cx, |cx| { - cx.new_view(|_cx| { - simple_message_notification::MessageNotification::new(format!("Error: {err:?}")) - }) - }); + struct WorkspaceErrorNotification; + + self.show_notification( + NotificationId::unique::(), + cx, + |cx| { + cx.new_view(|_cx| { + simple_message_notification::MessageNotification::new(format!("Error: {err:?}")) + }) + }, + ); } - pub fn dismiss_notification(&mut self, id: usize, cx: &mut ViewContext) { - let type_id = TypeId::of::(); - - self.dismiss_notification_internal(type_id, id, cx) + pub fn dismiss_notification(&mut self, id: &NotificationId, cx: &mut ViewContext) { + self.dismiss_notification_internal(id, cx) } pub fn show_toast(&mut self, toast: Toast, cx: &mut ViewContext) { - self.dismiss_notification::(toast.id, cx); + self.dismiss_notification(&toast.id, cx); self.show_notification(toast.id, cx, |cx| { cx.new_view(|_cx| match toast.on_click.as_ref() { Some((click_msg, on_click)) => { @@ -145,25 +178,19 @@ impl Workspace { }) } - pub fn dismiss_toast(&mut self, id: usize, cx: &mut ViewContext) { - self.dismiss_notification::(id, cx); + pub fn dismiss_toast(&mut self, id: &NotificationId, cx: &mut ViewContext) { + self.dismiss_notification(id, cx); } - fn dismiss_notification_internal( - &mut self, - type_id: TypeId, - id: usize, - cx: &mut ViewContext, - ) { - self.notifications - .retain(|(existing_type_id, existing_id, _)| { - if (*existing_type_id, *existing_id) == (type_id, id) { - cx.notify(); - false - } else { - true - } - }); + fn dismiss_notification_internal(&mut self, id: &NotificationId, cx: &mut ViewContext) { + self.notifications.retain(|(existing_id, _)| { + if existing_id == id { + cx.notify(); + false + } else { + true + } + }); } } diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index f843be9af2..5169b4cd88 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -83,6 +83,7 @@ use util::ResultExt; use uuid::Uuid; pub use workspace_settings::{AutosaveSetting, TabBarSettings, WorkspaceSettings}; +use crate::notifications::NotificationId; use crate::persistence::{ model::{DockData, DockStructure, SerializedItem, SerializedPane, SerializedPaneGroup}, SerializedAxis, @@ -191,16 +192,14 @@ impl_actions!( ] ); -#[derive(Deserialize)] pub struct Toast { - id: usize, + id: NotificationId, msg: Cow<'static, str>, - #[serde(skip)] on_click: Option<(Cow<'static, str>, Arc)>, } impl Toast { - pub fn new>>(id: usize, msg: I) -> Self { + pub fn new>>(id: NotificationId, msg: I) -> Self { Toast { id, msg: msg.into(), @@ -229,7 +228,7 @@ impl PartialEq for Toast { impl Clone for Toast { fn clone(&self) -> Self { Toast { - id: self.id, + id: self.id.clone(), msg: self.msg.clone(), on_click: self.on_click.clone(), } @@ -555,7 +554,7 @@ pub struct Workspace { status_bar: View, modal_layer: View, titlebar_item: Option, - notifications: Vec<(TypeId, usize, Box)>, + notifications: Vec<(NotificationId, Box)>, project: Model, follower_states: HashMap, FollowerState>, last_leaders_by_pane: HashMap, PeerId>, @@ -634,18 +633,32 @@ impl Workspace { } } - project::Event::Notification(message) => this.show_notification(0, cx, |cx| { - cx.new_view(|_| MessageNotification::new(message.clone())) - }), + project::Event::Notification(message) => { + struct ProjectNotification; + + this.show_notification( + NotificationId::unique::(), + cx, + |cx| cx.new_view(|_| MessageNotification::new(message.clone())), + ) + } project::Event::LanguageServerPrompt(request) => { + struct LanguageServerPrompt; + let mut hasher = DefaultHasher::new(); request.lsp_name.as_str().hash(&mut hasher); let id = hasher.finish(); - this.show_notification(id as usize, cx, |cx| { - cx.new_view(|_| notifications::LanguageServerPrompt::new(request.clone())) - }); + this.show_notification( + NotificationId::identified::(id as usize), + cx, + |cx| { + cx.new_view(|_| { + notifications::LanguageServerPrompt::new(request.clone()) + }) + }, + ); } _ => {} @@ -2834,7 +2847,7 @@ impl Workspace { .children( self.notifications .iter() - .map(|(_, _, notification)| notification.to_any()), + .map(|(_, notification)| notification.to_any()), ), ) } @@ -3831,13 +3844,19 @@ fn notify_if_database_failed(workspace: WindowHandle, cx: &mut AsyncA workspace .update(cx, |workspace, cx| { if (*db::ALL_FILE_DB_FAILED).load(std::sync::atomic::Ordering::Acquire) { - workspace.show_notification_once(0, cx, |cx| { - cx.new_view(|_| { - MessageNotification::new("Failed to load the database file.") - .with_click_message("Click to let us know about this error") - .on_click(|cx| cx.open_url(REPORT_ISSUE_URL)) - }) - }); + struct DatabaseFailedNotification; + + workspace.show_notification_once( + NotificationId::unique::(), + cx, + |cx| { + cx.new_view(|_| { + MessageNotification::new("Failed to load the database file.") + .with_click_message("Click to let us know about this error") + .on_click(|cx| cx.open_url(REPORT_ISSUE_URL)) + }) + }, + ); } }) .log_err(); diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 5824cb27d7..d120b11cc3 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -34,6 +34,7 @@ use task::{ static_source::{StaticSource, TrackedFile}, }; use theme::ActiveTheme; +use workspace::notifications::NotificationId; use terminal_view::terminal_panel::{self, TerminalPanel}; use util::{ @@ -253,9 +254,11 @@ pub fn initialize_workspace(app_state: Arc, cx: &mut AppContext) { .await .context("error creating CLI symlink")?; workspace.update(&mut cx, |workspace, cx| { + struct InstalledZedCli; + workspace.show_toast( Toast::new( - 0, + NotificationId::unique::(), format!( "Installed `zed` to {}. You can launch {} from your terminal.", path.to_string_lossy(), @@ -274,9 +277,11 @@ pub fn initialize_workspace(app_state: Arc, cx: &mut AppContext) { cx.spawn(|workspace, mut cx| async move { register_zed_scheme(&cx).await?; workspace.update(&mut cx, |workspace, cx| { + struct RegisterZedScheme; + workspace.show_toast( Toast::new( - 0, + NotificationId::unique::(), format!( "zed:// links will now open in {}.", ReleaseChannel::global(cx).display_name() @@ -555,14 +560,20 @@ fn open_log_file(workspace: &mut Workspace, cx: &mut ViewContext) { workspace .update(&mut cx, |workspace, cx| { let Some(log) = log else { - workspace.show_notification(29, cx, |cx| { - cx.new_view(|_| { - MessageNotification::new(format!( - "Unable to access/open log file at path {:?}", - paths::LOG.as_path() - )) - }) - }); + struct OpenLogError; + + workspace.show_notification( + NotificationId::unique::(), + cx, + |cx| { + cx.new_view(|_| { + MessageNotification::new(format!( + "Unable to access/open log file at path {:?}", + paths::LOG.as_path() + )) + }) + }, + ); return; }; let project = workspace.project().clone(); @@ -749,7 +760,9 @@ fn open_local_file( }) .detach(); } else { - workspace.show_notification(0, cx, |cx| { + struct NoOpenFolders; + + workspace.show_notification(NotificationId::unique::(), cx, |cx| { cx.new_view(|_| MessageNotification::new("This project has no folders open.")) }) }