From b8be148b4cf7c33d720e67bb323d5f496579749e Mon Sep 17 00:00:00 2001 From: aiongg Date: Sat, 14 Nov 2026 12:46:23 +0000 Subject: [PATCH] DES-10 review: link status after open, preflight without reading originals - Open queues every linked file's lookup (stat, then hash) for the background check instead of stating candidates on the UI thread. - links.list, links.update and relinkFolder use the open's status only while its check is still running; afterwards the file is looked at as it is now. - Preflight tells Modified from size, time and the remembered hash checks (the worker keeps them between passes) and the vector test from the mime type; it never loads a linked original. - A modification time of 0 never vouches for a file's bytes. - Migration keeps an unsettled time as UNVERIFIED_MTIME; recovery snapshots keep images not yet saved as links embedded. - Links panel test fixture has real files, so its Embedded assertions hold. --- crates/engine/src/asset_io.rs | 23 ++- crates/engine/src/cmd/links.rs | 21 ++- crates/engine/src/cmd/native_links.rs | 176 ++++++++++-------- crates/engine/src/cmd/native_links/tests.rs | 102 +++++++++- crates/engine/src/cmd/preflight.rs | 32 ++-- crates/engine/src/recovery.rs | 3 +- crates/ui-egui/src/panels/properties/tests.rs | 21 ++- crates/ui-egui/src/preflight_worker.rs | 12 +- crates/ui-egui/src/preflight_worker/tests.rs | 6 +- docs/linked-artwork.md | 6 +- 10 files changed, 290 insertions(+), 112 deletions(-) diff --git a/crates/engine/src/asset_io.rs b/crates/engine/src/asset_io.rs index 5cbcf500e..628a460bd 100644 --- a/crates/engine/src/asset_io.rs +++ b/crates/engine/src/asset_io.rs @@ -68,6 +68,12 @@ pub fn fingerprint_from(meta: &crate::io::Metadata) -> Fingerprint { Fingerprint { size: meta.len(), mtime } } +/// Whether `f` has a modification time to tell the file by: [`fingerprint_from`] gives 0 where +/// the file system reports none, and two such files of one size would look the same. +pub fn has_time(f: &Fingerprint) -> bool { + f.mtime != 0 +} + /// The files that may hold `source`'s bytes, in the order they are tried: the absolute path, /// then the relative one resolved against the document. Paths [`crate::io::document_path`] /// refuses are left out. @@ -127,7 +133,7 @@ fn read_candidate(path: &Path, source: &LinkedSource, trusted: Option, checks: &mut ImageChecks) - } } +/// Whether a linked image's files are there and none holds its bytes ([`linked_status`] +/// "modified"): Preflight's check, which reads a file only as the Links panel does, once per new +/// size and time (`checks`). +pub(crate) fn link_modified(a: &Asset, document: Option<&Path>, checks: &mut LinkListChecks) -> bool { + a.storage == Storage::Linked && linked_status(a, document, &mut checks.images).0 == "modified" +} + /// Is the linked file still the one the asset's bytes were loaded from, going by the size and /// time kept then ([`designcraft_doc::Asset::link_fingerprint`], [`loaded_fingerprint`])? Nothing /// is read when it is, also on the first check after a document is opened, which matters on a @@ -343,8 +350,10 @@ fn embedded_copy(a: &Asset) -> bool { /// [`status_cached`], and the file's size and time when a read found the asset's bytes in it /// although the fingerprint kept with the asset says nothing or something else: what -/// [`remember_link_files`] keeps, so the file isn't read again in another session. What the open -/// of the document found (`open`) answers first, while the asset's bytes are the ones it checked. +/// [`remember_link_files`] keeps, so the file isn't read again in another session. While the +/// background check the open of the document started (`open`) still has the asset's file to look +/// at, it is `checking`; after that the file is looked at as it is now, which may have changed +/// since. fn image_status( a: &designcraft_doc::Asset, document: Option<&Path>, @@ -355,8 +364,8 @@ fn image_status( return ("embedded", None); } if a.storage == Storage::Linked { - if let Some(status) = open.and_then(|o| o.status(a)) { - return (status.as_str(), None); + if open.is_some_and(|o| o.checking() && o.status(a) == Some(super::native_links::LinkStatus::Checking)) { + return ("checking", None); } return linked_status(a, document, checks); } @@ -419,9 +428,13 @@ pub(crate) fn checkable_size(len: usize) -> bool { /// while the file was changed in the last two: another change in that second keeps the time. A /// time in the future (clock skew, a file copied from another machine) counts as settled, or such /// a file would be read on every poll; a later save gives it a time unlike the remembered one. +/// A file without a time ([`crate::asset_io::has_time`]) never is. pub(crate) fn settled(f: &Fingerprint) -> bool { #[cfg(not(target_arch = "wasm32"))] { + if !crate::asset_io::has_time(f) { + return false; + } let now = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map_or(0, |d| i64::try_from(d.as_secs()).unwrap_or(i64::MAX)); f.mtime > now || now.saturating_sub(f.mtime) >= 2 } diff --git a/crates/engine/src/cmd/native_links.rs b/crates/engine/src/cmd/native_links.rs index 3fb0e58d7..156130b2a 100644 --- a/crates/engine/src/cmd/native_links.rs +++ b/crates/engine/src/cmd/native_links.rs @@ -42,7 +42,8 @@ pub fn specs() -> Vec { pub enum LinkStatus { /// A file holds the recorded bytes. Ok, - /// A file of the recorded size is being hashed. + /// The background check is looking for the file, and hashes one of the recorded size whose + /// time is new. Checking, /// Files are there, none with the recorded bytes. Modified, @@ -101,11 +102,13 @@ impl OpenLinks { /// One file check for the background thread. #[cfg_attr(target_arch = "wasm32", allow(dead_code))] enum Work { - /// Hash `candidates` (files of the recorded size) until one holds `hash`. - Verify { asset: AssetId, key: SourceKey, hash: String, len: u64, candidates: Vec }, - /// Compare the file at `path` with the embedded `bytes` of a document saved before linked - /// storage. - Migrate { asset: AssetId, key: SourceKey, bytes: Arc>, path: PathBuf, page: u32 }, + /// Find the file that holds `source`'s bytes: one at its absolute or relative path with the + /// recorded size and time, else one of the recorded size whose content hash is the recorded + /// one. + Find { asset: AssetId, key: SourceKey, source: LinkedSource }, + /// Compare the file at `source`'s path (the first of its candidates that has the size of the + /// embedded `bytes`) with the embedded bytes of a document saved before linked storage. + Migrate { asset: AssetId, key: SourceKey, bytes: Arc>, source: LinkedSource, page: u32 }, } /// What a [`Work`] found. `key` is the asset's bytes' key when the check started: a result for @@ -115,8 +118,10 @@ enum Outcome { /// The file at `path` holds the bytes; `fingerprint` is its size and time, when they can /// vouch for the bytes later ([`super::links::settled`]). Holds { asset: AssetId, key: SourceKey, path: PathBuf, fingerprint: Option }, - /// No candidate holds the bytes. + /// Files are there; none holds the bytes. Differs { asset: AssetId, key: SourceKey }, + /// No file at either path, or one that may not be followed. + Absent { asset: AssetId, key: SourceKey }, /// The embedded bytes are the file's. Confirmed { asset: AssetId, @@ -170,12 +175,12 @@ pub(crate) struct Opened { work: Vec, } -/// Point each linked asset of `d`, opened from the file `document`, at its file, and find which -/// file holds its bytes without reading any (a background check hashes the ones that need it, -/// [`start`]). A file found at the relative path becomes the link. With `migrate` (a document -/// opened from its file, neither from bytes nor recovered), embedded assets of a document saved -/// before linked storage whose linked file has the embedded bytes' size are queued for a byte -/// comparison. +/// Point each linked asset of `d`, opened from the file `document`, at its file, and queue the +/// look for the file that holds its bytes for the background check ([`start`]): nothing is read +/// or asked for its size here, so a share that doesn't answer can't stall the open. A file found +/// at the relative path becomes the link. With `migrate` (a document opened from its file, +/// neither from bytes nor recovered), embedded assets of a document saved before linked storage +/// are queued for a byte comparison with their linked files. pub(crate) fn resolve(d: &mut Document, document: Option<&Path>, migrate: bool) -> Opened { let mut opened = Opened::default(); let ids: Vec = d.assets.keys().copied().collect(); @@ -188,8 +193,15 @@ pub(crate) fn resolve(d: &mut Document, document: Option<&Path>, migrate: bool) }; let a = Arc::make_mut(asset); a.data = a.data.with_source(source.clone()); - let status = locate(a, &source, document, &mut opened.work); - opened.status.insert(id, (a.data.key(), status)); + let key = a.data.key(); + // No files on the web: the link record is kept as it is. + let status = if cfg!(target_arch = "wasm32") { + LinkStatus::Missing + } else { + opened.work.push(Work::Find { asset: id, key, source }); + LinkStatus::Checking + }; + opened.status.insert(id, (key, status)); } else if migrate && let Some(document) = document && let Some(work) = migration(asset, document) @@ -200,39 +212,6 @@ pub(crate) fn resolve(d: &mut Document, document: Option<&Path>, migrate: bool) opened } -/// [`resolve`] for one linked asset → its status; work for the background check goes to `work`. -fn locate(a: &mut Asset, source: &LinkedSource, document: Option<&Path>, work: &mut Vec) -> LinkStatus { - #[cfg(not(target_arch = "wasm32"))] - { - let found: Vec<(PathBuf, Fingerprint)> = crate::asset_io::candidates(source) - .into_iter() - .filter_map(|p| { - let meta = crate::io::file_metadata(&p)?; - Some((p, crate::asset_io::fingerprint_from(&meta))) - }) - .collect(); - if let Some((path, _)) = found.iter().find(|(_, f)| Some(*f) == source.fingerprint) { - adopt_path(a, path, document); - return LinkStatus::Ok; - } - let same_size: Vec = found.iter().filter(|(_, f)| f.size == source.len).map(|(p, _)| p.clone()).collect(); - match &source.hash { - Some(hash) if !same_size.is_empty() => { - work.push(Work::Verify { asset: a.id, key: a.data.key(), hash: hash.clone(), len: source.len, candidates: same_size }); - LinkStatus::Checking - } - _ if found.is_empty() => LinkStatus::Missing, - _ => LinkStatus::Modified, - } - } - #[cfg(target_arch = "wasm32")] - { - // No files on the web: the link record is kept as it is. - let _ = (a, source, document, work); - LinkStatus::Missing - } -} - /// `path` (a file found to hold `a`'s bytes) as `a`'s link, with its path relative to the /// document and its bytes read from there. fn adopt_path(a: &mut Asset, path: &Path, document: Option<&Path>) { @@ -267,8 +246,7 @@ fn legacy(a: &Asset) -> bool { a.storage == Storage::Embedded && a.link.is_some() && a.link_hash.is_none() } -/// The byte comparison that would let a [`legacy`] asset be saved as a link: its linked file -/// exists with the size of the embedded bytes. +/// The byte comparison that would let a [`legacy`] asset be saved as a link. fn migration(a: &Asset, document: &Path) -> Option { #[cfg(not(target_arch = "wasm32"))] { @@ -287,8 +265,7 @@ fn migration(a: &Asset, document: &Path) -> Option { fingerprint: None, hash: None, }; - let path = crate::asset_io::candidates(&source).into_iter().find(|p| crate::io::file_metadata(p).is_some_and(|m| m.len() == source.len))?; - Some(Work::Migrate { asset: a.id, key: a.data.key(), bytes, path, page: a.page }) + Some(Work::Migrate { asset: a.id, key: a.data.key(), bytes, source, page: a.page }) } #[cfg(target_arch = "wasm32")] { @@ -356,21 +333,34 @@ const CHUNK: usize = 1 << 20; #[cfg(not(target_arch = "wasm32"))] fn check(work: Work, cancel: &AtomicBool) -> Option { match work { - Work::Verify { asset, key, hash, len, candidates } => { - for path in candidates { - let Some(before) = fingerprint(&path) else { continue }; - match read_file(&path, len, cancel, |_, _| true)? { - Some(found) if found == hash && fingerprint(&path) == Some(before) => { - let fingerprint = super::links::settled(&before).then_some(before); - return Some(Outcome::Holds { asset, key, path, fingerprint }); + Work::Find { asset, key, source } => { + let found: Vec<(PathBuf, Fingerprint)> = + crate::asset_io::candidates(&source).into_iter().filter_map(|p| fingerprint(&p).map(|f| (p, f))).collect(); + if let Some((path, _)) = found.iter().find(|(_, f)| super::links::settled(f) && Some(*f) == source.fingerprint) { + return Some(Outcome::Holds { asset, key, path: path.clone(), fingerprint: None }); + } + if found.is_empty() { + return Some(Outcome::Absent { asset, key }); + } + let Some(hash) = &source.hash else { return Some(Outcome::Differs { asset, key }) }; + for (path, before) in found.iter().filter(|(_, f)| f.size == source.len) { + match read_file(path, source.len, cancel, |_, _| true)? { + Some(found) if found == *hash && fingerprint(path) == Some(*before) => { + let fingerprint = super::links::settled(before).then_some(*before); + return Some(Outcome::Holds { asset, key, path: path.clone(), fingerprint }); } _ => {} } } Some(Outcome::Differs { asset, key }) } - Work::Migrate { asset, key, bytes, path, page } => { - let Some(before) = fingerprint(&path) else { return Some(Outcome::Unconfirmed) }; + Work::Migrate { asset, key, bytes, source, page } => { + let len = bytes.len() as u64; + let Some((path, before)) = + crate::asset_io::candidates(&source).into_iter().find_map(|p| fingerprint(&p).filter(|f| f.size == len).map(|f| (p, f))) + else { + return Some(Outcome::Unconfirmed); + }; let same = |at: usize, chunk: &[u8]| at.checked_add(chunk.len()).and_then(|end| bytes.get(at..end)) == Some(chunk); let read = read_file(&path, bytes.len() as u64, cancel, same)?; // Changed while it was read: not the bytes it had. @@ -379,7 +369,10 @@ fn check(work: Work, cancel: &AtomicBool) -> Option { } let hash = crate::asset_io::content_hash(&bytes); let proxy = designcraft_render::proxy::make_proxy(&bytes, page); - Some(Outcome::Confirmed { asset, key, path, fingerprint: before, hash, proxy }) + // A time that can't vouch for the bytes (changed in the last seconds, or none) is + // kept as one no file has: saves then go by the hash, not by size and time. + let fingerprint = if super::links::settled(&before) { before } else { Fingerprint { size: len, mtime: super::links::UNVERIFIED_MTIME } }; + Some(Outcome::Confirmed { asset, key, path, fingerprint, hash, proxy }) } } } @@ -488,6 +481,13 @@ fn current(st: &mut DocState, id: AssetId, key: SourceKey) -> Option<&mut Asset> fn apply_one(st: &mut DocState, outcome: Outcome, document: Option<&Path>) -> bool { match outcome { Outcome::Holds { asset, key, path, fingerprint } => { + let unchanged = fingerprint.is_none() + && st.doc.assets.get(&asset).is_some_and(|a| a.data.key() == key && a.link.as_deref() == Some(path.to_string_lossy().as_ref())); + if unchanged { + // The file at the recorded path, with the recorded size and time: nothing to change. + st.links.status.insert(asset, (key, LinkStatus::Ok)); + return false; + } let Some(a) = current(st, asset, key) else { return false }; if let Some(f) = fingerprint { a.link_fingerprint = Some(f); @@ -501,9 +501,11 @@ fn apply_one(st: &mut DocState, outcome: Outcome, document: Option<&Path>) -> bo true } Outcome::Differs { asset, key } => { - if st.doc.assets.get(&asset).is_some_and(|a| a.data.key() == key) { - st.links.status.insert(asset, (key, LinkStatus::Modified)); - } + found(st, asset, key, LinkStatus::Modified); + false + } + Outcome::Absent { asset, key } => { + found(st, asset, key, LinkStatus::Missing); false } Outcome::Confirmed { asset, key, path, fingerprint, hash, proxy } => { @@ -527,6 +529,13 @@ fn apply_one(st: &mut DocState, outcome: Outcome, document: Option<&Path>) -> bo } } +/// Keep `status` for asset `id`, while its bytes are still `key`'s. +fn found(st: &mut DocState, id: AssetId, key: SourceKey, status: LinkStatus) { + if st.doc.assets.get(&id).is_some_and(|a| a.data.key() == key) { + st.links.status.insert(id, (key, status)); + } +} + /// The status line's message once `n` embedded images were found identical to their files. fn notice(n: usize) -> String { if n == 1 { @@ -543,22 +552,33 @@ fn notice(n: usize) -> String { /// linked storage, [`resolve`]) whose file has changed since the check is embedded in this save /// after all. pub(crate) fn for_destination(d: &Document, from: Option<&Path>, destination: &Path) -> Option { - let mut changes: Vec<(AssetId, Option, Option, Storage)> = Vec::new(); + rewrite(d, from, destination, false) +} + +/// [`for_destination`] for a recovery snapshot of `d`, whose file is `document`. Embedded images +/// to be saved as links stay as the document's file has them (embedded, to be compared with their +/// files again), so a recovered document never depends on a file the user hasn't saved a link to. +pub(crate) fn for_recovery(d: &Document, document: &Path) -> Option { + rewrite(d, Some(document), document, true) +} + +fn rewrite(d: &Document, from: Option<&Path>, destination: &Path, recovery: bool) -> Option { + let mut changes: Vec<(AssetId, Option, Option, bool)> = Vec::new(); for a in d.assets.values() { let Some(link) = a.link.as_deref() else { continue }; let absolute = crate::io::document_path(link, from).ok(); let new_link = absolute.as_ref().map(|p| p.to_string_lossy().into_owned()).filter(|p| p != link); let relative = absolute.as_deref().and_then(|p| relative_to(destination, p)).filter(|r| a.link_relative.as_ref() != Some(r)); - let storage = if a.storage == Storage::Linked && !a.data.is_linked() && !still_there(a, from) { Storage::Embedded } else { a.storage }; - if new_link.is_some() || relative.is_some() || storage != a.storage { - changes.push((a.id, new_link, relative, storage)); + let embed = a.storage == Storage::Linked && !a.data.is_linked() && (recovery || !still_there(a, from)); + if new_link.is_some() || relative.is_some() || embed { + changes.push((a.id, new_link, relative, embed)); } } if changes.is_empty() { return None; } let mut saved = d.clone(); - for (id, link, relative, storage) in changes { + for (id, link, relative, embed) in changes { let Some(a) = saved.assets.get_mut(&id).map(Arc::make_mut) else { continue }; if let Some(link) = link { a.link = Some(link); @@ -566,17 +586,27 @@ pub(crate) fn for_destination(d: &Document, from: Option<&Path>, destination: &P if let Some(relative) = relative { a.link_relative = Some(relative); } - a.storage = storage; + if embed { + a.storage = Storage::Embedded; + if recovery { + // As the document's file has it: the next open from that file compares it again. + a.link_hash = None; + a.proxy = None; + } + } } Some(saved) } -/// Whether a file still has the size and time `a`'s link recorded when its bytes were found in it. +/// Whether a file still has the size and time `a`'s link recorded when its bytes were found in it +/// (a time that can vouch for them, [`super::links::settled`]). fn still_there(a: &Asset, document: Option<&Path>) -> bool { #[cfg(not(target_arch = "wasm32"))] { let Some(source) = LinkedSource::of(a, document) else { return false }; - crate::asset_io::candidates(&source).iter().any(|p| fingerprint(p) == source.fingerprint) + crate::asset_io::candidates(&source) + .iter() + .any(|p| fingerprint(p).is_some_and(|f| super::links::settled(&f) && Some(f) == source.fingerprint)) } #[cfg(target_arch = "wasm32")] { diff --git a/crates/engine/src/cmd/native_links/tests.rs b/crates/engine/src/cmd/native_links/tests.rs index fa4318a04..73921136a 100644 --- a/crates/engine/src/cmd/native_links/tests.rs +++ b/crates/engine/src/cmd/native_links/tests.rs @@ -67,12 +67,20 @@ fn saved_linked(dir: &Dir) -> (PathBuf, AssetId) { (doc, id) } -fn open(path: &Path) -> Session { +/// Open the file at `path`, with its background check of linked files still to come. +fn opened(path: &Path) -> Session { let mut s = Session::new(); s.execute("file.open", &json!({"path": path.to_string_lossy()})).unwrap(); s } +/// Open the file at `path` and wait for its background check of linked files. +fn open(path: &Path) -> Session { + let mut s = opened(path); + wait(&mut s); + s +} + fn asset(s: &Session, id: AssetId) -> Arc { Arc::clone(&s.doc().unwrap().doc.assets[&id]) } @@ -109,7 +117,7 @@ fn a_linked_save_writes_no_original_and_records_the_relative_path() { let s = open(&doc); let a = asset(&s, id); assert_eq!((a.storage, a.link_relative.as_deref()), (Storage::Linked, Some("links/art.png"))); - assert!(a.data.is_linked() && a.data.resident().is_none(), "nothing is read on open"); + assert!(a.data.is_linked() && a.data.resident().is_none(), "nothing is read on open, nor by its check"); assert_eq!(status(&s, id), Some(LinkStatus::Ok), "the recorded size and time: no check needed"); assert!(!s.doc().unwrap().links.checking()); assert_eq!(a.data.load().unwrap().as_slice(), png(30)); @@ -146,7 +154,7 @@ fn the_absolute_file_wins_and_otherwise_the_one_with_the_bytes() { // The original changes (same size): the copy holds the bytes, found by its hash. flip_a_byte(&dir.0.join("links/art.png")); age(&dir.0.join("links/art.png"), 10); - let mut s = open(©.0.join("doc.designcraft")); + let mut s = opened(©.0.join("doc.designcraft")); assert_eq!(status(&s, id), Some(LinkStatus::Checking)); wait(&mut s); let a = asset(&s, id); @@ -162,7 +170,7 @@ fn a_touched_file_with_the_same_bytes_is_ok_and_keeps_its_new_time() { let (doc, id) = saved_linked(&dir); let image = dir.0.join("links/art.png"); age(&image, 1000); - let mut s = open(&doc); + let mut s = opened(&doc); let key = asset(&s, id).data.key(); assert_eq!(status(&s, id), Some(LinkStatus::Checking)); wait(&mut s); @@ -201,6 +209,7 @@ fn migration_links_confirmed_images_and_drops_the_copy_only_after_confirmation() let (same, changed) = (dir.0.join("links/same.png"), dir.0.join("links/changed.png")); std::fs::write(&same, png(30)).unwrap(); std::fs::write(&changed, png(31)).unwrap(); + age(&same, 100); let mut s = Session::new(); s.execute("file.new", &json!({})).unwrap(); let (a, b) = (place(&mut s, &png(30)), place(&mut s, &png(31))); @@ -212,7 +221,7 @@ fn migration_links_confirmed_images_and_drops_the_copy_only_after_confirmation() // One file changes since (same size, other bytes). flip_a_byte(&changed); - let mut s = open(&doc); + let mut s = opened(&doc); assert!(asset(&s, a).data.embedded_bytes().is_some() && asset(&s, a).storage == Storage::Embedded, "nothing changes before the comparison"); wait(&mut s); let (left, kept) = (asset(&s, a), asset(&s, b)); @@ -264,6 +273,7 @@ fn byte_opened_and_recovered_documents_are_not_migrated_and_recovery_holds_no_or drop(s); let mut r = Session::new(); r.execute("file.recovery.open", &json!({"dir": recovery.to_string_lossy()})).unwrap(); + wait(&mut r); let a = asset(&r, id); assert_eq!((a.storage, a.link_relative.as_deref()), (Storage::Linked, Some("links/art.png"))); assert_eq!(a.data.linked_source().and_then(|s| s.document.clone()), Some(doc.clone()), "resolved against the document's file"); @@ -293,3 +303,85 @@ fn save_a_copy_with_embedded_links_is_portable() { assert!(stored_assets(©).is_empty()); assert!(s.execute("file.saveACopy", &json!({"path": copy.to_string_lossy(), "embedLinks": 1})).is_err()); } + +#[test] +fn the_open_looks_for_files_in_the_background() { + let dir = Dir::new("background"); + let (doc, id) = saved_linked(&dir); + let mut s = opened(&doc); + assert_eq!(status(&s, id), Some(LinkStatus::Checking), "nothing is asked of the file system on open"); + assert!(s.doc().unwrap().links.checking()); + wait(&mut s); + assert_eq!(status(&s, id), Some(LinkStatus::Ok)); + assert!(!s.doc().unwrap().is_dirty()); +} + +/// The open's statuses don't outlive a change to the file: Update reads a file edited since, +/// and a file missing at the open counts once it is back. +#[test] +fn a_file_changed_or_restored_after_the_open_is_seen() { + let dir = Dir::new("after-open"); + let (doc, id) = saved_linked(&dir); + let image = dir.0.join("links/art.png"); + let mut s = open(&doc); + assert_eq!(status(&s, id), Some(LinkStatus::Ok)); + std::fs::write(&image, png(77)).unwrap(); + age(&image, 10); + let r = s.execute("links.update", &json!({"asset": id.0})).unwrap(); + assert_eq!(r["updated"], 1, "{r}"); + assert_eq!(asset(&s, id).data.load().unwrap().as_slice(), png(77)); + + let restored = Dir::new("restored"); + let (doc, id) = saved_linked(&restored); + let image = restored.0.join("links/art.png"); + let original = std::fs::read(&image).unwrap(); + std::fs::remove_file(&image).unwrap(); + let mut s = open(&doc); + assert_eq!(status(&s, id), Some(LinkStatus::Missing)); + std::fs::write(&image, &original).unwrap(); + age(&image, 10); + let rows = s.execute("links.list", &json!({})).unwrap(); + let row = rows.as_array().unwrap().iter().find(|r| r["asset"] == id.0).cloned().unwrap(); + assert_eq!(row["status"], "ok", "{row}"); +} + +#[test] +fn preflight_reads_no_linked_original() { + let dir = Dir::new("preflight"); + let (doc, id) = saved_linked(&dir); + let mut s = open(&doc); + let issues = s.execute("preflight.run", &json!({})).unwrap(); + assert!(!issues.to_string().contains("modifiedLink"), "{issues}"); + assert!(asset(&s, id).data.resident().is_none(), "the original isn't read"); + flip_a_byte(&dir.0.join("links/art.png")); + age(&dir.0.join("links/art.png"), 10); + let issues = s.execute("preflight.run", &json!({})).unwrap(); + assert!(issues.to_string().contains("modifiedLink"), "{issues}"); +} + +#[test] +fn recovery_keeps_images_not_yet_saved_as_links() { + let dir = Dir::new("recover-migrated"); + let image = dir.0.join("links/art.png"); + std::fs::write(&image, png(30)).unwrap(); + age(&image, 100); + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let id = place(&mut s, &png(30)); + link_to(&mut s, id, &image, false); + let doc = dir.0.join("doc.designcraft"); + s.execute("file.save", &json!({"path": doc.to_string_lossy()})).unwrap(); + let mut s = open(&doc); + assert_eq!(asset(&s, id).storage, Storage::Linked, "found identical to its file, to be saved as a link"); + s.execute("frame.create", &json!({"rect": [36, 36, 300, 200]})).unwrap(); + let recovery = dir.0.join("recovery"); + s.execute("file.recovery.save", &json!({"dir": recovery.to_string_lossy()})).unwrap(); + let entry = recovery.join(format!("{}.designcraft", s.doc().unwrap().uid)); + assert_eq!(stored_assets(&entry), [format!("assets/{}.png", id.0)], "the bytes stay in the snapshot"); + drop(s); + let mut r = Session::new(); + r.execute("file.recovery.open", &json!({"dir": recovery.to_string_lossy()})).unwrap(); + wait(&mut r); + let a = asset(&r, id); + assert_eq!((a.storage, a.data.embedded_bytes().map(|b| b.to_vec())), (Storage::Embedded, Some(png(30)))); +} diff --git a/crates/engine/src/cmd/preflight.rs b/crates/engine/src/cmd/preflight.rs index 157e1d486..8e651383b 100644 --- a/crates/engine/src/cmd/preflight.rs +++ b/crates/engine/src/cmd/preflight.rs @@ -9,7 +9,7 @@ use designcraft_geom::Affine; use serde::Serialize; use serde_json::{Value, json}; -use super::{CommandSpec, cmd, has_doc}; +use super::{CommandSpec, LinkListChecks, cmd, has_doc}; use crate::{Result, Session}; mod white_overprint; @@ -260,14 +260,23 @@ pub fn overset_warnings(s: &Session, pages: Option<&[usize]>) -> Vec { out } -pub fn check(s: &Session, min_ppi: f64) -> Vec { - let Some(st) = s.active() else { return vec![] }; - check_document(&st.doc, &s.cache, st.path.as_deref().map(std::path::Path::new), min_ppi) +/// The active document's issues. Linked files are looked at as `links.list` looks at them, with +/// what it remembers of them. +pub fn check(s: &mut Session, min_ppi: f64) -> Vec { + let mut links = std::mem::take(&mut s.link_checks); + let issues = match s.active() { + Some(st) => check_document(&st.doc, &s.cache, st.path.as_deref().map(std::path::Path::new), min_ppi, &mut links), + None => vec![], + }; + s.link_checks = links; + issues } /// [`check`] of document `d` saved at `document` (relative links resolve against it), with the -/// compositions in `cache`. It needs no session, so it can run on another thread. -pub fn check_document(d: &Document, cache: &Cache, document: Option<&std::path::Path>, min_ppi: f64) -> Vec { +/// compositions in `cache`. It needs no session, so it can run on another thread. `links` is what +/// the caller remembers of linked files between checks: a file is read only when its size is the +/// recorded one and its time is new. +pub fn check_document(d: &Document, cache: &Cache, document: Option<&std::path::Path>, min_ppi: f64, links: &mut LinkListChecks) -> Vec { let mut out = Vec::new(); // Stories: overset and fonts. let mut missing_fonts: Vec = Vec::new(); @@ -437,8 +446,7 @@ pub fn check_document(d: &Document, cache: &Cache, document: Option<&std::path:: if let Content::Graphic(g) = &it.content && let Some(a) = d.assets.get(&g.asset) && a.data.is_linked() - && !super::links::link_missing(a, document) - && a.data.load().err() == Some(designcraft_doc::Unavailable::Modified) + && super::links::link_modified(a, document, links) { out.push(Issue { severity: "warning", @@ -463,7 +471,7 @@ pub fn check_document(d: &Document, cache: &Cache, document: Option<&std::path:: }), Some(a) => { // Placed PDF and SVG pages export as vectors; `pixels` is only the preview's. - let vector = a.output_bytes().is_ok_and(|o| designcraft_images::is_pdf(&o.bytes) || designcraft_images::is_svg(&o.bytes)); + let vector = a.mime == "application/pdf" || a.mime == "image/svg+xml"; if let Some((pw, ph)) = a.pixels.filter(|_| !vector) { // Measure each image axis in points, not its rotated bounding box or // cropped frame. Either axis can fall below the minimum resolution. @@ -589,10 +597,10 @@ mod arabic_tests { let r = s.execute("frame.create", &json!({"rect": [0, 0, 300, 200], "content": "text", "text": text})).unwrap(); s.execute("text.select", &json!({"story": r["story"], "anchor": 0, "focus": text.len()})).unwrap(); s.execute("type.char", &json!({"attrs": {"diacriticPosition": "openType", "diacriticXOffset": 150}})).unwrap(); - assert!(!check(&s, 150.0).iter().any(|i| i.kind == "unsupportedTypography")); + assert!(!check(&mut s, 150.0).iter().any(|i| i.kind == "unsupportedTypography")); s.execute("type.para", &json!({"attrs": {"arabicJustification": "NaskhJustification", "paragraphKashidaWidth": 2}})).unwrap(); s.execute("type.char", &json!({"attrs": {"diacriticPosition": "tight"}})).unwrap(); - let issues = check(&s, 150.0); + let issues = check(&mut s, 150.0); let typography: Vec<_> = issues.iter().filter(|i| i.kind == "unsupportedTypography").collect(); assert_eq!(typography.len(), 3, "{typography:?}"); assert!(typography.iter().all(|i| i.severity == "warning")); @@ -683,7 +691,7 @@ mod mojikumi_nested_tests { pf.para.mojikumi = Some("MojikumiTable/Unrecognized".into()); cell.insert_note(0, "Note", pf); d.story_mut(sid).unwrap().insert_table(0, table); - let issues = check(&s, 150.0); + let issues = check(&mut s, 150.0); assert_eq!(issues.iter().filter(|i| i.kind == "unsupportedTypography" && i.message.contains("Unrecognized")).count(), 1); } } diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index 50fb291c7..d0c6500bc 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -751,8 +751,9 @@ impl Snapshot { let doc = d.preview_stash.clone().unwrap_or_else(|| d.doc.clone()); // Linked files are recorded as a save to the document's own file records them, never // copied in; relative paths are relative to that file, which recovery resolves them against. + // Embedded images not yet saved as links keep their bytes. let doc = match d.path.as_deref().map(Path::new) { - Some(path) => crate::cmd::native_links::for_destination(&doc, Some(path), path).map_or(doc, std::sync::Arc::new), + Some(path) => crate::cmd::native_links::for_recovery(&doc, path).map_or(doc, std::sync::Arc::new), None => doc, }; Snapshot { diff --git a/crates/ui-egui/src/panels/properties/tests.rs b/crates/ui-egui/src/panels/properties/tests.rs index 642293201..3bb36c2ee 100644 --- a/crates/ui-egui/src/panels/properties/tests.rs +++ b/crates/ui-egui/src/panels/properties/tests.rs @@ -20,6 +20,9 @@ fn language(app: &mut DesignApp) -> Value { text_attrs(app).unwrap()["chars"]["language"].clone() } +/// The graphic [`linked_graphics`] places. +const SVG: &[u8] = br##""##; + fn linked_graphics(paths: &[Option<&str>]) -> (DesignApp, Vec, Vec) { use designcraft_doc::{Asset, AssetId, Content, Graphic, ItemId}; use std::sync::Arc; @@ -39,10 +42,7 @@ fn linked_graphics(paths: &[Option<&str>]) -> (DesignApp, Vec"## - .to_vec(), - ), + data: designcraft_doc::AssetData::embedded(SVG.to_vec()), pixels: Some((32, 32)), ..Default::default() }), @@ -167,8 +167,14 @@ fn edit_page_shows_the_page_and_sets_its_size_alone() { #[test] fn links_panel_distinguishes_same_named_sources_and_keeps_link_actions() { - let first = "/project/assets/studio-a/photo.svg"; - let second = "/project/assets/studio-b/photo.svg"; + // Files holding the graphics' bytes, so their links are OK and can be embedded. + let dir = std::env::temp_dir().join(format!("dc-links-panel-same-name-{}", std::process::id())); + let (first, second) = (dir.join("studio-a/photo.svg"), dir.join("studio-b/photo.svg")); + for file in [&first, &second] { + std::fs::create_dir_all(file.parent().unwrap()).unwrap(); + std::fs::write(file, SVG).unwrap(); + } + let (first, second) = (first.to_str().unwrap(), second.to_str().unwrap()); let (app, items, assets) = linked_graphics(&[Some(first), Some(second), None]); let mut h = links_harness(app, 320.0); assert_eq!(h.get_all_by_label("photo.svg").count(), 3); @@ -184,12 +190,15 @@ fn links_panel_distinguishes_same_named_sources_and_keeps_link_actions() { // Embedding keeps the link for Unembed. let doc = &h.state().session.active().unwrap().doc; assert_eq!(doc.assets[&assets[1]].link.as_deref(), Some(second)); + assert_eq!(doc.assets[&assets[0]].link.as_deref(), Some(first)); + assert_eq!(h.get_all_by_label("Embedded").count(), 2); assert_eq!(h.get_all_by_label("photo.svg").count(), 3); h.get_by_label(first).click(); h.run_steps(3); h.get_by_label("Go To").click(); h.run_steps(3); assert_eq!(h.state().session.active().unwrap().selection.items, vec![items[0]]); + let _ = std::fs::remove_dir_all(&dir); } #[test] diff --git a/crates/ui-egui/src/preflight_worker.rs b/crates/ui-egui/src/preflight_worker.rs index 02b27832e..349cdfe63 100644 --- a/crates/ui-egui/src/preflight_worker.rs +++ b/crates/ui-egui/src/preflight_worker.rs @@ -13,6 +13,7 @@ use std::time::{Duration, Instant}; use designcraft_compose::Cache; use designcraft_doc::Document; +use designcraft_engine::cmd::LinkListChecks; use designcraft_engine::cmd::preflight::{Issue, check_document}; use crate::DesignApp; @@ -177,7 +178,7 @@ fn issues_at(app: &mut DesignApp, ctx: &egui::Context, now: f64) -> Option) -> Opti .spawn(move || { let _alive = alive; let mut own = None; + // The linked files read and what they held, so a file is read once per new time. + let mut links = LinkListChecks::default(); loop { let job = match jobs.recv_timeout(FORGET_AFTER) { Ok(job) => job, Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { own = None; + links = LinkListChecks::default(); continue; } Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => break, }; - if done_tx.send(check(&mut own, job)).is_err() { + if done_tx.send(check(&mut own, &mut links, job)).is_err() { break; } ctx.request_repaint(); @@ -394,9 +398,9 @@ fn spawn(ctx: egui::Context, live: &Arc) -> Opti /// Checks `job` with the worker's own compositions, `own`. #[cfg(not(target_arch = "wasm32"))] -fn check(own: &mut Option<(u64, Cache)>, job: Job) -> Found { +fn check(own: &mut Option<(u64, Cache)>, links: &mut LinkListChecks, job: Job) -> Found { let started = Instant::now(); - let issues = check_document(&job.doc, reader(own, &job), job.of.path.as_deref().map(std::path::Path::new), MIN_PPI); + let issues = check_document(&job.doc, reader(own, &job), job.of.path.as_deref().map(std::path::Path::new), MIN_PPI, links); // The checked state isn't kept alive by its check once the result is sent. drop(job.doc); Found { of: job.of, issues: Arc::new(issues), took: started.elapsed(), asked_at: 0.0 } diff --git a/crates/ui-egui/src/preflight_worker/tests.rs b/crates/ui-egui/src/preflight_worker/tests.rs index ba0bfad67..deaf92986 100644 --- a/crates/ui-egui/src/preflight_worker/tests.rs +++ b/crates/ui-egui/src/preflight_worker/tests.rs @@ -235,17 +235,17 @@ fn a_check_of_an_older_state_keeps_the_current_compositions() { let cache = Arc::clone(&app.session.cache); let current = cache.get(&doc, sid, None); let mut own = None; - let found = super::check(&mut own, super::Job { of: older, doc: Arc::clone(&older_doc), cache: Arc::clone(&cache) }); + let found = super::check(&mut own, &mut Default::default(), super::Job { of: older, doc: Arc::clone(&older_doc), cache: Arc::clone(&cache) }); assert_eq!(found.issues.iter().filter(|i| i.severity == "error").count(), 1); assert!(Arc::ptr_eq(&cache.get(&doc, sid, None), ¤t), "the current state's composition is kept"); // The worker keeps its compositions for the next check of the document over the same cache. let earlier = own.as_ref().unwrap().1.get(&older_doc, sid, None); let (of, doc) = Checked::new(&app).unwrap(); - super::check(&mut own, super::Job { of, doc, cache: Arc::clone(&cache) }); + super::check(&mut own, &mut Default::default(), super::Job { of, doc, cache: Arc::clone(&cache) }); assert!(Arc::ptr_eq(&own.as_ref().unwrap().1.get(&older_doc, sid, None), &earlier), "kept for the next check"); let reloaded = Arc::new(designcraft_compose::Cache::new()); let (of, doc) = Checked::new(&app).unwrap(); - super::check(&mut own, super::Job { of, doc, cache: Arc::clone(&reloaded) }); + super::check(&mut own, &mut Default::default(), super::Job { of, doc, cache: Arc::clone(&reloaded) }); assert!(own.as_ref().unwrap().1.base().is_some_and(|b| Arc::ptr_eq(b, &reloaded)), "a new cache after a font reload"); } diff --git a/docs/linked-artwork.md b/docs/linked-artwork.md index a46705352..b94c017a0 100644 --- a/docs/linked-artwork.md +++ b/docs/linked-artwork.md @@ -27,11 +27,11 @@ The proxy is drawn while the original loads, and in place of the original when t | Status | Meaning | |---|---| | OK | A file at the link's path holds the recorded bytes. | -| Checking | A file of the recorded size is being hashed in the background. | +| Checking | Just after the document opens: the file is being looked for in the background, and hashed when it has the recorded size and another time. | | Modified | A file exists, but its bytes differ from the recorded ones. | | Missing | No file at either path, or a path that is not followed (a path on another computer, a device). | -Status comes from the content hash. A file with the recorded size and modification time is OK without a read. A file with the recorded size and another time is hashed on a background thread: a matching hash is OK and the new time is remembered, a different hash is Modified. Modification time alone is never trusted to decide Modified, because it is unreliable on network shares. +Status comes from the content hash. A file with the recorded size and modification time is OK without a read. A file with the recorded size and another time is hashed on a background thread: a matching hash is OK and the new time is remembered, a different hash is Modified. Modification time alone is never trusted to decide Modified, because it is unreliable on network shares. A file whose file system reports no modification time is always told by its hash. A Modified file's bytes are never adopted silently. The proxy stays on screen until you update or relink. @@ -59,7 +59,7 @@ A document with any linked asset is written as format version 5; otherwise the v ## Opening -Opening reads no original. Each link is looked for at its absolute path, then at its path relative to the document. A match on size and time is OK at once; other candidates are hashed in the background. When both paths exist, the file whose hash matches wins. +Opening reads no original and asks nothing of the file system: a background check looks for each link at its absolute path, then at its path relative to the document. A match on size and time is OK without a read; other candidates of the recorded size are hashed. When both paths exist, the file whose hash matches wins. ## Old files with embedded images