From 8c01efc1126e6d58946a2209125df65e46d10145 Mon Sep 17 00:00:00 2001 From: clover caruso Date: Sat, 10 Oct 2026 19:04:20 -0700 Subject: [PATCH] fix: keep section mutations responsive across navigation Show submitted section and group names until their own storage operation finishes. Serialize notebook structure changes, resolve repeated renames by file identity, and reconcile successful catalogs without superseding newer navigation. Retarget open section workers in place after moves, preserving queued edits and the editor while a cancelled remote attempt finishes off-thread. Cover repeated and failed renames, stale labels across catalog refreshes, and a queued edit whose old remote stalls or fails after its section moves. Fixes #109 Refs #108 Assisted-by: gpt-6.1-sol --- Cargo.lock | 1 + crates/notebook/src/session.rs | 66 +++++++-- crates/notebook/src/worker.rs | 42 +++++- crates/notebook/tests/session.rs | 134 ++++++++++++++++++ crates/snowbound/Cargo.toml | 1 + crates/snowbound/src/library.rs | 91 ++++++++++++ crates/snowbound/src/main.rs | 86 ++++++++---- crates/snowbound/src/manage.rs | 232 ++++++++++++++++++++++++++++++- crates/snowbound/src/sidebar.rs | 8 +- 9 files changed, 610 insertions(+), 51 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 087c81c4505800a98784df46b085e5c2f2ef4e46..89343ba90781022fc673ee0c101e64f5c4ad9c5f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3627,6 +3627,7 @@ dependencies = [ "accesskit_consumer", "accesskit_winit", "arboard", + "async-lock", "base64", "block2 0.5.1", "canvas", diff --git a/crates/notebook/src/session.rs b/crates/notebook/src/session.rs index 5d0600ca57ccc2c1dadf774ff4fe43b6c8a4371d..9bfc62d2bf3d0cb9dbf18626745f85fd08c2b7e5 100644 --- a/crates/notebook/src/session.rs +++ b/crates/notebook/src/session.rs @@ -2139,10 +2139,30 @@ impl Section { } }); let observed: Arc> = Arc::new(Mutex::new(None)); - let worker = { - let (sender, notify) = (sender.clone(), Arc::clone(¬ify)); - let observed = Arc::clone(&observed); - replica.start_sync(Duration::from_secs(2), connect, move |result| { + let mut section = Self { + file, + replica, + worker: None, + events: Mutex::new(events), + sender, + notify, + observed, + }; + section.worker = Some(section.start_worker(connect, None)?); + Ok(section) + } + + fn start_worker( + &self, + connect: impl FnMut() -> io::Result + Send + 'static, + replacing: Option<&SyncWorker>, + ) -> Result { + let (sender, notify) = (self.sender.clone(), Arc::clone(&self.notify)); + let observed = Arc::clone(&self.observed); + Ok(self.replica.start_sync_replacing( + Duration::from_secs(2), + connect, + move |result| { let error = result.as_ref().err().map(reached); // Reaching the file as the last attempt did is no news to the host. let mut changed = false; @@ -2182,17 +2202,33 @@ impl Section { { notify(); } - })? - }; - Ok(Self { - file, - replica, - worker: Some(worker), - events: Mutex::new(events), - sender, - notify, - observed, - }) + }, + replacing, + )?) + } + + /// Follows a section file moved within its notebook, keeping its replica and queued edits. + pub fn relocate(&mut self, file: impl AsRef) -> Result<()> { + self.relocate_with(fs::absolute(file)?, |file| Ok(FileRemote(file.to_owned()))) + } + + /// `relocate`, with a host's publication target. Cancellation leaves any in-flight step + /// to finish off-thread; synchronization ownership serializes the replacement worker. + pub fn relocate_with( + &mut self, + file: impl AsRef, + mut connect: impl FnMut(&Path) -> io::Result + Send + 'static, + ) -> Result<()> { + let file = file.as_ref().to_owned(); + let target = file.clone(); + let previous = self.worker.take(); + if let Some(worker) = &previous { + worker.cancel(); + } + self.observed = Arc::new(Mutex::new(None)); + self.worker = Some(self.start_worker(move || connect(&target), previous.as_ref())?); + self.file = file; + Ok(()) } /// The absolute local path, or share-relative path for an SMB session. diff --git a/crates/notebook/src/worker.rs b/crates/notebook/src/worker.rs index df6ae0fd73a40e4c0cb805890ee505de559bb7b6..f978c9eb1b6021f2932456d1aa504fd5f34dc2c0 100644 --- a/crates/notebook/src/worker.rs +++ b/crates/notebook/src/worker.rs @@ -86,6 +86,11 @@ pub struct SyncWorker { } impl SyncWorker { + pub(crate) fn cancel(&self) { + self.signal.stopped.store(true, Ordering::Release); + self.signal.wake(); + } + /// Requests a retry, for example after a network reachability change. Working /// offline, it synchronizes once, as OneNote's Sync Now does. /// A pending contention backoff finishes before processing the notification. @@ -120,8 +125,7 @@ impl SyncWorker { /// A stopped worker leaves pending edits and uncertain attempts in the cache. /// Call outside the worker's own callback, which cannot join its calling thread. pub fn stop(mut self) -> Result<()> { - self.signal.stopped.store(true, Ordering::Release); - self.signal.wake(); + self.cancel(); let thread = self.thread.take().expect("Worker owns its thread"); #[cfg(target_arch = "wasm32")] return thread.finished().unwrap_or(Ok(())); @@ -150,8 +154,7 @@ impl SyncWorker { impl Drop for SyncWorker { fn drop(&mut self) { - self.signal.stopped.store(true, Ordering::Release); - self.signal.wake(); + self.cancel(); } } @@ -166,10 +169,25 @@ impl Replica { /// Cache/document errors stop the worker; inspect them through `observe` or `stop`. /// Remote calls and callbacks must be bounded for `stop` to have bounded latency. pub fn start_sync( + self: &Arc, + interval: Duration, + connect: F, + observe: O, + ) -> io::Result + where + R: Remote + 'static, + F: FnMut() -> io::Result + Send + 'static, + O: FnMut(&Result) + Send + 'static, + { + self.start_sync_replacing(interval, connect, observe, None) + } + + pub(crate) fn start_sync_replacing( self: &Arc, interval: Duration, mut connect: F, mut observe: O, + replacing: Option<&SyncWorker>, ) -> io::Result where R: Remote + 'static, @@ -187,10 +205,24 @@ impl Replica { .worker .lock() .map_err(|_| io::Error::other("Synchronization worker registration panicked"))?; - if owner.upgrade().is_some() { + let previous = owner.upgrade(); + if let Some(previous) = &previous + && !replacing.is_some_and(|worker| { + Arc::ptr_eq(previous, &worker.signal) && previous.stopped.load(Ordering::Acquire) + }) + { return Err(io::ErrorKind::WouldBlock.into()); } let (signal, receiver) = Signal::new(); + if let Some(previous) = previous { + signal + .offline + .store(previous.offline.load(Ordering::Acquire), Ordering::Release); + if let (Ok(previous), Ok(mut next)) = (previous.pause.lock(), signal.pause.lock()) { + next.length = previous.length; + next.waiting = previous.waiting; + } + } let replica = Arc::clone(self); let worker_signal = Arc::clone(&signal); let thread = task::spawn("onestore-sync", move || async move { diff --git a/crates/notebook/tests/session.rs b/crates/notebook/tests/session.rs index 8a20e57391d92fc9d7e631f92ab057c8a08075f5..2ab09726b442a1a266b5d85b27a48e8e1ece5450 100644 --- a/crates/notebook/tests/session.rs +++ b/crates/notebook/tests/session.rs @@ -1218,6 +1218,140 @@ fn every_section_gets_an_offline_copy_that_closing_the_notebook_discards() { /// A section file whose stamp reads are counted. struct Counted(std::path::PathBuf, Arc); +#[test] +fn a_moved_section_keeps_its_replica_while_the_old_remote_is_stalled() { + use std::sync::{Condvar, Mutex, mpsc}; + + struct Stalled { + file: std::path::PathBuf, + source: Vec, + entered: Arc>>>, + release: Arc<(Mutex, Condvar)>, + finished: Option>, + publications: Arc, + late_error: bool, + } + + impl Drop for Stalled { + fn drop(&mut self) { + if let Some(finished) = self.finished.take() { + let _ = finished.send(()); + } + } + } + + impl notebook::Remote for Stalled { + fn read(&mut self) -> std::io::Result> { + Ok(self.source.clone()) + } + + fn stamp(&mut self) -> std::io::Result { + if let Some(entered) = self.entered.lock().unwrap().take() { + entered.send(()).unwrap(); + let (lock, changed) = &*self.release; + let _ = changed + .wait_timeout_while(lock.lock().unwrap(), Duration::from_secs(5), |done| !*done) + .unwrap(); + if self.late_error { + return Err(std::io::ErrorKind::TimedOut.into()); + } + } + onestore::Stamp::of(&self.source).map_err(std::io::Error::other) + } + + fn publish( + &mut self, + transaction: &onestore::Transaction, + ) -> Result<(), onestore::CommitError> { + self.publications.fetch_add(1, Ordering::SeqCst); + transaction.commit_file(&self.file) + } + + fn confirm(&mut self, _: &onestore::Stamp) -> Result<(), onestore::CommitError> { + Ok(()) + } + } + + for late_error in [false, true] { + let directory = tempfile::tempdir().unwrap(); + let file = directory.path().join("Before.one"); + let target = directory.path().join("After.one"); + let source = onestore::create_section("Before.one", "Original", "Author").unwrap(); + std::fs::write(&file, &source).unwrap(); + let (entered, waiting) = mpsc::channel(); + let (connected, connecting) = mpsc::channel(); + let (finished, stopped) = mpsc::channel(); + let begin = Arc::new((Mutex::new(false), Condvar::new())); + let starting = Arc::clone(&begin); + let release = Arc::new((Mutex::new(false), Condvar::new())); + let gate = Arc::clone(&release); + let entered = Arc::new(Mutex::new(Some(entered))); + let publications = Arc::new(AtomicUsize::new(0)); + let attempts = Arc::clone(&publications); + let mut opening = true; + let mut section = Section::open_with( + &file, + directory.path().join("cache"), + move |file| { + let finishing = (!std::mem::replace(&mut opening, false)).then(|| finished.clone()); + if finishing.is_some() { + connected.send(()).unwrap(); + let (lock, changed) = &*starting; + let _ = changed + .wait_timeout_while(lock.lock().unwrap(), Duration::from_secs(5), |done| { + !*done + }) + .unwrap(); + } + Ok(Stalled { + file: file.to_owned(), + source: source.clone(), + entered: Arc::clone(&entered), + release: Arc::clone(&gate), + finished: finishing, + publications: Arc::clone(&attempts), + late_error, + }) + }, + || {}, + ) + .unwrap(); + connecting.recv_timeout(Duration::from_secs(3)).unwrap(); + let replica = Arc::clone(section.replica()); + let (space, ..) = section.pages().unwrap()[0]; + let before = section.page(space).unwrap(); + let edit = typed(§ion, space, &before, 0..0, "Moved "); + *begin.0.lock().unwrap() = true; + begin.1.notify_all(); + waiting.recv_timeout(Duration::from_secs(3)).unwrap(); + section.set_offline(true); + std::fs::rename(&file, &target).unwrap(); + let start = Instant::now(); + section.relocate(&target).unwrap(); + assert!(start.elapsed() < Duration::from_secs(1)); + assert!(Arc::ptr_eq(&replica, section.replica())); + assert_eq!(section.file(), target); + assert_same(section.page(space).unwrap(), &edited(&before, "Moved ")); + *release.0.lock().unwrap() = true; + release.1.notify_all(); + stopped.recv_timeout(Duration::from_secs(3)).unwrap(); + assert_eq!( + publications.load(Ordering::SeqCst), + usize::from(!late_error) + ); + assert!(section.sync_status().unwrap().error.is_none()); + assert!(!matches!( + section.status(edit).unwrap(), + Some(EditStatus::Published { .. }) + )); + section.set_offline(false); + published(§ion, edit); + assert_same(stored_page(&target, space), &edited(&before, "Moved ")); + assert!(!file.exists()); + section.close().unwrap(); + } +} + impl notebook::Remote for Counted { fn read(&mut self) -> std::io::Result> { onestore::read_file(&self.0) diff --git a/crates/snowbound/Cargo.toml b/crates/snowbound/Cargo.toml index 654d63eef495a7d9dea99fc772fa0f8343b57991..4efb2bd455b51cc9456bebba44fb890d56828f5d 100644 --- a/crates/snowbound/Cargo.toml +++ b/crates/snowbound/Cargo.toml @@ -23,6 +23,7 @@ notebook = { path = "../notebook" } serde = { version = "1", features = ["derive"] } serde_json = "1" pollster = "0.4" +async-lock = "3.4.2" wgpu = { workspace = true, optional = true } parley.workspace = true winit = "0.30.13" diff --git a/crates/snowbound/src/library.rs b/crates/snowbound/src/library.rs index 7678482809052655e2d117e301393237f3c6837a..23e0f0cc01d7055fa377de0781591eac0aed2ab3 100644 --- a/crates/snowbound/src/library.rs +++ b/crates/snowbound/src/library.rs @@ -8,6 +8,8 @@ use notebook::sidecar::themes::Themes; use notebook::smb::{Client, Credentials}; use onestore::protected::Key; use std::{ + borrow::Cow, + collections::BTreeMap, error::Error, io, path::{Path, PathBuf}, @@ -324,6 +326,8 @@ 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>, + pub(crate) renames: Arc)>>>, + pub(crate) structure: Arc>, themes: Mutex, /// The keys of its password-protected sections unlocked this run, kept as it is read again. keys: Arc, @@ -359,6 +363,8 @@ impl Library { watch: None, tag_art: Default::default(), kept: Mutex::new(crate::prefetch::Recent::new(KEPT)), + renames: Arc::default(), + structure: Arc::default(), themes: Default::default(), keys: Default::default(), #[cfg(feature = "live")] @@ -796,6 +802,8 @@ impl Library { watch, tag_art: Mutex::new(self.tag_art()), keys: Arc::clone(&self.keys), + structure: Arc::clone(&self.structure), + renames: Arc::clone(&self.renames), #[cfg(feature = "live")] joined: self.joined.clone(), ..Self::new(&self.location, name, &self.cache) @@ -882,6 +890,60 @@ impl Library { } } + /// Follows a moved section without closing its readable replica or waiting for the share. + pub(crate) fn relocate(&self, path: &str, section: &mut Section) -> Result<(), Box> { + #[cfg(feature = "live")] + if let Some(joined) = &self.joined { + let guest = Arc::clone(&joined.guest); + section.relocate_with(path, move |file| { + Ok(notebook::live::share::HostedRemote::new( + &guest, + &file.to_string_lossy(), + )) + })?; + if let Some(background) = &self.background { + background.hold(path, section); + } + return Ok(()); + } + match &self.server { + #[cfg(not(target_arch = "wasm32"))] + Some(server) => { + let file = match server.mount.root.as_str() { + "" => path.to_owned(), + root => format!("{root}/{path}"), + }; + let server = Arc::clone(server); + section.relocate_with(file, move |file| { + Ok(notebook::SmbRemote::new( + server.connect()?, + file.to_string_lossy(), + LIMIT, + )) + })?; + } + #[cfg(target_arch = "wasm32")] + Some(_) => unreachable!("The browser opens no notebook on a server"), + None => { + let file = Path::new(&self.location).join(path); + #[cfg(target_os = "macos")] + if self.in_icloud() { + section.relocate_with(file, |file| { + Ok(crate::icloud::Coordinated(file.to_owned())) + })?; + } else { + section.relocate(file)?; + } + #[cfg(not(target_os = "macos"))] + section.relocate(file)?; + } + } + if let Some(background) = &self.background { + background.hold(path, section); + } + Ok(()) + } + /// Opens a section, waiting natively until `deadline` for its current owner. fn open_by( &self, @@ -1147,6 +1209,35 @@ impl Library { key(&self.location, path) } + /// A committed entry label, replaced by its pending rename until storage answers. + pub(crate) fn label<'a>(&self, path: &str, committed: &'a str) -> Cow<'a, str> { + self.renames + .lock() + .ok() + .and_then(|renames| { + renames + .get(&self.entry_identity(path)?) + .map(|(name, _)| name.clone()) + }) + .map_or(Cow::Borrowed(committed), Cow::Owned) + } + + pub(crate) fn entry_identity(&self, path: &str) -> Option<[u8; 16]> { + let catalog = self.catalog()?; + catalog + .sections() + .find(|section| section.path == path) + .map(|section| section.file_id) + .or_else(|| { + folders(catalog, |_| true) + .into_iter() + .find(|folder| folder.path == path)? + .toc + .as_ref() + .map(|toc| toc.file_id) + }) + } + /// The readable sections of the folder at catalog path `folder`, as tabs in its order. pub fn tabs(&self, folder: &str) -> Vec { match &self.notebook { diff --git a/crates/snowbound/src/main.rs b/crates/snowbound/src/main.rs index f897c2f75d3845e2056de950716d031cec6fb2b9..9cf8ea007e5d1bec7a421596003f0085fff95eaa 100644 --- a/crates/snowbound/src/main.rs +++ b/crates/snowbound/src/main.rs @@ -568,6 +568,22 @@ enum Bar { } impl Session { + fn follow(&mut self, library: Arc, path: &str) -> Result<(), Box> { + let folder = path.rsplit_once('/').map_or("", |(folder, _)| folder); + let tabs = library.tabs(folder); + let tab = tabs + .iter() + .position(|tab| tab.path == path) + .ok_or("The section is not in its notebook")?; + if self.tabs[self.tab].path != path { + library.relocate(path, &mut self.section)?; + } + self.tabs = tabs; + self.tab = tab; + self.library = library; + Ok(()) + } + fn title(&self) -> &str { match self.conflict(self.space) { Some((_, version)) => &version.title, @@ -1797,6 +1813,11 @@ impl State { let typed = renaming .as_ref() .map(|(tab, name)| (*tab, name.to_string())); + let labels: Vec<_> = session + .tabs + .iter() + .map(|tab| session.library.label(&tab.path, &tab.name)) + .collect(); let tabs: Vec<_> = session .tabs .iter() @@ -1804,7 +1825,7 @@ impl State { .map(|(index, tab)| { let name = match &typed { Some((renamed, name)) if *renamed == index => name.as_str(), - _ => tab.name.as_str(), + _ => labels[index].as_ref(), }; let unread = unread.contains(&session.library.key(&tab.path)); (name, section_color(tab.color), unread) @@ -3848,7 +3869,35 @@ impl State { } let created = matches!(opening.loaded, Shown::Created(_)); match opening.loaded { - Shown::Section(session) | Shown::Created(session) => { + Shown::Section(mut session) | Shown::Created(mut session) => { + if let Some(library) = self + .notebooks + .iter() + .find(|listed| { + listed.location == session.library.location + && Arc::ptr_eq(&listed.structure, &session.library.structure) + && !Arc::ptr_eq(listed, &session.library) + }) + .cloned() + { + let identity = session + .library + .section_identity(&session.tabs[session.tab].path); + let path = library.catalog().and_then(|catalog| { + catalog + .sections() + .find(|section| Some(section.file_id) == identity) + .map(|section| section.path.clone()) + }); + if let Some(path) = path { + session.follow(library, &path)?; + } else if let Some(path) = library.first_section() { + return self.apply(Command::OpenSection(library, path)); + } else { + self.switching = None; + return self.adopt(library, None); + } + } // The settings keep the notebook shown, not the section. let other_notebook = self .session @@ -3954,7 +4003,7 @@ impl State { } /// Takes `library`, a notebook read again after a change, in place of the one it was; - /// the open section, now at catalog `path` in it, stays open, reopened where it moved. + /// the open section, now at catalog `path` in it, keeps its replica where it moved. /// Without `path` the notebook has no sections left, and one of its shows none. fn adopt(&mut self, library: Arc, path: Option<&str>) -> Result<(), Box> { self.list(&library); @@ -3968,7 +4017,11 @@ impl State { { self.persist()?; if let Some(session) = self.session.take() { - session.section.close()?; + crate::spawn_async(move || async move { + if let Err(error) = session.section.close_async().await { + eprintln!("Synchronization stopped: {error}"); + } + }); } self.sectionless = Some(Arc::clone(&library)); self.title(); @@ -3979,28 +4032,11 @@ impl State { let Some(session) = &mut self.session else { return Ok(()); }; - let folder = path.rsplit_once('/').map_or("", |(folder, _)| folder); - let space = session.space; - if session.tabs[session.tab].path == path { - session.tabs = library.tabs(folder); - session.tab = session - .tabs - .iter() - .position(|tab| tab.path == path) - .ok_or("The section is not in its notebook")?; - session.library = library; - } else { - // Its file moved: the replica it holds reopens on the file's new path. - self.persist()?; - if let Some(session) = self.session.take() { - session.section.close()?; - } - let section = library.open(path, notify(self.proxy.clone()))?; - let (session, page) = read_session(section, library, path.to_owned(), Some(space))?; - self.session = Some(session); - let response = self.view.refresh(page)?; - self.respond(response); + if !shown(&session.library) { + return Ok(()); } + session.follow(library, path)?; + self.title(); self.save_settings(); Ok(()) } diff --git a/crates/snowbound/src/manage.rs b/crates/snowbound/src/manage.rs index 7fd0494925836d11cc8a218120ff9c7fe934531f..d283635230e43072334fa9d6e60a2cdae22046cf 100644 --- a/crates/snowbound/src/manage.rs +++ b/crates/snowbound/src/manage.rs @@ -111,6 +111,63 @@ fn unused(taken: &[String], base: &str, numbered: bool) -> String { .expect("Some number is free") } +fn rename( + notebook: &mut Notebook, + library: &Library, + path: &str, + name: &str, + request: Option<&([u8; 16], Arc<()>)>, +) -> Result> { + let current = request + .and_then(|(identity, _)| { + notebook + .catalog() + .sections() + .find(|section| §ion.file_id == identity) + .map(|section| section.path.clone()) + .or_else(|| { + crate::library::folders(notebook.catalog(), |_| true) + .into_iter() + .find(|folder| { + folder + .toc + .as_ref() + .is_some_and(|toc| &toc.file_id == identity) + }) + .map(|folder| folder.path.clone()) + }) + }) + .unwrap_or_else(|| path.to_owned()); + let latest = request.is_none_or(|(identity, request)| { + library + .renames + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .get(identity) + .is_some_and(|(_, pending)| Arc::ptr_eq(pending, request)) + }); + if latest && crate::library::entry_name(¤t) != name { + Ok(notebook.rename(¤t, name)?) + } else { + Ok(current) + } +} + +fn finish_rename(library: &Library, request: Option<([u8; 16], Arc<()>)>) { + if let Some((identity, request)) = request { + let mut renames = library + .renames + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + if renames + .get(&identity) + .is_some_and(|(_, pending)| Arc::ptr_eq(pending, &request)) + { + renames.remove(&identity); + } + } +} + impl State { /// A page OneNote 2010 would create now, titled `title` in the Default font's face, with /// its date and time. @@ -781,7 +838,24 @@ impl State { .filter(|session| Arc::ptr_eq(&session.library, &library)) .map(|session| session.tabs[session.tab].path.clone()); let notify = notify(self.proxy.clone()); - self.load_async(move || async move { + let (proxy, requested) = (self.proxy.clone(), self.loading); + let showing = self.switching.is_none() && self.opening.is_none(); + let structure = Arc::clone(&library.structure); + let pending = match &change { + Structure::Rename { path, name } => library.entry_identity(path).map(|identity| { + let request = Arc::new(()); + library + .renames + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .insert(identity, (name.clone(), Arc::clone(&request))); + (identity, request) + }), + _ => None, + }; + let renaming = Arc::clone(&library); + let request = pending.clone(); + let read = move || async move { // A section kept open would hold a file the change moves. library.close_kept_async().await?; let mut notebook = library.reopen()?; @@ -817,7 +891,7 @@ impl State { (current, None) } Structure::Rename { path, name } => { - let renamed = notebook.rename(&path, &name)?; + let renamed = rename(&mut notebook, &library, &path, &name, request.as_ref())?; ( current.map(|current| follow(¤t, &path, &renamed)), None, @@ -892,8 +966,7 @@ impl State { let Some(path) = shown(&library, open) else { return Ok(Loaded::Library(library, None)); }; - // The open section, perhaps renamed or moved, stays open: its replica is this - // thread's to reopen only where no section holds it. + // The shown section keeps its replica when its file moves. if followed.as_deref() == Some(path.as_str()) { return Ok(Loaded::Library(library, Some(path))); } @@ -904,6 +977,75 @@ impl State { } else { Loaded::Section(Box::new(session), page) }) + }; + crate::spawn_async(move || async move { + let _changing = structure.lock().await; + let loaded: Result> = read().await; + let loaded = loaded.map_err(|error| crate::plain(&*error, "page")); + let _ = proxy.send_event(crate::UserEvent::Then(Box::new(move |state| { + finish_rename(&renaming, pending); + let loaded = match loaded { + Ok(loaded) => loaded, + Err(error) => { + platform::alert("Couldn't open", &error); + return Ok(()); + } + }; + let library = match &loaded { + Loaded::Library(library, _) => Arc::clone(library), + Loaded::Section(session, _) | Loaded::Created(session, _) => { + Arc::clone(&session.library) + } + _ => unreachable!("A structure change returns its notebook"), + }; + if !state + .notebooks + .iter() + .any(|listed| listed.location == library.location) + { + return Ok(()); + } + state.list(&library); + let current = state + .session + .as_ref() + .filter(|session| session.library.location == library.location) + .and_then(|session| { + session + .library + .section_identity(&session.tabs[session.tab].path) + }); + if let Some(identity) = current { + let path = library.catalog().and_then(|catalog| { + catalog + .sections() + .find(|section| section.file_id == identity) + .map(|section| section.path.clone()) + }); + if let Some(path) = path { + state.adopt(library, Some(&path))?; + } else if requested != state.loading || !showing { + if let Some(path) = library.first_section() { + state.commands.push(Command::OpenSection(library, path)); + } else { + state.adopt(library, None)?; + } + } + } else if state + .sectionless + .as_ref() + .is_some_and(|shown| shown.location == library.location) + { + state.sectionless = Some(library); + } + if showing && requested == state.loading { + match loaded { + Loaded::Library(library, path) => state.adopt(library, path.as_deref())?, + loaded => state.load(move || Ok(loaded)), + } + } + Ok(()) + }))); }); } @@ -1018,6 +1160,88 @@ mod tests { creation.dated(&date, &time).unwrap() } + #[test] + fn repeated_section_renames_follow_identity_and_keep_the_latest_label() { + let temporary = + std::env::temp_dir().join(format!("snowbound-rename-order-{}", std::process::id())); + let _ = notebook::fs::remove_dir_all(&temporary); + notebook::fs::create_dir_all(&temporary).unwrap(); + let root = temporary.join("Notebook"); + let cache = temporary.join("cache"); + let mut notebook = Notebook::create(&root, &cache, Notebook::NEW_COLOR, &dated()).unwrap(); + notebook.create_section("", "Taken", &dated()).unwrap(); + let library = Library::created(root.to_str().unwrap(), notebook, &cache); + let original = "New Section 1.one"; + let identity = library.entry_identity(original).unwrap(); + let pending = |library: &Library, name: &str| { + let request = Arc::new(()); + library + .renames + .lock() + .unwrap() + .insert(identity, (name.to_owned(), Arc::clone(&request))); + (identity, request) + }; + let first = pending(&library, "B"); + let mut notebook = library.reopen().unwrap(); + assert_eq!( + rename(&mut notebook, &library, original, "B", Some(&first)).unwrap(), + "B.one" + ); + let refreshed = library.with(notebook); + let failed = pending(&refreshed, "Taken"); + let mut notebook = refreshed.reopen().unwrap(); + assert!(rename(&mut notebook, &library, original, "Taken", Some(&failed)).is_err()); + let latest = pending(&library, "C"); + finish_rename(&library, Some(first)); + finish_rename(&refreshed, Some(failed)); + assert_eq!(library.label(original, "New Section 1"), "C"); + assert_eq!(refreshed.label("B.one", "B"), "C"); + assert_eq!( + rename(&mut notebook, &library, original, "C", Some(&latest)).unwrap(), + "C.one" + ); + let refreshed = refreshed.with(notebook); + assert_eq!(refreshed.entry_identity("C.one"), Some(identity)); + assert!(!root.join(original).exists()); + assert!(!root.join("B.one").exists()); + assert!(root.join("C.one").exists()); + finish_rename(&library, Some(latest)); + assert!(matches!( + refreshed.label("C.one", "C"), + std::borrow::Cow::Borrowed("C") + )); + + let mut notebook = refreshed.reopen().unwrap(); + for stale_name in ["Old", "C"] { + let stale = pending(&library, stale_name); + let same = pending(&refreshed, "C"); + assert_eq!( + rename(&mut notebook, &library, original, stale_name, Some(&stale)).unwrap(), + "C.one" + ); + finish_rename(&library, Some(stale)); + assert!( + library + .renames + .lock() + .unwrap() + .get(&identity) + .is_some_and(|(_, request)| Arc::ptr_eq(request, &same.1)) + ); + assert_eq!( + rename(&mut notebook, &library, original, "C", Some(&same)).unwrap(), + "C.one" + ); + finish_rename(&refreshed, Some(same)); + } + assert!(library.renames.lock().unwrap().is_empty()); + drop(notebook); + drop(refreshed); + drop(library); + notebook::fs::remove_dir_all(temporary).unwrap(); + } + /// Applies `ops` to the section file at `file` as one edit, as the section thread does, /// and publishes it. fn edit(file: &Path, ops: Vec) { diff --git a/crates/snowbound/src/sidebar.rs b/crates/snowbound/src/sidebar.rs index a5b80aed888fe789ca199c845648aaa8782d35d6..c69bd7c4b3a128bad3be860c0fb0ab7d42af5eb7 100644 --- a/crates/snowbound/src/sidebar.rs +++ b/crates/snowbound/src/sidebar.rs @@ -417,6 +417,7 @@ fn folder( false, ), }; + let name = library.label(§ion.path, &name); gap(ui, tree, &folder.path, Some(§ion.path)); let renaming = renamed(tree, §ion.path); let id = ui.id(("section", notebook, §ion.path)); @@ -463,12 +464,15 @@ fn folder( } let key = library.key(&group.path); let unfolded = !tree.folded.contains(&key); - let name = group.path.rsplit('/').next().unwrap_or_default(); + let name = library.label( + &group.path, + group.path.rsplit('/').next().unwrap_or_default(), + ); gap(ui, tree, &folder.path, Some(&group.path)); let renaming = renamed(tree, &group.path); let id = ui.id(("group", notebook, &group.path)); let row = Row { - label: name, + label: &name, icon: Leading::Icon(art::SECTION_GROUP), depth, dim: false, -- 2.54.0