Don't write over a library file that changed since it was saved

The name remembered for a user library file stays only while the file still
holds the bytes that were written. A file a device name could make, which can
no longer be read, is kept too, and the save goes to a new file.

Co-authored-by: Adithya Shankaran <adikshan11@users.noreply.github.com>

crates/engine/src/cmd/swatchlib.rs

@@ -104,9 +104,19 @@ pub struct Libraries<L = SwatchLibrary> {
     /// Loaded libraries copied into the user library folder ([`Libraries::copy_to_user`]): the
     /// loaded id and the User Defined id [`Libraries::get`] finds for it.
     copied: Vec<(String, String)>,
-    /// The name each User Defined file was saved under ([`Libraries::write`]), lower case id first:
-    /// a format that keeps no title (`.ase`) reads its name from the file name instead.
-    saved_as: HashMap<String, String>,
+    /// The name each User Defined file was saved under ([`Libraries::write`]), lower case id first,
+    /// with a hash of the bytes written. A format that keeps no title (`.ase`) reads its name from
+    /// the file name instead. [`Libraries::rescan`] drops an entry whose file no longer holds those
+    /// bytes, and keeps one whose file is missing so a file put back can be checked.
+    saved_as: HashMap<String, SavedAs>,
+}
+
+/// A User Defined file this session wrote: the library's name, and [`vectorcraft_doc::links::hash_bytes`]
+/// of what was written, so a file replaced since then is no longer that library's.
+#[derive(Clone, Debug)]
+struct SavedAs {
+    name: String,
+    hash: String,
 }
 
 #[derive(Clone, Debug)]
@@ -162,14 +172,23 @@ impl<L: LibraryFile> Libraries<L> {
     /// Re-read the library files of the user library folder (unreadable ones are skipped). The
     /// folder is the app's own: automation roots don't apply to it ([`crate::file_access`]). Each
     /// file is read inside [`crate::guard::catch_panic`], and a file whose reading panics is
-    /// skipped too.
+    /// skipped too. A file this session saved, whose bytes are no longer the ones written, is
+    /// forgotten ([`Libraries::write`]); a missing file is left remembered until it is back.
     pub fn rescan(&mut self) {
         self.extra.retain(|e| e.info.category != "user");
         let Some(dir) = self.user_dir.clone() else { return };
         for path in library_files(&dir, L::EXTS) {
             let file = file_name(&path);
-            let read = || crate::file_access::unconfined(|| read_file(&path)).ok().and_then(|b| L::read(&b, stem(&file)).ok());
-            let Some(lib) = crate::guard::catch_panic(read).ok().flatten() else { continue };
+            let read = || crate::file_access::unconfined(|| read_file(&path)).ok();
+            let Some(bytes) = crate::guard::catch_panic(read).ok().flatten() else { continue };
+            let id = format!("user/{file}").to_lowercase();
+            // Compared before parsing, so a file that no longer reads as a library is forgotten too.
+            let hash = vectorcraft_doc::links::hash_bytes(&bytes);
+            if self.saved_as.get(&id).is_some_and(|saved| saved.hash != hash) {
+                self.saved_as.remove(&id);
+            }
+            let parsed = || L::read(&bytes, stem(&file)).ok();
+            let Some(lib) = crate::guard::catch_panic(parsed).ok().flatten() else { continue };
             let info = LibraryInfo { id: format!("user/{file}"), name: library_tab_name(&file, lib.name()), category: "user" };
             self.extra.push(Extra { info, path: Some(path), lib: Arc::new(lib) });
         }
@@ -218,6 +237,9 @@ impl<L: LibraryFile> Libraries<L> {
             let mut file = library_file_name(name, ext);
             // `COM¹` is saved as `COM¹-`, which can be another library's own name, and a library put
             // under a numbered name holds a file another name can ask for: such a file is kept.
+            // `saved_as` is that memory for this run, and only while the file still holds what was
+            // written. A file that could not be read has no name to trust: one a device name could
+            // make is kept too. Any other unreadable file is replaced, as it was before.
             self.rescan();
             let device = crate::file_access::is_device_name;
             let id = format!("user/{file}").to_lowercase();
@@ -226,13 +248,19 @@ impl<L: LibraryFile> Libraries<L> {
             // have made has no known owner until it is saved here.
             let untitled = L::read(bytes, "\0").is_ok_and(|lib| lib.name() == "\0");
             let owner = |e: &Extra<L>| match self.saved_as.get(&e.info.id.to_lowercase()) {
-                Some(saved) => Some(saved.clone()),
+                Some(saved) => Some(saved.name.clone()),
                 None if untitled && device_file_name(e.lib.name()) => None,
                 None => Some(e.lib.name().to_string()),
             };
-            let held = self.extra.iter().any(|e| {
-                e.info.id.to_lowercase() == id && owner(e).is_none_or(|other| other != name && (device(name) || device(&other) || fallback(&other)))
-            });
+            let file_key = file.to_lowercase();
+            let on_disk = library_files(&dir, L::EXTS).iter().any(|p| file_name(p).to_lowercase() == file_key);
+            let listed = self.extra.iter().any(|e| e.info.category == "user" && e.info.id.to_lowercase() == id);
+            let unread = on_disk && !listed && (device(name) || device_file_name(stem(&file)));
+            let held = unread
+                || self.extra.iter().any(|e| {
+                    e.info.id.to_lowercase() == id
+                        && owner(e).is_none_or(|other| other != name && (device(name) || device(&other) || fallback(&other)))
+                });
             if held {
                 // Saved again, it goes back to the file it was given the first time.
                 let own = self
@@ -250,7 +278,8 @@ impl<L: LibraryFile> Libraries<L> {
             let path = std::path::Path::new(&dir).join(&file).to_string_lossy().to_string();
             // The app's own folder, under a name made safe: no root applies.
             crate::file_access::unconfined(|| create_dir(&dir).and_then(|()| write_file(&path, bytes)))?;
-            self.saved_as.insert(format!("user/{file}").to_lowercase(), name.to_string());
+            self.saved_as
+                .insert(format!("user/{file}").to_lowercase(), SavedAs { name: name.to_string(), hash: vectorcraft_doc::links::hash_bytes(bytes) });
             self.rescan();
             out["path"] = json!(path);
             out["library"] = json!(format!("user/{file}"));

crates/engine/src/tests/swatchlib.rs

@@ -832,6 +832,94 @@ fn device_named_libraries_without_a_title_keep_their_own_files() {
     }
 }
 
+/// A file this session saved for `COM¹`, then replaced, is not written over. The next save goes to
+/// a new file and stays there. That holds for `.ase` and `.gpl`, for bytes that still read as a
+/// library and for bytes that don't, including the file the library moved to. A file removed and
+/// put back is still its library's when the bytes are the ones written, and is kept when they are
+/// not. A name that is not a device name still replaces a file that cannot be read.
+#[test]
+fn a_replaced_device_library_file_is_kept() {
+    for format in ["ase", "gpl"] {
+        let other: Vec<u8> = if format == "ase" {
+            vectorcraft_testkit::ase::ase(&[vectorcraft_testkit::ase::Block::Color("Only", b"RGB ", &[1.0, 0.0, 0.0], 2)])
+        } else {
+            b"GIMP Palette\nName: Other\n0 0 0 Black\n".to_vec()
+        };
+        let save = |s: &mut Session| run(s, "swatch.library.save", json!({"user": true, "name": "COM¹", "format": format}));
+        let open = |name: &str| {
+            let dir = temp_dir(name);
+            let mut s = session();
+            s.swatch_libraries.set_user_dir(Some(dir.to_string_lossy().to_string()));
+            (dir, s)
+        };
+
+        // Replaced with another library.
+        let (dir, mut s) = open(&format!("replaced-lib-{format}"));
+        let first = save(&mut s);
+        let first_id = first["library"].as_str().unwrap().to_string();
+        let first_path = first["path"].as_str().unwrap().to_string();
+        assert_ne!(std::fs::read(&first_path).unwrap(), other);
+        std::fs::write(&first_path, &other).unwrap();
+        let moved = save(&mut s);
+        let moved_id = moved["library"].as_str().unwrap().to_string();
+        assert_ne!(moved_id, first_id);
+        assert_eq!(std::fs::read(&first_path).unwrap(), other, "{format}: the replaced file is left as it is");
+        assert_eq!(save(&mut s)["library"].as_str(), Some(moved_id.as_str()), "{format}: saving again replaces the new file");
+        let _ = std::fs::remove_dir_all(&dir);
+
+        // Replaced with bytes that are not a library, first the file it had, then the one it moved to.
+        let (dir, mut s) = open(&format!("replaced-unread-{format}"));
+        let first = save(&mut s);
+        let first_path = first["path"].as_str().unwrap().to_string();
+        std::fs::write(&first_path, b"not a library").unwrap();
+        let moved = save(&mut s);
+        let moved_id = moved["library"].as_str().unwrap().to_string();
+        let first_id = first["library"].as_str().unwrap();
+        assert_ne!(moved_id, first_id);
+        assert_eq!(std::fs::read(&first_path).unwrap(), b"not a library");
+        assert_eq!(save(&mut s)["library"].as_str(), Some(moved_id.as_str()), "{format}: saving again stays on the new file");
+        let moved_path = moved["path"].as_str().unwrap().to_string();
+        std::fs::write(&moved_path, b"still not").unwrap();
+        let again = save(&mut s);
+        let again_id = again["library"].as_str().unwrap().to_string();
+        assert!(again_id != moved_id && again_id != first_id);
+        assert_eq!(std::fs::read(&moved_path).unwrap(), b"still not");
+        assert_eq!(save(&mut s)["library"].as_str(), Some(again_id.as_str()));
+        let _ = std::fs::remove_dir_all(&dir);
+
+        // Removed, the folder listed again, then put back: the hash is checked when it reappears.
+        let (dir, mut s) = open(&format!("replaced-reappear-{format}"));
+        let first = save(&mut s);
+        let first_id = first["library"].as_str().unwrap().to_string();
+        let first_path = first["path"].as_str().unwrap().to_string();
+        let written = std::fs::read(&first_path).unwrap();
+        std::fs::remove_file(&first_path).unwrap();
+        run(&mut s, "swatch.library.list", json!({}));
+        std::fs::write(&first_path, &written).unwrap();
+        assert_eq!(save(&mut s)["library"].as_str(), Some(first_id.as_str()), "{format}: the same bytes are still this library");
+        std::fs::remove_file(&first_path).unwrap();
+        run(&mut s, "swatch.library.list", json!({}));
+        std::fs::write(&first_path, &other).unwrap();
+        let moved = save(&mut s);
+        let moved_id = moved["library"].as_str().unwrap().to_string();
+        assert_ne!(moved_id, first_id);
+        assert_eq!(std::fs::read(&first_path).unwrap(), other, "{format}: different bytes put back are kept");
+        assert_eq!(save(&mut s)["library"].as_str(), Some(moved_id.as_str()));
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    let dir = temp_dir("replaced-ordinary");
+    let mut s = session();
+    s.swatch_libraries.set_user_dir(Some(dir.to_string_lossy().to_string()));
+    let first = run(&mut s, "swatch.library.save", json!({"user": true, "name": "Greys", "format": "gpl"}));
+    let path = first["path"].as_str().unwrap().to_string();
+    std::fs::write(&path, b"not a library").unwrap();
+    let again = run(&mut s, "swatch.library.save", json!({"user": true, "name": "Greys", "format": "gpl"}));
+    assert_eq!(again["library"], first["library"], "a name that is not a device name replaces its own unreadable file");
+    assert!(std::fs::read(&path).unwrap().starts_with(b"GIMP Palette"));
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
 /// #3602: a library titled with a Windows device name is saved under a name that is not one.
 #[test]
 fn a_library_file_name_is_never_a_windows_device_name() {