From 9361b752ce57b06d0f55af85dc5f515f794ea8f2 Mon Sep 17 00:00:00 2001 From: clover caruso Date: Fri, 2 Oct 2026 21:42:58 -0700 Subject: [PATCH] fix: Back and Forward find pages by identity, and alerts never show raw I/O errors History entries name the section by file identity and the page by its own identity, so a section renamed, moved into a group or reordered, and a page moved to another section, still resolve. Entries that no longer resolve (page or section deleted, notebook closed) are dropped and skipped silently; Back and Forward are disabled when nothing valid remains. Forward after renaming the target section used to alert "Couldn't open / entity not found". File and network failures in alerts now read in plain words ("This page was moved or deleted.") through crate::plain. Assisted-by: claude-opus-5.5 --- crates/canvas/src/search.rs | 3 + crates/snowbound/src/commands.rs | 5 +- crates/snowbound/src/desktop_linux.rs | 9 +- crates/snowbound/src/library.rs | 15 +- crates/snowbound/src/main.rs | 73 ++++- crates/snowbound/src/manage.rs | 12 +- crates/snowbound/src/navigation.rs | 366 +++++++++++++++++++++----- crates/snowbound/src/print.rs | 4 +- crates/snowbound/src/protection.rs | 10 +- crates/snowbound/src/rename.rs | 5 +- crates/snowbound/src/save_as.rs | 4 +- crates/snowbound/src/tags.rs | 5 +- crates/snowbound/src/unpack.rs | 2 +- crates/snowbound/src/web.rs | 2 +- crates/snowbound/tests/replay.rs | 98 +++++++ 15 files changed, 515 insertions(+), 98 deletions(-) diff --git a/crates/canvas/src/search.rs b/crates/canvas/src/search.rs index fb8fea6dd31c2f4eea7a10202665964d48da79e6..8a127fab4a9ea754dfd34c6ea2463e90b1e13c0e 100644 --- a/crates/canvas/src/search.rs +++ b/crates/canvas/src/search.rs @@ -285,6 +285,8 @@ pub struct Entry { /// The section's key, which the host chooses. pub section: String, pub space: ExGuid, + /// The page's own identity, which it keeps moving to another section. + pub identity: Option<[u8; 16]>, pub title: String, /// When the page last changed, in any unit that orders. pub modified: u64, @@ -301,6 +303,7 @@ impl Entry { Self { section: section.to_owned(), space, + identity: page.identity, folded_title: fold(&page.title), folded_text: fold(&text), title: page.title.clone(), diff --git a/crates/snowbound/src/commands.rs b/crates/snowbound/src/commands.rs index 7e7ba93609dd97e7b2945770019649fac29663da..11ceec71783f2a0b116ff5a8afcf7c0e059790d9 100644 --- a/crates/snowbound/src/commands.rs +++ b/crates/snowbound/src/commands.rs @@ -1234,8 +1234,9 @@ impl State { Id::Copy => enabled(selected), Id::Paste => enabled(text && !field), Id::SelectAll => enabled(field || page), - Id::Back => enabled(!modal && self.trail.open()[0]), - Id::Forward => enabled(!modal && self.trail.open()[1]), + Id::Back | Id::Forward => { + enabled(!modal && self.can_travel()[usize::from(id == Id::Forward)]) + } Id::ZoomIn | Id::ZoomOut | Id::ActualSize => enabled(page), Id::Sidebar => Status { enabled: !welcome && !modal, diff --git a/crates/snowbound/src/desktop_linux.rs b/crates/snowbound/src/desktop_linux.rs index 074a26effd6e809e1e51c916076c009f76f9b9cc..bccf670c84739a8195ed37a8a55b8d9a77977b78 100644 --- a/crates/snowbound/src/desktop_linux.rs +++ b/crates/snowbound/src/desktop_linux.rs @@ -435,7 +435,9 @@ pub fn install() { "To uninstall it, open Options from the notebook menu. Uninstall is under Updates.", ); } - Err(error) => crate::platform::alert("Couldn't install Snowbound", &error.to_string()), + Err(error) => { + crate::platform::alert("Couldn't install Snowbound", &crate::plain(&error, "file")) + } } } @@ -525,7 +527,10 @@ fn remove() { } refresh_caches(&data); match failed { - Some(error) => crate::platform::alert("Couldn't uninstall Snowbound", &error.to_string()), + Some(error) => crate::platform::alert( + "Couldn't uninstall Snowbound", + &crate::plain(&error, "file"), + ), None => *INSTALLED.lock().unwrap() = Installed::No, } } diff --git a/crates/snowbound/src/library.rs b/crates/snowbound/src/library.rs index cbbb25997a97fc1ffd889f9cf4d2aa2bdd1cdf9c..5617e18f523e22fcd93735808b3b3c3072a37bd6 100644 --- a/crates/snowbound/src/library.rs +++ b/crates/snowbound/src/library.rs @@ -409,7 +409,10 @@ impl Library { tag_art: Mutex::new(Arc::new( notebook.as_ref().map(read_tag_art).unwrap_or_default(), )), - notebook: notebook.map(Some).map_err(|error| error.to_string()), + notebook: notebook.map(Some).map_err(|error| { + eprintln!("{location}: {error}"); + crate::plain(&error, "notebook") + }), notice, ..Self::new(location, name, cache) } @@ -544,13 +547,7 @@ impl Library { .reopen() .and_then(|notebook| Ok(notebook.rename_folder(name)?)); if let Err(error) = renamed { - let kind = match error.downcast_ref::() { - Some(notebook::Error::Io(error) | notebook::Error::RemoteIo(error)) => { - Some(error.kind()) - } - _ => error.downcast_ref::().map(io::Error::kind), - }; - return Err(match kind { + return Err(match crate::io_kind(&*error) { Some(io::ErrorKind::WouldBlock | io::ErrorKind::ResourceBusy) => { "Another computer is saving to this notebook. Try again in a moment.".into() } @@ -564,7 +561,7 @@ impl Library { Some(io::ErrorKind::InvalidInput) => "A folder name can’t contain \\ / : * ? \" \ < > | or end with a dot or space." .into(), - _ => error.to_string(), + _ => crate::plain(&*error, "folder"), }); } // The folder's own name shows now. diff --git a/crates/snowbound/src/main.rs b/crates/snowbound/src/main.rs index 999a1d477d31e5a84ea74a89d7074be7c4b7d6ec..8639fbd391ea73a6506ef14af8660a3fce4256e5 100644 --- a/crates/snowbound/src/main.rs +++ b/crates/snowbound/src/main.rs @@ -3489,7 +3489,11 @@ impl State { since: Instant::now(), }))) }); - let _ = sender.send((id, laid.map_err(|error| error.to_string()))); + let laid = laid.map_err(|error| { + eprintln!("{error}"); + plain(&*error, "page") + }); + let _ = sender.send((id, laid)); redraw.wake(); }); } @@ -3508,7 +3512,6 @@ impl State { if id == self.loading { self.switching = None; } - eprintln!("{error}"); platform::alert("Couldn't open", &error); } } @@ -5766,6 +5769,51 @@ fn divider(ui: &mut Ui, theme: &Theme) { ); } +/// The kind of the file or network failure behind `error`, if one is. +pub fn io_kind(error: &(dyn Error + 'static)) -> Option { + let mut next = Some(error); + while let Some(error) = next { + if let Some(error) = error.downcast_ref::() { + return Some(error.kind()); + } + // Transparent variants forward `source` past the I/O error they hold. + if let Some(notebook::Error::Io(error) | notebook::Error::RemoteIo(error)) = + error.downcast_ref() + { + return Some(error.kind()); + } + next = error.source(); + } + None +} + +/// `error` as an alert tells it about `thing`: a file or network failure in plain words, +/// never the system's; any other error as it reads. +pub fn plain(error: &(dyn Error + 'static), thing: &str) -> String { + use std::io::ErrorKind::*; + match io_kind(error) { + None | Some(Other) => error.to_string(), + Some(NotFound) => format!("This {thing} was moved or deleted."), + Some(PermissionDenied | ReadOnlyFilesystem) => { + format!("You don't have permission to use this {thing}.") + } + Some(StorageFull | QuotaExceeded) => { + "The disk is full. Free up space, then try again.".into() + } + Some(WouldBlock | ResourceBusy) => { + format!("This {thing} is in use. Try again in a moment.") + } + Some(AlreadyExists) => { + "Something with that name is already there. Choose another name.".into() + } + Some( + TimedOut | ConnectionRefused | ConnectionReset | ConnectionAborted | NotConnected + | HostUnreachable | NetworkUnreachable | NetworkDown | BrokenPipe, + ) => "The server can't be reached. Check your connection, then try again.".into(), + Some(_) => format!("Something went wrong with this {thing}. Try again."), + } +} + /// The session for `section`, at catalog `path` in `library`, showing `space` or its first /// page, and that page. fn read_session( @@ -6636,6 +6684,27 @@ mod tests { use super::*; use canvas::document::TextPosition; + /// Alerts tell a file or network failure in plain words, however deep it is wrapped, + /// and pass the app's own messages on. + #[test] + fn alerts_never_show_the_systems_words() { + use std::io::{Error as Io, ErrorKind}; + let missing: Box = Box::new(Io::from(ErrorKind::NotFound)); + assert_eq!(plain(&*missing, "page"), "This page was moved or deleted."); + let wrapped: Box = Box::new(notebook::Error::Io(Io::from_raw_os_error(2))); + assert_eq!(plain(&*wrapped, "page"), "This page was moved or deleted."); + let discovered = notebook::discover::Error::Io { + path: "/a".into(), + error: Io::from(ErrorKind::TimedOut), + }; + assert!(plain(&discovered, "notebook").starts_with("The server can't be reached.")); + let refused: Box = "The section is password protected".into(); + assert_eq!( + plain(&*refused, "page"), + "The section is password protected" + ); + } + /// A notebook opened from the desktop is listed once and shown first, whether it was /// listed already or not. #[test] diff --git a/crates/snowbound/src/manage.rs b/crates/snowbound/src/manage.rs index 790e4595f29cdd3cb0c5944d99f0276cc622702d..61d8eae2d3c55b88d68784e96b00c2e6b09bd6e7 100644 --- a/crates/snowbound/src/manage.rs +++ b/crates/snowbound/src/manage.rs @@ -554,7 +554,10 @@ impl State { None => Ok(()), }); if let Err(error) = closed { - return platform::alert("Couldn't rename the folder", &error.to_string()); + return platform::alert( + "Couldn't rename the folder", + &crate::plain(&*error, "section"), + ); } } let showing = shown.is_some() || self.sectionless.as_ref().is_some_and(ours); @@ -584,7 +587,7 @@ impl State { }); match colored { Ok(colored) => reopened = colored, - Err(error) => problem = Some(error.to_string()), + Err(error) => problem = Some(crate::plain(&*error, "notebook")), } } let reopened = Arc::new(reopened); @@ -651,7 +654,10 @@ impl State { Structure::NewSection { .. } => match self.dated_page(None) { Ok(page) => Some(page), Err(error) => { - return platform::alert("Couldn't add the section", &error.to_string()); + return platform::alert( + "Couldn't add the section", + &crate::plain(&*error, "section"), + ); } }, _ => None, diff --git a/crates/snowbound/src/navigation.rs b/crates/snowbound/src/navigation.rs index b37c3f008bc80dff941c1e8fb4aa0b17e591271e..c77ed6795b6532f30ef044ca6d1a984b8c636493 100644 --- a/crates/snowbound/src/navigation.rs +++ b/crates/snowbound/src/navigation.rs @@ -15,12 +15,36 @@ pub struct Place { pub page: ExGuid, } +/// A page visited, as Back and Forward find it again after its section is renamed or moved: +/// its notebook's location, its section's file identity, and the page within it, by space +/// and by the identity it keeps moving to another section. +#[derive(Clone, Debug, PartialEq)] +pub struct Visit { + notebook: String, + section: [u8; 16], + space: ExGuid, + page: Option<[u8; 16]>, + /// Whether the section was in the recycle bin, where a deleted section keeps its identity. + binned: bool, +} + +impl Visit { + /// Whether `other` is this page, wherever it has moved since. + fn same(&self, other: &Visit) -> bool { + self.notebook == other.notebook + && match (self.page, other.page) { + (Some(page), Some(other)) => page == other, + _ => self.section == other.section && self.space == other.space, + } + } +} + /// Pages visited before and after the one shown, and those shown lately. #[derive(Default)] pub struct Trail { - back: Vec, - here: Option, - forward: Vec, + back: Vec, + here: Option, + forward: Vec, /// Each page shown lately once, latest first, kept between launches. pub recent: Vec, } @@ -33,25 +57,33 @@ const RECENT: usize = 8; impl Trail { /// Follows the notebook at `from` to `to`, where it moved. pub fn moved(&mut self, from: &str, to: &str) { - let places = (self.back.iter_mut()) + let visits = (self.back.iter_mut()) .chain(&mut self.here) .chain(&mut self.forward) - .chain(&mut self.recent); - for place in places.filter(|place| place.notebook == from) { - place.notebook = to.to_owned(); + .map(|visit| &mut visit.notebook); + let places = self.recent.iter_mut().map(|place| &mut place.notebook); + for notebook in visits.chain(places).filter(|notebook| *notebook == from) { + *notebook = to.to_owned(); } } - /// Notes `place` shown. Arriving anywhere but where Back or Forward went drops the pages - /// ahead, as a browser does. - pub fn visit(&mut self, place: Place) { + /// Notes `place` shown, as the recent pages list it. + pub fn remember(&mut self, place: Place) { self.recent.retain(|recent| *recent != place); - self.recent.insert(0, place.clone()); + self.recent.insert(0, place); self.recent.truncate(RECENT); - if self.here.as_ref() == Some(&place) { + } + + /// Notes `visit` shown. Arriving anywhere but where Back or Forward went drops the pages + /// ahead, as a browser does. + pub fn visit(&mut self, visit: Visit) { + if let Some(here) = &mut self.here + && here.same(&visit) + { + *here = visit; return; } - if let Some(here) = self.here.replace(place) { + if let Some(here) = self.here.replace(visit) { self.back.push(here); if self.back.len() > KEPT { self.back.remove(0); @@ -60,24 +92,26 @@ impl Trail { self.forward.clear(); } - /// Steps back, or `forward`, to the nearest place `exists` accepts, dropping those it - /// refuses; the place to show. - pub fn step(&mut self, forward: bool, exists: impl Fn(&Place) -> bool) -> Option { + /// Steps back, or `forward`, to the nearest place `exists` accepts; the place to show. + /// Places it refuses, on either side, are dropped. + pub fn step(&mut self, forward: bool, exists: impl Fn(&Visit) -> bool) -> Option { + self.back.retain(&exists); + self.forward.retain(&exists); let (from, to) = if forward { (&mut self.forward, &mut self.back) } else { (&mut self.back, &mut self.forward) }; - let place = std::iter::from_fn(|| from.pop()).find(|place| exists(place))?; - if let Some(here) = self.here.replace(place.clone()) { + let visit = from.pop()?; + if let Some(here) = self.here.replace(visit.clone()) { to.push(here); } - Some(place) + Some(visit) } - /// Whether Back and Forward have somewhere to go. - pub fn open(&self) -> [bool; 2] { - [!self.back.is_empty(), !self.forward.is_empty()] + /// Whether Back and Forward have somewhere `exists` accepts to go. + pub fn open(&self, exists: impl Fn(&Visit) -> bool) -> [bool; 2] { + [&self.back, &self.forward].map(|side| side.iter().rev().any(&exists)) } /// Drops the recent pages whose section `listed` refuses, and those whose section `index` @@ -98,24 +132,90 @@ impl Trail { } } +/// Where `visit`'s page is in `notebooks`, as `index` has read them and `open` tells whether +/// the section open, by key, lists a page: the notebook, the section's catalog path and the +/// page. A page gone from its section is followed by its identity; none once it is nowhere. +fn find( + visit: &Visit, + notebooks: &[Arc], + open: impl Fn(&str, ExGuid) -> Option, + index: &Index, +) -> Option<(Arc, String, ExGuid)> { + let library = (notebooks.iter()).find(|library| library.location == visit.notebook)?; + let path = match &library.notebook { + Ok(Some(notebook)) => (crate::library::folders(notebook.catalog(), |_| true).into_iter()) + .flat_map(|folder| &folder.sections) + .find(|section| section.file_id == visit.section) + .map(|section| section.path.clone()), + Ok(None) => Some(library.location.clone()), + Err(_) => None, + } + .filter(|path| crate::recycle::binned(path) == visit.binned); + if let Some(path) = path { + let key = library.key(&path); + // A section the index has yet to read is taken to hold the page. + let listed = open(&key, visit.space).unwrap_or_else(|| { + index.get(&key, visit.space).is_some() + || !index.entries().iter().any(|entry| entry.section == key) + }); + if listed { + return Some((Arc::clone(library), path, visit.space)); + } + } + let page = visit.page?; + index.entries().iter().find_map(|entry| { + let (location, path) = entry.section.split_once('\n')?; + (entry.identity == Some(page) && location == visit.notebook) + .then(|| (Arc::clone(library), path.to_owned(), entry.space)) + }) +} + impl State { /// Notes the page just shown for Back and Forward, and among the recent pages. pub(crate) fn visited(&mut self) { let Some(session) = &self.session else { return; }; + let path = &session.tabs[session.tab].path; let place = Place { notebook: session.library.location.clone(), - section: session.tabs[session.tab].path.clone(), + section: path.clone(), page: session.space, }; + if let Ok(section) = session.section.identity() { + self.trail.visit(Visit { + notebook: place.notebook.clone(), + section, + space: session.space, + page: self.view.editor.identity(), + binned: crate::recycle::binned(path), + }); + } let moved = self.trail.recent.first() != Some(&place); - self.trail.visit(place); + self.trail.remember(place); if moved { self.save_settings(); } } + /// Where `visit`'s page is now, as `find` follows it. + fn find(&self, visit: &Visit) -> Option<(Arc, String, ExGuid)> { + let lists = |key: &str, page| { + let session = self + .session + .as_ref() + .filter(|session| session.key() == key)?; + Some(session.pages.iter().any(|(space, ..)| *space == page)) + }; + let index = (self.search.index.lock()).unwrap_or_else(|poison| poison.into_inner()); + find(visit, &self.notebooks, lists, &index) + } + + /// Whether Back and Forward have a page to go to. + pub(crate) fn can_travel(&self) -> [bool; 2] { + self.trail.open(|visit| self.find(visit).is_some()) + } + /// Whether `library`'s section at `path` is the one open. pub(crate) fn open(&self, library: &Library, path: &str) -> bool { self.session.as_ref().is_some_and(|session| { @@ -134,28 +234,15 @@ impl State { } } - /// Shows the page visited before the one shown, or after it going `forward`. A page - /// deleted from the open section since is passed over. + /// Shows the page visited before the one shown, or after it going `forward`, wherever it + /// went since. Pages since deleted, and those of notebooks since closed, are passed over. pub(crate) fn travel(&mut self, forward: bool) { - let session = self.session.as_ref(); - let exists = |place: &Place| { - session.is_none_or(|session| { - session.library.location != place.notebook - || session.tabs[session.tab].path != place.section - || session.pages.iter().any(|(page, ..)| *page == place.page) - }) - }; - let Some(place) = self.trail.step(forward, exists) else { - return; - }; - let Some(library) = self - .notebooks - .iter() - .find(|library| library.location == place.notebook) - else { - return; - }; - self.go(Arc::clone(library), place.section, place.page); + let mut trail = std::mem::take(&mut self.trail); + let visit = trail.step(forward, |visit| self.find(visit).is_some()); + self.trail = trail; + if let Some((library, path, space)) = visit.and_then(|visit| self.find(&visit)) { + self.go(library, path, space); + } } } @@ -240,53 +327,95 @@ mod tests { assert_eq!(stroke(&mut swipe, 200.0, 0.0, false, 4000), None); } + fn visit(section: u8, page: u32) -> Visit { + let mut identity = [0; 16]; + identity[0] = section; + identity[1..5].copy_from_slice(&page.to_le_bytes()); + Visit { + notebook: "/notebooks/Personal".into(), + section: [section; 16], + space: ExGuid { + guid: [7; 16], + n: page, + }, + page: Some(identity), + binned: false, + } + } + #[test] fn back_and_forward_retrace_visits_across_sections() { let mut trail = Trail::default(); - assert_eq!(trail.open(), [false, false]); - for (section, page) in [("A.one", 1), ("A.one", 2), ("B.one", 1)] { - trail.visit(place(section, page)); + let all = |_: &Visit| true; + assert_eq!(trail.open(all), [false, false]); + for (section, page) in [(1, 1), (1, 2), (2, 1)] { + trail.visit(visit(section, page)); } // Showing the same page again, as a reload does, is no visit. - trail.visit(place("B.one", 1)); - let all = |_: &Place| true; - assert_eq!(trail.step(false, all), Some(place("A.one", 2))); + trail.visit(visit(2, 1)); + assert_eq!(trail.step(false, all), Some(visit(1, 2))); // Arriving where Back went is not a new visit. - trail.visit(place("A.one", 2)); - assert_eq!(trail.open(), [true, true]); - assert_eq!(trail.step(false, all), Some(place("A.one", 1))); + trail.visit(visit(1, 2)); + assert_eq!(trail.open(all), [true, true]); + assert_eq!(trail.step(false, all), Some(visit(1, 1))); assert_eq!(trail.step(false, all), None); - assert_eq!(trail.step(true, all), Some(place("A.one", 2))); - assert_eq!(trail.step(true, all), Some(place("B.one", 1))); + assert_eq!(trail.step(true, all), Some(visit(1, 2))); + assert_eq!(trail.step(true, all), Some(visit(2, 1))); assert_eq!(trail.step(true, all), None); // A page opened after going back drops the pages ahead. trail.step(false, all); - trail.visit(place("C.one", 5)); - assert_eq!(trail.open(), [true, false]); - assert_eq!(trail.step(false, all), Some(place("A.one", 2))); + trail.visit(visit(3, 5)); + assert_eq!(trail.open(all), [true, false]); + assert_eq!(trail.step(false, all), Some(visit(1, 2))); } #[test] - fn back_passes_over_pages_since_deleted() { + fn back_and_forward_pass_over_pages_gone_and_go_dark_without_any() { let mut trail = Trail::default(); - for page in 1..=3 { - trail.visit(place("A.one", page)); + for page in 1..=4 { + trail.visit(visit(1, page)); } - let kept = |place: &Place| place.page.n != 2; - assert_eq!(trail.step(false, kept), Some(place("A.one", 1))); - assert_eq!(trail.step(true, kept), Some(place("A.one", 3))); - assert_eq!(trail.open(), [true, false]); + let all = |_: &Visit| true; + trail.step(false, all); + // Pages 1 and 2 behind, 4 ahead; 2 and 4 are gone since. + let kept = |visit: &Visit| ![2, 4].contains(&visit.space.n); + assert_eq!(trail.open(kept), [true, false]); + assert_eq!(trail.step(true, kept), None); + assert_eq!(trail.step(false, kept), Some(visit(1, 1))); + // What was passed over is gone for good. + assert_eq!(trail.step(true, all), Some(visit(1, 3))); + assert_eq!(trail.open(all), [true, false]); + } + + #[test] + fn arriving_at_a_page_moved_since_is_no_new_visit() { + let mut trail = Trail::default(); + for page in 1..=2 { + trail.visit(visit(1, page)); + } + let all = |_: &Visit| true; + trail.step(false, all); + // Page 1 now shows from section 2, under a new space, as Back found it. + let moved = Visit { + section: [2; 16], + space: visit(2, 9).space, + ..visit(1, 1) + }; + trail.visit(moved.clone()); + assert_eq!(trail.open(all), [false, true]); + assert_eq!(trail.step(true, all), Some(visit(1, 2))); + assert_eq!(trail.step(false, all), Some(moved)); } #[test] fn recent_pages_are_kept_once_latest_first_and_bounded() { let mut trail = Trail::default(); for page in [1, 2, 1] { - trail.visit(place("A.one", page)); + trail.remember(place("A.one", page)); } assert_eq!(trail.recent, [place("A.one", 1), place("A.one", 2)]); for page in 0..20 { - trail.visit(place("B.one", page)); + trail.remember(place("B.one", page)); } assert_eq!(trail.recent.len(), RECENT); assert_eq!(trail.recent[0], place("B.one", 19)); @@ -322,7 +451,7 @@ mod tests { place("A.one", 2), kept.clone(), ] { - trail.visit(gone); + trail.remember(gone); } let listed = |place: &Place| place.section != "C.one"; assert!(trail.prune(&index, listed)); @@ -334,7 +463,7 @@ mod tests { fn back_keeps_its_last_hundred_places() { let mut trail = Trail::default(); for page in 0..150 { - trail.visit(place("A.one", page)); + trail.visit(visit(1, page)); } let mut steps = 0; while trail.step(false, |_| true).is_some() { @@ -342,4 +471,101 @@ mod tests { } assert_eq!(steps, KEPT); } + + /// Back and Forward find a page by its section's identity and its own, in the cases the + /// path it was visited at no longer opens: the section renamed, moved into a group or + /// deleted, the page moved or deleted, the notebook closed. + #[test] + fn visits_are_found_wherever_their_section_and_page_went() { + use notebook::session::Notebook; + use onestore::PageCreation; + let temporary = + std::env::temp_dir().join(format!("snowbound-visits-{}", std::process::id())); + let _ = notebook::fs::remove_dir_all(&temporary); + notebook::fs::create_dir_all(&temporary).unwrap(); + let root = temporary.join("Visited"); + let location = root.to_str().unwrap(); + let cache = temporary.join("cache"); + let dated = || PageCreation::new(None, Some(""), "Author").unwrap(); + let mut notebook = + Notebook::create(location, &cache, Notebook::NEW_COLOR, &dated()).unwrap(); + notebook.create_section("", "A", &dated()).unwrap(); + notebook.create_section("", "B", &dated()).unwrap(); + let mut library = Arc::new(Library::created(location, notebook, &cache)); + let a = library.section_identity("A.one").unwrap(); + let page = |n| ExGuid { guid: [9; 16], n }; + let shown = Visit { + notebook: location.to_owned(), + section: a, + space: page(1), + page: Some([5; 16]), + binned: false, + }; + let mut index = Index::default(); + let found = |library: &Arc, index: &Index, open: Option<&str>| { + let lists = |key: &str, space| (Some(key) == open).then_some(space == page(2)); + find(&shown, std::slice::from_ref(library), lists, index) + .map(|(_, path, space)| (path, space.n)) + }; + assert_eq!(found(&library, &index, None), Some(("A.one".into(), 1))); + + let change = |library: &mut Arc, change: &dyn Fn(&mut Notebook)| { + let mut notebook = library.reopen().unwrap(); + change(&mut notebook); + *library = Arc::new(library.with(notebook)); + }; + change(&mut library, &|notebook| { + drop(notebook.rename("A.one", "Renamed").unwrap()) + }); + assert_eq!( + found(&library, &index, None), + Some(("Renamed.one".into(), 1)) + ); + change(&mut library, &|notebook| { + notebook.create_group("", "Group").unwrap(); + notebook.move_entry("Renamed.one", "Group").unwrap(); + }); + let moved = "Group/Renamed.one"; + assert_eq!(found(&library, &index, None), Some((moved.into(), 1))); + + // The open section's own list, and the index's of others, tell a page deleted. + let key = library.key(moved); + let open = Some(key.as_str()); + assert_eq!(found(&library, &index, open), None); + let entry = |path: &str, space, identity| { + let mut page = onestore::page::Page { + title: String::new(), + identity: None, + created: None, + margin_origin: [0.0; 2], + rtl: false, + color: None, + rule_lines: None, + objects: Vec::new(), + definitions: Default::default(), + }; + page.identity = identity; + canvas::search::Entry::new(&library.key(path), space, &page, 0) + }; + index.set(entry(moved, page(2), None)); + assert_eq!(found(&library, &index, None), None); + // A page moved to another section is followed by its identity. + index.set(entry("B.one", page(7), Some([5; 16]))); + assert_eq!(found(&library, &index, None), Some(("B.one".into(), 7))); + assert_eq!(found(&library, &index, open), Some(("B.one".into(), 7))); + + // A section deleted to the recycle bin keeps its identity there. + let index = Index::default(); + change(&mut library, &|notebook| notebook.delete(moved).unwrap()); + let notebooks = [library]; + assert!( + find(&shown, ¬ebooks, |_, _| None, &index).is_none(), + "the section deleted" + ); + assert!( + find(&shown, &[], |_, _| None, &index).is_none(), + "the notebook closed" + ); + notebook::fs::remove_dir_all(&temporary).unwrap(); + } } diff --git a/crates/snowbound/src/print.rs b/crates/snowbound/src/print.rs index 4ad85d94d792d2cda716c191dd89b58eff3855b2..b9beba4d60f3c9465de59f2b39b8a4d4e52ad32e 100644 --- a/crates/snowbound/src/print.rs +++ b/crates/snowbound/src/print.rs @@ -273,7 +273,7 @@ impl State { if go { self.printing.last = Some(setup); if let Err(error) = self.print(setup, export) { - platform::alert("Couldn't print", &error.to_string()); + platform::alert("Couldn't print", &crate::plain(&*error, "page")); } } } @@ -379,7 +379,7 @@ impl State { } else { "Couldn't print" }; - let done = done.map_err(|error| error.to_string()); + let done = done.map_err(|error| crate::plain(&*error, "file")); let _ = proxy.send_event(UserEvent::Then(Box::new(move |state| { match done { Ok(Some(pdf)) => printer::print(&state.window, &pdf, &title), diff --git a/crates/snowbound/src/protection.rs b/crates/snowbound/src/protection.rs index 23a30e49e635cd180b2268358f33464f423c843e..40ffa26631cb33a9c0c50088974158510639e4bd 100644 --- a/crates/snowbound/src/protection.rs +++ b/crates/snowbound/src/protection.rs @@ -540,7 +540,10 @@ impl State { Some(Dialog::Unlock(Zeroizing::default(), true)) } Err(error) => { - crate::platform::alert("Couldn't unlock the section", &error.to_string()); + crate::platform::alert( + "Couldn't unlock the section", + &crate::plain(&error, "section"), + ); None } }, @@ -622,7 +625,10 @@ impl State { Ok(()) }); if let Err(error) = closed { - return crate::platform::alert("Couldn't set the password", &error.to_string()); + return crate::platform::alert( + "Couldn't set the password", + &crate::plain(&*error, "section"), + ); } self.prefetch.forget(&library.key(&path)); } diff --git a/crates/snowbound/src/rename.rs b/crates/snowbound/src/rename.rs index 7779c3fbbc2dffddcde22e876f5b3307cb8153b4..0c64c728319d6b0c2f2ac0724696d42b64510e70 100644 --- a/crates/snowbound/src/rename.rs +++ b/crates/snowbound/src/rename.rs @@ -115,7 +115,10 @@ impl State { } Target::Page(space) => { if let Err(error) = self.retitle(space, name) { - crate::platform::alert("Couldn't rename the page", &error.to_string()); + crate::platform::alert( + "Couldn't rename the page", + &crate::plain(&*error, "page"), + ); } } } diff --git a/crates/snowbound/src/save_as.rs b/crates/snowbound/src/save_as.rs index 61b9c348d0be8f342078e972a37e735c80614ff9..e98f4c11226b869c7c1cef62d554e0cabd1cbb87 100644 --- a/crates/snowbound/src/save_as.rs +++ b/crates/snowbound/src/save_as.rs @@ -144,7 +144,7 @@ impl State { let dialog = self.save_as.take().expect("The dialog is open"); self.ui.close_popup(id()); if go && let Err(error) = self.save(dialog) { - platform::alert("Couldn't save", &error.to_string()); + platform::alert("Couldn't save", &crate::plain(&*error, "file")); } } @@ -213,7 +213,7 @@ impl State { notebook::fs::write(&path, bytes)?; Ok(()) })(); - let written = written.map_err(|error| error.to_string()); + let written = written.map_err(|error| crate::plain(&*error, "file")); let _ = proxy.send_event(UserEvent::Then(Box::new(move |_| { written.map_err(|error| { platform::alert("Couldn't save", &error); diff --git a/crates/snowbound/src/tags.rs b/crates/snowbound/src/tags.rs index 287c23c41ec8f0afc8484465fe5ffb7a7e7b8e54..2eb7950e3f669491f2f6c9a3454e41f0b02e9301 100644 --- a/crates/snowbound/src/tags.rs +++ b/crates/snowbound/src/tags.rs @@ -621,7 +621,10 @@ impl State { } }); if let Err(error) = written { - return platform::alert("Couldn't keep the picture", &error.to_string()); + return platform::alert( + "Couldn't keep the picture", + &crate::plain(&error, "picture"), + ); } tag.art = Some(art); if tag.shape == 0 { diff --git a/crates/snowbound/src/unpack.rs b/crates/snowbound/src/unpack.rs index 8fedad8d7051e6fa1208afb1922d539b609b2cb9..ea21c94e1fb1e9a6bf4746a374327c74b1f994db 100644 --- a/crates/snowbound/src/unpack.rs +++ b/crates/snowbound/src/unpack.rs @@ -233,7 +233,7 @@ impl State { } Ok(location) })() - .map_err(|error| error.to_string()); + .map_err(|error| crate::plain(&*error, "package")); let _ = proxy.send_event(UserEvent::Then(Box::new(move |state: &mut State| { match unpacked { Ok(location) => state.open_notebook(location, None), diff --git a/crates/snowbound/src/web.rs b/crates/snowbound/src/web.rs index a34cc7abda43caf72f320b9d2a9da542644d3d0b..2581e0ce290e64592bbaaf6b085dae4e17322664 100644 --- a/crates/snowbound/src/web.rs +++ b/crates/snowbound/src/web.rs @@ -612,7 +612,7 @@ pub fn open_file(path: &Path) { &bytes, "application/octet-stream", ), - Err(error) => alert("Couldn't open the file", &error.to_string()), + Err(error) => alert("Couldn't open the file", &crate::plain(&error, "file")), } } diff --git a/crates/snowbound/tests/replay.rs b/crates/snowbound/tests/replay.rs index d756ac71451bb32acf1d80db881fa6a5da8748b4..cad9fc302f62c992210107a05471808f27ad4203 100644 --- a/crates/snowbound/tests/replay.rs +++ b/crates/snowbound/tests/replay.rs @@ -274,6 +274,104 @@ fn undo_walks_back_through_new_pages_and_their_titles() { } } +/// Whether `tree`'s toolbar offers Back and Forward. +fn travels(tree: &str) -> [bool; 2] { + ["Back", "Forward"].map(|name| { + tree.lines() + .find(|line| line.trim_start().starts_with(&format!("Button \"{name}\""))) + .is_some_and(|line| !line.ends_with("[disabled]")) + }) +} + +const BACK: [&str; 4] = [ + "modifiers command control", + "key Left", + "modifiers", + "settle", +]; +const FORWARD: [&str; 4] = [ + "modifiers command control", + "key Right", + "modifiers", + "settle", +]; +const NEW_PAGE: [&str; 4] = ["modifiers command", "key n", "modifiers", "settle"]; + +/// Forward reaches a page whose section was renamed after going Back from it, where the +/// path it was visited at no longer opens. +#[test] +fn forward_follows_a_section_renamed_since() { + let scratch = Scratch::new("forward-renamed"); + let notebook = + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus/cross-container/candidate"); + let mut steps = Vec::from(NEW_PAGE); + steps.push("type Two"); + steps.extend(BACK); + // The section's tab menu, Rename. + steps.extend(["move 110 53", "press right", "release right", "settle"]); + steps.extend([ + "key Down", + "key Enter", + "settle", + "type Renamed", + "key Enter", + "settle", + ]); + steps.push("accessibility renamed"); + steps.extend(FORWARD); + steps.push("accessibility forward"); + let [renamed, forward] = replay(&scratch, Some(¬ebook), &steps) + .try_into() + .unwrap(); + assert!(scratch.0.join("notebook/Renamed.one").exists()); + assert_eq!(travels(&renamed), [false, true], "{renamed}"); + let shown = |tree: &str| page_tabs(tree).into_iter().find(|tab| tab.starts_with('*')); + assert_eq!(shown(&forward).as_deref(), Some("*Two"), "{forward}"); + assert!(forward.contains(r#"Tab "Renamed" [selected]"#), "{forward}"); + assert_eq!(travels(&forward), [true, false], "{forward}"); +} + +/// Back passes over a page deleted since it was shown, and goes dark once nothing is left +/// behind. +#[test] +fn back_passes_over_a_page_deleted_since() { + let scratch = Scratch::new("back-deleted"); + let notebook = + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus/cross-container/candidate"); + let mut steps = Vec::new(); + for title in ["type Two", "type Three"] { + steps.extend(NEW_PAGE); + steps.push(title); + } + // Two's tab menu, Delete. + steps.extend([ + "settle", + "move 1003 198", + "press right", + "release right", + "settle", + ]); + steps.extend(["key Down", "key Down", "key Enter", "settle"]); + steps.push("accessibility deleted"); + steps.extend(BACK); + steps.push("accessibility back"); + let [deleted, back] = replay(&scratch, Some(¬ebook), &steps) + .try_into() + .unwrap(); + let shown = |tree: &str| page_tabs(tree).into_iter().find(|tab| tab.starts_with('*')); + assert!( + !page_tabs(&deleted).contains(&"Two".to_owned()), + "{deleted}" + ); + assert_eq!(shown(&deleted).as_deref(), Some("*Three"), "{deleted}"); + assert_eq!( + shown(&back).as_deref(), + Some("*Delete into a table"), + "{back}" + ); + assert_eq!(travels(&back), [false, true], "{back}"); +} + /// The search box's results in `tree`, one a line. fn search_results(tree: &str) -> Vec<&str> { tree.lines() -- 2.54.0