From 85a3e4fb09fed22b4b1849cad623efd1cf7756fd Mon Sep 17 00:00:00 2001 From: clover caruso Date: Mon, 5 Oct 2026 01:58:00 -0700 Subject: [PATCH] fix: keep notebook switches responsive during cloud theme reads Refresh notebook themes off the frame thread, retain the last good cache on read failures, and apply loaded themes when the worker reports back. Preserve newer local theme edits when a slower read completes. Verified with a stalled FIFO theme read: the previous release stops painting; the fixed app opens Options and switches appearance before the read completes. App tests, Clippy, and formatting pass. Assisted-by: gpt-6.1-sol --- crates/snowbound/src/library.rs | 165 +++++++++++++++++++++++++++----- crates/snowbound/src/main.rs | 3 +- crates/snowbound/src/themes.rs | 7 +- crates/snowbound/src/undo.rs | 2 +- 4 files changed, 149 insertions(+), 28 deletions(-) diff --git a/crates/snowbound/src/library.rs b/crates/snowbound/src/library.rs index 0c71c06099e3b9985ecda2493aecfd85bd284152..944eaa61f25b7112960b6dbf939fd06ad71e1320 100644 --- a/crates/snowbound/src/library.rs +++ b/crates/snowbound/src/library.rs @@ -324,8 +324,7 @@ pub struct Library { /// Sections opened ahead of being shown, or left open after, most recent first, which /// `open` hands out before opening any. kept: Mutex>, - /// The notebook's style themes, and when they were read. - themes: Mutex)>>, + themes: Mutex, /// The keys of its password-protected sections unlocked this run, kept as it is read again. keys: Arc, /// The share a notebook another computer shares by Live Share is reached through. @@ -338,6 +337,13 @@ pub struct Library { #[derive(Default)] pub struct Keys(Mutex>); +#[derive(Default)] +struct ThemeCache { + value: Arc, + checked: Option, + reading: bool, +} + impl Library { /// `location` named `name`, with no notebook read, server, sync or kept sections. fn new(location: &str, name: String, cache: &Path) -> Self { @@ -645,27 +651,53 @@ impl Library { }); } - /// The notebook's style themes, read again after a while so other machines' changes - /// reach pages opened later; a section opened on its own has none. - pub fn themes(&self) -> Arc { + /// The notebook's last-read themes, refreshed off the frame thread. + pub fn themes(self: &Arc) -> Arc { let mut kept = self .themes .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - if let Some((read, themes)) = &*kept - && read.elapsed() < Duration::from_secs(30) + let themes = Arc::clone(&kept.value); + if !matches!(self.notebook, Ok(Some(_))) + || kept.reading + || kept + .checked + .is_some_and(|read| read.elapsed() < Duration::from_secs(30)) { - return Arc::clone(themes); + return themes; } - let themes = match &self.notebook { - Ok(Some(notebook)) => notebook.themes().unwrap_or_else(|error| { - eprintln!("{}: reading its themes failed: {error}", self.location); - Themes::default() - }), - _ => Themes::default(), - }; - let themes = Arc::new(themes); - *kept = Some((Instant::now(), Arc::clone(&themes))); + kept.reading = true; + drop(kept); + let (library, before) = (Arc::clone(self), Arc::clone(&themes)); + crate::spawn(move || { + let read = match &library.notebook { + Ok(Some(notebook)) => notebook.themes(), + _ => Ok(Themes::default()), + }; + let mut kept = library + .themes + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + kept.reading = false; + kept.checked = Some(Instant::now()); + match read { + Ok(mut read) => { + // A local edit may have landed while the file was being read. + if !Arc::ptr_eq(&before, &kept.value) { + notebook::sidecar::themes::merge(&mut read, (*kept.value).clone()); + } + let changed = *kept.value != read; + kept.value = Arc::new(read); + drop(kept); + if changed { + notify_background(); + } + } + Err(error) => { + eprintln!("{}: reading its themes failed: {error}", library.location); + } + } + }); themes } @@ -675,13 +707,15 @@ impl Library { if !matches!(self.notebook, Ok(Some(_))) { return; } - let mut themes = (*self.themes()).clone(); - notebook::sidecar::themes::merge(&mut themes, change.clone()); - *self - .themes - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) = - Some((Instant::now(), Arc::new(themes))); + self.themes(); + { + let mut kept = self + .themes + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + notebook::sidecar::themes::merge(Arc::make_mut(&mut kept.value), change.clone()); + kept.checked = Some(Instant::now()); + } let library = Arc::clone(self); crate::spawn(move || { let kept = library @@ -1471,6 +1505,89 @@ pub fn locate(path: &Path) -> Located { mod tests { use super::*; + #[cfg(unix)] + #[test] + fn stalled_theme_reads_leave_the_cache_usable_and_keep_local_edits() { + use notebook::sidecar::themes::{Assignment, Scope, merge}; + use std::{ + io::Write, + os::unix::{ffi::OsStrExt, fs::OpenOptionsExt}, + }; + + let root = + std::env::temp_dir().join(format!("snowbound-stalled-themes-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(root.join("notebook/.snowbound")).unwrap(); + let path = root.join("notebook/.snowbound/themes.json"); + let filename = std::ffi::CString::new(path.as_os_str().as_bytes()).unwrap(); + assert_eq!(unsafe { libc::mkfifo(filename.as_ptr(), 0o600) }, 0); + let library = Arc::new(Library::notebook( + root.join("notebook").to_str().unwrap(), + &root.join("cache"), + )); + let cached = |library: Arc| { + let (send, receive) = std::sync::mpsc::channel(); + std::thread::spawn(move || send.send(library.themes()).unwrap()); + receive + .recv_timeout(Duration::from_secs(1)) + .expect("theme access must not wait for storage") + }; + assert_eq!(*cached(Arc::clone(&library)), Themes::default()); + let deadline = Instant::now() + Duration::from_secs(3); + let mut writer = loop { + match std::fs::OpenOptions::new() + .write(true) + .custom_flags(libc::O_NONBLOCK) + .open(&path) + { + Ok(writer) => break writer, + Err(error) + if error.raw_os_error() == Some(libc::ENXIO) && Instant::now() < deadline => + { + std::thread::sleep(Duration::from_millis(5)); + } + Err(error) => panic!("theme reader did not start: {error}"), + } + }; + assert_eq!(*cached(Arc::clone(&library)), Themes::default()); + let assignment = |theme: &str, assigned| Themes { + assignments: vec![Assignment { + scope: Scope::Notebook, + theme: Some(theme.to_owned()), + assigned, + }], + ..Themes::default() + }; + let local = assignment("local", 10); + { + let mut kept = library.themes.lock().unwrap(); + merge(Arc::make_mut(&mut kept.value), local.clone()); + } + assert_eq!(*cached(Arc::clone(&library)), local); + writer + .write_all(&serde_json::to_vec(&assignment("remote", 5)).unwrap()) + .unwrap(); + drop(writer); + while library.themes.lock().unwrap().reading { + assert!(Instant::now() < deadline, "theme read did not finish"); + std::thread::sleep(Duration::from_millis(5)); + } + assert_eq!(*cached(Arc::clone(&library)), local); + std::fs::remove_file(&path).unwrap(); + std::fs::create_dir(&path).unwrap(); + library.themes.lock().unwrap().checked = Some(Instant::now() - Duration::from_secs(31)); + assert_eq!(*cached(Arc::clone(&library)), local); + while library.themes.lock().unwrap().reading { + assert!( + Instant::now() < deadline, + "failed theme read did not finish" + ); + std::thread::sleep(Duration::from_millis(5)); + } + assert_eq!(*cached(library), local); + std::fs::remove_dir_all(root).unwrap(); + } + /// A section kept open is what `open` hands out next, without opening it again, and /// readying a section another holds leaves it to that one at once. #[test] diff --git a/crates/snowbound/src/main.rs b/crates/snowbound/src/main.rs index 333af46c7e9b037c3a22cdb9a7e4ef2d8c24ef97..7fcd736bbbab151d665760eb47247c9782db70fb 100644 --- a/crates/snowbound/src/main.rs +++ b/crates/snowbound/src/main.rs @@ -3960,7 +3960,7 @@ impl State { /// Follows a page shown in place of another. fn opened(&mut self) -> Result<(), Box> { - if let Err(error) = self.wear_theme() { + if let Err(error) = self.wear_theme(true) { eprintln!("Restyling the page failed: {error}"); } // A page a search result shows leaves the keys with the search. @@ -4243,6 +4243,7 @@ impl State { /// Applies what the section, and each notebook's closed sections, reported since the /// last poll. fn synced(&mut self) -> Result<(), Box> { + self.wear_theme(false)?; // Every notebook's changes are taken, so none is reported again. let reported: Vec<(Arc, Vec)> = (self.notebooks.iter()) .filter_map(|library| { diff --git a/crates/snowbound/src/themes.rs b/crates/snowbound/src/themes.rs index 359f3ebdd92089615af6ff5ee27c46939906490b..ee9128e2439a76db8c2bdad7ec1af17bdbef7b32 100644 --- a/crates/snowbound/src/themes.rs +++ b/crates/snowbound/src/themes.rs @@ -162,11 +162,14 @@ impl State { /// Dresses the open page in its theme: new text and Enter take the theme's styles, and /// style objects the theme gives otherwise are restyled, as one edit. A page with no theme /// keeps what it holds. - pub(crate) fn wear_theme(&mut self) -> Result<(), Box> { + pub(crate) fn wear_theme(&mut self, force: bool) -> Result<(), Box> { let theme = self.page_theme(); let sheet = (theme.as_ref()) .map(|theme| theme.sheet(self.section_color())) .unwrap_or_default(); + if !force && self.view.editor.styles == sheet { + return Ok(()); + } self.view.editor.styles = sheet.clone(); let Some(session) = &self.session else { return Ok(()); @@ -730,7 +733,7 @@ impl State { self.ui.close_popup(id()); self.themes = None; if chosen.is_some() - && let Err(error) = self.wear_theme() + && let Err(error) = self.wear_theme(true) { eprintln!("Restyling the page failed: {error}"); } diff --git a/crates/snowbound/src/undo.rs b/crates/snowbound/src/undo.rs index 7c54603f2ee5b61cacde8eafa8d35f137cceedf1..c040325a8d39d62d57e5a3a3af72c1314a41c2de 100644 --- a/crates/snowbound/src/undo.rs +++ b/crates/snowbound/src/undo.rs @@ -1021,7 +1021,7 @@ impl State { change: Change::Themes { from: to, to: from }, }; self.undo.note(taken, undo); - return self.wear_theme(); + return self.wear_theme(true); } let Some(section) = section else { let library = self -- 2.54.0