From 1891578f4f06c1aec7b4eb958025288f26a23b81 Mon Sep 17 00:00:00 2001 From: mavrick Date: Sun, 25 Oct 2026 09:24:57 -0500 Subject: [PATCH 1/8] feat(ui): autosave unsaved work and offer it back after a crash While the project has unsaved changes, the desktop app keeps a recovery copy in the app-data folder's recovery/ (every 5 minutes by default, Settings > Autosave, ui.autosave), written atomically on a background thread without layer secrets. Save, New, Open and a clean quit remove it. A copy left at start is offered in a dialog: Recover (opens unsaved under its original file, warning if that file changed since), Discard or Later. Copies from repeated crashes are kept, up to five, and offered newest first. Only the run holding the run marker autosaves, so a second copy can't touch the first one's work. The browser has no store yet. Closes #873. Co-Authored-By: Claude Opus 5.5 --- GEOLIBRE_PARITY.md | 5 +- ROADMAP.md | 6 +- apps/rustgis-web/src/web.rs | 2 + apps/rustgis/src/main.rs | 46 ++- crates/data/src/dirs.rs | 2 +- crates/engine/src/project.rs | 16 +- crates/engine/src/tests.rs | 17 + crates/ui-egui/examples/ui_shot.rs | 6 + crates/ui-egui/src/autosave.rs | 629 +++++++++++++++++++++++++++++ crates/ui-egui/src/lib.rs | 26 ++ crates/ui-egui/src/ribbon.rs | 25 ++ crates/ui-egui/tests/autosave.rs | 327 +++++++++++++++ docs/installation.md | 7 +- docs/user-guide/index.md | 4 +- docs/user-guide/workspace.md | 37 +- 15 files changed, 1138 insertions(+), 17 deletions(-) create mode 100644 crates/ui-egui/src/autosave.rs create mode 100644 crates/ui-egui/tests/autosave.rs diff --git a/GEOLIBRE_PARITY.md b/GEOLIBRE_PARITY.md index a62238d8b..5380a9373 100644 --- a/GEOLIBRE_PARITY.md +++ b/GEOLIBRE_PARITY.md @@ -214,8 +214,9 @@ storymaps.md_ `postMessage` (`apps/rustgis-web/src/embed.rs`); no npm client. - [ ] **Import QGIS (`.qgs/.qgz`) and ArcGIS Pro (`.aprx/.mapx`) projects**. Missing (_Exploratory_). -- [ ] **Autosave, project history, crash recovery, snapshot compare**. Partial: undo and per-layer - archive restore (_Roadmap: solid ground_ for autosave and crash recovery). +- [ ] **Autosave, project history, crash recovery, snapshot compare**. Partial: undo, per-layer + archive restore, and autosave with crash recovery in the desktop app + ([#873](https://github.com/opengeos/RustGIS/issues/873)); the browser has neither yet. - [ ] **Startup preferences and shareable interface settings**. Partial: start-up preferences, Pane Sets and ribbon/QAT customization. - [ ] **Layer metadata with STAC Item export**. Missing (_Roadmap: data at the source_). diff --git a/ROADMAP.md b/ROADMAP.md index db45e11cc..448e7a6ea 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -54,7 +54,8 @@ Correct, robust and recoverable, with the foundations other themes stand on. GDA94↔GDA2020 and OSGB36, and a notice whenever a layer is read without its shift; GDA94 ([#730](https://github.com/opengeos/RustGIS/issues/730)) is one case. _Coordinate first._ - Reloading files changed on disk. -- Autosave and crash recovery: reopen unsaved work after a crash. +- Autosave and crash recovery in the browser: reopen unsaved work after a crash. The desktop app + has them ([#873](https://github.com/opengeos/RustGIS/issues/873)). - Projects that survive a move: data paths kept relative to the project, and Repair Data Source for a file that moved or was renamed. - Signed Windows builds (no SmartScreen warning), then in-place updates; settings an @@ -173,7 +174,8 @@ Open questions rather than plans; discuss before building. - Layered workspace, command registry with undo, project files (`.rgis`), `rustgis-cli`, the web app (WebAssembly, installable with offline start, opened with a project or data by its web address), 18 interface languages; a ribbon laid out in the order of the work (Home, Data, - Style, Edit, Analyze, View, Publish), start-up preferences, Check for Updates. + Style, Edit, Analyze, View, Publish), start-up preferences, Check for Updates, autosave and + crash recovery in the desktop app. - Data: files by their web address; GeoJSON, CSV/TSV (x/y in any EPSG system, or WKT), Excel and OpenDocument spreadsheets, GeoRSS, LandXML, encoded polylines, Shapefile (zipped too, with the coordinate system in its `.prj`), GeoPackage, GeoParquet, FlatGeobuf, KML/KMZ diff --git a/apps/rustgis-web/src/web.rs b/apps/rustgis-web/src/web.rs index 961a4c8b7..74d82b6ed 100644 --- a/apps/rustgis-web/src/web.rs +++ b/apps/rustgis-web/src/web.rs @@ -817,6 +817,8 @@ fn services(inbox: Inbox, font_inbox: Inbox, notes: Notes, ctx: egui::Context, e save_pref: Some(save_pref), // No keychain in the browser: script on the page could read anything stored (#754). secrets: None, + // No autosave in the browser yet: localStorage is too small for a project (#873). + recovery: None, fetch_font: Some(Box::new(move |url: &str| { let (url, inbox, ctx) = (url.to_string(), font_sink.clone(), font_ctx.clone()); wasm_bindgen_futures::spawn_local(async move { diff --git a/apps/rustgis/src/main.rs b/apps/rustgis/src/main.rs index d57c2cd65..4d6ee66e2 100644 --- a/apps/rustgis/src/main.rs +++ b/apps/rustgis/src/main.rs @@ -154,7 +154,11 @@ mod running { use std::path::Path; /// The marker, held while this run lasts. - pub struct Marker(File); + pub struct Marker { + file: File, + /// This run has it to itself: no other copy can take it while this one runs. + locked: bool, + } /// Takes `/running` for this run: the marker (none when another copy holds it or the /// folder can't be written) and whether the previous run didn't close cleanly. @@ -169,8 +173,8 @@ mod running { return (None, false); } }; - match file.try_lock() { - Ok(()) => {} + let locked = match file.try_lock() { + Ok(()) => true, Err(std::fs::TryLockError::WouldBlock) => { log::info!("the run marker is held: another copy of RustGIS is running"); return (None, false); @@ -183,8 +187,9 @@ mod running { // and a marker nobody clears would make every later launch look unclean. Err(std::fs::TryLockError::Error(e)) => { log::warn!("could not lock the run marker ({e}): a second copy running alongside will look like an unclean exit"); + false } - } + }; // A clean exit leaves the marker empty: anything in it (whatever it reads as) means the // last run didn't close, and so does a marker that can't be read. let mut bytes = Vec::new(); @@ -199,13 +204,19 @@ mod running { if let Err(e) = marked { log::warn!("could not mark this run: {e}"); } - (Some(Marker(file)), unclean) + (Some(Marker { file, locked }), unclean) } impl Marker { + /// Whether no other copy can hold the marker while this run lasts: only then is the + /// recovery folder this run's alone. + pub fn locked(&self) -> bool { + self.locked + } + /// A clean exit: the next launch starts as usual. Dropping the file releases the lock. pub fn close(self) { - if let Err(e) = self.0.set_len(0) { + if let Err(e) = self.file.set_len(0) { log::warn!("could not clear the run marker: {e}"); } } @@ -249,10 +260,23 @@ impl eframe::App for App { /// Every way eframe closes the app comes here, Cmd+Q on macOS included, which may not /// return from `run_native`: the run ended cleanly. fn on_exit(&mut self) { + self.0.end_autosave(); running::close(&self.1); } } +/// This run's recovery folder, `recovery` in the app-data folder (see `docs/installation.md`). +fn recovery_store() -> Option> { + let dir = rustgis_engine::data::dirs::app_data_dir()?.join("recovery"); + match rustgis_ui_egui::FolderStore::open(&dir) { + Ok(store) => Some(Box::new(store)), + Err(e) => { + log::warn!("no autosave: {e}"); + None + } + } +} + /// Opens the files named on the command line: a project or a project package as the project, any /// other file as data. Returns what didn't open, one message per file. fn open_files(app: &mut RustGisApp, files: &[&String]) -> Vec { @@ -319,7 +343,13 @@ fn main() -> eframe::Result { } else { RustGisApp::new(Session::new(Project::default())) }; - let mut app = app.with_services(prefs::services()); + let mut services = prefs::services(); + // Autosave, in a recovery folder only the copy holding the run marker uses: a second copy + // running alongside would offer, or overwrite, the first one's unsaved work. + if marker.as_ref().is_some_and(running::Marker::locked) { + services.recovery = recovery_store(); + } + let mut app = app.with_services(services); // Settings › Start-up; `--sample` still opens the sample. let blank = if args.iter().any(|a| a == "--sample") { Some(false) } else { (!files.is_empty()).then_some(true) }; if let Err(e) = app.start(&rustgis_ui_egui::Start { blank, after_crash: unclean, ..Default::default() }) { @@ -403,6 +433,8 @@ mod tests { let _cleanup = TempDir(dir.clone()); let (first, unclean) = running::start(Some(&dir)); assert!(first.is_some() && !unclean, "a first run"); + // Held alone: autosave may use the recovery folder. + assert!(first.as_ref().is_some_and(running::Marker::locked)); // Another copy running alongside can't take it, and doesn't count as a crash. let (second, unclean) = running::start(Some(&dir)); assert!(second.is_none() && !unclean); diff --git a/crates/data/src/dirs.rs b/crates/data/src/dirs.rs index 2142a3392..cd26faaee 100644 --- a/crates/data/src/dirs.rs +++ b/crates/data/src/dirs.rs @@ -48,7 +48,7 @@ pub fn config_dir() -> Option { resolve(Kind::Config, OS, &env) } -/// App data kept on this computer: installed plugins and programs. +/// App data kept on this computer: installed plugins and programs, recovery copies of unsaved work. pub fn app_data_dir() -> Option { resolve(Kind::AppData, OS, &env) } diff --git a/crates/engine/src/project.rs b/crates/engine/src/project.rs index c8358dbaa..088c0e123 100644 --- a/crates/engine/src/project.rs +++ b/crates/engine/src/project.rs @@ -39,7 +39,7 @@ pub fn open_command(path: &str) -> &'static str { /// Format version written to files. pub const FORMAT: u32 = 1; /// Largest project file read (512 MiB). -const MAX_BYTES: u64 = 512 << 20; +pub const MAX_BYTES: u64 = 512 << 20; /// The address a saved file keeps. Only web addresses can contain layer credentials. A tile /// template (`pmtiles:…@archive`, `cog:…@archive`) is judged by its archive, and an address read @@ -676,6 +676,20 @@ impl Project { text.map(|text| (text, n)) } + /// The project as an autosaved recovery copy keeps it: the saved file's text, without + /// passwords, keys, tokens or signatures in layer addresses, from a copy of the project + /// (autosave writes it off the interface thread). Unlike [`Self::saved_text`] it isn't + /// remembered as a file this process wrote, so frequent copies can't push the user's own saves + /// out of the snapshots that restore secrets. Text too large to open again is refused. + pub fn recovery_text(mut self) -> Result { + strip_addresses(self.addresses_mut()); + let text = serde_json::to_string(&self).map_err(|e| e.to_string())?; + if text.len() as u64 > MAX_BYTES { + return Err("the project is too large to keep a recovery copy".into()); + } + Ok(text) + } + fn restore_addresses(&mut self, text: &str) { let id = identity(text); let kept = kept(); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 53b676bbc..8083ae540 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -11144,6 +11144,23 @@ fn database_layers_wait_for_their_connection_login() { assert_eq!(result["failed"], 2); } +/// An autosaved recovery copy (#873) leaves out layer secrets as a save does, from a copy of the +/// project (the open one keeps them), and isn't registered as a file this process wrote: frequent +/// copies don't evict saved snapshots, and opening a copy's text restores no secret. +#[test] +fn recovery_copies_keep_no_layer_tokens_and_restore_none() { + let mut s = world(); + let url = "https://gis.example/arcgis/rest/services/a/MapServer/tile/{z}/{y}/{x}?token=s3cret"; + s.run("map.addTiles", &json!({"url":url,"name":"Secured"})).unwrap(); + let text = s.project.clone().recovery_text().unwrap(); + assert!(!text.contains("s3cret") && text.contains("MapServer/tile/{z}/{y}/{x}")); + assert!(s.map().tiles.iter().any(|t| t.url == url), "the open project keeps its token"); + let mut t = Session::new(Project::default()); + t.run("project.openText", &json!({"text":text})).unwrap(); + assert!(t.map().tiles.iter().any(|t| t.url.contains("MapServer/tile"))); + assert!(!serde_json::to_string(&t.project).unwrap().contains("s3cret")); +} + #[test] fn saved_projects_keep_no_layer_tokens() { let mut s = world(); diff --git a/crates/ui-egui/examples/ui_shot.rs b/crates/ui-egui/examples/ui_shot.rs index 10236b8a4..33bd23e54 100644 --- a/crates/ui-egui/examples/ui_shot.rs +++ b/crates/ui-egui/examples/ui_shot.rs @@ -23,6 +23,8 @@ //! `{"width": w, "height": h}` on the first line sets the size (default //! 1480×900 at 2×; `"dt": seconds` there sets each frame's length, default 0.25, and a //! short one such as 0.016 catches the map mid-motion). The window opens the World Atlas sample. +//! `"recovery": "/abs/dir"` on the first line gives the run that recovery folder, as the desktop +//! app has one (Settings › Autosave; a copy left there is offered by the recovery dialog). //! //! Add `"ribbonSplit": true` to the first line for a compact reusable split-button showcase //! instead of the app. Its "Disable arrows", "Disable enclosing UI", and "Long two-line labels" @@ -130,6 +132,10 @@ fn main() { let ribbon_split = lines.first().and_then(|l| l.get("ribbonSplit")).and_then(Value::as_bool).unwrap_or(false); let mut split_showcase = RibbonSplitShowcase::default(); let mut app = RustGisApp::with_sample(); + if let Some(dir) = lines.first().and_then(|l| l.get("recovery")).and_then(Value::as_str) { + let store = rustgis_ui_egui::FolderStore::open(std::path::Path::new(dir)).expect("recovery folder"); + app = app.with_services(rustgis_ui_egui::Services { recovery: Some(Box::new(store)), ..Default::default() }); + } // One shared wgpu render state: the app's GPU map backend and the harness's renderer must // live on the same device for the map's paint callback to be valid. let rs = { diff --git a/crates/ui-egui/src/autosave.rs b/crates/ui-egui/src/autosave.rs new file mode 100644 index 000000000..025134161 --- /dev/null +++ b/crates/ui-egui/src/autosave.rs @@ -0,0 +1,629 @@ +//! Autosave and crash recovery (#873). While the project has unsaved changes, a recovery copy of +//! it is kept through [`Services::recovery`](crate::Services), rewritten at most once per +//! interval and only when the project changed since the last copy. Saving, starting another +//! project and a clean exit remove it, so a copy left over at start means the last run ended +//! without closing (a crash, a kill, a power cut): the app offers it then ([`offer`]). +//! +//! The desktop app keeps the copy in a folder ([`FolderStore`]); a shell without a store (the +//! browser, for now) has no autosave. + +use egui::vec2; +use rustgis_engine::Project; +use serde::{Deserialize, Serialize}; +use serde_json::{Value, json}; +use web_time::{Duration, Instant, SystemTime, UNIX_EPOCH}; + +use crate::RustGisApp; +use crate::theme::{Tokens, semibold}; + +/// The preference the interval is kept under, in minutes (`0` is off). +pub(crate) const MINUTES_PREF: &str = "autosave_minutes"; +/// The interval until the user picks one. +pub(crate) const DEFAULT_MINUTES: u32 = 5; +/// The longest interval `ui.autosave` takes. +const MAX_MINUTES: u64 = 60; +/// The intervals Settings › Autosave offers (`0` is off). +pub(crate) const CHOICES: [u32; 7] = [0, 1, 2, 5, 10, 15, 30]; + +/// What a recovery copy holds beside the project: what the recovery dialog says about it and +/// where saving the recovered work writes. +#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct Recovery { + /// The project's name. + #[serde(default)] + pub name: String, + /// The file the project was opened from or saved to, if any. + #[serde(default)] + pub path: Option, + /// When the copy was made, in seconds since 1970 (UTC). + #[serde(default)] + pub time: u64, +} + +/// Where a shell keeps the recovery copy. Writing and clearing may happen in the background, but +/// in the order they were asked for: a copy cleared after a save is never written again by an +/// earlier, slower write. +pub trait RecoveryStore { + /// Replaces this run's copy with `project` (a copy of the open one) and `about` it. + fn write(&self, project: Project, about: Recovery); + /// Removes this run's copy (the work was saved, or replaced, or the app closes cleanly). + fn clear(&self); + /// Waits, for a bounded time, until what was asked for has been done: before the app exits. + fn flush(&self); + /// The copy an earlier run left that is offered now: the newest of those waiting to be + /// recovered or discarded (runs that crashed again after Later each leave one). + fn pending(&self) -> Option; + /// How many copies are waiting, the offered one included. + fn waiting(&self) -> usize; + /// The project text of the offered copy. + fn read_pending(&self) -> Result; + /// Forgets the offered copy; the next one waiting is offered. + fn discard_pending(&self); + /// Makes the offered copy this run's copy, once it was recovered: it is kept, unchanged, + /// until the recovered work is saved or changes again. + fn adopt_pending(&self); + /// Why the last copy couldn't be written, once. + fn take_error(&self) -> Option; +} + +/// The app's autosave state. +pub struct AutosaveState { + /// Minutes between copies; `0` is off. + pub(crate) minutes: u32, + /// The session revision the last copy was made at, while one is kept. + written: Option, + /// When the last copy was made. + last: Option, + /// The copy an earlier run left, while the user hasn't recovered or discarded it. + offer: Option, + /// How many more copies wait behind the offered one. + more: usize, + /// Whether the offered copy's file changed on disk after the copy was made. + changed_since: bool, + /// The dialog was put off until the next start (Later, or other copies after a recovery). + later: bool, + /// Why recovering the offered copy failed, shown in the dialog. + offer_error: Option, + /// The last write error told, so a failing folder isn't reported at every interval. + told: Option, +} + +impl Default for AutosaveState { + /// Every few minutes ([`DEFAULT_MINUTES`]), nothing written yet and nothing offered. + fn default() -> Self { + AutosaveState { + minutes: DEFAULT_MINUTES, + written: None, + last: None, + offer: None, + more: 0, + changed_since: false, + later: false, + offer_error: None, + told: None, + } + } +} + +/// Seconds since 1970, now. +fn now_secs() -> u64 { + SystemTime::now().duration_since(UNIX_EPOCH).map_or(0, |d| d.as_secs()) +} + +/// The open project's description for its recovery copy. +fn about(app: &RustGisApp) -> Recovery { + let path = app.session.path.as_ref().map(|p| std::path::absolute(p).unwrap_or_else(|_| p.clone()).display().to_string()); + Recovery { name: app.session.project.name.clone(), path, time: now_secs() } +} + +/// Reads the interval preference and the copy an earlier run left (from [`RustGisApp::with_services`]). +pub(crate) fn load(app: &mut RustGisApp) { + let minutes = app.services.load_pref.as_ref().and_then(|load| load(MINUTES_PREF)).and_then(|v| v.trim().parse::().ok()); + // A stored value out of range (edited by hand) means the default. + app.autosave.minutes = minutes.filter(|m| u64::from(*m) <= MAX_MINUTES).unwrap_or(DEFAULT_MINUTES); + next_offer(app); +} + +/// Offers the next copy waiting in the store, if any. +fn next_offer(app: &mut RustGisApp) { + let store = app.services.recovery.as_ref(); + let pending = store.and_then(|s| s.pending()); + app.autosave.more = store.map_or(0, |s| s.waiting()).saturating_sub(1); + app.autosave.changed_since = pending.as_ref().is_some_and(changed_since); + app.autosave.offer = pending; + app.autosave.offer_error = None; +} + +/// Whether the copy's file was changed on disk after the copy was made. +fn changed_since(r: &Recovery) -> bool { + let Some(path) = &r.path else { return false }; + #[cfg(not(target_arch = "wasm32"))] + { + let modified = std::fs::metadata(path).and_then(|m| m.modified()).ok(); + modified.and_then(|m| m.duration_since(std::time::UNIX_EPOCH).ok()).is_some_and(|d| d.as_secs() > r.time) + } + #[cfg(target_arch = "wasm32")] + { + let _ = path; + false + } +} + +/// Each frame: keeps the recovery copy in step with the project. A copy is made when the project +/// has unsaved changes it doesn't hold yet and the interval has passed since the last one; it is +/// removed once the project has none (saved, or another one started). +pub(crate) fn tick(app: &mut RustGisApp, ctx: &egui::Context) { + let Some(store) = &app.services.recovery else { return }; + if let Some(e) = store.take_error() + && app.autosave.told.as_ref() != Some(&e) + { + app.autosave.told = Some(e.clone()); + let text = crate::i18n::fmt("Autosave couldn't keep a recovery copy: {error}", &[("error", &e)]); + app.session.notify(text, true); + } + if app.autosave.minutes == 0 || !app.session.dirty { + if app.autosave.written.take().is_some() { + store.clear(); + } + return; + } + if app.autosave.written == Some(app.session.revision) { + return; + } + let interval = Duration::from_secs(u64::from(app.autosave.minutes) * 60); + let now = Instant::now(); + if let Some(since) = app.autosave.last.map(|l| now.saturating_duration_since(l)) + && since < interval + { + ctx.request_repaint_after(interval - since); + return; + } + store.write(app.session.project.clone(), about(app)); + app.autosave.written = Some(app.session.revision); + app.autosave.last = Some(now); +} + +/// A clean exit: this run's copy goes, and what was asked of the store is done before the +/// process ends. The copy an earlier run left stays until it is recovered or discarded. +pub(crate) fn end(app: &mut RustGisApp) { + if let Some(store) = &app.services.recovery { + store.clear(); + store.flush(); + } + app.autosave.written = None; +} + +/// `ui.autosave`: sets the interval (`minutes`, 0 to 60; `0` is off) and says what autosave +/// does: the interval, and the copy waiting to be recovered. +pub(crate) fn command(app: &mut RustGisApp, p: &Value) -> Result { + if let Some(m) = p.get("minutes") { + if app.services.recovery.is_none() { + return Err("autosave needs the desktop app".into()); + } + let m = m.as_u64().filter(|m| *m <= MAX_MINUTES).ok_or("minutes is a whole number from 0 (off) to 60")?; + app.autosave.minutes = u32::try_from(m).unwrap_or(DEFAULT_MINUTES); + if let Some(save) = &app.services.save_pref { + save(MINUTES_PREF, &m.to_string()); + } + } + Ok(json!({ + "available": app.services.recovery.is_some(), + "minutes": app.autosave.minutes, + "pending": app.autosave.offer.as_ref().map(|r| json!({"name": r.name, "path": r.path, "time": r.time})), + })) +} + +/// `ui.recover`: opens the copy an earlier run left as the project, unsaved, with its original +/// file (so Save writes there); `{"discard": true}` deletes it instead. +pub(crate) fn recover(app: &mut RustGisApp, p: &Value) -> Result { + let discard = match p.get("discard") { + None | Some(Value::Null) => false, + Some(Value::Bool(b)) => *b, + Some(_) => return Err("discard is true or false".into()), + }; + let none = "there is no unsaved work to recover"; + let (Some(store), Some(offered)) = (&app.services.recovery, app.autosave.offer.clone()) else { return Err(none.into()) }; + if discard { + store.discard_pending(); + next_offer(app); + return Ok(json!({"discarded": true, "waiting": app.autosave.offer.as_ref().map(|_| app.autosave.more + 1).unwrap_or(0)})); + } + // Opening it replaces the open project: never one with work of its own not saved (after + // Later), which it would lose with its recovery copy. + if app.session.dirty { + return Err("the open project has unsaved changes: save them before recovering other work".into()); + } + let text = store.read_pending()?; + app.run("project.openText", json!({"text": text}))?; + app.session.path = offered.path.as_ref().map(std::path::PathBuf::from); + app.session.dirty = true; + // The copy becomes this run's as it is: a crash now loses nothing, and nothing is written. + if let Some(store) = &app.services.recovery { + store.adopt_pending(); + } + app.autosave.written = Some(app.session.revision); + app.autosave.last = Some(Instant::now()); + next_offer(app); + let text = crate::i18n::fmt("Recovered unsaved work in {name}. Save to keep it.", &[("name", &app.session.project.name)]); + app.session.notify(text, false); + // Other copies wait for the next start: recovering one now would replace this work. + let waiting = app.autosave.offer.as_ref().map(|_| app.autosave.more + 1).unwrap_or(0); + if waiting > 0 { + app.autosave.later = true; + let text = + crate::i18n::fmt("Other copies of unsaved work will be offered at the next start ({n}).", &[("n", &waiting.to_string())]); + app.session.notify(text, false); + } + Ok(json!({"name": app.session.project.name, "path": offered.path, "waiting": waiting})) +} + +/// The dialog's first line: what was kept, and how long ago (`then`, seconds since 1970). Whole +/// sentences, one per unit, so each translates as a sentence; only data is filled in. +fn kept_line(name: &str, then: u64) -> String { + let secs = now_secs().saturating_sub(then); + let (template, n) = match secs { + 0..60 => ("RustGIS didn't close properly last time. A copy of {name} with unsaved changes was kept less than a minute ago.", 0), + 60..7_200 => { + ("RustGIS didn't close properly last time. A copy of {name} with unsaved changes was kept {n} minutes ago.", secs / 60) + } + 7_200..172_800 => { + ("RustGIS didn't close properly last time. A copy of {name} with unsaved changes was kept {n} hours ago.", secs / 3_600) + } + _ => ("RustGIS didn't close properly last time. A copy of {name} with unsaved changes was kept {n} days ago.", secs / 86_400), + }; + crate::i18n::fmt(template, &[("name", name), ("n", &n.to_string())]) +} + +/// The recovery dialog, at start while a copy an earlier run left is waiting: Recover, Discard, +/// or Later (it is offered again at the next start). +pub(crate) fn offer(app: &mut RustGisApp, ctx: &egui::Context) { + if app.autosave.later { + return; + } + let Some(r) = app.autosave.offer.clone() else { return }; + let t = Tokens::get(ctx); + let name = if r.name.trim().is_empty() { tl_raw!("Untitled").to_string() } else { r.name.clone() }; + let file = r.path.as_deref().map(|p| std::path::Path::new(p).file_name().map_or(p.to_string(), |n| n.to_string_lossy().into_owned())); + let mut decision = None; + egui::Modal::new(egui::Id::unique("autosave_recover")).show(ctx, |ui| { + ui.set_width(420.0); + ui.label(egui::RichText::new(tl!("Recover unsaved work?")).font(semibold(15.0))); + ui.add_space(4.0); + ui.label(kept_line(&name, r.time)); + ui.add_space(4.0); + match &file { + Some(f) => { + ui.label(crate::i18n::fmt("It opens unsaved; saving replaces {file}.", &[("file", f)])); + if app.autosave.changed_since { + ui.label( + egui::RichText::new(crate::i18n::fmt("{file} has changed since this copy was made.", &[("file", f)])).color(t.red), + ); + } + } + None => { + ui.label(tl!("It was never saved: it opens unsaved, and Save asks where to write it.")); + } + } + if app.autosave.more > 0 { + ui.label(crate::i18n::fmt("Older copies waiting: {n}.", &[("n", &app.autosave.more.to_string())])); + } + if let Some(e) = &app.autosave.offer_error { + ui.add_space(4.0); + ui.label(egui::RichText::new(crate::i18n::fmt("It didn't open: {error}", &[("error", e)])).color(t.red)); + } + ui.add_space(10.0); + ui.with_layout(egui::Layout::right_to_left(egui::Align::Min), |ui| { + if app.autosave.offer_error.is_none() + && crate::widgets::dialog_button(ui, tl!("Recover"), Some(t.accent), vec2(84.0, 28.0)).clicked() + { + decision = Some("recover"); + } + if crate::widgets::dialog_button(ui, tl!("Discard"), None, vec2(84.0, 28.0)).clicked() { + decision = Some("discard"); + } + if crate::widgets::dialog_button(ui, tl!("Later"), None, vec2(84.0, 28.0)).clicked() + || ui.input(|i| i.key_pressed(egui::Key::Escape)) + { + decision = Some("later"); + } + }); + }); + match decision { + Some("recover") => { + if let Err(e) = app.run("ui.recover", Value::Null) { + app.autosave.offer_error = Some(e); + } + } + Some("discard") => { + let _ = app.run("ui.recover", json!({"discard": true})); + } + Some(_) => app.autosave.later = true, + None => {} + } +} + +#[cfg(not(target_arch = "wasm32"))] +pub use folder::FolderStore; + +/// The desktop's store: one file for this run's copy and one for each copy earlier runs left, +/// in a folder of their own, written by a background thread. +#[cfg(not(target_arch = "wasm32"))] +mod folder { + use std::io::{BufRead, Read}; + use std::path::{Path, PathBuf}; + use std::sync::mpsc::{Receiver, Sender, channel}; + use std::sync::{Arc, Mutex}; + use std::time::Duration; + + use rustgis_engine::Project; + + use super::{Recovery, RecoveryStore}; + + /// This run's copy. + const LIVE: &str = "autosave.recovery"; + /// Copies earlier runs left are `recovered-.recovery`. + const PENDING_PREFIX: &str = "recovered-"; + const EXTENSION: &str = ".recovery"; + /// The most copies kept waiting: the oldest beyond these go. + const MAX_WAITING: usize = 5; + /// The first line of a copy, describing it, is at most this long. + const MAX_HEADER: u64 = 64 * 1024; + /// How long a clean exit waits for a copy still being written. + const FLUSH_WAIT: Duration = Duration::from_secs(30); + + /// What the background thread is asked to do, in order. + enum Job { + Write(Box, Recovery), + Clear, + Remove(PathBuf), + Adopt(PathBuf), + Flush(Sender<()>), + } + + /// A recovery folder. A copy is one file: a line of JSON describing it ([`Recovery`]), then the + /// project's saved text. One file written atomically means the description always belongs to + /// the project after it, whenever the app stops. + pub struct FolderStore { + jobs: Mutex>, + /// The copies earlier runs left, newest first. + waiting: Mutex>, + error: Arc>>, + } + + impl FolderStore { + /// Takes `dir` as the recovery folder for this run, which must be the only one using it + /// (the desktop app holds the run marker). A copy the last run left there joins those + /// waiting to be offered; none is ever replaced by another, and only the oldest beyond + /// [`MAX_WAITING`] are removed. + pub fn open(dir: &Path) -> Result { + std::fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?; + // The copies hold the project's data: this user's only. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700)); + } + let live = dir.join(LIVE); + if live.is_file() { + let set_aside = unused_pending_name(dir); + std::fs::rename(&live, &set_aside).map_err(|e| format!("{}: {e}", live.display()))?; + } + let mut waiting: Vec<(PathBuf, Recovery)> = std::fs::read_dir(dir) + .map_err(|e| format!("{}: {e}", dir.display()))? + .filter_map(Result::ok) + .map(|entry| entry.path()) + .filter(|p| p.is_file() && is_pending(p)) + .map(|p| { + let about = describe(&p); + (p, about) + }) + .collect(); + // Newest first; copies of the same second by name, the later set aside first. + waiting.sort_by(|a, b| b.1.time.cmp(&a.1.time).then_with(|| b.0.cmp(&a.0))); + for (old, _) in waiting.drain(MAX_WAITING.min(waiting.len())..) { + log::warn!("autosave: removing {}, the oldest of more than {MAX_WAITING} copies", old.display()); + remove(&old); + } + let (jobs, rx) = channel(); + let error: Arc>> = Arc::default(); + let (worker_dir, worker_error) = (dir.to_path_buf(), error.clone()); + std::thread::Builder::new() + .name("autosave".into()) + .spawn(move || work(&worker_dir, &rx, &worker_error)) + .map_err(|e| format!("could not start autosave: {e}"))?; + Ok(FolderStore { jobs: Mutex::new(jobs), waiting: Mutex::new(waiting), error }) + } + + /// Queues `job` for the background thread; one that stopped (it can't, short of a panic) + /// is logged, and the copy is then not kept. + fn send(&self, job: Job) { + let jobs = self.jobs.lock().unwrap_or_else(|e| e.into_inner()); + if jobs.send(job).is_err() { + log::warn!("autosave stopped: its thread is gone"); + } + } + + /// The copies waiting, locked. + fn list(&self) -> std::sync::MutexGuard<'_, Vec<(PathBuf, Recovery)>> { + self.waiting.lock().unwrap_or_else(|e| e.into_inner()) + } + + /// Takes the offered copy off the list, for `job` to do with its file. + fn take_offered(&self, job: fn(PathBuf) -> Job) { + let mut waiting = self.list(); + if waiting.is_empty() { + return; + } + let (path, _) = waiting.remove(0); + drop(waiting); + self.send(job(path)); + } + } + + /// Whether `path` is a copy an earlier run left. + fn is_pending(path: &Path) -> bool { + path.file_name().and_then(|n| n.to_str()).is_some_and(|n| n.starts_with(PENDING_PREFIX) && n.ends_with(EXTENSION)) + } + + /// A name for a copy set aside that no other copy has: `recovered-.recovery`, one past the + /// highest number there. + fn unused_pending_name(dir: &Path) -> PathBuf { + let highest = std::fs::read_dir(dir) + .into_iter() + .flatten() + .filter_map(Result::ok) + .filter_map(|e| { + let name = e.file_name().to_str()?.to_string(); + name.strip_prefix(PENDING_PREFIX)?.strip_suffix(EXTENSION)?.parse::().ok() + }) + .max(); + let n = highest.map_or(1, |h| h.saturating_add(1)); + dir.join(format!("{PENDING_PREFIX}{n}{EXTENSION}")) + } + + /// A copy's description, from its first line; a copy whose line doesn't read (cut short, + /// garbled) is still offered, as an untitled project of its file's time. + fn describe(path: &Path) -> Recovery { + let mut line = String::new(); + let read = std::fs::File::open(path).and_then(|f| std::io::BufReader::new(f.take(MAX_HEADER)).read_line(&mut line)); + if read.is_ok() + && let Ok(r) = serde_json::from_str::(line.trim_end()) + { + return r; + } + let time = std::fs::metadata(path) + .and_then(|m| m.modified()) + .ok() + .and_then(|m| m.duration_since(std::time::UNIX_EPOCH).ok()) + .map_or(0, |d| d.as_secs()); + Recovery { time, ..Recovery::default() } + } + + /// The background thread: does what it is asked in order. A write followed by another is + /// skipped: only the latest copy matters. + fn work(dir: &Path, rx: &Receiver, error: &Mutex>) { + let fail = |e: String| { + log::warn!("autosave: {e}"); + *error.lock().unwrap_or_else(|p| p.into_inner()) = Some(e); + }; + let mut next = rx.recv().ok(); + while let Some(job) = next.take() { + if let Job::Write(..) = job + && let Ok(later) = rx.try_recv() + { + match later { + Job::Write(..) => { + next = Some(later); + continue; + } + other => next = Some(other), + } + } + match job { + Job::Write(project, about) => { + if let Err(e) = write(dir, *project, &about) { + fail(e); + } + } + Job::Clear => remove(&dir.join(LIVE)), + Job::Remove(path) => remove(&path), + Job::Adopt(path) => { + if let Err(e) = std::fs::rename(&path, dir.join(LIVE)) { + fail(format!("could not keep the recovered copy: {e}")); + } + } + Job::Flush(done) => { + let _ = done.send(()); + } + } + if next.is_none() { + next = rx.recv().ok(); + } + } + } + + /// Writes this run's copy: the description line, then the project's text, in one atomic + /// write, so a stop part way leaves the previous copy whole. + fn write(dir: &Path, project: Project, about: &Recovery) -> Result<(), String> { + let text = project.recovery_text()?; + let header = serde_json::to_string(about).map_err(|e| e.to_string())?; + let mut bytes = Vec::with_capacity(header.len() + 1 + text.len()); + bytes.extend_from_slice(header.as_bytes()); + bytes.push(b'\n'); + bytes.extend_from_slice(text.as_bytes()); + rustgis_engine::data::io::write_atomic(&dir.join(LIVE), &bytes).map_err(|e| e.to_string()) + } + + /// Removes a copy; one already gone is no error. + fn remove(path: &Path) { + if let Err(e) = std::fs::remove_file(path) + && e.kind() != std::io::ErrorKind::NotFound + { + log::warn!("autosave: could not remove {}: {e}", path.display()); + } + } + + impl RecoveryStore for FolderStore { + /// Serialized and written by the background thread; a newer copy queued behind it wins. + fn write(&self, project: Project, about: Recovery) { + self.send(Job::Write(Box::new(project), about)); + } + + /// Done after any copy queued before it, so a slow write can't bring the copy back. + fn clear(&self) { + self.send(Job::Clear); + } + + /// Waits up to [`FLUSH_WAIT`] for the background thread to reach this point. A copy still + /// being written then is left, and offered at the next start as if this run hadn't closed. + fn flush(&self) { + let (done, wait) = channel(); + self.send(Job::Flush(done)); + if wait.recv_timeout(FLUSH_WAIT).is_err() { + log::warn!("autosave: a recovery copy was still being written at exit"); + } + } + + /// The newest waiting, as [`FolderStore::open`] found them. + fn pending(&self) -> Option { + self.list().first().map(|(_, about)| about.clone()) + } + + /// As many as [`FolderStore::open`] found, less those recovered or discarded since. + fn waiting(&self) -> usize { + self.list().len() + } + + /// Read here, on the caller's thread (the user is waiting for it), at most a project's + /// largest size; a copy cut short or not text is refused with why. + fn read_pending(&self) -> Result { + let path = self.list().first().map(|(p, _)| p.clone()).ok_or("there is no unsaved work to recover")?; + let mut bytes = Vec::new(); + std::fs::File::open(&path) + .and_then(|f| f.take(rustgis_engine::project::MAX_BYTES + MAX_HEADER + 1).read_to_end(&mut bytes)) + .map_err(|e| format!("{}: {e}", path.display()))?; + if bytes.len() as u64 > rustgis_engine::project::MAX_BYTES + MAX_HEADER { + return Err("the recovery copy is too large".into()); + } + let text = String::from_utf8(bytes).map_err(|_| "the recovery copy isn't text".to_string())?; + let (_, project) = text.split_once('\n').ok_or("the recovery copy is incomplete")?; + Ok(project.to_string()) + } + + /// The offered copy's file goes after what is queued before it; the next one is offered. + fn discard_pending(&self) { + self.take_offered(Job::Remove); + } + + /// Renamed into this run's place, unchanged: nothing is written again. A rename that + /// fails is told as a failed write is. + fn adopt_pending(&self) { + self.take_offered(Job::Adopt); + } + + /// The background thread's last failure, taken so it is told once. + fn take_error(&self) -> Option { + self.error.lock().unwrap_or_else(|e| e.into_inner()).take() + } + } +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 2407dfcb3..ed37ee24b 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -14,6 +14,7 @@ mod analysispane; #[cfg(not(target_arch = "wasm32"))] mod assistantkeys; mod assistantpane; +mod autosave; mod backstage; mod bookmarkpane; mod catalogsui; @@ -85,6 +86,9 @@ use rustgis_engine::render::rustgis_tiles::failure::Failure; use rustgis_engine::{Project, Session}; use serde_json::{Value, json}; +#[cfg(not(target_arch = "wasm32"))] +pub use autosave::FolderStore; +pub use autosave::{Recovery, RecoveryStore}; pub use chrome::{DoubleClick, TitlebarMetrics}; pub use rustgis_assistant; pub use rustgis_engine; @@ -238,6 +242,10 @@ pub struct Services { /// Makes an HTTP request (the AI assistant's calls to its model) and puts the response, of any /// status, in the slot (the browser's fetch; the desktop sends it on a thread). pub http: Option>)>>, + /// Where autosave keeps the recovery copy of unsaved work (the desktop's recovery folder). + /// `None` turns autosave off: the browser for now, and a second copy of the desktop app + /// running alongside the first, which mustn't touch its copy. + pub recovery: Option>, } /// A new private folder for one print job: unique, created fresh (never an existing path) and, @@ -806,6 +814,8 @@ pub struct RustGisApp { pub quit_requested: bool, logo: Option, pub services: Services, + /// Autosave's interval, its last copy, and the copy an earlier run left (#873). + pub(crate) autosave: autosave::AutosaveState, /// The MCP relay for `rustgis-cli mcp --attach`: a loopback listener on the session this app /// already has open, started by the `ui.mcpRelay` command and off until then. #[cfg(not(target_arch = "wasm32"))] @@ -896,6 +906,7 @@ impl RustGisApp { self.map.set_render_state(rs); } + /// The app around `session`, with no platform hooks (see [`Self::with_services`]). pub fn new(session: Session) -> Self { // The installed fonts a label can be drawn in, indexed while the app starts. rustgis_engine::render::families::warm_up(); @@ -952,14 +963,18 @@ impl RustGisApp { label_fonts_indexed: false, shown_lang: i18n::Lang::EN, seen_project: None, + autosave: Default::default(), } } + /// Installs the shell's platform hooks and reads what they keep: preferences, saved + /// connections and the recovery copy an earlier run left. pub fn with_services(mut self, services: Services) -> Self { // Both saved database passwords and S3 keys use the same store, installed before prefs. self.session.database_logins.set_store(services.secrets.clone()); self.services = services; sourcespane::load_connections(&mut self); + autosave::load(&mut self); if let Some(lang) = self.services.load_pref.as_ref().and_then(|load| load("language")) { self.ui.language = lang; } @@ -1483,6 +1498,12 @@ impl RustGisApp { } } + /// A clean exit (the desktop app's `on_exit`): the recovery copy of this run's unsaved work + /// is removed, as recovery is for a run that didn't close. Call it once, as the app closes. + pub fn end_autosave(&mut self) { + autosave::end(self); + } + /// Whether this shell opens files by path (the desktop app; the web app picks them in the /// browser instead). pub(crate) fn opens_files(&self) -> bool { @@ -1772,6 +1793,7 @@ impl RustGisApp { } } + /// Runs a `ui.*` command: panes, dialogs and the app's own settings. fn run_ui(&mut self, id: &str, p: &Value) -> Result { // A pane's command toggles it; `{"close": true}` closes it, in view or not (a tab's ✕), // whatever else the command is given (SQL to run, a question to ask). @@ -1894,6 +1916,8 @@ impl RustGisApp { } self.apply_worldview(); } + "ui.autosave" => return autosave::command(self, p), + "ui.recover" => return autosave::recover(self, p), "ui.startup" => { // `project` (#377) or, as before, `blank`; `project` wins when both are given. let start_with = match p.get("project") { @@ -3421,6 +3445,7 @@ impl RustGisApp { // on the session directly) becomes Last Project now, not a frame later the app may not // reach. self.note_last_project(); + autosave::tick(self, ctx); self.ensure_script_font(); // Fonts downloaded by the web app. let fonts: Vec<(String, Vec)> = match &self.services.font_inbox { @@ -3573,6 +3598,7 @@ impl RustGisApp { graphicformat::save_draft(self); dialogs::show(self, &ctx); mapview::confirm_delete(self, &ctx); + autosave::offer(self, &ctx); pluginsui::show(self, &ctx); update::show(self, &ctx); showme::tick(self, &ctx); diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 03d4f1d46..a9bc6d35f 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -3155,6 +3155,7 @@ fn plugin_tab(app: &mut RustGisApp, ui: &mut Ui, plugin: &str) { } } +/// The Settings tab: ribbon customization, language, start-up and (on the desktop) autosave. fn settings_tab(app: &mut RustGisApp, ui: &mut Ui) { group(ui, "Customize", |ui| { let hidden = app.ui.hidden_tabs.clone(); @@ -3185,6 +3186,30 @@ fn settings_tab(app: &mut RustGisApp, ui: &mut Ui) { worldview_menu(app, ui); }); group(ui, "Start-up", |ui| start_project_menu(app, ui)); + // Only a shell that keeps recovery copies (the desktop app) has autosave. + if app.services.recovery.is_some() { + group(ui, "Autosave", |ui| autosave_menu(app, ui)); + } +} + +/// An interval's name: "Off" or "Every 5 min". +fn autosave_interval(minutes: u32) -> String { + if minutes == 0 { tl!("Off").to_string() } else { crate::i18n::fmt("Every {n} min", &[("n", &minutes.to_string())]) } +} + +/// Settings › Autosave: how often a recovery copy of unsaved work is kept (#873). +fn autosave_menu(app: &mut RustGisApp, ui: &mut Ui) { + let minutes = app.autosave.minutes; + let label = format!("{}\n{}", tl!("Autosave"), autosave_interval(minutes)); + let tip = "How often RustGIS keeps a copy of unsaved work, to offer it back after a crash."; + big_menu(ui, app, "save", &label, tip, move |ui, app| { + for m in crate::autosave::CHOICES { + if ui.add(egui::Button::selectable(minutes == m, autosave_interval(m))).clicked() { + let _ = app.run("ui.autosave", json!({"minutes": m})); + ui.close(); + } + } + }); } /// Settings › Start-up › Start With: the World Atlas sample, an empty project, or (in the diff --git a/crates/ui-egui/tests/autosave.rs b/crates/ui-egui/tests/autosave.rs new file mode 100644 index 000000000..0f411d406 --- /dev/null +++ b/crates/ui-egui/tests/autosave.rs @@ -0,0 +1,327 @@ +//! Autosave and crash recovery (#873), through the app's frames and its recovery dialog: a copy of +//! unsaved work is kept in the recovery folder, removed once the work is saved or the app closes +//! cleanly, and offered back by the next start when a run ended without closing. +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex}; + +use egui::accesskit::Role; +use egui_kittest::kittest::Queryable; +use rustgis_ui_egui::{FolderStore, RustGisApp, Services}; +use serde_json::json; + +/// This run's copy and the one an earlier run left, as the folder store names them. +const LIVE: &str = "autosave.recovery"; +/// The name the first copy set aside gets. +const PENDING: &str = "recovered-1.recovery"; + +/// A folder removed when the test ends, passed or failed. +struct TempDir(PathBuf); + +impl TempDir { + /// A fresh, empty folder for test `name`, apart from other tests and runs. + fn new(name: &str) -> TempDir { + let dir = std::env::temp_dir().join(format!("rustgis-autosave-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + TempDir(dir) + } +} + +impl Drop for TempDir { + /// Removes the folder and what the test left in it. + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +/// Preferences shared by every run of one test, as `prefs.json` is between launches. +type Prefs = Arc>>; + +/// One launch of the desktop app: the sample, with the recovery folder `dir` (holding the run +/// marker) and the preferences `prefs`. +fn launch(dir: &Path, prefs: &Prefs) -> egui_kittest::Harness<'static, RustGisApp> { + let (load, save) = (prefs.clone(), prefs.clone()); + let services = Services { + load_pref: Some(Box::new(move |k| load.lock().unwrap().get(k).cloned())), + save_pref: Some(Box::new(move |k, v| { + save.lock().unwrap().insert(k.to_string(), v.to_string()); + })), + recovery: Some(Box::new(FolderStore::open(&dir.join("recovery")).unwrap())), + ..Default::default() + }; + let mut app = RustGisApp::with_sample().with_services(services); + app.start(&Default::default()).unwrap(); + egui_kittest::Harness::builder().with_size(egui::vec2(1400.0, 860.0)).with_max_steps(64).build_ui_state( + |ui, app: &mut RustGisApp| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + app, + ) +} + +/// Runs `n` frames. A click is handled on the frame after it and the dialog it closes was +/// drawn by then, so its result shows a frame later; a dialog that grew is centred again a frame +/// after that. +fn frames(h: &mut egui_kittest::Harness<'_, RustGisApp>, n: usize) { + for _ in 0..n { + h.step(); + } +} + +/// Runs a few frames, then waits until the store has done what they asked of it. +fn settle(h: &mut egui_kittest::Harness<'_, RustGisApp>) { + for _ in 0..3 { + h.step(); + } + h.state().services.recovery.as_ref().unwrap().flush(); +} + +/// A file in the recovery folder of a launch in `dir`. +fn file(dir: &Path, name: &str) -> PathBuf { + dir.join("recovery").join(name) +} + +/// An edit to the project: a new layer. +fn edit(h: &mut egui_kittest::Harness<'_, RustGisApp>, name: &str) { + h.state_mut().run("layer.template", json!({"template": "points", "name": name})).unwrap(); +} + +#[test] +fn unsaved_work_is_kept_and_offered_back_after_a_crash() { + let dir = TempDir::new("crash"); + let prefs = Prefs::default(); + let project = dir.0.join("Parcels.rgis"); + let mut h = launch(&dir.0, &prefs); + // Nothing unsaved, nothing kept; nor after a change of view only. + settle(&mut h); + h.state_mut().run("map.zoomFull", json!({})).unwrap(); + settle(&mut h); + assert!(!file(&dir.0, LIVE).exists()); + h.state_mut().run("project.saveAs", json!({"path": project.display().to_string()})).unwrap(); + edit(&mut h, "Inspections"); + settle(&mut h); + let copy = std::fs::read_to_string(file(&dir.0, LIVE)).unwrap(); + let (about, text) = copy.split_once('\n').unwrap(); + let about: serde_json::Value = serde_json::from_str(about).unwrap(); + assert_eq!(about["name"], "Parcels"); + assert_eq!(about["path"], json!(std::path::absolute(&project).unwrap().display().to_string())); + assert!(text.contains("Inspections")); + // The app stops without closing: no clean exit. Then the file is changed on disk (another + // program, a copy synced from elsewhere) after the copy was made. + drop(h); + let later = std::time::SystemTime::now() + std::time::Duration::from_secs(120); + std::fs::File::options().write(true).open(&project).unwrap().set_modified(later).unwrap(); + + let mut h = launch(&dir.0, &prefs); + assert!(file(&dir.0, PENDING).is_file() && !file(&dir.0, LIVE).exists(), "set aside to be offered"); + assert_eq!(h.state_mut().run("ui.autosave", json!({})).unwrap()["pending"]["name"], "Parcels"); + h.step(); + h.get_by_label("Recover unsaved work?"); + h.get_by_label("Parcels.rgis has changed since this copy was made."); + h.get_by_role_and_label(Role::Button, "Recover").click(); + settle(&mut h); + assert!(h.query_by_label("Recover unsaved work?").is_none()); + let app = h.state(); + assert!(app.session.map().layer_by_name("Inspections").is_some()); + assert_eq!(app.session.path.as_deref(), Some(std::path::absolute(&project).unwrap().as_path()), "Save writes back to its file"); + assert!(app.session.dirty, "recovered work is unsaved"); + // The copy is this run's now, unchanged, until the work is saved. + assert!(!file(&dir.0, PENDING).exists() && file(&dir.0, LIVE).is_file()); + h.state_mut().run("project.save", json!({})).unwrap(); + settle(&mut h); + assert!(!file(&dir.0, LIVE).exists(), "saved work needs no copy"); + assert!(std::fs::read_to_string(&project).unwrap().contains("Inspections")); +} + +#[test] +fn a_clean_exit_leaves_nothing_to_recover() { + let dir = TempDir::new("clean"); + let prefs = Prefs::default(); + let mut h = launch(&dir.0, &prefs); + edit(&mut h, "Unsaved"); + settle(&mut h); + assert!(file(&dir.0, LIVE).is_file()); + h.state_mut().end_autosave(); + assert!(!file(&dir.0, LIVE).exists()); + drop(h); + let mut h = launch(&dir.0, &prefs); + h.step(); + assert!(h.query_by_label("Recover unsaved work?").is_none()); + assert!(h.state_mut().run("ui.recover", json!({})).is_err(), "nothing to recover"); +} + +#[test] +fn later_offers_it_again_and_discard_deletes_it() { + let dir = TempDir::new("later"); + let prefs = Prefs::default(); + let mut h = launch(&dir.0, &prefs); + edit(&mut h, "Kept"); + settle(&mut h); + drop(h); + // Later: this run goes on with what it opened; the copy waits for the next start, and this + // run's own unsaved work doesn't replace it. + let mut h = launch(&dir.0, &prefs); + h.step(); + h.get_by_role_and_label(Role::Button, "Later").click(); + frames(&mut h, 2); + assert!(h.query_by_label("Recover unsaved work?").is_none()); + assert!(h.state().session.map().layer_by_name("Kept").is_none()); + edit(&mut h, "Other"); + settle(&mut h); + h.state_mut().end_autosave(); + drop(h); + let mut h = launch(&dir.0, &prefs); + assert!(std::fs::read_to_string(file(&dir.0, PENDING)).unwrap().contains("Kept")); + h.step(); + h.get_by_role_and_label(Role::Button, "Discard").click(); + settle(&mut h); + assert!(h.query_by_label("Recover unsaved work?").is_none()); + assert!(!file(&dir.0, PENDING).exists()); + assert!(h.state().session.map().layer_by_name("Kept").is_none()); +} + +#[test] +fn a_damaged_copy_is_refused_and_can_be_discarded() { + let dir = TempDir::new("damaged"); + let prefs = Prefs::default(); + let pending = file(&dir.0, PENDING); + let bad: [&[u8]; 6] = [ + b"", + b"no header line", + b"{\"name\":\"Cut\"}\n{\"format\":1,\"maps\":[{\"name\"", + b"{\"name\":\"Newer\"}\n{\"format\":999}", + b"garbled header\n{}{}", + &[0xff, 0xfe, b'\n', 0x80], + ]; + for bytes in bad { + std::fs::create_dir_all(pending.parent().unwrap()).unwrap(); + std::fs::write(&pending, bytes).unwrap(); + let mut h = launch(&dir.0, &prefs); + h.step(); + h.get_by_role_and_label(Role::Button, "Recover").click(); + frames(&mut h, 4); + assert!(h.query_by_label_contains("It didn't open").is_some(), "{bytes:?}"); + assert!(h.query_by_role_and_label(Role::Button, "Recover").is_none(), "a copy that didn't open isn't offered again"); + assert!(h.state().session.map().layer_by_name("Countries").is_some(), "the sample is still open"); + h.get_by_role_and_label(Role::Button, "Discard").click(); + settle(&mut h); + assert!(!pending.exists()); + } +} + +#[test] +fn the_interval_is_a_setting_and_off_keeps_no_copy() { + let dir = TempDir::new("interval"); + let prefs = Prefs::default(); + let mut h = launch(&dir.0, &prefs); + let r = h.state_mut().run("ui.autosave", json!({})).unwrap(); + assert_eq!((r["available"].clone(), r["minutes"].clone(), r["pending"].clone()), (json!(true), json!(5), json!(null))); + for bad in [json!(-1), json!(61), json!(2.5), json!("5"), json!(null), json!(u64::MAX)] { + assert!(h.state_mut().run("ui.autosave", json!({"minutes": bad})).is_err(), "{bad}"); + } + for bad in [json!("yes"), json!(1), json!([])] { + assert!(h.state_mut().run("ui.recover", json!({"discard": bad})).is_err(), "{bad}"); + } + edit(&mut h, "Unsaved"); + settle(&mut h); + assert!(file(&dir.0, LIVE).is_file()); + // Off: the copy goes, and none is made while it stays off. + assert_eq!(h.state_mut().run("ui.autosave", json!({"minutes": 0})).unwrap()["minutes"], 0); + settle(&mut h); + assert!(!file(&dir.0, LIVE).exists()); + edit(&mut h, "Another"); + settle(&mut h); + assert!(!file(&dir.0, LIVE).exists()); + assert_eq!(prefs.lock().unwrap().get("autosave_minutes").map(String::as_str), Some("0")); + // Remembered by the next launch; a value edited out of range means the default. + drop(h); + let mut h = launch(&dir.0, &prefs); + assert_eq!(h.state_mut().run("ui.autosave", json!({})).unwrap()["minutes"], 0); + prefs.lock().unwrap().insert("autosave_minutes".into(), "9000".into()); + let mut h2 = launch(&dir.0, &prefs); + assert_eq!(h2.state_mut().run("ui.autosave", json!({})).unwrap()["minutes"], 5); + // A shell without a recovery store (the browser) has no autosave to set. + let mut web = RustGisApp::with_sample(); + assert_eq!(web.run("ui.autosave", json!({})).unwrap()["available"], false); + assert!(web.run("ui.autosave", json!({"minutes": 5})).is_err()); + assert!(web.run("ui.recover", json!({})).is_err()); +} + +/// Later, more work, another crash: neither copy replaces the other. The newest is offered first, +/// recovering it leaves the other for the next start (it would replace the recovered work), and +/// recovery never replaces a project with unsaved work of its own. +#[test] +fn copies_from_two_crashes_are_both_offered_in_turn() { + let dir = TempDir::new("two"); + let prefs = Prefs::default(); + let mut h = launch(&dir.0, &prefs); + edit(&mut h, "First"); + settle(&mut h); + drop(h); + let mut h = launch(&dir.0, &prefs); + h.step(); + h.get_by_role_and_label(Role::Button, "Later").click(); + frames(&mut h, 2); + edit(&mut h, "Second"); + let refused = h.state_mut().run("ui.recover", json!({})).unwrap_err(); + assert!(refused.contains("unsaved changes"), "{refused}"); + assert!(h.state().session.map().layer_by_name("Second").is_some(), "the open work is kept"); + settle(&mut h); + drop(h); + + let mut h = launch(&dir.0, &prefs); + assert!(h.state_mut().run("ui.autosave", json!({})).unwrap()["pending"].is_object()); + h.step(); + h.get_by_label("Older copies waiting: 1."); + h.get_by_role_and_label(Role::Button, "Recover").click(); + settle(&mut h); + assert!(h.state().session.map().layer_by_name("Second").is_some(), "the newest first"); + assert!(h.query_by_label("Recover unsaved work?").is_none(), "the other waits for the next start"); + h.state_mut().end_autosave(); + drop(h); + + let mut h = launch(&dir.0, &prefs); + h.step(); + h.get_by_role_and_label(Role::Button, "Recover").click(); + settle(&mut h); + assert!(h.state().session.map().layer_by_name("First").is_some()); + assert!(h.state_mut().run("ui.recover", json!({})).is_err(), "none left"); +} + +/// The recovery commands with junk parameters (as `hostile_params_never_panic` does for the +/// engine's): refused or ignored, never a panic, and the waiting copy isn't touched by a refusal. +#[test] +fn recovery_commands_take_junk_parameters() { + let dir = TempDir::new("junk"); + let prefs = Prefs::default(); + let mut h = launch(&dir.0, &prefs); + edit(&mut h, "Kept"); + settle(&mut h); + drop(h); + let mut h = launch(&dir.0, &prefs); + let junk = [ + json!(null), + json!(true), + json!(-1), + json!(1e308), + json!(u64::MAX), + json!("x"), + json!([1, 2]), + json!({"nested": {"minutes": 5}}), + json!("\u{0}"), + ]; + for v in &junk { + for key in ["minutes", "discard", "text", "path"] { + let _ = h.state_mut().run("ui.autosave", json!({key: v})); + if key != "discard" || v.as_bool() != Some(true) { + let _ = h.state_mut().run("ui.recover", json!({key: v})); + } + } + let _ = h.state_mut().run("ui.autosave", v.clone()); + } + // `{"text": …}` and the like are ignored: the first such run recovered the copy. + assert!(h.state().session.map().layer_by_name("Kept").is_some()); + settle(&mut h); +} diff --git a/docs/installation.md b/docs/installation.md index 29073ddd9..a2d117dc0 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -219,12 +219,12 @@ The desktop app and the command line keep their files in three folders: | Folder | Holds | Windows | macOS | Linux | | --- | --- | --- | --- | --- | | Settings | preferences (`prefs.json`), the crash log, AI Assistant sign-ins and skills | `%APPDATA%\RustGIS` | `~/Library/Application Support/RustGIS` | `$XDG_CONFIG_HOME/rustgis` (`~/.config/rustgis`) | -| Data | installed plugins and the GeoLibre runner | `%LOCALAPPDATA%\RustGIS` | `~/Library/Application Support/RustGIS` | `$XDG_DATA_HOME/rustgis` (`~/.local/share/rustgis`) | +| Data | installed plugins, the GeoLibre runner and recovery copies of unsaved work | `%LOCALAPPDATA%\RustGIS` | `~/Library/Application Support/RustGIS` | `$XDG_DATA_HOME/rustgis` (`~/.local/share/rustgis`) | | Cache | GDAL and DuckDB conversions | `%LOCALAPPDATA%\RustGIS\cache` | `~/Library/Caches/RustGIS` | `$XDG_CACHE_HOME/rustgis` (`~/.cache/rustgis`) | Set `RUSTGIS_CONFIG_DIR`, `RUSTGIS_APP_DATA_DIR` or `RUSTGIS_CACHE_DIR` to use another folder instead, for a portable install on a USB drive, for example. The folder is used as is, and RustGIS -keeps the same files in it (`plugins/` and `geolibre/` in the data folder, `gdal/` and `duckdb/` +keeps the same files in it (`plugins/`, `geolibre/` and `recovery/` in the data folder, `gdal/` and `duckdb/` in the cache folder). `RUSTGIS_PLUGINS_DIR`, `RUSTGIS_GDAL_CACHE` and `RUSTGIS_DUCKDB_CACHE` name the plugins folder and the GDAL and DuckDB caches themselves, ahead of these. @@ -241,6 +241,9 @@ It holds the version, the operating system, the error message and where in RustG An error message can quote what RustGIS was working on, such as a file path or a layer name, so read the log first and remove anything you don't want to share. +Unsaved work isn't lost with it: the next launch offers the copy autosave kept (see +[Autosave and recovery](user-guide/workspace.md#autosave-and-recovery)). + ## If the app doesn't start The desktop app draws its window with the graphics card through Direct3D 12 on Windows, Metal on diff --git a/docs/user-guide/index.md b/docs/user-guide/index.md index 970e8d62f..9bc10d025 100644 --- a/docs/user-guide/index.md +++ b/docs/user-guide/index.md @@ -112,7 +112,7 @@ ribbon, arrows at its ends scroll it. | **View** | Display (Graticule, Dark Mode, GPU Rendering), View (Globe, Projection, Global; Local on the desktop), Effects, Scene, Windows, Labeling, Navigation, Link, Workspace, Animation, Time (Time Slider), Profile (Profile; Time Series on the desktop) | | **Publish** | While a layout is open, its groups first (Page, Map Frames, Map Surrounds, Shapes, Text, Export); then Report, Presentation, Output (Export Map, Export Features, Print Map, Capture To Clipboard, Report, Presentation, Export To 3D), Package, Share As, Layout, Save As, Thumbnail | | **Plugins** | Plugins (Plugin Manager, Install From File, Browse Plugins), plus a group for each enabled [plugin](../plugins.md) with commands or tools; a plugin with its own tab adds that tab instead | -| **Settings** | Customize (Ribbon, Quick Access), Language (interface language and worldview), Start-up (Start With, Projection, Extent) | +| **Settings** | Customize (Ribbon, Quick Access), Language (interface language and worldview), Start-up (Start With, Projection, Extent), Autosave on the desktop | | **Help** | Performance (Diagnostic Monitor), Learn (Learning Resources), Help (About, Check for Updates on the desktop, Command Search, Shortcuts) | The tabs stay the same whatever is selected: Style and Data act on the layer selected in Layers. @@ -133,6 +133,8 @@ other files RustGIS reads and writes. Changes to the project are commands, and each can be undone (++ctrl+z++) and redone (++ctrl+y++). Undo keeps the last 40 changes; starting or opening another project clears it. +The desktop app also keeps a copy of unsaved work and offers it back after a crash: see +[Autosave and recovery](workspace.md#autosave-and-recovery). Exports and other files already written are not undone. ## Keyboard shortcuts diff --git a/docs/user-guide/workspace.md b/docs/user-guide/workspace.md index c2feec1cd..c41f46f65 100644 --- a/docs/user-guide/workspace.md +++ b/docs/user-guide/workspace.md @@ -19,6 +19,38 @@ Saved layer addresses omit passwords, access keys, tokens and signatures. The op keeps them, but after restarting RustGIS a protected source may need you to sign in to its connection again or add it again with credentials. +### Autosave and recovery + +The desktop app keeps a recovery copy of unsaved work. While the project has changes that aren't +saved, it writes a copy every few minutes (5 by default), and only when something changed since +the last one. Choose how often, or turn it off, in **Settings › Autosave** +(`ui.autosave={"minutes": 10}`; `0` is off). The copy is written in the background, so a large +project doesn't hold up the app. It leaves out layer passwords, keys and tokens, as a saved project +does. It is kept in the `recovery` folder of the app's +[data folder](../installation.md#settings-data-and-cache-folders), not beside your project. +Saving, opening or starting another project, and quitting RustGIS remove it. + +If RustGIS crashes, is closed by the system or loses power, the next launch asks **Recover unsaved +work?** and says how old the copy is. + +- **Recover** opens the work unsaved, under its file's name, so **Save** writes it back to that + file. RustGIS warns you if the file was changed after the copy was made. A project that was never + saved stays untitled, and **Save** asks where to write it. +- **Discard** deletes the copy. +- **Later** keeps the copy for the next launch. + +If RustGIS crashes again before you decide, neither copy replaces the other. Up to five are kept +and offered newest first, one per launch once you recover one. Recovering never replaces a project +that has unsaved changes of its own: save it first. + +The same choices are commands: `ui.recover` opens the copy, and `ui.recover={"discard": true}` +deletes it. `ui.autosave` reports the interval and any copy waiting. + +Quitting doesn't ask about unsaved changes, and their copy is removed: save before you quit. A +second copy of RustGIS running alongside the first doesn't autosave, so it can't replace the first +one's copy. The browser app doesn't autosave yet; it warns before you leave a page with unsaved +changes. + ### QGIS projects and styles A QGIS project (`.qgs`, or a `.qgz` archive) opens as a new map of the current project: **Open** @@ -185,6 +217,8 @@ duplication and nudging have their own [layout shortcuts](layouts.md#layouts). - **Language**: **Settings › Language** switches the interface among [18 languages](../i18n.md) and chooses the [worldview](maps.md#worldview) of the built-in borders. +- **Autosave**: **Settings › Autosave** sets how often the desktop app keeps a + [recovery copy](#autosave-and-recovery) of unsaved work, or turns it off. - **Start-up**: **Settings › Start-up** sets what RustGIS opens with: **Start With** the World Atlas sample or a blank map, or, in the desktop app, **Last Project** (the `.rgis` project last opened or saved) or **Choose Project…** (one you pick once), the **Projection** the map opens in (a @@ -192,7 +226,8 @@ duplication and nudging have their own [layout shortcuts](layouts.md#layouts). full extent, or **Use Current View** to keep the view you are looking at). A project opened at start keeps its own projection and view; the Projection and Extent settings apply to the sample and a blank map. If the project no longer opens (moved or deleted), RustGIS starts with the sample - and says why. After a crash it starts with the sample too, in case the project is what crashed; + and says why. After a crash it starts with the sample too, in case the project is what crashed, and offers + back any [unsaved work](#autosave-and-recovery); starting `rustgis --sample` also skips the start-up project. The choices are remembered on this computer, or in this browser for the web app. Opening files from the command line starts a blank map. From 13131a5f6cc13cfdc70bddafbc00952777fac346 Mon Sep 17 00:00:00 2001 From: mavrick Date: Sun, 25 Oct 2026 10:40:39 -0500 Subject: [PATCH 2/8] fix(ui): retry a failed recovery copy; keep Recover until open work is saved Review follow-ups for #896: - A recovery copy that failed in the background is made again at the next interval, though the project hasn't changed since; the failure is still told once. - While the open project has unsaved changes, the recovery dialog says so and holds Recover back until they are saved, instead of failing and hiding Recover for the rest of the session. - The copy's age reads "a minute ago" between one and two minutes, not "1 minutes ago". Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/autosave.rs | 123 +++++++++++++++++++++++++++++-- crates/ui-egui/tests/autosave.rs | 25 +++++++ 2 files changed, 141 insertions(+), 7 deletions(-) diff --git a/crates/ui-egui/src/autosave.rs b/crates/ui-egui/src/autosave.rs index 025134161..ca742d98c 100644 --- a/crates/ui-egui/src/autosave.rs +++ b/crates/ui-egui/src/autosave.rs @@ -154,12 +154,14 @@ fn changed_since(r: &Recovery) -> bool { /// removed once the project has none (saved, or another one started). pub(crate) fn tick(app: &mut RustGisApp, ctx: &egui::Context) { let Some(store) = &app.services.recovery else { return }; - if let Some(e) = store.take_error() - && app.autosave.told.as_ref() != Some(&e) - { - app.autosave.told = Some(e.clone()); - let text = crate::i18n::fmt("Autosave couldn't keep a recovery copy: {error}", &[("error", &e)]); - app.session.notify(text, true); + if let Some(e) = store.take_error() { + // The copy that failed isn't kept: the work is due again, at the next interval. + app.autosave.written = None; + if app.autosave.told.as_ref() != Some(&e) { + app.autosave.told = Some(e.clone()); + let text = crate::i18n::fmt("Autosave couldn't keep a recovery copy: {error}", &[("error", &e)]); + app.session.notify(text, true); + } } if app.autosave.minutes == 0 || !app.session.dirty { if app.autosave.written.take().is_some() { @@ -263,7 +265,8 @@ fn kept_line(name: &str, then: u64) -> String { let secs = now_secs().saturating_sub(then); let (template, n) = match secs { 0..60 => ("RustGIS didn't close properly last time. A copy of {name} with unsaved changes was kept less than a minute ago.", 0), - 60..7_200 => { + 60..120 => ("RustGIS didn't close properly last time. A copy of {name} with unsaved changes was kept a minute ago.", 1), + 120..7_200 => { ("RustGIS didn't close properly last time. A copy of {name} with unsaved changes was kept {n} minutes ago.", secs / 60) } 7_200..172_800 => { @@ -307,13 +310,24 @@ pub(crate) fn offer(app: &mut RustGisApp, ctx: &egui::Context) { if app.autosave.more > 0 { ui.label(crate::i18n::fmt("Older copies waiting: {n}.", &[("n", &app.autosave.more.to_string())])); } + // Recovering replaces the open project: not one with unsaved work of its own (an edit + // made before answering). Recover comes back once that work is saved. + let blocked = app.session.dirty; + if blocked { + ui.add_space(4.0); + ui.label( + egui::RichText::new(tl!("The open project has unsaved changes: save them, then recover this copy.")).color(t.text_dim), + ); + } if let Some(e) = &app.autosave.offer_error { ui.add_space(4.0); ui.label(egui::RichText::new(crate::i18n::fmt("It didn't open: {error}", &[("error", e)])).color(t.red)); } ui.add_space(10.0); ui.with_layout(egui::Layout::right_to_left(egui::Align::Min), |ui| { + // A copy that didn't open isn't offered again; Discard and Later remain. if app.autosave.offer_error.is_none() + && !blocked && crate::widgets::dialog_button(ui, tl!("Recover"), Some(t.accent), vec2(84.0, 28.0)).clicked() { decision = Some("recover"); @@ -627,3 +641,98 @@ mod folder { } } } + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::panic)] +mod tests { + use std::cell::RefCell; + use std::rc::Rc; + + use rustgis_engine::{Project, Session}; + use serde_json::json; + + use super::{Recovery, RecoveryStore, kept_line, now_secs, tick}; + use crate::{RustGisApp, Services}; + + /// A store that counts the copies asked of it and fails the next one when told to. + #[derive(Clone, Default)] + struct Counting(Rc)>>); + + impl RecoveryStore for Counting { + /// Counts the copy; a failure set by the test is reported as the worker's would be. + fn write(&self, _: Project, _: Recovery) { + self.0.borrow_mut().0 += 1; + } + /// Nothing to remove. + fn clear(&self) {} + /// Nothing is in flight. + fn flush(&self) {} + /// Nothing waits. + fn pending(&self) -> Option { + None + } + /// Nothing waits. + fn waiting(&self) -> usize { + 0 + } + /// Nothing waits. + fn read_pending(&self) -> Result { + Err("none".into()) + } + /// Nothing waits. + fn discard_pending(&self) {} + /// Nothing waits. + fn adopt_pending(&self) {} + /// The failure set by the test, once. + fn take_error(&self) -> Option { + self.0.borrow_mut().1.take() + } + } + + /// A copy that failed in the background (#896 review) is made again at the next interval, + /// though the project hasn't changed since, and the failure is told once. + #[test] + fn a_failed_copy_is_retried() { + let store = Counting::default(); + let mut app = RustGisApp::new(Session::new(Project::default())) + .with_services(Services { recovery: Some(Box::new(store.clone())), ..Default::default() }); + let ctx = egui::Context::default(); + app.run("layer.template", json!({"template": "points", "name": "Unsaved"})).unwrap(); + tick(&mut app, &ctx); + assert_eq!(store.0.borrow().0, 1); + // Unchanged: nothing more, even once the interval has passed. + app.autosave.last = None; + tick(&mut app, &ctx); + assert_eq!(store.0.borrow().0, 1); + // The write failed: told, and made again once the interval has passed. + app.autosave.last = Some(web_time::Instant::now()); + store.0.borrow_mut().1 = Some("disk full".into()); + tick(&mut app, &ctx); + assert!(app.session.notices.iter().any(|n| n.error && n.text.contains("disk full"))); + assert_eq!(store.0.borrow().0, 1, "not before the interval"); + app.autosave.last = None; + tick(&mut app, &ctx); + assert_eq!(store.0.borrow().0, 2); + store.0.borrow_mut().1 = Some("disk full".into()); + let told = app.session.notices.len(); + tick(&mut app, &ctx); + assert_eq!(app.session.notices.len(), told, "the same failure is told once"); + } + + /// The dialog's age reads as a whole sentence in every range: never "1 minutes ago". + #[test] + fn the_copy_s_age_reads_naturally() { + let now = now_secs(); + for (ago, says) in [ + (5, "less than a minute ago"), + (90, "a minute ago"), + (150, "2 minutes ago"), + (3 * 3_600, "3 hours ago"), + (3 * 86_400, "3 days ago"), + ] { + let line = kept_line("Parcels", now - ago); + assert!(line.contains(says) && line.contains("Parcels"), "{ago}: {line}"); + } + assert!(!kept_line("Parcels", now - 61).contains("1 minutes")); + } +} diff --git a/crates/ui-egui/tests/autosave.rs b/crates/ui-egui/tests/autosave.rs index 0f411d406..483ce2916 100644 --- a/crates/ui-egui/tests/autosave.rs +++ b/crates/ui-egui/tests/autosave.rs @@ -325,3 +325,28 @@ fn recovery_commands_take_junk_parameters() { assert!(h.state().session.map().layer_by_name("Kept").is_some()); settle(&mut h); } + +/// Work done before answering the dialog (#896 review): Recover waits until it is saved, rather +/// than failing and going for the rest of the session, and then recovers the copy. +#[test] +fn recover_returns_once_the_open_work_is_saved() { + let dir = TempDir::new("blocked"); + let prefs = Prefs::default(); + let mut h = launch(&dir.0, &prefs); + edit(&mut h, "Kept"); + settle(&mut h); + drop(h); + let mut h = launch(&dir.0, &prefs); + edit(&mut h, "Meanwhile"); + frames(&mut h, 2); + h.get_by_label("The open project has unsaved changes: save them, then recover this copy."); + assert!(h.query_by_role_and_label(Role::Button, "Recover").is_none()); + let saved = dir.0.join("Meanwhile.rgis"); + h.state_mut().run("project.saveAs", json!({"path": saved.display().to_string()})).unwrap(); + frames(&mut h, 2); + assert!(h.query_by_label_contains("unsaved changes: save them").is_none()); + h.get_by_role_and_label(Role::Button, "Recover").click(); + settle(&mut h); + assert!(h.state().session.map().layer_by_name("Kept").is_some()); + assert!(h.query_by_label("Recover unsaved work?").is_none()); +} From d9c96f405f4500c4f3a9355dbfba5027f5296652 Mon Sep 17 00:00:00 2001 From: mavrick Date: Sun, 25 Oct 2026 11:48:04 -0500 Subject: [PATCH 3/8] fix(ui): wake the interface on a failed recovery copy; keep its cleanup Review follow-ups for #896: - The folder store's background thread wakes the interface after a failed copy (RecoveryStore::set_wake), so the failure is told on an idle window too, not at the next input. - A failed copy is retried through a separate flag instead of forgetting the last written revision, so saving after a failure still removes the copy an earlier write left. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/autosave.rs | 65 ++++++++++++++++++++++++++------ crates/ui-egui/tests/autosave.rs | 21 ++++++++++- 2 files changed, 74 insertions(+), 12 deletions(-) diff --git a/crates/ui-egui/src/autosave.rs b/crates/ui-egui/src/autosave.rs index ca742d98c..be5a1c4dc 100644 --- a/crates/ui-egui/src/autosave.rs +++ b/crates/ui-egui/src/autosave.rs @@ -64,6 +64,9 @@ pub trait RecoveryStore { fn adopt_pending(&self); /// Why the last copy couldn't be written, once. fn take_error(&self) -> Option; + /// Gives the store a way to wake the interface when a copy fails in the background, so the + /// failure is told on an idle window too. A store that fails only when asked needs none. + fn set_wake(&self, _wake: Box) {} } /// The app's autosave state. @@ -72,6 +75,9 @@ pub struct AutosaveState { pub(crate) minutes: u32, /// The session revision the last copy was made at, while one is kept. written: Option, + /// The last copy failed in the background: due again at the next interval, unchanged or not. + /// Kept apart from `written`, so a save still removes the earlier copy the failure left. + failed: bool, /// When the last copy was made. last: Option, /// The copy an earlier run left, while the user hasn't recovered or discarded it. @@ -86,6 +92,8 @@ pub struct AutosaveState { offer_error: Option, /// The last write error told, so a failing folder isn't reported at every interval. told: Option, + /// The store was given the interface's wake. + wake_set: bool, } impl Default for AutosaveState { @@ -94,6 +102,7 @@ impl Default for AutosaveState { AutosaveState { minutes: DEFAULT_MINUTES, written: None, + failed: false, last: None, offer: None, more: 0, @@ -101,6 +110,7 @@ impl Default for AutosaveState { later: false, offer_error: None, told: None, + wake_set: false, } } } @@ -154,9 +164,14 @@ fn changed_since(r: &Recovery) -> bool { /// removed once the project has none (saved, or another one started). pub(crate) fn tick(app: &mut RustGisApp, ctx: &egui::Context) { let Some(store) = &app.services.recovery else { return }; + if !app.autosave.wake_set { + let ctx = ctx.clone(); + store.set_wake(Box::new(move || ctx.request_repaint())); + app.autosave.wake_set = true; + } if let Some(e) = store.take_error() { - // The copy that failed isn't kept: the work is due again, at the next interval. - app.autosave.written = None; + // The work is due again, at the next interval. + app.autosave.failed = true; if app.autosave.told.as_ref() != Some(&e) { app.autosave.told = Some(e.clone()); let text = crate::i18n::fmt("Autosave couldn't keep a recovery copy: {error}", &[("error", &e)]); @@ -167,9 +182,10 @@ pub(crate) fn tick(app: &mut RustGisApp, ctx: &egui::Context) { if app.autosave.written.take().is_some() { store.clear(); } + app.autosave.failed = false; return; } - if app.autosave.written == Some(app.session.revision) { + if app.autosave.written == Some(app.session.revision) && !app.autosave.failed { return; } let interval = Duration::from_secs(u64::from(app.autosave.minutes) * 60); @@ -182,6 +198,7 @@ pub(crate) fn tick(app: &mut RustGisApp, ctx: &egui::Context) { } store.write(app.session.project.clone(), about(app)); app.autosave.written = Some(app.session.revision); + app.autosave.failed = false; app.autosave.last = Some(now); } @@ -402,8 +419,13 @@ mod folder { /// The copies earlier runs left, newest first. waiting: Mutex>, error: Arc>>, + /// Called by the background thread after a failure: the interface's repaint. + wake: Wake, } + /// The interface's wake, shared with the background thread; none until the app sets it. + type Wake = Arc>>>; + impl FolderStore { /// Takes `dir` as the recovery folder for this run, which must be the only one using it /// (the desktop app holds the run marker). A copy the last run left there joins those @@ -440,12 +462,13 @@ mod folder { } let (jobs, rx) = channel(); let error: Arc>> = Arc::default(); - let (worker_dir, worker_error) = (dir.to_path_buf(), error.clone()); + let wake: Wake = Arc::default(); + let (worker_dir, worker_error, worker_wake) = (dir.to_path_buf(), error.clone(), wake.clone()); std::thread::Builder::new() .name("autosave".into()) - .spawn(move || work(&worker_dir, &rx, &worker_error)) + .spawn(move || work(&worker_dir, &rx, &worker_error, &worker_wake)) .map_err(|e| format!("could not start autosave: {e}"))?; - Ok(FolderStore { jobs: Mutex::new(jobs), waiting: Mutex::new(waiting), error }) + Ok(FolderStore { jobs: Mutex::new(jobs), waiting: Mutex::new(waiting), error, wake }) } /// Queues `job` for the background thread; one that stopped (it can't, short of a panic) @@ -515,10 +538,13 @@ mod folder { /// The background thread: does what it is asked in order. A write followed by another is /// skipped: only the latest copy matters. - fn work(dir: &Path, rx: &Receiver, error: &Mutex>) { + fn work(dir: &Path, rx: &Receiver, error: &Mutex>, wake: &Wake) { let fail = |e: String| { log::warn!("autosave: {e}"); *error.lock().unwrap_or_else(|p| p.into_inner()) = Some(e); + if let Some(wake) = wake.lock().unwrap_or_else(|p| p.into_inner()).as_ref() { + wake(); + } }; let mut next = rx.recv().ok(); while let Some(job) = next.take() { @@ -639,6 +665,11 @@ mod folder { fn take_error(&self) -> Option { self.error.lock().unwrap_or_else(|e| e.into_inner()).take() } + + /// Kept for the background thread, which calls it after each failure. + fn set_wake(&self, wake: Box) { + *self.wake.lock().unwrap_or_else(|e| e.into_inner()) = Some(wake); + } } } @@ -654,17 +685,20 @@ mod tests { use super::{Recovery, RecoveryStore, kept_line, now_secs, tick}; use crate::{RustGisApp, Services}; - /// A store that counts the copies asked of it and fails the next one when told to. + /// A store that counts the copies and removals asked of it and fails the next copy when + /// told to. #[derive(Clone, Default)] - struct Counting(Rc)>>); + struct Counting(Rc, usize)>>); impl RecoveryStore for Counting { /// Counts the copy; a failure set by the test is reported as the worker's would be. fn write(&self, _: Project, _: Recovery) { self.0.borrow_mut().0 += 1; } - /// Nothing to remove. - fn clear(&self) {} + /// Counts the removal. + fn clear(&self) { + self.0.borrow_mut().2 += 1; + } /// Nothing is in flight. fn flush(&self) {} /// Nothing waits. @@ -717,6 +751,15 @@ mod tests { let told = app.session.notices.len(); tick(&mut app, &ctx); assert_eq!(app.session.notices.len(), told, "the same failure is told once"); + // Saved after a failure (#896 review): the copy an earlier write left is still removed. + app.run( + "project.saveAs", + json!({"path": std::env::temp_dir().join(format!("rustgis-retry-{}.rgis", std::process::id())).display().to_string()}), + ) + .unwrap(); + tick(&mut app, &ctx); + assert_eq!(store.0.borrow().2, 1, "removed once saved"); + let _ = std::fs::remove_file(std::env::temp_dir().join(format!("rustgis-retry-{}.rgis", std::process::id()))); } /// The dialog's age reads as a whole sentence in every range: never "1 minutes ago". diff --git a/crates/ui-egui/tests/autosave.rs b/crates/ui-egui/tests/autosave.rs index 483ce2916..afa4fa517 100644 --- a/crates/ui-egui/tests/autosave.rs +++ b/crates/ui-egui/tests/autosave.rs @@ -6,7 +6,7 @@ use std::sync::{Arc, Mutex}; use egui::accesskit::Role; use egui_kittest::kittest::Queryable; -use rustgis_ui_egui::{FolderStore, RustGisApp, Services}; +use rustgis_ui_egui::{FolderStore, RecoveryStore, RustGisApp, Services}; use serde_json::json; /// This run's copy and the one an earlier run left, as the folder store names them. @@ -350,3 +350,22 @@ fn recover_returns_once_the_open_work_is_saved() { assert!(h.state().session.map().layer_by_name("Kept").is_some()); assert!(h.query_by_label("Recover unsaved work?").is_none()); } + +/// A copy that fails in the background wakes the interface (#896 review), so the failure is told +/// on an idle window too, not at the next input. +#[test] +fn a_failed_copy_wakes_the_interface() { + use std::sync::atomic::{AtomicBool, Ordering}; + let dir = TempDir::new("wake"); + let recovery = dir.0.join("recovery"); + let store = FolderStore::open(&recovery).unwrap(); + // A folder where the copy goes: the write can't replace it. + std::fs::create_dir_all(recovery.join(LIVE).join("blocked")).unwrap(); + let woken = std::sync::Arc::new(AtomicBool::new(false)); + let flag = woken.clone(); + store.set_wake(Box::new(move || flag.store(true, Ordering::SeqCst))); + store.write(rustgis_engine::sample::world_project(), rustgis_ui_egui::Recovery::default()); + store.flush(); + assert!(woken.load(Ordering::SeqCst), "woken after the failure"); + assert!(store.take_error().is_some()); +} From 93b8a8ab710bc6c3a322b49b6b67eb6f824961aa Mon Sep 17 00:00:00 2001 From: mavrick Date: Sun, 25 Oct 2026 12:51:28 -0500 Subject: [PATCH 4/8] fix(ui): write a skipped recovery copy when the newer one fails Review follow-ups for #896: - The background thread keeps the write it skipped for a newer one until that one is done; if it fails, the skipped copy is written instead, before any later job, so the folder holds the newest copy that could be written. The loop takes its writer as a parameter for the test. - Say at the clone why it is cheap (Arc-shared layer data), and move the repeated count of waiting copies into a helper. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/autosave.rs | 85 ++++++++++++++++++++++++++++++++-- 1 file changed, 81 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/autosave.rs b/crates/ui-egui/src/autosave.rs index be5a1c4dc..75df5649e 100644 --- a/crates/ui-egui/src/autosave.rs +++ b/crates/ui-egui/src/autosave.rs @@ -134,6 +134,11 @@ pub(crate) fn load(app: &mut RustGisApp) { next_offer(app); } +/// How many copies wait to be offered, the one offered now included. +fn waiting(app: &RustGisApp) -> usize { + app.autosave.offer.as_ref().map_or(0, |_| app.autosave.more + 1) +} + /// Offers the next copy waiting in the store, if any. fn next_offer(app: &mut RustGisApp) { let store = app.services.recovery.as_ref(); @@ -196,6 +201,9 @@ pub(crate) fn tick(app: &mut RustGisApp, ctx: &egui::Context) { ctx.request_repaint_after(interval - since); return; } + // Cheap: layers' features (`Features`), raster pixels and embedded bytes are `Arc`s shared + // with the copy, as they are with the undo snapshot every edit already takes; only the + // project's own settings are copied here. Serializing and writing happen on the store's thread. store.write(app.session.project.clone(), about(app)); app.autosave.written = Some(app.session.revision); app.autosave.failed = false; @@ -245,7 +253,7 @@ pub(crate) fn recover(app: &mut RustGisApp, p: &Value) -> Result if discard { store.discard_pending(); next_offer(app); - return Ok(json!({"discarded": true, "waiting": app.autosave.offer.as_ref().map(|_| app.autosave.more + 1).unwrap_or(0)})); + return Ok(json!({"discarded": true, "waiting": waiting(app)})); } // Opening it replaces the open project: never one with work of its own not saved (after // Later), which it would lose with its recovery copy. @@ -266,7 +274,7 @@ pub(crate) fn recover(app: &mut RustGisApp, p: &Value) -> Result let text = crate::i18n::fmt("Recovered unsaved work in {name}. Save to keep it.", &[("name", &app.session.project.name)]); app.session.notify(text, false); // Other copies wait for the next start: recovering one now would replace this work. - let waiting = app.autosave.offer.as_ref().map(|_| app.autosave.more + 1).unwrap_or(0); + let waiting = waiting(app); if waiting > 0 { app.autosave.later = true; let text = @@ -536,9 +544,19 @@ mod folder { Recovery { time, ..Recovery::default() } } - /// The background thread: does what it is asked in order. A write followed by another is - /// skipped: only the latest copy matters. + /// How the background thread writes a copy (a test can make one fail). + type WriteCopy = fn(&Path, Project, &Recovery) -> Result<(), String>; + + /// The background thread: does what it is asked in order, writing copies with [`write`]. fn work(dir: &Path, rx: &Receiver, error: &Mutex>, wake: &Wake) { + work_with(dir, rx, error, wake, write); + } + + /// [`work`] with its writer. A write followed by another is skipped, as only the latest copy + /// matters, but kept until the next one is done: if that one fails (a project grown past the + /// size a copy may have), the skipped one is written instead, before any later job, so the + /// folder holds the newest copy that could be written. + fn work_with(dir: &Path, rx: &Receiver, error: &Mutex>, wake: &Wake, write: WriteCopy) { let fail = |e: String| { log::warn!("autosave: {e}"); *error.lock().unwrap_or_else(|p| p.into_inner()) = Some(e); @@ -547,12 +565,16 @@ mod folder { } }; let mut next = rx.recv().ok(); + let mut skipped: Option<(Box, Recovery)> = None; while let Some(job) = next.take() { if let Job::Write(..) = job && let Ok(later) = rx.try_recv() { match later { Job::Write(..) => { + if let Job::Write(project, about) = job { + skipped = Some((project, about)); + } next = Some(later); continue; } @@ -561,7 +583,13 @@ mod folder { } match job { Job::Write(project, about) => { + let earlier = skipped.take(); if let Err(e) = write(dir, *project, &about) { + if let Some((project, about)) = earlier + && let Err(again) = write(dir, *project, &about) + { + log::warn!("autosave: the copy before it failed too: {again}"); + } fail(e); } } @@ -671,6 +699,55 @@ mod folder { *self.wake.lock().unwrap_or_else(|e| e.into_inner()) = Some(wake); } } + + #[cfg(test)] + #[allow(clippy::unwrap_used)] + mod tests { + use std::sync::mpsc::channel; + use std::sync::{Arc, Mutex}; + + use rustgis_engine::Project; + + use super::{Job, LIVE, Recovery, work_with}; + + /// Writes copies as the store does, except a project named "newer", which is refused + /// as too large. + fn newer_fails(dir: &std::path::Path, project: Project, about: &Recovery) -> Result<(), String> { + if project.name == "newer" { + return Err("the project is too large to keep a recovery copy".into()); + } + super::write(dir, project, about) + } + + /// The jobs given, in order, done by the background thread's loop with [`newer_fails`]: + /// what the folder holds then, and the error told. + fn run(dir: &std::path::Path, jobs: Vec) -> (Option, Option) { + let (tx, rx) = channel(); + for job in jobs { + tx.send(job).unwrap(); + } + drop(tx); + let error = Mutex::new(None); + work_with(dir, &rx, &error, &Arc::default(), newer_fails); + (std::fs::read_to_string(dir.join(LIVE)).ok(), error.into_inner().unwrap()) + } + + /// A skipped copy is written when the newer one that replaced it fails (#896 review), and + /// still in order: a removal queued after both removes it. + #[test] + fn a_skipped_copy_stands_in_for_a_newer_one_that_failed() { + let dir = std::env::temp_dir().join(format!("rustgis-autosave-skip-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let copy = |name: &str| Job::Write(Box::new(Project::new(name)), Recovery { name: name.into(), ..Recovery::default() }); + let (held, error) = run(&dir, vec![copy("older"), copy("newer")]); + assert!(held.unwrap().contains("\"older\""), "the skipped copy is kept"); + assert!(error.unwrap().contains("too large"), "the newer one's failure is told"); + let (held, _) = run(&dir, vec![copy("older"), copy("newer"), Job::Clear]); + assert!(held.is_none(), "removed after, as asked"); + let _ = std::fs::remove_dir_all(&dir); + } + } } #[cfg(test)] From 895321b701ce7cb0e04f7594aa66e235b9596d17 Mon Sep 17 00:00:00 2001 From: mavrick Date: Sun, 25 Oct 2026 14:58:52 -0500 Subject: [PATCH 5/8] fix(ui): guard the autosave thread; say when autosave is off Review follow-ups for #896: - Each job on the autosave thread runs under catch_unwind: a panic is told like a failed copy, and the thread goes on with the next job instead of stopping without a word. - When the recovery folder can't be had (no app-data folder, or one that doesn't open), the desktop app starts without autosave and says so in a notice, not only in the log. - The user guide says autosave is off where the settings folder can't lock files, and that after a crash the app offers the recovery copy autosave kept, if any. Co-Authored-By: Claude Opus 5.5 --- apps/rustgis/src/main.rs | 44 +++++++++++++++++++++++++--------- crates/ui-egui/src/autosave.rs | 42 ++++++++++++++++++++++++++++---- docs/user-guide/workspace.md | 5 ++-- 3 files changed, 74 insertions(+), 17 deletions(-) diff --git a/apps/rustgis/src/main.rs b/apps/rustgis/src/main.rs index 2ea119984..825ac75f0 100644 --- a/apps/rustgis/src/main.rs +++ b/apps/rustgis/src/main.rs @@ -269,16 +269,12 @@ impl eframe::App for App { } } -/// This run's recovery folder, `recovery` in the app-data folder (see `docs/installation.md`). -fn recovery_store() -> Option> { - let dir = rustgis_engine::data::dirs::app_data_dir()?.join("recovery"); - match rustgis_ui_egui::FolderStore::open(&dir) { - Ok(store) => Some(Box::new(store)), - Err(e) => { - log::warn!("no autosave: {e}"); - None - } - } +/// This run's recovery folder, `recovery` in the app-data folder `data` (see +/// `docs/installation.md`), or why there is none: the app then starts without autosave and says so. +fn recovery_store(data: Option) -> Result, String> { + let dir = data.ok_or("this computer has no folder for RustGIS's app data")?.join("recovery"); + let store = rustgis_ui_egui::FolderStore::open(&dir)?; + Ok(Box::new(store)) } /// Opens the files named on the command line: a project or a project package as the project, any @@ -350,8 +346,12 @@ fn main() -> eframe::Result { let mut services = prefs::services(); // Autosave, in a recovery folder only the copy holding the run marker uses: a second copy // running alongside would offer, or overwrite, the first one's unsaved work. + let mut no_autosave = None; if marker.as_ref().is_some_and(running::Marker::locked) { - services.recovery = recovery_store(); + match recovery_store(rustgis_engine::data::dirs::app_data_dir()) { + Ok(store) => services.recovery = Some(store), + Err(e) => no_autosave = Some(e), + } } let mut app = app.with_services(services); // Settings › Start-up; `--sample` still opens the sample. @@ -368,6 +368,12 @@ fn main() -> eframe::Result { ); app.session.notify(text, true); } + if let Some(e) = no_autosave { + log::warn!("no autosave: {e}"); + rustgis_ui_egui::i18n::set_current(app.chosen_lang()); + let text = rustgis_ui_egui::i18n::fmt("Autosave is off: {error}", &[("error", &e)]); + app.session.notify(text, true); + } for e in plugin_errors { log::warn!("{e}"); app.session.notify(e, true); @@ -478,6 +484,22 @@ mod tests { assert!(app.session.path.is_none(), "a package opens unsaved, so Save asks where to write a project"); } + /// A recovery folder that can't be made (a file in its way) or no app-data folder at all gives + /// a reason, which the app shows as it starts without autosave (#896 review); a usable one + /// gives the store. + #[test] + fn a_recovery_folder_that_does_not_open_says_why() { + let dir = std::env::temp_dir().join(format!("rustgis-recovery-store-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let _cleanup = TempDir(dir.clone()); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("recovery"), "a file, not a folder").unwrap(); + let blocked = super::recovery_store(Some(dir.clone())).err().unwrap(); + assert!(blocked.contains("recovery"), "{blocked}"); + assert!(super::recovery_store(None).err().unwrap().contains("app data")); + assert!(super::recovery_store(Some(dir.join("fresh"))).is_ok()); + } + #[test] fn crash_times_are_utc_dates() { assert_eq!(crash::utc(0), "1970-01-01 00:00:00"); diff --git a/crates/ui-egui/src/autosave.rs b/crates/ui-egui/src/autosave.rs index 75df5649e..ed89d281d 100644 --- a/crates/ui-egui/src/autosave.rs +++ b/crates/ui-egui/src/autosave.rs @@ -552,8 +552,8 @@ mod folder { work_with(dir, rx, error, wake, write); } - /// [`work`] with its writer. A write followed by another is skipped, as only the latest copy - /// matters, but kept until the next one is done: if that one fails (a project grown past the + /// [`work`] with its writer. Each job runs guarded: a panic is told as a failure and the + /// thread goes on. A write followed by another is skipped, as only the latest copy matters, but kept until the next one is done: if that one fails (a project grown past the /// size a copy may have), the skipped one is written instead, before any later job, so the /// folder holds the newest copy that could be written. fn work_with(dir: &Path, rx: &Receiver, error: &Mutex>, wake: &Wake, write: WriteCopy) { @@ -581,7 +581,9 @@ mod folder { other => next = Some(other), } } - match job { + // A panic in a job (serializing a hostile project, say) is told like a failure and the + // thread goes on: autosave must not stop without a word (see `Session::run`). + let done = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| match job { Job::Write(project, about) => { let earlier = skipped.take(); if let Err(e) = write(dir, *project, &about) { @@ -603,6 +605,9 @@ mod folder { Job::Flush(done) => { let _ = done.send(()); } + })); + if done.is_err() { + fail("autosave failed unexpectedly; it goes on with the next copy".into()); } if next.is_none() { next = rx.recv().ok(); @@ -701,7 +706,7 @@ mod folder { } #[cfg(test)] - #[allow(clippy::unwrap_used)] + #[allow(clippy::unwrap_used, clippy::panic)] mod tests { use std::sync::mpsc::channel; use std::sync::{Arc, Mutex}; @@ -732,6 +737,35 @@ mod folder { (std::fs::read_to_string(dir.join(LIVE)).ok(), error.into_inner().unwrap()) } + /// Writes copies as the store does, except a project named "boom", whose copy panics. + fn boom_panics(dir: &std::path::Path, project: Project, about: &Recovery) -> Result<(), String> { + if project.name == "boom" { + panic!("a hostile project"); + } + super::write(dir, project, about) + } + + /// A job that panics is told as a failure and the thread goes on with the next one + /// (#896 review): autosave doesn't stop without a word. + #[test] + fn a_panicking_copy_is_told_and_the_next_one_still_written() { + let dir = std::env::temp_dir().join(format!("rustgis-autosave-panic-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let copy = |name: &str| Job::Write(Box::new(Project::new(name)), Recovery { name: name.into(), ..Recovery::default() }); + let (tx, rx) = channel(); + // A removal between keeps the two copies from being merged into one. + for job in [copy("boom"), Job::Clear, copy("after")] { + tx.send(job).unwrap(); + } + drop(tx); + let error = Mutex::new(None); + work_with(&dir, &rx, &error, &Arc::default(), boom_panics); + assert!(std::fs::read_to_string(dir.join(LIVE)).unwrap().contains("\"after\"")); + assert!(error.into_inner().unwrap().unwrap().contains("unexpectedly")); + let _ = std::fs::remove_dir_all(&dir); + } + /// A skipped copy is written when the newer one that replaced it fails (#896 review), and /// still in order: a removal queued after both removes it. #[test] diff --git a/docs/user-guide/workspace.md b/docs/user-guide/workspace.md index a420f5443..3198223d5 100644 --- a/docs/user-guide/workspace.md +++ b/docs/user-guide/workspace.md @@ -48,7 +48,8 @@ deletes it. `ui.autosave` reports the interval and any copy waiting. Quitting doesn't ask about unsaved changes, and their copy is removed: save before you quit. A second copy of RustGIS running alongside the first doesn't autosave, so it can't replace the first -one's copy. The browser app doesn't autosave yet; it warns before you leave a page with unsaved +one's copy. Nor does RustGIS autosave when its settings folder can't lock files (some network +or synced folders), where it can't tell a second copy apart. The browser app doesn't autosave yet; it warns before you leave a page with unsaved changes. ### QGIS projects and styles @@ -245,7 +246,7 @@ duplication and nudging have their own [layout shortcuts](layouts.md#layouts). start keeps its own projection and view; the Projection and Extent settings apply to the sample and a blank map. If the project no longer opens (moved or deleted), RustGIS starts with the sample and says why. After a crash it starts with the sample too, in case the project is what crashed, and offers - back any [unsaved work](#autosave-and-recovery); + back the [recovery copy](#autosave-and-recovery) autosave kept of unsaved work, if any; starting `rustgis --sample` also skips the start-up project. The choices are remembered on this computer, or in this browser for the web app. Opening files from the command line starts a blank map. From c1ba9d0e9fcdca38d29f70c27fe88d05f7115d0d Mon Sep 17 00:00:00 2001 From: mavrick Date: Sun, 25 Oct 2026 16:58:46 -0500 Subject: [PATCH 6/8] fix(ui): say why a run doesn't autosave; a panicking copy falls back too Review follow-ups for #896: - running::start returns why there is no run marker (another copy holds it, or the settings folder can't be written), and the app shows "Autosave is off: ..." for that and for a marker it couldn't lock, as it does when the recovery folder doesn't open. - A copy whose writing panics comes back as a failed write, so the copy skipped for it is written instead. Co-Authored-By: Claude Opus 5.5 --- apps/rustgis/src/main.rs | 43 ++++++++++++++++++++-------------- crates/ui-egui/src/autosave.rs | 23 +++++++++++++++--- docs/user-guide/workspace.md | 6 ++--- 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/apps/rustgis/src/main.rs b/apps/rustgis/src/main.rs index 825ac75f0..5dc2a1e88 100644 --- a/apps/rustgis/src/main.rs +++ b/apps/rustgis/src/main.rs @@ -164,24 +164,24 @@ mod running { locked: bool, } - /// Takes `/running` for this run: the marker (none when another copy holds it or the - /// folder can't be written) and whether the previous run didn't close cleanly. - pub fn start(dir: Option<&Path>) -> (Option, bool) { - let Some(dir) = dir else { return (None, false) }; + /// Takes `/running` for this run: the marker, or why there is none (another copy holds + /// it, or the folder can't be written), and whether the previous run didn't close cleanly. + pub fn start(dir: Option<&Path>) -> (Result, bool) { + let Some(dir) = dir else { return (Err("this computer has no settings folder for RustGIS".into()), false) }; let file = std::fs::create_dir_all(dir) .and_then(|_| OpenOptions::new().read(true).write(true).create(true).truncate(false).open(dir.join("running"))); let mut file = match file { Ok(f) => f, Err(e) => { log::warn!("no run marker in {} ({e}): an unclean exit won't be noticed", dir.display()); - return (None, false); + return (Err(format!("the settings folder {} can't be written ({e})", dir.display())), false); } }; let locked = match file.try_lock() { Ok(()) => true, Err(std::fs::TryLockError::WouldBlock) => { log::info!("the run marker is held: another copy of RustGIS is running"); - return (None, false); + return (Err("another copy of RustGIS is running, and only the first keeps a recovery copy".into()), false); } // The folder can't lock files (some network or FUSE folders): keep the marker unlocked, // so unclean exits are still noticed. Without a lock, copies running side by side share @@ -208,7 +208,7 @@ mod running { if let Err(e) = marked { log::warn!("could not mark this run: {e}"); } - (Some(Marker { file, locked }), unclean) + (Ok(Marker { file, locked }), unclean) } impl Marker { @@ -346,13 +346,19 @@ fn main() -> eframe::Result { let mut services = prefs::services(); // Autosave, in a recovery folder only the copy holding the run marker uses: a second copy // running alongside would offer, or overwrite, the first one's unsaved work. - let mut no_autosave = None; - if marker.as_ref().is_some_and(running::Marker::locked) { - match recovery_store(rustgis_engine::data::dirs::app_data_dir()) { - Ok(store) => services.recovery = Some(store), - Err(e) => no_autosave = Some(e), - } - } + // Without the marker to itself, a run can't tell whether another copy uses the folder: it + // doesn't autosave, and says why. + let no_autosave = match &marker { + Ok(m) if m.locked() => match recovery_store(rustgis_engine::data::dirs::app_data_dir()) { + Ok(store) => { + services.recovery = Some(store); + None + } + Err(e) => Some(e), + }, + Ok(_) => Some("the settings folder can't lock files, so a second copy of RustGIS couldn't be told apart".into()), + Err(why) => Some(why.clone()), + }; let mut app = app.with_services(services); // Settings › Start-up; `--sample` still opens the sample. let blank = if args.iter().any(|a| a == "--sample") { Some(false) } else { (!files.is_empty()).then_some(true) }; @@ -402,7 +408,7 @@ fn main() -> eframe::Result { setup.instance_descriptor.backends = backends(backend, env.as_deref(), cfg!(windows)); } let options = eframe::NativeOptions { viewport, wgpu_options, ..Default::default() }; - let marker: running::Shared = std::sync::Arc::new(std::sync::Mutex::new(marker)); + let marker: running::Shared = std::sync::Arc::new(std::sync::Mutex::new(marker.ok())); let held = marker.clone(); let result = eframe::run_native("RustGIS", options, Box::new(move |_cc| Ok(Box::new(App(app, held))))); running::close(&marker); @@ -442,12 +448,13 @@ mod tests { // Removed however the test ends (declared first, so dropped after the markers). let _cleanup = TempDir(dir.clone()); let (first, unclean) = running::start(Some(&dir)); - assert!(first.is_some() && !unclean, "a first run"); + assert!(first.is_ok() && !unclean, "a first run"); // Held alone: autosave may use the recovery folder. - assert!(first.as_ref().is_some_and(running::Marker::locked)); + assert!(first.as_ref().is_ok_and(running::Marker::locked)); // Another copy running alongside can't take it, and doesn't count as a crash. let (second, unclean) = running::start(Some(&dir)); - assert!(second.is_none() && !unclean); + // It says why, which the app shows as it starts without autosave. + assert!(second.as_ref().err().is_some_and(|why| why.contains("another copy")) && !unclean); // This run ends without closing (a crash): the next one notices, once it closes cleanly. drop(first); let (after_crash, unclean) = running::start(Some(&dir)); diff --git a/crates/ui-egui/src/autosave.rs b/crates/ui-egui/src/autosave.rs index ed89d281d..5748d6d7d 100644 --- a/crates/ui-egui/src/autosave.rs +++ b/crates/ui-egui/src/autosave.rs @@ -553,7 +553,8 @@ mod folder { } /// [`work`] with its writer. Each job runs guarded: a panic is told as a failure and the - /// thread goes on. A write followed by another is skipped, as only the latest copy matters, but kept until the next one is done: if that one fails (a project grown past the + /// thread goes on. A write followed by another is skipped, as only the latest copy matters, + /// but kept until the next one is done: if that one fails or panics (a project grown past the /// size a copy may have), the skipped one is written instead, before any later job, so the /// folder holds the newest copy that could be written. fn work_with(dir: &Path, rx: &Receiver, error: &Mutex>, wake: &Wake, write: WriteCopy) { @@ -564,6 +565,11 @@ mod folder { wake(); } }; + // A copy whose writing panics fails like any other, so the skipped one still stands in. + let write = |project: Project, about: &Recovery| { + std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| write(dir, project, about))) + .unwrap_or_else(|_| Err("writing the copy failed unexpectedly".into())) + }; let mut next = rx.recv().ok(); let mut skipped: Option<(Box, Recovery)> = None; while let Some(job) = next.take() { @@ -586,9 +592,9 @@ mod folder { let done = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| match job { Job::Write(project, about) => { let earlier = skipped.take(); - if let Err(e) = write(dir, *project, &about) { + if let Err(e) = write(*project, &about) { if let Some((project, about)) = earlier - && let Err(again) = write(dir, *project, &about) + && let Err(again) = write(*project, &about) { log::warn!("autosave: the copy before it failed too: {again}"); } @@ -763,6 +769,17 @@ mod folder { work_with(&dir, &rx, &error, &Arc::default(), boom_panics); assert!(std::fs::read_to_string(dir.join(LIVE)).unwrap().contains("\"after\"")); assert!(error.into_inner().unwrap().unwrap().contains("unexpectedly")); + // A copy skipped for one that panics is written instead (#896 review). + std::fs::remove_file(dir.join(LIVE)).unwrap(); + let (tx, rx) = channel(); + for job in [copy("older"), copy("boom")] { + tx.send(job).unwrap(); + } + drop(tx); + let error = Mutex::new(None); + work_with(&dir, &rx, &error, &Arc::default(), boom_panics); + assert!(std::fs::read_to_string(dir.join(LIVE)).unwrap().contains("\"older\"")); + assert!(error.into_inner().unwrap().unwrap().contains("unexpectedly")); let _ = std::fs::remove_dir_all(&dir); } diff --git a/docs/user-guide/workspace.md b/docs/user-guide/workspace.md index 3198223d5..1e6842757 100644 --- a/docs/user-guide/workspace.md +++ b/docs/user-guide/workspace.md @@ -48,9 +48,9 @@ deletes it. `ui.autosave` reports the interval and any copy waiting. Quitting doesn't ask about unsaved changes, and their copy is removed: save before you quit. A second copy of RustGIS running alongside the first doesn't autosave, so it can't replace the first -one's copy. Nor does RustGIS autosave when its settings folder can't lock files (some network -or synced folders), where it can't tell a second copy apart. The browser app doesn't autosave yet; it warns before you leave a page with unsaved -changes. +one's copy; it says so as it starts. Nor does RustGIS autosave when its settings folder can't lock +files (some network or synced folders), where it can't tell a second copy apart. The browser app +doesn't autosave yet; it warns before you leave a page with unsaved changes. ### QGIS projects and styles From a5c08b20ed3fa89a6c93a33204157306e2ee71c0 Mon Sep 17 00:00:00 2001 From: mavrick Date: Sun, 25 Oct 2026 20:10:28 -0500 Subject: [PATCH 7/8] fix(ui): keep a viable copy with three writes queued; mark runs safely Review follow-ups for #896: - The autosave thread holds back one copy at a time: with one held, the current write is done (falling back to the held copy) before the next queued job is looked at, so a viable older copy is never dropped when newer ones can't be written. - The run marker is written over its old content and then cut to length, instead of emptied first: a write that fails leaves a set marker set, so the next launch still notices an unclean exit. Co-Authored-By: Claude Opus 5.5 --- apps/rustgis/src/main.rs | 6 +++++- crates/ui-egui/src/autosave.rs | 11 +++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/apps/rustgis/src/main.rs b/apps/rustgis/src/main.rs index 5dc2a1e88..874520507 100644 --- a/apps/rustgis/src/main.rs +++ b/apps/rustgis/src/main.rs @@ -204,7 +204,11 @@ mod running { true } }; - let marked = file.set_len(0).and_then(|_| file.rewind()).and_then(|_| file.write_all(b"running")).and_then(|_| file.flush()); + // Written over what is there, then cut to its length: a write that fails leaves the old + // content, so a marker that was set never reads as a clean exit. + const RUNNING: &[u8] = b"running"; + let marked = + file.rewind().and_then(|_| file.write_all(RUNNING)).and_then(|_| file.set_len(RUNNING.len() as u64)).and_then(|_| file.flush()); if let Err(e) = marked { log::warn!("could not mark this run: {e}"); } diff --git a/crates/ui-egui/src/autosave.rs b/crates/ui-egui/src/autosave.rs index 5748d6d7d..1d1e51fa7 100644 --- a/crates/ui-egui/src/autosave.rs +++ b/crates/ui-egui/src/autosave.rs @@ -573,7 +573,10 @@ mod folder { let mut next = rx.recv().ok(); let mut skipped: Option<(Box, Recovery)> = None; while let Some(job) = next.take() { + // One copy is held back at a time: with one held, this write is done (falling back to + // the held one) before another is looked at, so a viable older copy is never dropped. if let Job::Write(..) = job + && skipped.is_none() && let Ok(later) = rx.try_recv() { match later { @@ -721,10 +724,10 @@ mod folder { use super::{Job, LIVE, Recovery, work_with}; - /// Writes copies as the store does, except a project named "newer", which is refused + /// Writes copies as the store does, except projects named "newer…", which are refused /// as too large. fn newer_fails(dir: &std::path::Path, project: Project, about: &Recovery) -> Result<(), String> { - if project.name == "newer" { + if project.name.starts_with("newer") { return Err("the project is too large to keep a recovery copy".into()); } super::write(dir, project, about) @@ -796,6 +799,10 @@ mod folder { assert!(error.unwrap().contains("too large"), "the newer one's failure is told"); let (held, _) = run(&dir, vec![copy("older"), copy("newer"), Job::Clear]); assert!(held.is_none(), "removed after, as asked"); + // Three queued (#896 review): the two newer ones fail, the oldest is still written. + let (held, error) = run(&dir, vec![copy("older"), copy("newer"), copy("newer still")]); + assert!(held.unwrap().contains("\"older\""), "the viable copy is kept"); + assert!(error.is_some()); let _ = std::fs::remove_dir_all(&dir); } } From 8597b85215f517c6bae54c78d270d59613c5eacf Mon Sep 17 00:00:00 2001 From: mavrick Date: Sun, 25 Oct 2026 22:09:37 -0500 Subject: [PATCH 8/8] fix(ui): leaner recovery reads; untitled for non-Unicode paths; skip doomed copies Review follow-ups for #896: - read_pending drops the description line in place, so a copy near the size limit isn't held twice. - A project file name that isn't Unicode is described as untitled rather than by a lossy spelling, so Save after Recover asks where to write. - A failure to make the recovery folder private is logged. - A copy queued and cleared at once (a save, the exit) isn't written, so quitting waits only for a write already under way. - A recovered copy that couldn't become this run's copy is removed by the next clear or newer copy instead of being offered again after a save. - Test comments describe behaviour, not review history. Co-Authored-By: Claude Opus 5.5 --- apps/rustgis/src/main.rs | 2 +- crates/ui-egui/src/autosave.rs | 100 +++++++++++++++++++++++-------- crates/ui-egui/tests/autosave.rs | 4 +- 3 files changed, 78 insertions(+), 28 deletions(-) diff --git a/apps/rustgis/src/main.rs b/apps/rustgis/src/main.rs index 874520507..ef6dd9a66 100644 --- a/apps/rustgis/src/main.rs +++ b/apps/rustgis/src/main.rs @@ -496,7 +496,7 @@ mod tests { } /// A recovery folder that can't be made (a file in its way) or no app-data folder at all gives - /// a reason, which the app shows as it starts without autosave (#896 review); a usable one + /// a reason, which the app shows as it starts without autosave; a usable one /// gives the store. #[test] fn a_recovery_folder_that_does_not_open_says_why() { diff --git a/crates/ui-egui/src/autosave.rs b/crates/ui-egui/src/autosave.rs index 1d1e51fa7..2c9522323 100644 --- a/crates/ui-egui/src/autosave.rs +++ b/crates/ui-egui/src/autosave.rs @@ -121,8 +121,11 @@ fn now_secs() -> u64 { } /// The open project's description for its recovery copy. +/// A file name that isn't Unicode (legal on Unix) can't be kept exactly in the copy's description: +/// the project is described as untitled then, so Save asks where to write rather than writing to +/// a lossy spelling of the name. fn about(app: &RustGisApp) -> Recovery { - let path = app.session.path.as_ref().map(|p| std::path::absolute(p).unwrap_or_else(|_| p.clone()).display().to_string()); + let path = app.session.path.as_ref().and_then(|p| std::path::absolute(p).unwrap_or_else(|_| p.clone()).to_str().map(str::to_string)); Recovery { name: app.session.project.name.clone(), path, time: now_secs() } } @@ -445,7 +448,9 @@ mod folder { #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - let _ = std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700)); + if let Err(e) = std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700)) { + log::warn!("autosave: could not make {} private: {e}", dir.display()); + } } let live = dir.join(LIVE); if live.is_file() { @@ -572,15 +577,24 @@ mod folder { }; let mut next = rx.recv().ok(); let mut skipped: Option<(Box, Recovery)> = None; + // Recovered copies that couldn't become this run's copy: removed once a newer copy is + // written or the copy is cleared (until then, a crash leaves them to be offered again). + let mut leftover: Vec = Vec::new(); while let Some(job) = next.take() { - // One copy is held back at a time: with one held, this write is done (falling back to - // the held one) before another is looked at, so a viable older copy is never dropped. if let Job::Write(..) = job - && skipped.is_none() && let Ok(later) = rx.try_recv() { match later { - Job::Write(..) => { + // Removed as soon as written (a save, a clean exit): neither copy is made, so + // an exit doesn't wait for one. + Job::Clear => { + skipped = None; + next = Some(later); + continue; + } + // One copy is held back at a time: with one held, this write is done (falling + // back to the held one) before the next, so a viable older copy is never dropped. + Job::Write(..) if skipped.is_none() => { if let Job::Write(project, about) = job { skipped = Some((project, about)); } @@ -595,19 +609,29 @@ mod folder { let done = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| match job { Job::Write(project, about) => { let earlier = skipped.take(); - if let Err(e) = write(*project, &about) { - if let Some((project, about)) = earlier - && let Err(again) = write(*project, &about) - { - log::warn!("autosave: the copy before it failed too: {again}"); + let written = match write(*project, &about) { + Ok(()) => true, + Err(e) => { + let fallback = earlier.map(|(project, about)| write(*project, &about)); + if let Some(Err(again)) = &fallback { + log::warn!("autosave: the copy before it failed too: {again}"); + } + fail(e); + matches!(fallback, Some(Ok(()))) } - fail(e); + }; + if written { + leftover.drain(..).for_each(|p| remove(&p)); } } - Job::Clear => remove(&dir.join(LIVE)), + Job::Clear => { + remove(&dir.join(LIVE)); + leftover.drain(..).for_each(|p| remove(&p)); + } Job::Remove(path) => remove(&path), Job::Adopt(path) => { if let Err(e) = std::fs::rename(&path, dir.join(LIVE)) { + leftover.push(path); fail(format!("could not keep the recovered copy: {e}")); } } @@ -687,9 +711,11 @@ mod folder { if bytes.len() as u64 > rustgis_engine::project::MAX_BYTES + MAX_HEADER { return Err("the recovery copy is too large".into()); } - let text = String::from_utf8(bytes).map_err(|_| "the recovery copy isn't text".to_string())?; - let (_, project) = text.split_once('\n').ok_or("the recovery copy is incomplete")?; - Ok(project.to_string()) + // The description line goes in place: a copy near the size limit isn't held twice. + let mut text = String::from_utf8(bytes).map_err(|_| "the recovery copy isn't text".to_string())?; + let header = text.find('\n').ok_or("the recovery copy is incomplete")?; + text.drain(..=header); + Ok(text) } /// The offered copy's file goes after what is queued before it; the next one is offered. @@ -754,8 +780,8 @@ mod folder { super::write(dir, project, about) } - /// A job that panics is told as a failure and the thread goes on with the next one - /// (#896 review): autosave doesn't stop without a word. + /// A job that panics is told as a failure and the thread goes on with the next one: + /// autosave doesn't stop without a word. #[test] fn a_panicking_copy_is_told_and_the_next_one_still_written() { let dir = std::env::temp_dir().join(format!("rustgis-autosave-panic-{}", std::process::id())); @@ -763,8 +789,8 @@ mod folder { std::fs::create_dir_all(&dir).unwrap(); let copy = |name: &str| Job::Write(Box::new(Project::new(name)), Recovery { name: name.into(), ..Recovery::default() }); let (tx, rx) = channel(); - // A removal between keeps the two copies from being merged into one. - for job in [copy("boom"), Job::Clear, copy("after")] { + // A removal of another file between keeps the two copies from being merged into one. + for job in [copy("boom"), Job::Remove(dir.join("unrelated")), copy("after")] { tx.send(job).unwrap(); } drop(tx); @@ -772,7 +798,7 @@ mod folder { work_with(&dir, &rx, &error, &Arc::default(), boom_panics); assert!(std::fs::read_to_string(dir.join(LIVE)).unwrap().contains("\"after\"")); assert!(error.into_inner().unwrap().unwrap().contains("unexpectedly")); - // A copy skipped for one that panics is written instead (#896 review). + // A copy skipped for one that panics is written instead. std::fs::remove_file(dir.join(LIVE)).unwrap(); let (tx, rx) = channel(); for job in [copy("older"), copy("boom")] { @@ -786,7 +812,31 @@ mod folder { let _ = std::fs::remove_dir_all(&dir); } - /// A skipped copy is written when the newer one that replaced it fails (#896 review), and + /// A recovered copy that can't become this run's copy is still removed by the next clear, + /// so a save or a clean exit doesn't leave it to be offered again; a write followed at once + /// by a clear isn't made at all. + #[test] + fn a_copy_that_could_not_be_adopted_is_cleared_and_a_doomed_write_skipped() { + let dir = std::env::temp_dir().join(format!("rustgis-autosave-adopt-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let pending = dir.join("recovered-1.recovery"); + std::fs::write(&pending, "{}\n{}").unwrap(); + // A folder where this run's copy goes: the rename can't replace it. + std::fs::create_dir_all(dir.join(LIVE).join("blocked")).unwrap(); + let (held, error) = run(&dir, vec![Job::Adopt(pending.clone()), Job::Clear]); + assert!(held.is_none() && error.unwrap().contains("recovered copy")); + assert!(!pending.exists(), "removed by the clear"); + std::fs::remove_dir_all(dir.join(LIVE)).unwrap(); + // Written, then cleared at once: skipped, so nothing is written (and nothing waits). + // A copy the writer would refuse: had it been attempted, its failure would be told. + let copy = Job::Write(Box::new(Project::new("newer, doomed")), Recovery::default()); + let (held, error) = run(&dir, vec![copy, Job::Clear]); + assert!(held.is_none() && error.is_none()); + let _ = std::fs::remove_dir_all(&dir); + } + + /// A skipped copy is written when the newer one that replaced it fails, and /// still in order: a removal queued after both removes it. #[test] fn a_skipped_copy_stands_in_for_a_newer_one_that_failed() { @@ -799,7 +849,7 @@ mod folder { assert!(error.unwrap().contains("too large"), "the newer one's failure is told"); let (held, _) = run(&dir, vec![copy("older"), copy("newer"), Job::Clear]); assert!(held.is_none(), "removed after, as asked"); - // Three queued (#896 review): the two newer ones fail, the oldest is still written. + // Three queued: the two newer ones fail, the oldest is still written. let (held, error) = run(&dir, vec![copy("older"), copy("newer"), copy("newer still")]); assert!(held.unwrap().contains("\"older\""), "the viable copy is kept"); assert!(error.is_some()); @@ -858,7 +908,7 @@ mod tests { } } - /// A copy that failed in the background (#896 review) is made again at the next interval, + /// A copy that failed in the background is made again at the next interval, /// though the project hasn't changed since, and the failure is told once. #[test] fn a_failed_copy_is_retried() { @@ -886,7 +936,7 @@ mod tests { let told = app.session.notices.len(); tick(&mut app, &ctx); assert_eq!(app.session.notices.len(), told, "the same failure is told once"); - // Saved after a failure (#896 review): the copy an earlier write left is still removed. + // Saved after a failure: the copy an earlier write left is still removed. app.run( "project.saveAs", json!({"path": std::env::temp_dir().join(format!("rustgis-retry-{}.rgis", std::process::id())).display().to_string()}), diff --git a/crates/ui-egui/tests/autosave.rs b/crates/ui-egui/tests/autosave.rs index afa4fa517..4e80c3e29 100644 --- a/crates/ui-egui/tests/autosave.rs +++ b/crates/ui-egui/tests/autosave.rs @@ -326,7 +326,7 @@ fn recovery_commands_take_junk_parameters() { settle(&mut h); } -/// Work done before answering the dialog (#896 review): Recover waits until it is saved, rather +/// Work done before answering the dialog: Recover waits until it is saved, rather /// than failing and going for the rest of the session, and then recovers the copy. #[test] fn recover_returns_once_the_open_work_is_saved() { @@ -351,7 +351,7 @@ fn recover_returns_once_the_open_work_is_saved() { assert!(h.query_by_label("Recover unsaved work?").is_none()); } -/// A copy that fails in the background wakes the interface (#896 review), so the failure is told +/// A copy that fails in the background wakes the interface, so the failure is told /// on an idle window too, not at the next input. #[test] fn a_failed_copy_wakes_the_interface() {