From ecadca70d80fcf09f1bd9f5ab9ec71e03b3d22f4 Mon Sep 17 00:00:00 2001 From: LynithDev <61880709+LynithDev@users.noreply.github.com> Date: Mon, 3 Aug 2026 13:45:31 +0200 Subject: [PATCH] feat(notifications): only file notifications worth reviewing --- .../src/components/notifications.rs | 13 +- packages/oneclient_app/src/events.rs | 1 + packages/oneclient_app/src/hooks/actions.rs | 41 +- packages/oneclient_app/src/install.rs | 8 +- .../oneclient_app/src/layout/app_shell.rs | 3 + .../src/layout/settings_shell.rs | 1 + packages/oneclient_app/src/notifications.rs | 363 +++++++++++++++--- packages/oneclient_app/src/updater.rs | 9 +- .../src/view/app/cluster/logs.rs | 11 +- packages/oneclient_app/src/view/app/debug.rs | 4 + .../src/bundles/overrides.rs | 3 + packages/oneclient_core/src/dev.rs | 6 +- packages/oneclient_core/src/game/launch.rs | 4 + packages/oneclient_events/src/bus.rs | 86 ++++- packages/oneclient_events/src/event.rs | 49 +++ packages/oneclient_events/src/lib.rs | 2 +- 16 files changed, 535 insertions(+), 69 deletions(-) diff --git a/packages/oneclient_app/src/components/notifications.rs b/packages/oneclient_app/src/components/notifications.rs index f1a5fb35..3776550c 100644 --- a/packages/oneclient_app/src/components/notifications.rs +++ b/packages/oneclient_app/src/components/notifications.rs @@ -8,7 +8,7 @@ use crate::{ ui::{divider, relative_time}, components::{Button, ButtonVariant, Icon, IconType, OverlayPopup, ScrollArea}, hooks::{use_dispatch, use_notifications_snapshot}, - notifications::{InboxEntry, NotificationActionKind}, + notifications::{InboxEntry, NotificationActionKind, NotificationState}, theme::colors, transfer::TransferStats, utils::{format_duration_hms, format_size}, @@ -39,7 +39,13 @@ struct NotificationPanel; impl Component for NotificationPanel { fn render(&self) -> impl IntoElement { - let inbox = use_notifications_snapshot().inbox; + // Transient notifications ride the same inbox so their toast can find + // them, but they are not what this panel is for. + let inbox: Vec = NotificationState::center_entries( + &use_notifications_snapshot().inbox, + ) + .cloned() + .collect(); let intro = use_animation(|conf| { conf.on_creation(OnCreation::Run); @@ -491,7 +497,8 @@ struct Footer; impl Component for Footer { fn render(&self) -> impl IntoElement { let dispatch = use_dispatch(); - let is_empty = use_notifications_snapshot().inbox.is_empty(); + let is_empty = + NotificationState::center_entries(&use_notifications_snapshot().inbox).count() == 0; rect() .horizontal() diff --git a/packages/oneclient_app/src/events.rs b/packages/oneclient_app/src/events.rs index 694cc82c..de6f7012 100644 --- a/packages/oneclient_app/src/events.rs +++ b/packages/oneclient_app/src/events.rs @@ -384,6 +384,7 @@ pub fn report_startup_failure( title: "Launcher failed to start".into(), body: message, level: oneclient_events::Level::Error, + persistence: oneclient_events::Persistence::Persistent, }, )), ); diff --git a/packages/oneclient_app/src/hooks/actions.rs b/packages/oneclient_app/src/hooks/actions.rs index 4bf25c2e..bee81e53 100644 --- a/packages/oneclient_app/src/hooks/actions.rs +++ b/packages/oneclient_app/src/hooks/actions.rs @@ -23,7 +23,7 @@ use oneclient_common::domain::{ContentType, ProviderId}; use oneclient_core::settings::LauncherSettings; use oneclient_core::settings::store::{save_global_profile, save_settings_and_apply}; use oneclient_db::models::ClusterId; -use oneclient_events::{Answer, Level}; +use oneclient_events::{Answer, Level, Persistence}; use tokio::sync::mpsc; use crate::components::IconType; @@ -613,7 +613,9 @@ impl Actions { icon: None, progress: None, actions: Vec::new(), + persistence: Persistence::Transient, }, + persistence: None, } } @@ -775,6 +777,9 @@ impl Actions { icon: Some(IconType::Download01), progress: None, actions: Vec::new(), + // The package is in the list the user is looking at; the + // list is the record, not the notification. + persistence: Persistence::Transient, }, Err(err) => NotificationSpec { title: "Install failed".to_string(), @@ -783,6 +788,7 @@ impl Actions { icon: None, progress: None, actions: Vec::new(), + persistence: Persistence::Persistent, }, }; @@ -1179,6 +1185,9 @@ impl Actions { icon: Some(IconType::DownloadCloud02), progress: None, actions: Vec::new(), + // Applied automatically on the way into the game, so the user is + // told what changed at the worst possible moment to read it. + persistence: Persistence::Persistent, }); self.with_engine(|app| { @@ -1243,6 +1252,8 @@ impl Actions { icon: Some(IconType::DownloadCloud02), progress: None, actions: Vec::new(), + // The user pressed Update on this row and watched it go. + persistence: Persistence::Transient, }, Err(err) => NotificationSpec { title: "Update failed".to_string(), @@ -1251,6 +1262,7 @@ impl Actions { icon: None, progress: None, actions: Vec::new(), + persistence: Persistence::Persistent, }, }; @@ -1386,6 +1398,10 @@ async fn repair_and_relaunch( events .notify("Repair complete") .body(report.summary()) + // The launcher repaired the install on its own initiative, between the + // user pressing Play and the game appearing; the summary is the only + // account of what it changed. + .persistent() .send(); if let Err(err) = oneclient_core::launch_cluster(state, cluster_id, account, true).await { @@ -1394,10 +1410,16 @@ async fn repair_and_relaunch( } } +/// The front-end twin of [`oneclient_events::NotificationBuilder`], for +/// notifications the UI raises itself. Same persistence rules, so a call site +/// reads identically whichever side of the bus it lives on. #[must_use = "the notification is not raised until `.send()` is called"] pub struct NotificationBuilder { actions: Actions, spec: NotificationSpec, + /// Explicit choice; resolved against the level in `send`, so the order the + /// builder is called in cannot change where the notification lands. + persistence: Option, } impl NotificationBuilder { @@ -1419,6 +1441,18 @@ impl NotificationBuilder { self.level(Level::Error) } + /// Files this notification in the notification center. + pub fn persistent(mut self) -> Self { + self.persistence = Some(Persistence::Persistent); + self + } + + /// Shows this notification and forgets it. + pub fn transient(mut self) -> Self { + self.persistence = Some(Persistence::Transient); + self + } + pub fn icon(mut self, icon: IconType) -> Self { self.spec.icon = Some(icon); self @@ -1439,7 +1473,10 @@ impl NotificationBuilder { self } - pub fn send(self) { + pub fn send(mut self) { + self.spec.persistence = self + .persistence + .unwrap_or_else(|| Persistence::for_level(self.spec.level)); self.actions.push_notification(self.spec); } } diff --git a/packages/oneclient_app/src/install.rs b/packages/oneclient_app/src/install.rs index 742f9009..f6ba55e1 100644 --- a/packages/oneclient_app/src/install.rs +++ b/packages/oneclient_app/src/install.rs @@ -8,7 +8,7 @@ use std::sync::Arc; use oneclient_core::LauncherState; use oneclient_content::packages::PackageStore; -use oneclient_events::Level; +use oneclient_events::{Level, Persistence}; use crate::components::IconType; use crate::notifications::{ @@ -137,6 +137,9 @@ pub async fn cluster_update_notification( label: "View changes".to_string(), kind: NotificationActionKind::OpenClusterUpdate(vec![summary]), }], + // Packages moved under the user without them asking, and "View changes" + // is only worth offering for as long as it is still reachable. + persistence: Persistence::Persistent, }) } @@ -179,6 +182,9 @@ pub async fn combined_cluster_update_spec( label: "View changes".to_string(), kind: NotificationActionKind::OpenClusterUpdate(summaries), }], + // Same as the single-cluster case: a background sync the user did not + // ask for, with changes they may want to look at afterwards. + persistence: Persistence::Persistent, }) } diff --git a/packages/oneclient_app/src/layout/app_shell.rs b/packages/oneclient_app/src/layout/app_shell.rs index de469032..3dd1bc20 100644 --- a/packages/oneclient_app/src/layout/app_shell.rs +++ b/packages/oneclient_app/src/layout/app_shell.rs @@ -217,6 +217,9 @@ fn copy_error_button(message: &str, dispatch: crate::Actions) -> impl IntoElemen .notify("Copy failed") .body("Could not copy the error to the clipboard.") .error() + // An error, but one the user answers by pressing Copy + // again; nothing survives it that is worth reviewing. + .transient() .send(); } else { dispatch diff --git a/packages/oneclient_app/src/layout/settings_shell.rs b/packages/oneclient_app/src/layout/settings_shell.rs index 8d22a856..d543e243 100644 --- a/packages/oneclient_app/src/layout/settings_shell.rs +++ b/packages/oneclient_app/src/layout/settings_shell.rs @@ -720,6 +720,7 @@ impl Component for SidebarInfo { .notify("Copy failed") .body("Could not copy system information to the clipboard.") .error() + .transient() .send(); } else { dispatch diff --git a/packages/oneclient_app/src/notifications.rs b/packages/oneclient_app/src/notifications.rs index 3d1346a6..d435cd5f 100644 --- a/packages/oneclient_app/src/notifications.rs +++ b/packages/oneclient_app/src/notifications.rs @@ -2,7 +2,8 @@ use std::collections::HashMap; use std::time::{Duration, Instant}; use oneclient_events::{ - Answer, Choice, Event, GroupedProgressEvent, Level, Notification, ProgressEvent, TaskCategory, + Answer, Choice, Event, GroupedProgressEvent, Level, Notification, Persistence, ProgressEvent, + TaskCategory, }; use oneclient_content::packages::ProviderId; use oneclient_core::BrowserPackageUpdate; @@ -76,6 +77,11 @@ impl ClusterUpdateSummary { } } +/// One notification, as the engine holds it. +/// +/// Both surfaces read these: the toast stack shows the ones with a live toast, +/// the notification center shows the ones worth coming back to. Which is which +/// is [`InboxEntry::persistence`] — see [`InboxEntry::in_center`]. #[derive(Clone, Debug, PartialEq)] pub struct InboxEntry { pub level: Level, @@ -91,6 +97,9 @@ pub struct InboxEntry { pub tasks: Vec, /// Live transfer stats for grouped downloads (bytes/sec, seconds remaining). pub transfer: Option, + /// Whether this outlives its toast. Transient entries are dropped the + /// moment their toast goes, so the center only ever holds what matters. + pub persistence: Persistence, } /// One row in the expandable task list, an aggregate over all children of a @@ -113,6 +122,28 @@ pub struct NotificationSpec { pub icon: Option, pub progress: Option<(u64, u64)>, pub actions: Vec, + pub persistence: Persistence, +} + +impl NotificationSpec { + /// A bare title and body: no icon, no progress, no actions. The shape the + /// engine builds internally, which callers then fill in around. + pub fn plain( + title: impl Into, + body: impl Into, + level: Level, + persistence: Persistence, + ) -> Self { + Self { + title: title.into(), + body: body.into(), + level, + icon: None, + progress: None, + actions: Vec::new(), + persistence, + } + } } impl InboxEntry { @@ -123,6 +154,16 @@ impl InboxEntry { pub fn click_dismissable(&self) -> bool { self.dismissable() && self.actions.is_empty() } + + /// Whether this belongs in the notification center. + /// + /// Work still in flight is included whatever its persistence says: a + /// download is exactly the thing a user opens the center to check on, and + /// it is only transient in the sense that nothing is left to review once it + /// finishes. + pub fn in_center(&self) -> bool { + self.persistence.is_persistent() || self.is_loading + } } #[derive(Clone, Debug)] @@ -401,8 +442,19 @@ impl NotificationState { } } + /// Unread entries the center would actually show. A toast the user is + /// looking at right now must not also light up the badge that means "you + /// missed something". pub fn unread_count(inbox: &[InboxEntry]) -> usize { - inbox.iter().filter(|entry| !entry.read).count() + inbox + .iter() + .filter(|entry| !entry.read && entry.in_center()) + .count() + } + + /// The entries the notification center renders, newest first. + pub fn center_entries(inbox: &[InboxEntry]) -> impl Iterator { + inbox.iter().filter(|entry| entry.in_center()) } /// Folds one notification into the engine state. Does not build a @@ -420,10 +472,12 @@ impl NotificationState { Event::Notification(Notification::Message(message)) => { let entry_id = self.push_inbox( inbox, - message.title, - message.body, - message.level, - None, + NotificationSpec::plain( + message.title, + message.body, + message.level, + message.persistence, + ), false, ); self.push_ephemeral_toast(entry_id, MESSAGE_TOAST_TTL); @@ -441,8 +495,13 @@ impl NotificationState { }) => { self.handle_progress(inbox, id, label, current, total); } - Event::Progress(ProgressEvent::Complete { id, title, body }) => { - self.handle_progress_complete(inbox, id, title, body); + Event::Progress(ProgressEvent::Complete { + id, + title, + body, + persistence, + }) => { + self.handle_progress_complete(inbox, id, title, body, persistence); } Event::Progress(ProgressEvent::Grouped(event)) => { self.handle_grouped_progress(inbox, event); @@ -467,11 +526,20 @@ impl NotificationState { (timers, None) } - pub fn toggle_center(&mut self, _inbox: &mut [InboxEntry], center_open: bool) -> bool { + pub fn toggle_center(&mut self, inbox: &mut Vec, center_open: bool) -> bool { let next = !center_open; if next { - self.active_toasts.clear(); + // Opening the center swallows every toast, which for a transient + // one is the end of its life: no toast is left to expire and drop + // it, so it would sit in the panel it was never meant to reach. + let dropped: Vec = std::mem::take(&mut self.active_toasts) + .into_iter() + .map(|toast| toast.entry_id) + .collect(); self.pending_timers.clear(); + for entry_id in dropped { + self.drop_if_transient(inbox, entry_id); + } } next } @@ -492,15 +560,15 @@ impl NotificationState { else { return; }; - let entry = inbox.iter().find(|e| e.id == entry_id); - if entry.is_some_and(|e| !e.dismissable()) { + if inbox + .iter() + .find(|e| e.id == entry_id) + .is_some_and(|e| !e.dismissable()) + { return; } - let has_progress = entry.is_some_and(|e| e.progress.is_some()); self.active_toasts.remove(pos); - if !has_progress { - self.forget_entry(inbox, entry_id); - } + self.drop_if_transient(inbox, entry_id); } #[allow(dead_code)] @@ -509,9 +577,26 @@ impl NotificationState { .retain(|toast| toast.toast_id != toast_id); } - pub fn expire_toast(&mut self, _inbox: &[InboxEntry], entry_id: u64) { + pub fn expire_toast(&mut self, inbox: &mut Vec, entry_id: u64) { self.active_toasts .retain(|toast| toast.entry_id != entry_id); + self.drop_if_transient(inbox, entry_id); + } + + /// The one place a toast ending decides whether anything is left behind. + /// + /// A transient notification exists for the length of its toast and no + /// longer; letting it fall into the center is what buried the failures that + /// belong there. Work still in flight is exempt: closing the toast on a + /// running download hides the toast, not the download. + fn drop_if_transient(&mut self, inbox: &mut Vec, entry_id: u64) { + let drop = inbox + .iter() + .find(|e| e.id == entry_id) + .is_some_and(|e| !e.in_center()); + if drop { + self.forget_entry(inbox, entry_id); + } } pub fn mark_read(&mut self, inbox: &mut [InboxEntry], entry_id: u64) { @@ -537,38 +622,14 @@ impl NotificationState { self.grouped_entries.retain(|_, &mut v| v != entry_id); } + /// Files a new entry and returns its id. Raises no toast: the progress + /// paths arm their own, and which kind depends on what they are reporting. fn push_inbox( &mut self, inbox: &mut Vec, - title: String, - body: String, - level: Level, - progress: Option<(u64, u64)>, + spec: NotificationSpec, is_loading: bool, ) -> u64 { - let id = self.next_id; - self.next_id += 1; - inbox.insert( - 0, - InboxEntry { - id, - title, - body, - level, - icon: None, - progress, - is_loading, - read: false, - created_at: Instant::now(), - actions: Vec::new(), - tasks: Vec::new(), - transfer: None, - }, - ); - id - } - - pub fn push_custom(&mut self, inbox: &mut Vec, spec: NotificationSpec) -> u64 { let NotificationSpec { title, body, @@ -576,10 +637,9 @@ impl NotificationState { icon, progress, actions, + persistence, } = spec; - let is_loading = progress.is_some_and(|(current, total)| total == 0 || current < total); - let id = self.next_id; self.next_id += 1; inbox.insert( @@ -597,8 +657,16 @@ impl NotificationState { actions, tasks: Vec::new(), transfer: None, + persistence, }, ); + id + } + + pub fn push_custom(&mut self, inbox: &mut Vec, spec: NotificationSpec) -> u64 { + let progress = spec.progress; + let is_loading = progress.is_some_and(|(current, total)| total == 0 || current < total); + let id = self.push_inbox(inbox, spec, is_loading); if progress.is_some() { self.ensure_progress_toast(id); @@ -669,10 +737,19 @@ impl NotificationState { } else { let entry_id = self.push_inbox( inbox, - label.clone(), - body.clone(), - Level::Info, - progress, + NotificationSpec { + progress, + // Work in flight shows in the center on its own + // (`in_center`); once it is over there is nothing left to + // review, unless a caller replaces it with a result that + // says otherwise. + ..NotificationSpec::plain( + label.clone(), + body.clone(), + Level::Info, + Persistence::Transient, + ) + }, !done, ); self.progress_entries.insert(id, entry_id); @@ -697,13 +774,22 @@ impl NotificationState { id: Uuid, title: String, body: String, + persistence: Persistence, ) { if let Some(entry_id) = self.progress_entries.remove(&id) { self.update_inbox_entry(inbox, entry_id, title, body, None, false); + // The card was a download, which is never filed; what it just + // became might be. + if let Some(entry) = inbox.iter_mut().find(|e| e.id == entry_id) { + entry.persistence = persistence; + } self.ensure_progress_toast(entry_id); } else { - let entry_id = - self.push_inbox(inbox, title, body, Level::Info, None, false); + let entry_id = self.push_inbox( + inbox, + NotificationSpec::plain(title, body, Level::Info, persistence), + false, + ); self.push_ephemeral_toast(entry_id, MESSAGE_TOAST_TTL); } } @@ -717,10 +803,12 @@ impl NotificationState { GroupedProgressEvent::Start { session_id, title } => { let entry_id = self.push_inbox( inbox, - title.clone(), - "Preparing...".to_string(), - Level::Info, - None, + NotificationSpec::plain( + title.clone(), + "Preparing...", + Level::Info, + Persistence::Transient, + ), true, ); self.grouped_entries.insert(session_id, entry_id); @@ -859,6 +947,7 @@ impl NotificationState { icon, progress: _, actions, + persistence, } = spec; match entry_id.and_then(|id| inbox.iter_mut().find(|e| e.id == id)) { @@ -874,6 +963,9 @@ impl NotificationState { entry.actions = actions; entry.tasks = Vec::new(); entry.transfer = None; + // The result decides, not the download that produced it: the + // progress card was transient, its outcome may not be. + entry.persistence = persistence; // The progress toast (if any) is already armed for this entry; // leaving it in `active_toasts` lets the loop give it a dismiss // timer now that it is no longer loading. @@ -891,6 +983,7 @@ impl NotificationState { icon, progress: None, actions, + persistence, }, ); } @@ -943,6 +1036,164 @@ fn progress_body(label: &str, current: u64, total: u64) -> String { format!("{label} - {percent}%") } +#[cfg(test)] +mod persistence_tests { + use super::*; + use oneclient_events::Message; + + fn message(title: &str, persistence: Persistence) -> Event { + Event::Notification(Notification::Message(Message { + title: title.into(), + body: String::new(), + level: Level::Info, + persistence, + })) + } + + /// Raises a notification and returns the entry it created. + fn raise( + state: &mut NotificationState, + inbox: &mut Vec, + persistence: Persistence, + ) -> u64 { + state.dispatch(inbox, message("Copied to clipboard", persistence)); + inbox[0].id + } + + #[test] + fn a_transient_notification_does_not_outlive_its_toast() { + let mut state = NotificationState::default(); + let mut inbox = Vec::new(); + let id = raise(&mut state, &mut inbox, Persistence::Transient); + + state.expire_toast(&mut inbox, id); + + assert!(inbox.is_empty(), "a toast that timed out leaves nothing behind"); + } + + #[test] + fn a_persistent_notification_is_filed_when_its_toast_goes() { + let mut state = NotificationState::default(); + let mut inbox = Vec::new(); + let id = raise(&mut state, &mut inbox, Persistence::Persistent); + + state.expire_toast(&mut inbox, id); + + assert_eq!(inbox.len(), 1); + assert_eq!(NotificationState::center_entries(&inbox).count(), 1); + } + + /// Closing a toast by hand is not "file this for later" — it is the user + /// saying they are done with it. + #[test] + fn dismissing_a_transient_toast_forgets_it_too() { + let mut state = NotificationState::default(); + let mut inbox = Vec::new(); + let id = raise(&mut state, &mut inbox, Persistence::Transient); + + state.dismiss_toast(&mut inbox, id); + + assert!(inbox.is_empty()); + } + + /// Opening the center clears the toast stack, which is the one path where + /// a transient entry has no toast left to take it away with. + #[test] + fn opening_the_center_does_not_strand_a_transient_notification() { + let mut state = NotificationState::default(); + let mut inbox = Vec::new(); + raise(&mut state, &mut inbox, Persistence::Transient); + raise(&mut state, &mut inbox, Persistence::Persistent); + + assert!(state.toggle_center(&mut inbox, false)); + + assert_eq!(inbox.len(), 1); + assert_eq!(inbox[0].persistence, Persistence::Persistent); + } + + /// The badge means "you missed something", so a toast the user is looking + /// at right now must not raise it. + #[test] + fn a_transient_toast_does_not_light_up_the_unread_badge() { + let mut state = NotificationState::default(); + let mut inbox = Vec::new(); + raise(&mut state, &mut inbox, Persistence::Transient); + + assert_eq!(NotificationState::unread_count(&inbox), 0); + + raise(&mut state, &mut inbox, Persistence::Persistent); + assert_eq!(NotificationState::unread_count(&inbox), 1); + } + + /// A download is transient, but checking on one is half the reason to open + /// the center — so it belongs there right up until it stops running. + #[test] + fn a_running_download_shows_in_the_center_and_then_stops() { + let mut state = NotificationState::default(); + let mut inbox = Vec::new(); + let id = Uuid::new_v4(); + + state.dispatch( + &mut inbox, + Event::Progress(ProgressEvent::Update { + id, + label: "Downloading assets".into(), + current: 20, + total: 100, + }), + ); + assert_eq!(NotificationState::center_entries(&inbox).count(), 1); + + state.dispatch( + &mut inbox, + Event::Progress(ProgressEvent::Complete { + id, + title: "Downloaded".into(), + body: "Done".into(), + persistence: Persistence::Transient, + }), + ); + let entry_id = inbox[0].id; + state.expire_toast(&mut inbox, entry_id); + + assert!(inbox.is_empty()); + } + + /// The updater's whole point: the download is noise, "restart to apply" is + /// the instruction, and it must survive the five seconds it is on screen. + #[test] + fn a_completion_can_be_worth_keeping_even_when_the_download_was_not() { + let mut state = NotificationState::default(); + let mut inbox = Vec::new(); + let id = Uuid::new_v4(); + + state.dispatch( + &mut inbox, + Event::Progress(ProgressEvent::Update { + id, + label: "Downloading OneClient".into(), + current: 1, + total: 100, + }), + ); + state.dispatch( + &mut inbox, + Event::Progress(ProgressEvent::Complete { + id, + title: "Finished Downloading".into(), + body: "Restart to apply.".into(), + persistence: Persistence::Persistent, + }), + ); + + let entry_id = inbox[0].id; + state.expire_toast(&mut inbox, entry_id); + + assert_eq!(inbox.len(), 1); + assert_eq!(inbox[0].title, "Finished Downloading"); + } +} + #[cfg(test)] mod package_update_tests { use super::*; diff --git a/packages/oneclient_app/src/updater.rs b/packages/oneclient_app/src/updater.rs index 88c473c7..097ccf9d 100644 --- a/packages/oneclient_app/src/updater.rs +++ b/packages/oneclient_app/src/updater.rs @@ -1,7 +1,7 @@ use std::cell::Cell; use cargo_packager_updater::{Config, Update, check_update}; -use oneclient_events::{Choice, EventBus, Prompt}; +use oneclient_events::{Choice, EventBus, Persistence, Prompt}; use uuid::Uuid; /// Choice id for the update prompt, so the overlay can recognise it among any @@ -82,6 +82,7 @@ async fn run_simulated_update() -> anyhow::Result<()> { progress_id, "Finished Downloading", format!("OneClient {FAKE_VERSION} is ready. Restart to apply."), + Persistence::Persistent, ); Ok(()) @@ -111,6 +112,9 @@ async fn run_check(auto_install: bool) -> anyhow::Result<()> { "OneClient {} is available. Download the latest package from {} to update.", update.version, RELEASES_URL )) + // Asks the user to go and download a package by hand; useless if it + // scrolls past while they are elsewhere. + .persistent() .send(); return Ok(()); } @@ -191,6 +195,9 @@ async fn download_and_install(update: Update, events: EventBus) -> anyhow::Resul progress_id, "Finished Downloading", format!("OneClient {version} is ready. Restart to apply."), + // The update is only live after a restart the user has to perform, + // so this has to outlive the five seconds it is on screen for. + Persistence::Persistent, ); Ok(()) }) diff --git a/packages/oneclient_app/src/view/app/cluster/logs.rs b/packages/oneclient_app/src/view/app/cluster/logs.rs index 887fb50c..68be5d96 100644 --- a/packages/oneclient_app/src/view/app/cluster/logs.rs +++ b/packages/oneclient_app/src/view/app/cluster/logs.rs @@ -134,6 +134,10 @@ impl Component for ClusterLogs { .body(format!("{} (copied to clipboard)", result.url)) .info() .icon(IconType::LinkExternal01) + // The body is a URL the user uploaded a log in order to + // share; the next clipboard write is all it takes to lose + // it, so the notification is the second copy. + .persistent() .send(); } MutationStateData::Settled { res: Err(err), .. } => { @@ -143,7 +147,12 @@ impl Component for ClusterLogs { } handled_upload.set(Some(msg.clone())); - dispatch.notify("Upload failed").body(msg).error().send(); + dispatch + .notify("Upload failed") + .body(msg) + .error() + .transient() + .send(); } _ => {} }); diff --git a/packages/oneclient_app/src/view/app/debug.rs b/packages/oneclient_app/src/view/app/debug.rs index 9158bd55..020c5965 100644 --- a/packages/oneclient_app/src/view/app/debug.rs +++ b/packages/oneclient_app/src/view/app/debug.rs @@ -438,6 +438,8 @@ fn send_cluster_update(dispatch: &crate::Actions, summaries: Vec { @@ -946,6 +949,7 @@ fn run_damage(dispatch: &crate::Actions, kind: DamageKind, cluster_id: i64) { .notify("Simulation failed") .body(err.to_string()) .error() + .transient() .send(); } } diff --git a/packages/oneclient_content/src/bundles/overrides.rs b/packages/oneclient_content/src/bundles/overrides.rs index 797d7e03..3aa70e26 100644 --- a/packages/oneclient_content/src/bundles/overrides.rs +++ b/packages/oneclient_content/src/bundles/overrides.rs @@ -221,6 +221,9 @@ fn notify_conflicts( "{bundle_name}: {} config file(s) you edited were left untouched by the update: {listed}{suffix}", conflicts.len() )) + // The update did not do what the bundle asked, and the list of files it + // skipped is the only record of where the cluster now differs. + .persistent() .send(); } diff --git a/packages/oneclient_core/src/dev.rs b/packages/oneclient_core/src/dev.rs index 87d5f196..d1cf1adf 100644 --- a/packages/oneclient_core/src/dev.rs +++ b/packages/oneclient_core/src/dev.rs @@ -72,7 +72,11 @@ fn spawn_notification_handler(mut rx: oneclient_events::EventReceiver) { pb.set_position(current); } } - Event::Progress(ProgressEvent::Complete { id, title, body }) => { + // A terminal has no notification center, so there is nothing + // for `persistence` to mean here. + Event::Progress(ProgressEvent::Complete { + id, title, body, .. + }) => { if let Some(pb) = progress_bars.remove(&id) { pb.finish_with_message(format!("{title}: {body}")); } else { diff --git a/packages/oneclient_core/src/game/launch.rs b/packages/oneclient_core/src/game/launch.rs index b3a6458f..14a88719 100644 --- a/packages/oneclient_core/src/game/launch.rs +++ b/packages/oneclient_core/src/game/launch.rs @@ -620,6 +620,10 @@ pub async fn offer_repair( .events .notify("Repair complete") .body(report.summary()) + // What a repair replaced is the first thing anyone asks after + // the next crash, and it is several minutes of downloading + // after the user stopped watching. + .persistent() .send(); } Err(err) => { diff --git a/packages/oneclient_events/src/bus.rs b/packages/oneclient_events/src/bus.rs index 41d048cd..89182894 100644 --- a/packages/oneclient_events/src/bus.rs +++ b/packages/oneclient_events/src/bus.rs @@ -2,7 +2,9 @@ use tokio::sync::{mpsc, oneshot}; use uuid::Uuid; use crate::error::{EventError, EventResult}; -use crate::event::{Event, GameEvent, LaunchStage, Level, Message, Notification, ProgressEvent, Signal}; +use crate::event::{ + Event, GameEvent, LaunchStage, Level, Message, Notification, Persistence, ProgressEvent, Signal, +}; use crate::prompt::{Answer, Chosen, Prompt, PromptRequest}; /// The sending half of the event bus. @@ -62,7 +64,9 @@ impl EventBus { title: title.into(), body: String::new(), level: Level::Info, + persistence: Persistence::Transient, }, + persistence: None, } } @@ -84,11 +88,22 @@ impl EventBus { } /// Converts the in-flight progress entry `id` into a finished message. - pub fn finish_progress(&self, id: Uuid, title: impl Into, body: impl Into) { + /// + /// `persistence` is spelled out rather than defaulted: a finished download + /// is usually nothing, and occasionally the "restart to apply" the whole + /// download was for, and only the caller knows which. + pub fn finish_progress( + &self, + id: Uuid, + title: impl Into, + body: impl Into, + persistence: Persistence, + ) { self.emit(ProgressEvent::Complete { id, title: title.into(), body: body.into(), + persistence, }); } @@ -172,6 +187,11 @@ impl EventBus { pub struct NotificationBuilder<'a> { bus: &'a EventBus, message: Message, + /// The caller's explicit choice, resolved against the level in `send`. + /// Kept out of `message` so `.error().transient()` and + /// `.transient().error()` mean the same thing; a builder where the call + /// order silently changes where a notification lands is a trap. + persistence: Option, } impl NotificationBuilder<'_> { @@ -193,7 +213,23 @@ impl NotificationBuilder<'_> { self.level(Level::Error) } - pub fn send(self) { + /// Files this notification in the notification center. + pub fn persistent(mut self) -> Self { + self.persistence = Some(Persistence::Persistent); + self + } + + /// Shows this notification and forgets it. Use on an error the user is + /// already looking at and can simply retry. + pub fn transient(mut self) -> Self { + self.persistence = Some(Persistence::Transient); + self + } + + pub fn send(mut self) { + self.message.persistence = self + .persistence + .unwrap_or_else(|| Persistence::for_level(self.message.level)); self.bus.emit(self.message); } } @@ -330,5 +366,49 @@ mod tests { assert_eq!(message.title, "Done"); assert_eq!(message.body, ""); assert_eq!(message.level, Level::Info); + assert_eq!(message.persistence, Persistence::Transient); + } + + async fn persistence_of(build: impl FnOnce(&EventBus)) -> Persistence { + let (bus, mut rx) = EventBus::channel(); + build(&bus); + + let Some(Event::Notification(Notification::Message(message))) = rx.recv().await else { + panic!("expected a message"); + }; + message.persistence + } + + /// The default that keeps the center worth opening: an ordinary "done" + /// notice is seen and forgotten, a failure is filed. + #[tokio::test] + async fn the_level_decides_when_the_caller_does_not() { + assert_eq!( + persistence_of(|bus| bus.notify("Copied").send()).await, + Persistence::Transient + ); + assert_eq!( + persistence_of(|bus| bus.notify("Install failed").error().send()).await, + Persistence::Persistent + ); + } + + /// Both overrides exist because both defaults are wrong somewhere: a failed + /// clipboard copy is an error worth no follow-up, and "Update available" is + /// an info the user will want to find again. + #[tokio::test] + async fn an_explicit_choice_wins_whichever_order_it_is_made_in() { + assert_eq!( + persistence_of(|bus| bus.notify("Copy failed").error().transient().send()).await, + Persistence::Transient + ); + assert_eq!( + persistence_of(|bus| bus.notify("Copy failed").transient().error().send()).await, + Persistence::Transient + ); + assert_eq!( + persistence_of(|bus| bus.notify("Update available").persistent().send()).await, + Persistence::Persistent + ); } } diff --git a/packages/oneclient_events/src/event.rs b/packages/oneclient_events/src/event.rs index 1d9050f2..e1dd856f 100644 --- a/packages/oneclient_events/src/event.rs +++ b/packages/oneclient_events/src/event.rs @@ -85,6 +85,7 @@ pub struct Message { pub title: String, pub body: String, pub level: Level, + pub persistence: Persistence, } #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] @@ -94,6 +95,49 @@ pub enum Level { Error, } +/// Whether a message is worth keeping once the user has seen it. +/// +/// Every message is shown the same way — as a toast — so this is not about +/// visibility. It is about what the notification center is *for*: a place to +/// come back to when you missed something. "Copied to clipboard" answers a +/// question the user asked half a second ago and has already had answered; +/// filing it only buries the install that failed while they were away. +/// +/// The default is [`Persistence::Transient`], so a new notification has to earn +/// its place rather than take one by omission. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] +pub enum Persistence { + /// Toasted, then forgotten. Confirmations of something the user just did. + #[default] + Transient, + /// Toasted, then filed in the notification center until dismissed. + /// Failures, and news about work the user did not start. + Persistent, +} + +impl Persistence { + /// What a message of this level gets when the caller expresses no opinion. + /// + /// Errors file themselves: an error is by definition something that did not + /// happen the way the user wanted, and a toast they were not looking at is + /// the one case where "review it later" is the whole point. Anything else + /// has to ask for it with [`crate::NotificationBuilder::persistent`], which + /// is also why the two are separate axes — a failed clipboard copy is an + /// error nobody needs filed, and it says so. + #[must_use] + pub fn for_level(level: Level) -> Self { + match level { + Level::Error => Self::Persistent, + Level::Info => Self::Transient, + } + } + + #[must_use] + pub fn is_persistent(self) -> bool { + matches!(self, Self::Persistent) + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum ProgressEvent { /// A single task's progress, keyed by `id` so repeated updates replace one @@ -108,10 +152,15 @@ pub enum ProgressEvent { /// Turn the in-flight [`ProgressEvent::Update`] with this `id` into a /// finished message in place, so a download and its completion notice are /// one card instead of two. + /// + /// Carries its own [`Persistence`] because the card it replaces has none to + /// inherit: progress is only ever worth watching live, while what it turns + /// into may be the one thing the user has to act on. Complete { id: Uuid, title: String, body: String, + persistence: Persistence, }, /// A tree of related tasks. See [`crate::progress`]. diff --git a/packages/oneclient_events/src/lib.rs b/packages/oneclient_events/src/lib.rs index 1f5cc583..bc4e7215 100644 --- a/packages/oneclient_events/src/lib.rs +++ b/packages/oneclient_events/src/lib.rs @@ -19,7 +19,7 @@ pub mod prompt; pub use bus::{EventBus, EventReceiver, NotificationBuilder}; pub use error::{EventError, EventResult}; pub use event::{ - Event, GameEvent, LaunchStage, Level, Message, Notification, ProgressEvent, Signal, + Event, GameEvent, LaunchStage, Level, Message, Notification, Persistence, ProgressEvent, Signal, }; pub use progress::{ GroupedProgressChild, GroupedProgressEvent, GroupedProgressSession, TaskCategory, TaskPhase,