feat(gui): add file thumbnails and a browser status bar - #20
Conversation
narrrl
left a comment
narrrl
left a comment
There was a problem hiding this comment.
Reviewed the full diff against main. The feature is well-structured — the rel_to_mount component hardening, the daemon-side fetch_max generation ordering, the rewritten connect_bind widget-tree walks, the Notify handling in generate_thumbs, and the quota_display/listing_summary arithmetic all check out.
Three issues below are blocking, and they share a shape: several distinct conditions collapse into one "no thumbnail exists" signal that is then cached permanently, so an ordinary transient — navigating away mid-batch, a stray getxattr, exiftool not installed yet — bakes in a missing thumbnail that no amount of retrying recovers. Worth fixing together.
The rest are ordinary follow-ups; the two "rw" entries are almost certainly meant to be "raw".
One process note: the local Clippy and GUI checks ran against a pkg-config shim (GTK 4.8 local vs 4.12 required), so the fork CI run is the only one that actually linked. I'll re-run the gate on a 4.12 box before merging.
14 inline comments follow.
| } | ||
| }, | ||
| None if reply.pending => pending.push(request.clone()), | ||
| None => { |
There was a problem hiding this comment.
Blocking: a stale-generation reply permanently blacklists the whole batch.
Core::file_thumbs returns path: None, pending: false for every item when the request's generation is older than the daemon's. Control connections are served concurrently, one thread each (pdfs-fuse/src/control.rs:1155), so the CancelFileThumbs { G+1 } that cancel_file_thumbnails fires on navigation can be handled before the still-in-flight FileThumbs { G }. This arm cannot tell that apart from "no thumbnail exists" and calls store_missing on all 32 uids.
missing is never cleared, only LRU-evicted at 2048 entries — so those images show the generic icon for the rest of the process lifetime, even after returning to the folder.
Repro: scroll a folder of JPEGs, switch to Trash before the batch replies, come back — no thumbnails, ever.
Fix: only store_missing when the key is still present in wanted (after a cancel it is not), or distinguish "cancelled" from "no thumbnail exists" on the wire.
| .cache | ||
| .cached_thumbnail_path(uid, ttype, *modified) | ||
| .is_some() | ||
| || self.no_thumbnail.lock().get(&(uid.clone(), ttype)) == Some(modified) |
There was a problem hiding this comment.
Blocking: no_thumbnail is shared with the remote-miss cache, so a remote "no thumbnail" answer suppresses local generation forever.
This skips generation when no_thumbnail[(uid, ttype)] == modified, but that same map is populated by Core::thumbnail (lib.rs:3600) whenever the remote answers "this node has no thumbnail" — which, as this PR's own comment notes, is always true for ordinary Drive files. Any getxattr("user.proton.thumbnail") on a mounted image (FUSE path at filesystem.rs:2163) writes that miss; afterwards file_thumbs refuses to generate locally and replies path: None, pending: false, which the GUI turns into a permanent miss.
"Remote has none" and "we generated and it was undecodable" need separate keys.
| let image = match direct { | ||
| Some(image) => image, | ||
| None if is_raw_image_name(name) => { | ||
| let (preview, orientation) = extract_raw_preview(bytes, name)?; |
There was a problem hiding this comment.
Blocking: a missing or slow exiftool is recorded as a permanent THUMB_NONE.
extract_raw_preview returns None for "exiftool absent", "exiftool timed out", and "temp file failed" just as it does for "not an image". The caller persists db::THUMB_NONE, and photo_thumbs then filters those nodes out forever — installing exiftool afterwards does not recover them.
This matters most for upgrading installs, which won't have the new packaging dependency at the moment the daemon first walks their library. Only the genuinely-undecodable case should be recorded as a permanent verdict; environmental failures should be left alone so the next attempt can retry.
| let status = { | ||
| let mut status = self.thumbnail_build.lock(); | ||
| if status.running { | ||
| return status.clone(); |
There was a problem hiding this comment.
A second build request silently attaches to a build of a different folder.
start_thumbnail_build returns the in-progress status unchanged when status.running, including its path. The GUI (browser.rs:1480) treats that reply as its own job: it paints "Building thumbnails in <other folder>…", polls it, and on completion reloads and re-enables the button — while the folder the user actually selected was never scanned, with no error surfaced.
Either reject the request (CoreError::invalid) or queue the new root.
| status | ||
| } | ||
|
|
||
| fn run_thumbnail_build(&self, root: PathBuf) { |
There was a problem hiding this comment.
The recursive build has no cancel path and no bound.
run_thumbnail_build walks the entire subtree and download_files every image at full size (RAWs included, buffered whole in RAM in generate_thumbs, then copied again to /tmp by RawTempFile). Started at the mount root on a large account this is hours of traffic and tens of GB. The GUI disables its own button and offers no stop, and there is no CancelThumbnailBuild request — the only escape is stopping the daemon.
Worth at least a cancel request, or a confirmation when the selected root is "".
| .as_nanos(); | ||
| for _ in 0..16 { | ||
| let nonce = RAW_TEMP_NONCE.fetch_add(1, Ordering::Relaxed); | ||
| let path = std::env::temp_dir().join(format!( |
There was a problem hiding this comment.
RAW staging writes fully decrypted file contents to std::env::temp_dir() rather than the app's own owner-checked state/cache dir.
Two consequences: a SIGKILL leaves plaintext RAWs behind in /tmp (the state dir is deliberately 0600 and owner-checked; /tmp is not), and on the common tmpfs /tmp up to THUMB_GEN_CONCURRENCY (4) copies of a 50–100 MB RAW sit in RAM simultaneously in addition to the bytes Vec the caller still holds. A small /tmp makes create fail and the thumbnail silently unavailable.
Staging under the cache dir the daemon already sizes would avoid both.
| return None; | ||
| } | ||
| }; | ||
| let mut stdout = child.stdout.take()?; |
There was a problem hiding this comment.
child.stdout.take()? returns from exiftool_query without kill()/wait(), leaving a zombie exiftool for the daemon's lifetime.
Rare — stdout is always piped here — but this is the failure path, so it should still reap.
| | "orf" | ||
| | "pef" | ||
| | "raf" | ||
| | "rw" |
There was a problem hiding this comment.
"rw" looks like a typo for "raw". rw is not a camera RAW extension; Panasonic/Leica use .RAW, which is absent from both this list and is_raw_image_name.
As written, .raw files get no tile at all and .rw files are advertised as thumbnailable to both the GUI and the recursive build.
| | "orf" | ||
| | "pef" | ||
| | "raf" | ||
| | "rw" |
There was a problem hiding this comment.
Same "rw" / "raw" typo as in is_thumbnail_image_name above.
| ui.browser.thumbnail_build_running.set(false); | ||
| ui.browser | ||
| .build_thumbnails | ||
| .set_sensitive(*ui.mounted.borrow()); |
There was a problem hiding this comment.
The error row never goes away.
thumbnail_build_failed shows thumbnail_build_row with an error message, and nothing hides it again — navigating to another page and back leaves the stale failure text pinned under the Files header until the user starts another build. repaint_thumbnail_build hides the row on !running; the failure path should do the same (after a toast), or load_browser should clear it.
2 participants
Summary
Implementation notes
Ordinary Drive files do not rely on the Photos thumbnail API. Their content is downloaded through the existing Drive/cache path, decoded locally, scaled, and stored in the existing thumbnail cache. The file modification time is used as the cache validity tag, so a changed file does not reuse a stale preview.
Camera RAW support stages a private temporary file, asks ExifTool for the best embedded preview, applies orientation, and removes the temporary file on drop. The Debian, Arch, and Fedora package metadata now includes the corresponding ExifTool runtime package.
The recursive builder validates the requested relative path before traversing it. Foreground listing work is bounded and cancellable; a recursive build is intentionally independent so navigating away does not cancel an explicitly requested tree build.
This keeps the current v1.9 browser behavior, including multi-selection, bulk actions, undo, the details pane, and Google Takeout support.
Validation
cargo fmt --all -- --checkcargo check -p pdfs-fusecargo test -p pdfs-core -p pdfs-fuse— 515 passed, 1 optional real-RAW-fixture test ignoredscripts/fuse-acceptance.sh --offline-only— 15 passedcargo check -p pdfs-guicargo clippy --workspace --all-targets -- -D warningsgit diff --check origin/main...HEADThe local machine has GTK 4.8/libadwaita 1.2, while current upstream requires GTK 4.12/libadwaita 1.5. The local GUI check and Clippy run used a temporary pkg-config metadata shim for Rust type/lint checking; the linked GTK 4.12 build and full workspace tests passed in the Ubuntu 24.04 fork CI run above.