From 406e7701cea6d5b94d964596e657874a5a45f13d Mon Sep 17 00:00:00 2001 From: Harald Date: Sat, 14 Nov 2026 17:56:45 +0100 Subject: [PATCH] fix(storage): merge the manifest instead of overwriting a change from another device A manifest changed by a sync (e.g. a page added on another device) could be overwritten by the next autosave before disk_watch took it over, which dropped that page from the list. save_manifest now notices that the file is not the one the notebook in memory is based on and merges the two, with that base as common ancestor; if there is nothing new to save here, the other version simply stays. Co-Authored-By: Claude Fable 5.2 --- docs/FILE_FORMAT.de.md | 1 + docs/FILE_FORMAT.md | 1 + src/disk_watch.rs | 109 ++++++++++++++++++++++++++++++++++++++++- src/storage.rs | 98 +++++++++++++++++++++++++++++++----- 4 files changed, 194 insertions(+), 15 deletions(-) diff --git a/docs/FILE_FORMAT.de.md b/docs/FILE_FORMAT.de.md index b04f184..5aa22a8 100644 --- a/docs/FILE_FORMAT.de.md +++ b/docs/FILE_FORMAT.de.md @@ -178,6 +178,7 @@ Liegen die Notizbücher in einem Ordner, den Nextcloud, Google Drive, Syncthing - Solange alle lokalen Änderungen gespeichert sind, prüft die App den Ordner alle 3 Sekunden (und wenn das Fenster den Fokus bekommt). Geänderte Seitendateien werden neu geladen, ein geändertes `manifest.json` bringt neue, entfernte oder umsortierte Seiten und Titel mit. - Konfliktkopien der Sync-Clients (`page_ (….json`, `page__….json`, `page_.sync-conflict-….json`) werden als neue Seiten hinter dem Original eingefügt und in `.imported` umbenannt. - Bevor die App eine Seitendatei überschreibt, die sich seit ihrem letzten Lesen oder Schreiben geändert hat, sichert sie den vorgefundenen Inhalt als `page_ (conflict ).json` – außer sie hat selbst nichts geändert (dann bleibt die Datei, wie sie ist). +- Dasselbe gilt für `manifest.json`, nur werden die beiden Fassungen zusammengeführt statt kopiert: Auf beiden Seiten hinzugefügte oder entfernte Seiten, Titel und Zähler werden kombiniert, mit dem zuletzt von der App gelesenen oder geschriebenen Manifest als gemeinsamem Vorgänger. ### Neuere Versionen Dateien mit einer höheren `format_version` als der unterstützten werden nicht geöffnet – so kann eine ältere App-Version keine Inhalte verlieren, die sie nicht kennt. diff --git a/docs/FILE_FORMAT.md b/docs/FILE_FORMAT.md index f83eda2..8cabecf 100644 --- a/docs/FILE_FORMAT.md +++ b/docs/FILE_FORMAT.md @@ -178,6 +178,7 @@ If the notebooks are kept in a folder synced by Nextcloud, Google Drive, Syncthi - While all local changes are saved, the app checks the folder every 3 seconds (and when the window gets the focus). Changed page files are reloaded, a changed `manifest.json` brings in new, removed or reordered pages and titles. - Conflict copies of sync clients (`page_ (….json`, `page__….json`, `page_.sync-conflict-….json`) are added as new pages after the original and renamed to `.imported`. - Before the app overwrites a page file that changed since it last read or wrote it, it keeps the content found there as `page_ (conflict ).json`, unless it has no changes of its own (then the file is left as it is). +- The same goes for `manifest.json`, except that the two versions are merged instead of copied: pages added or removed on either side, titles and counters are combined, with the manifest the app last read or wrote as common ancestor. ### Newer versions Files with a higher `format_version` than supported are not opened – this way an older app version cannot lose content it does not know. diff --git a/src/disk_watch.rs b/src/disk_watch.rs index 8397456..39ae942 100644 --- a/src/disk_watch.rs +++ b/src/disk_watch.rs @@ -166,12 +166,16 @@ impl DiskWatch { // Page order, titles, new and removed pages let mut fresh_pages: HashSet = HashSet::new(); if changed("manifest.json") { - match storage::load_manifest(&self.dir) { - Ok(theirs) => match self.apply_manifest(notebook, manifest, theirs) { + match storage::load_manifest_as_base(&self.dir) { + Ok((theirs, base)) => match self.apply_manifest(notebook, manifest, theirs) { + // Taken over: from now on our saves build on this manifest Ok(Some(new_ids)) => { + base.adopt(); fresh_pages = new_ids; outcome.structure_changed = true; } + // Nothing to take over for the pages. Whatever else differs is merged + // at the next save, so the base stays as it is. Ok(None) => {} Err(()) => { retry.insert("manifest.json".to_string()); @@ -557,6 +561,107 @@ mod tests { assert_eq!(conflict_copies(dir.path()).len(), 1); } + /// Writes the manifest the way a sync client does: behind the back of `storage`. + fn write_manifest_from_another_device(dir: &Path, manifest: &NotebookManifest) { + fs::write( + dir.join("manifest.json"), + serde_json::to_vec(manifest).unwrap(), + ) + .unwrap(); + } + + fn page_ids_on_disk(dir: &Path) -> Vec { + let bytes = fs::read(dir.join("manifest.json")).unwrap(); + let manifest: NotebookManifest = serde_json::from_slice(&bytes).unwrap(); + manifest.pages.iter().map(|p| p.id).collect() + } + + #[test] + fn test_saving_the_manifest_keeps_a_page_added_on_another_device() { + let dir = tempfile::tempdir().unwrap(); + let (mut notebook, mut manifest) = saved_notebook(dir.path()); + let now = Instant::now(); + let mut watch = DiskWatch::new(dir.path().to_path_buf(), now); + + // The other device adds a second page + let mut theirs = notebook.clone(); + let mut their_manifest = manifest.clone(); + let index = theirs.add_page(PageBackground::Lined); + theirs.pages[index].add_stroke(stroke(6, 400.0)); + theirs.next_stroke_id = 7; + their_manifest.sync_from_notebook(&theirs); + write_from_another_device(dir.path(), &theirs.pages[index]); + write_manifest_from_another_device(dir.path(), &their_manifest); + let their_page = theirs.pages[index].id; + + // Before the check ran, a stroke is drawn and saved here, twice + for id in [2, 3] { + notebook.current_page_mut().add_stroke(stroke(id, 200.0)); + notebook.next_stroke_id = id + 1; + manifest.sync_from_notebook(¬ebook); + storage::save_page(dir.path(), &PageData::from(notebook.current_page())).unwrap(); + storage::save_manifest(dir.path(), &manifest).unwrap(); + assert_eq!(page_ids_on_disk(dir.path()), vec![1, their_page]); + } + + // The check takes their page over, and saving stays an ordinary save afterwards + let outcome = watch.check(&mut notebook, &mut manifest, now); + assert!(outcome.structure_changed); + assert_eq!(notebook.pages.len(), 2); + assert_eq!(notebook.pages[0].strokes.len(), 3); + assert!(notebook.next_stroke_id >= 7); + manifest.sync_from_notebook(¬ebook); + storage::save_manifest(dir.path(), &manifest).unwrap(); + assert_eq!(page_ids_on_disk(dir.path()), vec![1, their_page]); + + // A page removed here afterwards is really gone + notebook.pages.pop(); + notebook.current_page_index = 0; + manifest.sync_from_notebook(¬ebook); + storage::save_manifest(dir.path(), &manifest).unwrap(); + assert_eq!(page_ids_on_disk(dir.path()), vec![1]); + } + + #[test] + fn test_saving_an_unchanged_manifest_keeps_the_one_from_another_device() { + let dir = tempfile::tempdir().unwrap(); + let (notebook, manifest) = saved_notebook(dir.path()); + + let mut their_manifest = manifest.clone(); + their_manifest.title = "Physik".to_string(); + write_manifest_from_another_device(dir.path(), &their_manifest); + + storage::save_manifest(dir.path(), &manifest).unwrap(); + let bytes = fs::read(dir.path().join("manifest.json")).unwrap(); + let on_disk: NotebookManifest = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(on_disk.title, "Physik"); + drop(notebook); + } + + #[test] + fn test_manifest_changed_on_both_sides_is_merged() { + let dir = tempfile::tempdir().unwrap(); + let (mut notebook, mut manifest) = saved_notebook(dir.path()); + let ours_new = notebook.add_page(PageBackground::Blank); + let ours_new = notebook.pages[ours_new].id; + manifest.sync_from_notebook(¬ebook); + storage::save_manifest(dir.path(), &manifest).unwrap(); + + // They rename the notebook; we remove our new page again + let mut their_manifest = manifest.clone(); + their_manifest.title = "Physik".to_string(); + write_manifest_from_another_device(dir.path(), &their_manifest); + notebook.pages.pop(); + notebook.current_page_index = 0; + manifest.sync_from_notebook(¬ebook); + storage::save_manifest(dir.path(), &manifest).unwrap(); + + let bytes = fs::read(dir.path().join("manifest.json")).unwrap(); + let on_disk: NotebookManifest = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(on_disk.title, "Physik"); + assert!(!on_disk.pages.iter().any(|p| p.id == ours_new)); + } + #[test] fn test_page_still_being_synced_is_tried_again() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/storage.rs b/src/storage.rs index 5457d5d..1ad856e 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -590,16 +590,32 @@ pub fn atomic_write_json(path: &Path, value: &T) -> Result<(), Sto /// Speichert das Manifest atomar in `manifest.json`. /// Sichert das bisherige Manifest als `manifest.json.bak` vor dem Überschreiben. +/// +/// Like a page (see [`save_page`]), the manifest may have been changed behind our back by a +/// sync before `disk_watch` took the change over, e.g. the other device added a page. +/// Overwriting it would drop that page from the list. The manifest on disk is therefore +/// merged with ours, with the manifest the notebook in memory is based on as common ancestor +/// ([`ManifestBase`]); if we have nothing new ourselves, theirs simply stays. pub fn save_manifest(notebook_dir: &Path, manifest: &NotebookManifest) -> Result<(), StorageError> { let path = notebook_dir.join("manifest.json"); - if path.exists() { + let ours = serde_json::to_vec(manifest)?; + let key = seen_key(&path); + let mut merged = None; + if let Ok(theirs) = fs::read(&path) { + let base = manifest_bases().get(&key).cloned(); + if base.as_ref() != Some(&theirs) && theirs != ours { + if base.as_ref() == Some(&ours) { + // Nothing changed here since the notebook was loaded or last saved + return Ok(()); + } + // Theirs unreadable (e.g. half written): ours replaces it + merged = merge_manifests(base.as_deref(), &ours, &theirs); + } let bak_path = notebook_dir.join("manifest.json.v0.bak"); if !bak_path.exists() { - if let Ok(content) = fs::read_to_string(&path) { - if let Ok(old_m) = serde_json::from_str::(&content) { - if old_m.format_version == 0 { - let _ = fs::copy(&path, &bak_path); - } + if let Ok(old_m) = serde_json::from_slice::(&theirs) { + if old_m.format_version == 0 { + let _ = fs::copy(&path, &bak_path); } } } @@ -607,10 +623,51 @@ pub fn save_manifest(notebook_dir: &Path, manifest: &NotebookManifest) -> Result let rolling_bak = notebook_dir.join("manifest.json.bak"); let _ = fs::copy(&path, &rolling_bak); } - atomic_write_json(&path, manifest) + atomic_write_file(&path, merged.as_ref().unwrap_or(&ours))?; + // Ours, not the merged file: the notebook in memory does not contain their part until + // `disk_watch` has taken it over, so the next save has to merge it in again + manifest_bases().insert(key, ours); + Ok(()) } -fn load_manifest_from_file(path: &Path) -> Result { +/// Three-way merge of manifests as JSON (page lists by page id, see +/// `cloud_sync::merge_json`); `None` if one of them can't be read or the result is no +/// valid manifest. +fn merge_manifests(base: Option<&[u8]>, ours: &[u8], theirs: &[u8]) -> Option> { + let ours: serde_json::Value = serde_json::from_slice(ours).ok()?; + let theirs: serde_json::Value = serde_json::from_slice(theirs).ok()?; + let base: Option = base.and_then(|b| serde_json::from_slice(b).ok()); + let merged = crate::cloud_sync::merge_json(base.as_ref(), &ours, &theirs); + serde_json::from_value::(merged.clone()).ok()?; + serde_json::to_vec(&merged).ok() +} + +/// Content of `manifest.json` that the notebooks in memory are based on, by file. +fn manifest_bases() -> std::sync::MutexGuard<'static, std::collections::HashMap>> { + static BASES: std::sync::OnceLock< + std::sync::Mutex>>, + > = std::sync::OnceLock::new(); + BASES + .get_or_init(Default::default) + .lock() + .unwrap_or_else(|e| e.into_inner()) +} + +/// A manifest as read from disk, to be declared the state the notebook in memory is based on +/// once it has been taken over (see [`save_manifest`]). Reading a manifest alone, e.g. for +/// the list of notebooks, must not do that. +pub struct ManifestBase { + key: PathBuf, + bytes: Vec, +} + +impl ManifestBase { + pub fn adopt(self) { + manifest_bases().insert(self.key, self.bytes); + } +} + +fn load_manifest_from_file(path: &Path) -> Result<(NotebookManifest, Vec), StorageError> { let mut file = File::open(path)?; let mut json_str = String::new(); file.read_to_string(&mut json_str)?; @@ -630,7 +687,7 @@ fn load_manifest_from_file(path: &Path) -> Result Result/`. A copied or renamed folder still carries the id of the original in its /// manifest, so the id is always taken from the folder. pub fn load_manifest(notebook_dir: &Path) -> Result { - let mut manifest = load_manifest_or_backup(notebook_dir)?; + load_manifest_as_base(notebook_dir).map(|(manifest, _)| manifest) +} + +/// Like [`load_manifest`], for a manifest that is taken over into the open notebook: call +/// [`ManifestBase::adopt`] once that has happened. +pub fn load_manifest_as_base( + notebook_dir: &Path, +) -> Result<(NotebookManifest, ManifestBase), StorageError> { + let (mut manifest, bytes) = load_manifest_or_backup(notebook_dir)?; // Canonical, so that a path like `.` still yields the folder name let dir = notebook_dir.canonicalize(); let name = dir @@ -653,10 +718,16 @@ pub fn load_manifest(notebook_dir: &Path) -> Result Result { +fn load_manifest_or_backup( + notebook_dir: &Path, +) -> Result<(NotebookManifest, Vec), StorageError> { let path = notebook_dir.join("manifest.json"); if !path.exists() { let bak_path = notebook_dir.join("manifest.json.bak"); @@ -871,7 +942,8 @@ pub fn save_notebook( /// Lädt ein Notizbuch aus einem Ordner (erste Seite sofort, Folgeseiten als Platzhalter für Lazy Loading). pub fn load_notebook(notebook_dir: &Path) -> Result<(Notebook, NotebookManifest), StorageError> { let _ = cleanup_orphaned_temp_files(notebook_dir, std::time::Duration::from_secs(3600)); - let manifest = load_manifest(notebook_dir)?; + let (manifest, base) = load_manifest_as_base(notebook_dir)?; + base.adopt(); let page_ids = manifest.page_ids(); if page_ids.is_empty() { let nb = Notebook::new();