improve flow

gpx-rs/engine/src/engine/command/apply.rs

@@ -16,8 +16,6 @@ pub enum CommandError {
     NothingToDo,
     /// The input data could not be parsed.
     InvalidData(String),
-    /// The engine is in the outdated state.
-    Outdated,
 }
 
 impl fmt::Display for CommandError {
@@ -25,7 +23,6 @@ impl fmt::Display for CommandError {
         match self {
             Self::NothingToDo => write!(f, "nothing to do"),
             Self::InvalidData(err) => write!(f, "invalid data: {err}"),
-            Self::Outdated => write!(f, "saved by a newer version"),
         }
     }
 }

gpx-rs/engine/src/engine/mod.rs

@@ -27,8 +27,6 @@ pub struct Engine {
     routing_buffer: RoutingBuffer,
     categories: TrackpointCategories,
     clipboard: Option<Clipboard>,
-    /// Whether what is stored was written by a newer version (see [`Engine::mark_outdated`]).
-    outdated: bool,
 }
 
 impl Engine {
@@ -137,7 +135,6 @@ impl Engine {
     /// from the ones that were here are reported as added, removed or modified (all the files
     /// that were here, as they are not the same objects), and the order as changed if it is.
     pub fn restore(&mut self, restored: Restored) -> Outcome {
-        self.outdated = false;
         let selection_before = self.selection.clone();
         let order_before = self.order.0.clone();
         let before = self.stack.current().cloned();
@@ -166,14 +163,6 @@ impl Engine {
         }
     }
 
-    /// Tells that what is stored was written by a newer version of the app, which cannot be read
-    /// (see [`crate::StorageError::Newer`]). The files are left as they are, but cannot be changed
-    /// from now on, as the changes could not be kept: [`Engine::execute`] refuses with
-    /// [`CommandError::Outdated`], until something that is not newer is restored.
-    pub fn mark_outdated(&mut self) {
-        self.outdated = true;
-    }
-
     /// A file as GPX (UTF-8), `None` if it does not exist.
     pub fn export(&self, id: &FileId, options: ExportOptions) -> Option<Vec<u8>> {
         let file = self.stack.current()?.get(id)?;
@@ -218,12 +207,8 @@ impl Engine {
     }
 
     /// Single entry point for every action coming from the frontend. Says what it changed, or why
-    /// it did nothing: an edit that fails or has nothing to do, or any change of the files while
-    /// they are outdated (see [`Engine::is_outdated`]).
+    /// it did nothing: an edit that fails or has nothing to do.
     pub fn execute(&mut self, action: Action) -> Result<Outcome, CommandError> {
-        if self.outdated {
-            return Err(CommandError::Outdated);
-        }
         let selection_before = self.selection.clone();
         let order_before = self.order.0.clone();
         let clipboard_before = self.clipboard.clone();

gpx-rs/engine/src/engine/tests.rs

@@ -1434,36 +1434,3 @@ fn test_overwriting_forgets_the_history() {
     engine.restore(restored_from(&engine, &ids, vec![]));
     assert!(!engine.can_undo());
 }
-
-#[test]
-fn test_files_cannot_be_changed_when_what_is_stored_is_from_a_newer_version() {
-    let mut engine = TestEngine::default();
-    new(&mut engine, "file");
-    let id = engine.order()[0];
-
-    engine.mark_outdated();
-
-    for action in [
-        Action::Edit(Command::Reverse(crate::Reverse)),
-        Action::Undo,
-        Action::Redo,
-        Action::Reorder {
-            file_ids: vec![id],
-            index: 0,
-        },
-    ] {
-        assert_eq!(engine.execute(action), Err(CommandError::Outdated));
-    }
-    assert_eq!(engine.order(), [id]);
-
-    // so is anything else, the selection included
-    let result = engine.execute(Action::Select {
-        selection: Selection::Empty,
-        mode: SelectMode::Replace,
-    });
-    assert_eq!(result, Err(CommandError::Outdated));
-
-    // reading a storage that is not newer lifts it
-    engine.restore(restored_from(&engine, &[id], vec![]));
-    assert_ne!(engine.execute(Action::Undo), Err(CommandError::Outdated));
-}

gpx-rs/engine/src/storage/persister.rs

@@ -171,6 +171,9 @@ impl Persister {
 
     /// Writes what changed since the last save. Nothing is written if nothing changed.
     ///
+    /// Nothing is written while what is stored comes from a newer version (`Failed(Newer)`, see
+    /// [`Persister::restore`]).
+    ///
     /// The write only happens if nothing else was committed since the persister read or wrote
     /// last: otherwise it is refused (`Stale`), nothing is written, and the persister has to read
     /// the storage again ([`Persister::restore`]) before it can write.
@@ -180,7 +183,7 @@ impl Persister {
         snapshot: Snapshot,
     ) -> Result<(), CommitError> {
         if self.read_only {
-            return Ok(());
+            return Err(StorageError::Newer.into());
         }
         let mut plan = self.plan(snapshot);
         if plan.batch.is_empty() {
@@ -525,11 +528,12 @@ mod tests {
             let result = block_on(persister.restore(&storage));
             assert_eq!(result.unwrap_err(), StorageError::Newer, "{what}");
 
-            // whatever is saved afterwards, nothing is written: the newer version needs its data
+            // whatever is saved afterwards is refused: the newer version needs its data
             let mut changed = snapshot.clone();
             changed.order.clear();
             changed.files.clear();
-            block_on(persister.save(&storage, changed)).unwrap();
+            let result = block_on(persister.save(&storage, changed));
+            assert_eq!(result, Err(CommitError::Failed(StorageError::Newer)), "{what}");
             assert!(storage.batches.borrow().is_empty(), "{what}");
             assert_eq!(storage.inner.counts(), before, "{what}");
         }

gpx-rs/wasm/src/persistence.rs

@@ -18,7 +18,8 @@ use crate::ts::{Outcome, StorageOpened};
 // wait: `schedule_save` starts a loop that writes what changed since the last write, until
 // nothing has. When another tab wrote in between, the write is refused (the storage has a newer
 // head than this tab last saw): the tab reads the storage again and overwrites what it holds, see
-// `sync_storage`. The settings are opaque to the engine, the frontend keeps its own copy of them.
+// `sync_storage`. When what is stored was written by a newer version of the app, the write is
+// refused too, and the frontend is told, so that it makes the user reload the page. The settings are opaque to the engine, the frontend keeps its own copy of them.
 
 thread_local! {
     static STORAGE: RefCell<Option<Rc<IdbStorage>>> = const { RefCell::new(None) };
@@ -28,10 +29,18 @@ thread_local! {
     /// Resolve functions of the promises of `flush_storage`.
     /// Called after the files were written, and when a write was refused because they were
     /// written by someone else since: see `open_storage`.
-    static LISTENERS: RefCell<Option<(js_sys::Function, js_sys::Function)>> = const { RefCell::new(None) };
+    static LISTENERS: RefCell<Option<Listeners>> = const { RefCell::new(None) };
     static WAITERS: RefCell<Vec<js_sys::Function>> = const { RefCell::new(Vec::new()) };
 }
 
+/// What the frontend is told: the files were written, a write was refused because another tab
+/// wrote since, a write was refused because what is stored comes from a newer version.
+struct Listeners {
+    saved: js_sys::Function,
+    behind: js_sys::Function,
+    newer: js_sys::Function,
+}
+
 #[wasm_bindgen]
 extern "C" {
     #[wasm_bindgen(js_namespace = console, js_name = error)]
@@ -52,7 +61,7 @@ pub(crate) fn schedule_save() {
         return;
     }
     wasm_bindgen_futures::spawn_local(async {
-        let (mut saved, mut behind) = (false, false);
+        let (mut saved, mut behind, mut newer) = (false, false, false);
         while DIRTY.replace(false) {
             let Some(storage) = storage() else { break };
             let Some(snapshot) = with_engine(Engine::snapshot) else {
@@ -71,16 +80,24 @@ pub(crate) fn schedule_save() {
                     DIRTY.set(false);
                     behind = true;
                 }
+                // nothing is written, so that the newer version finds its data again
+                Err(engine::CommitError::Failed(engine::StorageError::Newer)) => {
+                    DIRTY.set(false);
+                    newer = true;
+                }
                 // what failed is written with the next change
                 Err(error) => console_error(&error.to_string()),
             }
         }
         SAVING.set(false);
         if saved {
-            notify(|(saved, _)| saved);
+            notify(|listeners| &listeners.saved);
         }
         if behind {
-            notify(|(_, behind)| behind);
+            notify(|listeners| &listeners.behind);
+        }
+        if newer {
+            notify(|listeners| &listeners.newer);
         }
         for resolve in WAITERS.with(|waiters| std::mem::take(&mut *waiters.borrow_mut())) {
             let _ = resolve.call0(&JsValue::UNDEFINED);
@@ -88,7 +105,7 @@ pub(crate) fn schedule_save() {
     });
 }
 
-fn notify(pick: impl FnOnce(&(js_sys::Function, js_sys::Function)) -> &js_sys::Function) {
+fn notify(pick: impl FnOnce(&Listeners) -> &js_sys::Function) {
     let callback = LISTENERS.with(|l| l.borrow().as_ref().map(|l| pick(l).clone()));
     if let Some(callback) = callback {
         let _ = callback.call0(&JsValue::UNDEFINED);
@@ -98,17 +115,19 @@ fn notify(pick: impl FnOnce(&(js_sys::Function, js_sys::Function)) -> &js_sys::F
 /// Opens the database `name` (it is created if it does not exist), puts the files that it holds
 /// in the engine, which has to be empty, and returns the settings (JSON strings by key) and the
 /// outcome of putting the files in. The changes of the files that follow are saved, unless some of
-/// what is stored was written by a newer version of the app: then nothing is read, and the files
-/// cannot be changed (the outcome has the `Outdated` error), so that the newer version finds its
-/// data again.
+/// what is stored was written by a newer version of the app: then nothing is read and nothing is
+/// ever written, so that the newer version finds its data again.
 ///
-/// `on_saved` is called after the files were written (to tell the other tabs), and `on_behind` when
-/// a write was refused because another tab wrote since (call `sync_storage` then).
+/// `on_saved` is called after the files were written (to tell the other tabs), `on_behind` when
+/// a write was refused because another tab wrote since (call `sync_storage` then), and `on_newer`
+/// when what is stored comes from a newer version (now, or when it is read again): the page has to
+/// be reloaded, the changes are not kept.
 #[wasm_bindgen]
 pub async fn open_storage(
     name: &str,
     on_saved: js_sys::Function,
     on_behind: js_sys::Function,
+    on_newer: js_sys::Function,
 ) -> Result<StorageOpened, JsValue> {
     let empty = with_engine(|e| e.snapshot().files.is_empty()).unwrap_or(false);
     if !empty {
@@ -117,11 +136,22 @@ pub async fn open_storage(
     let storage = IdbStorage::open(name).await.map_err(to_js_error)?;
     let mut persister = Persister::default();
     let outcome = restore_engine(persister.restore(&storage).await).map_err(to_js_error)?;
+    let newer = outcome.is_none();
+    let outcome = outcome.unwrap_or_else(|| outcome_object(&engine::Outcome::default(), None));
     let settings = storage.load_settings().await.map_err(to_js_error)?;
 
     PERSISTER.with(|slot| *slot.borrow_mut() = Some(persister));
     STORAGE.with(|slot| *slot.borrow_mut() = Some(Rc::new(storage)));
-    LISTENERS.with(|slot| *slot.borrow_mut() = Some((on_saved, on_behind)));
+    LISTENERS.with(|slot| {
+        *slot.borrow_mut() = Some(Listeners {
+            saved: on_saved,
+            behind: on_behind,
+            newer: on_newer,
+        })
+    });
+    if newer {
+        notify(|listeners| &listeners.newer);
+    }
 
     let settings_object = Object::new();
     for (key, value) in settings {
@@ -134,20 +164,19 @@ pub async fn open_storage(
 }
 
 /// Overwrites the files of the engine with what was read, and tells what that did. If what is
-/// stored comes from a newer version of the app, the files are left as they are and cannot be
-/// changed anymore: the error of the outcome says so, as for the edits that are refused afterwards.
+/// stored comes from a newer version of the app, the files are left as they are and `None` is
+/// returned: the persister does not write anymore (see `Persister::save`).
 fn restore_engine(
     read: Result<engine::Restored, engine::StorageError>,
-) -> Result<Outcome, engine::StorageError> {
+) -> Result<Option<Outcome>, engine::StorageError> {
     match read {
-        Ok(restored) => Ok(match with_engine_mut(|engine| engine.restore(restored)) {
-            Some(outcome) => outcome_object(&outcome, None),
-            None => outcome_object(&engine::Outcome::default(), None),
-        }),
-        Err(engine::StorageError::Newer) => {
-            with_engine_mut(Engine::mark_outdated);
-            Ok(fail(engine::CommandError::Outdated.to_string()))
-        }
+        Ok(restored) => Ok(Some(
+            match with_engine_mut(|engine| engine.restore(restored)) {
+                Some(outcome) => outcome_object(&outcome, None),
+                None => outcome_object(&engine::Outcome::default(), None),
+            },
+        )),
+        Err(engine::StorageError::Newer) => Ok(None),
         Err(error) => Err(error),
     }
 }
@@ -191,7 +220,10 @@ pub async fn sync_storage() -> Result<Outcome, JsValue> {
         }
         let outcome = restore_engine(persister.restore(&*storage).await)?;
         DIRTY.set(false);
-        Ok(outcome)
+        if outcome.is_none() {
+            notify(|listeners| &listeners.newer);
+        }
+        Ok(outcome.unwrap_or_else(|| outcome_object(&engine::Outcome::default(), None)))
     }
     .await;
     PERSISTER.with(|slot| *slot.borrow_mut() = Some(persister));