outdated state

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

@@ -16,6 +16,8 @@ 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 {
@@ -23,6 +25,7 @@ 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,9 +27,13 @@ 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,
     /// What the tests of the last action read, see `Engine::run`.
     #[cfg(test)]
     last: Outcome,
+    #[cfg(test)]
+    last_error: Option<CommandError>,
 }
 
 impl Engine {
@@ -138,6 +142,7 @@ 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();
@@ -163,15 +168,23 @@ impl Engine {
             order_changed: self.order.0 != order_before,
             selection_changed: self.selection != selection_before,
             clipboard_changed: false,
-            error: None,
         };
         #[cfg(test)]
         {
             self.last = outcome.clone();
+            self.last_error = None;
         }
         outcome
     }
 
+    /// 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)?;
@@ -215,19 +228,30 @@ impl Engine {
         self.coordinates_cache.waypoints(id)
     }
 
-    /// Single entry point for every action coming from the frontend. Says what it changed.
-    pub fn execute(&mut self, action: Action) -> Outcome {
+    /// 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`]).
+    pub fn execute(&mut self, action: Action) -> Result<Outcome, CommandError> {
+        let result = self.apply(action);
+        #[cfg(test)]
+        {
+            self.last = result.clone().unwrap_or_default();
+            self.last_error = result.as_ref().err().cloned();
+        }
+        result
+    }
+
+    fn apply(&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();
         let is_edit = matches!(action, Action::Edit(_));
-        let mut error = None;
         // the files change iff there is a diff, but selection and order actions have none
         let mut diff = match action {
-            Action::Edit(command) => self.edit(command).unwrap_or_else(|err| {
-                error = Some(err);
-                None
-            }),
+            Action::Edit(command) => self.edit(command)?,
             Action::Undo => self.stack.undo(),
             Action::Redo => self.stack.redo(),
             Action::Select { selection, mode } => {
@@ -264,27 +288,21 @@ impl Engine {
             || self.selection != selection_before
             || self.order.0 != order_before
             || self.clipboard != clipboard_before;
-        if is_edit && !changed && error.is_none() {
-            error = Some(CommandError::NothingToDo);
+        if is_edit && !changed {
+            return Err(CommandError::NothingToDo);
         }
         if changed {
             self.refresh(diff.as_ref());
             sort_diff(&mut diff, &self.order.0);
         }
         // after the refresh, which also syncs the order and selection with the files (undo, redo)
-        let outcome = Outcome {
+        Ok(Outcome {
             changed,
             diff,
             order_changed: self.order.0 != order_before,
             selection_changed: self.selection != selection_before,
             clipboard_changed: self.clipboard != clipboard_before,
-            error,
-        };
-        #[cfg(test)]
-        {
-            self.last = outcome.clone();
-        }
-        outcome
+        })
     }
 
     /// Puts the selection in the clipboard, if there is one.
@@ -352,7 +370,7 @@ impl Engine {
     /// Executes an action and tells whether it changed anything. What it did is kept for the
     /// accessors below, which the tests read right after.
     pub(crate) fn run(&mut self, action: Action) -> bool {
-        self.execute(action).changed
+        self.execute(action).is_ok_and(|outcome| outcome.changed)
     }
 
     pub(crate) fn last_diff(&self) -> Option<&Diff> {
@@ -360,7 +378,7 @@ impl Engine {
     }
 
     pub(crate) fn last_error(&self) -> Option<&CommandError> {
-        self.last.error.as_ref()
+        self.last_error.as_ref()
     }
 
     pub(crate) fn order_changed(&self) -> bool {

gpx-rs/engine/src/engine/outcome.rs

@@ -1,6 +1,7 @@
-use crate::{CommandError, Diff};
+use crate::Diff;
 
-/// What an action did, returned by [`crate::Engine::execute`] and [`crate::Engine::restore`].
+/// What an action did, returned by [`crate::Engine::execute`] (unless it was refused) and
+/// [`crate::Engine::restore`].
 ///
 /// It describes that action only: it is not kept by the engine, so reading it never depends on
 /// what was done since.
@@ -15,7 +16,4 @@ pub struct Outcome {
     pub order_changed: bool,
     pub selection_changed: bool,
     pub clipboard_changed: bool,
-    /// Why an edit did nothing: the command failed, or it had nothing to change. `None` if it
-    /// succeeded, and for the actions that do not edit the files.
-    pub error: Option<CommandError>,
 }

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

@@ -1268,46 +1268,49 @@ fn test_an_edit_that_changes_nothing_is_not_an_undo_step() {
 fn test_execute_returns_what_the_action_did() {
     let mut engine = Engine::default();
     let data = std::fs::read("data/simple.gpx").unwrap();
-    let outcome = engine.execute(Action::Edit(Command::Load(Load {
-        data: &data,
-        name: "file",
-    })));
-    assert!(outcome.changed && outcome.error.is_none());
+    let outcome = engine
+        .execute(Action::Edit(Command::Load(Load {
+            data: &data,
+            name: "file",
+        })))
+        .unwrap();
+    assert!(outcome.changed);
     assert_eq!(outcome.diff.as_ref().unwrap().added, engine.order());
     assert!(outcome.order_changed && outcome.selection_changed);
     assert!(!outcome.clipboard_changed);
 
     // the outcome of an action does not depend on what is done after it
     let kept = outcome.clone();
-    engine.execute(Action::Undo);
+    engine.execute(Action::Undo).unwrap();
     assert_eq!(outcome, kept);
 
     // a selection only changes the selection
-    engine.execute(Action::Redo);
-    let outcome = engine.execute(Action::Select {
-        selection: files(engine.order()),
-        mode: SelectMode::Replace,
-    });
+    engine.execute(Action::Redo).unwrap();
+    let outcome = engine
+        .execute(Action::Select {
+            selection: files(engine.order()),
+            mode: SelectMode::Replace,
+        })
+        .unwrap();
     assert!(outcome.changed && outcome.selection_changed);
     assert!(outcome.diff.is_none() && !outcome.order_changed);
 
     // an edit with nothing to do says why
-    engine.execute(Action::Select {
-        selection: Selection::Empty,
-        mode: SelectMode::Replace,
-    });
-    let outcome = engine.execute(Action::Edit(Command::Reverse(crate::Reverse)));
-    assert!(!outcome.changed);
-    assert_eq!(outcome.error, Some(CommandError::NothingToDo));
-    assert_eq!(outcome.diff, None);
+    engine
+        .execute(Action::Select {
+            selection: Selection::Empty,
+            mode: SelectMode::Replace,
+        })
+        .unwrap();
+    let result = engine.execute(Action::Edit(Command::Reverse(crate::Reverse)));
+    assert_eq!(result, Err(CommandError::NothingToDo));
 
     // a failing one too
-    let outcome = engine.execute(Action::Edit(Command::Load(Load {
+    let result = engine.execute(Action::Edit(Command::Load(Load {
         data: b"<gpx><trk></gpx>",
         name: "file",
     })));
-    assert!(!outcome.changed);
-    assert!(matches!(outcome.error, Some(CommandError::InvalidData(_))));
+    assert!(matches!(result, Err(CommandError::InvalidData(_))));
 }
 
 // What the engine does when it is overwritten with what is stored
@@ -1322,8 +1325,6 @@ fn restored_from(engine: &Engine, keep: &[FileId], extra: Vec<Rc<File>>) -> crat
         order: files.iter().map(|file| file.id).collect(),
         files,
         categories: engine.categories().clone(),
-        unreadable: 0,
-        read_only: false,
     }
 }
 
@@ -1372,3 +1373,38 @@ 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 = Engine::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]);
+
+    // looking at the files still works
+    let outcome = engine
+        .execute(Action::Select {
+            selection: Selection::Empty,
+            mode: SelectMode::Replace,
+        })
+        .unwrap();
+    assert!(outcome.selection_changed);
+
+    // 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/mod.rs

@@ -31,11 +31,20 @@ use uuid::Uuid;
 use crate::FileId;
 
 #[derive(Debug, Clone, PartialEq, Eq)]
-pub struct StorageError(pub String);
+pub enum StorageError {
+    /// The storage could not be read or written.
+    Failed(String),
+    /// Some of what is stored was written by a newer version of the app, which this one cannot
+    /// read. Nothing is written then, so that the newer version finds its data again.
+    Newer,
+}
 
 impl std::fmt::Display for StorageError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "storage error: {}", self.0)
+        match self {
+            Self::Failed(message) => write!(f, "storage error: {message}"),
+            Self::Newer => write!(f, "stored by a newer version"),
+        }
     }
 }
 
@@ -215,7 +224,7 @@ mod tests {
 
     #[test]
     fn test_storage_error_is_readable() {
-        let error = StorageError("disk full".into());
+        let error = StorageError::Failed("disk full".into());
         assert_eq!(error.to_string(), "storage error: disk full");
         let _: &dyn std::error::Error = &error;
     }

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

@@ -28,12 +28,6 @@ pub struct Restored {
     pub files: Vec<Rc<File>>,
     pub order: Vec<FileId>,
     pub categories: TrackpointCategories,
-    /// The files that could not be read: they are dropped at the next save.
-    pub unreadable: usize,
-    /// Whether some of what is stored was written by a newer version, which this one cannot read.
-    /// Nothing is written then, so that the newer version finds its data again: the files that
-    /// could be read are only kept in memory, and the persister does not save.
-    pub read_only: bool,
 }
 
 /// Works out what to write to a [`Storage`] so that it holds a [`Snapshot`], knowing what it
@@ -82,6 +76,10 @@ struct Plan {
 
 impl Persister {
     /// Reads the files of the storage. The persister then knows that they are saved.
+    ///
+    /// If some of what is stored was written by a newer version, nothing is read (`Newer`), and the
+    /// persister does not save until it reads a storage that is not newer: the newer version
+    /// needs its data.
     pub async fn restore<S: Storage>(&mut self, storage: &S) -> Result<Restored, StorageError> {
         let data = storage.load().await?;
         *self = Self::default();
@@ -127,40 +125,42 @@ impl Persister {
                 .clone()
         };
 
-        let mut restored = Restored::default();
         let mut files: HashMap<FileId, Rc<File>> = HashMap::new();
         for (id, bytes) in &data.files {
             self.stored_files.insert(*id);
             let file = newer
                 .read(decode_file(bytes))
                 .and_then(|record| record.build(*id, &trackpoint_chunk, &waypoint_chunk));
-            match file {
-                Some(file) => {
-                    files.insert(*id, Rc::new(file));
-                }
-                None => restored.unreadable += 1,
+            // a file that cannot be read is left out, and dropped at the next save
+            if let Some(file) = file {
+                files.insert(*id, Rc::new(file));
             }
         }
+        if newer.0.get() {
+            // `*self` was reset: only what is needed to not write, and to know the storage
+            self.read_only = true;
+            self.head = data.head;
+            return Err(StorageError::Newer);
+        }
 
-        restored.order = stored_order
+        let mut order: Vec<FileId> = stored_order
             .iter()
             .flatten()
             .filter(|id| files.contains_key(id))
             .copied()
             .collect();
-        let ordered: HashSet<FileId> = restored.order.iter().copied().collect();
-        restored
-            .order
-            .extend(files.keys().filter(|id| !ordered.contains(id)).copied());
-        restored.files = restored.order.iter().map(|id| files[id].clone()).collect();
+        let ordered: HashSet<FileId> = order.iter().copied().collect();
+        order.extend(files.keys().filter(|id| !ordered.contains(id)).copied());
+        let restored = Restored {
+            files: order.iter().map(|id| files[id].clone()).collect(),
+            order,
+            categories: categories.into_inner(),
+        };
 
         self.files = files;
         self.chunks = chunk_bytes.into_keys().collect();
         self.order = stored_order;
-        restored.categories = categories.into_inner();
         self.head = data.head;
-        self.read_only = newer.0.get();
-        restored.read_only = self.read_only;
         Ok(restored)
     }
 
@@ -340,7 +340,6 @@ mod tests {
 
         let mut persister = Persister::default();
         let restored = block_on(persister.restore(&storage)).unwrap();
-        assert_eq!(restored.unreadable, 0);
         assert_eq!(restored.order, snapshot.order);
         assert_eq!(restored.categories, snapshot.categories);
         for file in &restored.files {
@@ -472,8 +471,6 @@ mod tests {
         .unwrap();
         let mut persister = Persister::default();
         let restored = block_on(persister.restore(&storage)).unwrap();
-        assert_eq!(restored.unreadable, 1);
-        assert!(!restored.read_only);
         assert_eq!(restored.files.len(), FILES.len() - 1);
         assert!(restored.files.iter().all(|file| file.id != broken));
 
@@ -525,8 +522,8 @@ mod tests {
             let (storage, snapshot) = stored_with(damage);
             let before = storage.inner.counts();
             let mut persister = Persister::default();
-            let restored = block_on(persister.restore(&storage)).unwrap();
-            assert!(restored.read_only, "{what}");
+            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
             let mut changed = snapshot.clone();
@@ -544,12 +541,11 @@ mod tests {
             batch.put_files.push((snapshot.order[0], from_the_future()));
         });
         let mut persister = Persister::default();
-        assert!(block_on(persister.restore(&storage)).unwrap().read_only);
+        assert!(block_on(persister.restore(&storage)).is_err());
 
         // the same persister, on a storage that is not newer
         let clean = Recording::default();
-        let restored = block_on(persister.restore(&clean)).unwrap();
-        assert!(!restored.read_only);
+        block_on(persister.restore(&clean)).unwrap();
         block_on(persister.save(&clean, snapshot_of(&FILES))).unwrap();
         assert_eq!(clean.inner.counts().files, FILES.len());
     }