dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Import into the library, which is on the server

Browse Source 
There was a local destination, defaulting to ~/Pictures, and an "upload" switch
that could be turned off. That was wrong twice over. DarkRoom's library *is* a
folder on a Nextcloud server (FR-NC-6) — there is no local library — so a
user-chosen local destination built a second pile of photographs that no view
in the application ever lists, and made "where did my import go" a question
with two answers.

An import now has exactly one destination and the page asks nothing about it.
With no account there is nowhere to go at all, so Import is refused rather than
quietly filling a folder.

What lands on the device is a staging copy in a directory the app owns, the
same shape `export` uses for its outbox and for the reason its module docs
give: staging first is the only path, not a fallback for being offline. The
bytes have to reach disk before the network — streaming a card straight to the
server would let a move-import erase a card against an in-flight upload, and
would make importing impossible with no connection (FR-NC-10). A staged file is
removed once the server confirms it; one that is not confirmed stays queued, and
the next import drains it.

And the rule that was stated but never enforced: `retirable` was reported and
`dr_ingest::retire` was never called by anything, so a move-import silently
behaved as a copy. The card is now emptied by the worker, of exactly those
photographs the *server* has confirmed — not those merely written here, because
the staging copy is removed moments later and anything unconfirmed would then
exist nowhere at all.

FR-NC-7b said "copied locally first ... then queued for upload", which is a
staging area; it has been rewritten to say so in terms that do not also permit
what was built.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-22 21:09:26 +02:00
co-authored by Claude Opus 5
parent ce666c768e
commit d04087af83
8 changed files with 336 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/import.rs
+215 -9
View File
@@ -19,6 +19,26 @@
//! user sees "1,847 photographs, 61.2 GB" and then a progress bar, rather than
//! a frozen window and then a progress bar.
//!
//! # There is no local library, so there is no destination to choose
//!
//! DarkRoom's library is a folder on a Nextcloud server (FR-NC-6). An import
//! therefore has exactly one place to go, and the interface has nothing to ask
//! about it.
//!
//! What lands on this machine is a **staging copy**, in a directory the app
//! owns beside the catalog — the same shape `export` uses for its outbox, and
//! for the same reason its module docs give: staging first is the only path,
//! not a fallback for the offline case.
//!
//! The bytes have to touch disk before they touch the network. Streaming a
//! card straight to the server would mean a move-import erasing a card against
//! an in-flight upload, which is the one operation here with no undo, and it
//! would make importing impossible with no connection, which FR-NC-10 forbids.
//!
//! A staged file is removed once the server confirms it. One that is not
//! confirmed stays where it is, and the next import drains it — so an import
//! made on a train finishes when the train arrives somewhere.
//!
//! # The import does not catalogue what it wrote
//!
//! It writes files into the library and then asks for a scan, rather than
@@ -33,7 +53,7 @@ use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::mpsc::{Receiver, Sender};
use std::sync::Arc;
use dr_ingest::{Candidate, DupKey, Imported, Ingest, Options, Report, Shot};
use dr_ingest::{Candidate, DupKey, Imported, Ingest, Options, Report, Shot, TransferMode};
use dr_plat::{DirRef, LocalStorage, Storage, WritableStorage};
use dr_sync::{RemoteBackend, RemotePath};
use dr_sync_nextcloud::{AppCredentials, NextcloudBackend};
@@ -56,8 +76,9 @@ pub type Cancel = Arc<AtomicBool>;
pub struct Request {
/// Where the card is mounted.
pub card: PathBuf,
/// The library root the template's folders are created under.
pub library: PathBuf,
/// Where the copies are staged before they go up. Owned by the app, not
/// chosen by the user — see the module docs.
pub staging: PathBuf,
/// The optional second destination (FR-CAT-10).
pub backup: Option<PathBuf>,
/// The catalog, opened by the worker for its duplicate queries.
@@ -68,8 +89,9 @@ pub struct Request {
/// Which file types to take off the card.
pub filter: FormatFilter,
pub options: Options,
/// Where to send the originals afterwards, if anywhere (FR-NC-7b).
pub upload: Option<Upload>,
/// Where the photographs are going. An import without one has nowhere to
/// put anything, and the page refuses to start.
pub upload: Upload,
}
/// TRACES: FR-NC-7a | FR-NC-7b
@@ -89,6 +111,22 @@ pub struct Upload {
/// Where the thumbnail shards live, so a thumbnail made during the import
/// can be filed under the server's `oc:fileid` once the upload assigns one.
pub thumbs: PathBuf,
/// Where copies wait between disk and the server.
///
/// Per account, beside the catalog, like the export outbox: a file queued
/// for one server is meaningless to another, and a staging directory
/// shared between accounts would upload a card to whichever was signed in
/// when the connection came back.
pub staging: PathBuf,
}
/// TRACES: FR-NC-7b
/// Where an account's imports wait between disk and the server.
pub fn staging_dir(server: &str, user_id: &str) -> PathBuf {
crate::library::catalog_path(server, user_id)
.parent()
.map(|p| p.join("staging"))
.unwrap_or_else(|| std::env::temp_dir().join("darkroom-import-staging"))
}
impl std::fmt::Debug for Upload {
@@ -98,6 +136,7 @@ impl std::fmt::Debug for Upload {
.field("user_id", &self.user_id)
.field("library", &self.library)
.field("thumbs", &self.thumbs)
.field("staging", &self.staging)
.finish_non_exhaustive()
}
}
@@ -166,6 +205,18 @@ pub struct Outcome {
/// and the card has not been touched. Reported so the user knows the
/// server does not have them yet.
pub upload_failed: usize,
/// Card files actually deleted, for a move-import.
///
/// Never more than `retirable`, and zero for a copy-import. Reported
/// because "the card is now empty" is a claim the user needs to be able to
/// check against what they expected.
pub retired: usize,
/// Staged copies still waiting to go up.
///
/// The same files as `upload_failed`, named for what happens next rather
/// than for what went wrong: they are queued, and the next import sends
/// them. An import made with no connection is this and nothing else.
pub staged: usize,
}
/// Start an import.
@@ -188,7 +239,16 @@ pub fn spawn(request: Request, cancel: Cancel) -> Receiver<Message> {
fn run(request: Request, cancel: &Cancel, tx: &Sender<Message>) -> Result<(), String> {
let card = LocalStorage::with_root(CARD, request.card.clone());
let library = LocalStorage::with_root(LIBRARY, request.library.clone());
// Created rather than assumed: the first import on a machine finds no
// staging directory, and a failure here is worth reporting as itself
// rather than as every file failing to be written.
if let Err(e) = std::fs::create_dir_all(&request.staging) {
return Err(format!(
"could not prepare {}: {e}",
request.staging.display()
));
}
let library = LocalStorage::with_root(LIBRARY, request.staging.clone());
let backup = request
.backup
.as_ref()
@@ -242,7 +302,8 @@ fn run(request: Request, cancel: &Cancel, tx: &Sender<Message>) -> Result<(), St
// FR-NC-7b. Everything above has already finished: every file is on disk
// and its digest checked, so nothing from here on can cost the user a
// photograph — only a transfer.
if let Some(upload) = &request.upload {
{
let upload = &request.upload;
let (sent, thumbnails) = upload_all(upload, &library, &report.imported, cancel, tx);
outcome.thumbnails = thumbnails;
outcome.uploaded = sent.iter().filter(|s| s.transferred).count();
@@ -264,6 +325,36 @@ fn run(request: Request, cancel: &Cancel, tx: &Sender<Message>) -> Result<(), St
// now. The metadata tier covers the interval, which is why this is not
// worth waiting for a sync to do properly.
record_digests(catalog.connection(), &upload.library, &sent);
// The staging copy has done its job the moment the server confirms it:
// the photograph is in the library, and keeping a second copy in a
// directory no view ever shows would be a slow disk leak. What is *not*
// confirmed stays exactly where it is — that is the queue FR-NC-7b
// describes, and the next import drains it.
outcome.staged = report.imported.len() - sent.len();
clear_staged(&library, &report.imported, &sent);
// TRACES: FR-CAT-10 | FR-NC-7b
// The card, last of all, and only for what the server has.
//
// The rule this enforces: a copy-import never touches the card, and a
// move-import empties it only of photographs that are confirmed on the
// server — not merely written to disk here, because the staging copy
// above has just been removed and this machine is no longer holding
// them either. Anything the upload did not take keeps its card copy,
// which is then the only copy that exists.
//
// Done here rather than handed to the caller: `dr_ingest::retire` is
// deliberately awkward to reach, and a caller that forgot to call it
// was exactly the bug this replaces — Move behaved as Copy, silently.
if request.options.mode == TransferMode::Move {
let confirmed = confirmed_sources(&report.imported, &sent);
outcome.retirable = confirmed.len();
let failures = dr_ingest::retire(&card, &confirmed);
outcome.retired = confirmed.len() - failures.len();
} else {
outcome.retirable = 0;
}
}
let _ = tx.send(Message::Finished(outcome));
@@ -478,6 +569,43 @@ async fn file_thumbnails(
filed
}
/// The card sources whose photographs the server now holds.
///
/// Paired positionally with `imported`, which is how `upload_all` builds
/// `sent`: one entry per original it got an answer about, in order. A file the
/// upload skipped or failed on has no entry, and so is never returned here.
fn confirmed_sources(imported: &[Imported], sent: &[Sent]) -> Vec<dr_types::SourceRef> {
imported
.iter()
.zip(sent.iter())
.map(|(image, _)| image.source.clone())
.collect()
}
/// TRACES: FR-NC-7b
/// Remove the staging copies the server has confirmed.
///
/// Keyed on what came back from the upload rather than on what was imported:
/// a file the server did not take must survive, because the staging directory
/// is the only place it exists once the card is put away.
///
/// Failures are logged, not propagated. A staged file that cannot be deleted
/// costs disk; refusing to finish an import over it would cost the user the
/// report telling them their photographs are safe.
fn clear_staged(library: &LocalStorage, imported: &[Imported], sent: &[Sent]) {
let confirmed: std::collections::HashSet<&str> =
sent.iter().map(|s| s.remote_path.as_str()).collect();
for (image, item) in imported.iter().zip(sent.iter()) {
if !confirmed.contains(item.remote_path.as_str()) {
continue;
}
if let Err(e) = library.remove_file(&image.written) {
log::warn!("could not clear the staged copy of {}: {e}", image.name);
}
}
}
/// Read an imported file back out of the library.
fn read_all(library: &LocalStorage, image: &Imported) -> Result<Vec<u8>, String> {
use std::io::Read;
@@ -640,6 +768,8 @@ pub fn summarise(report: &Report) -> Outcome {
uploaded: 0,
already_on_server: 0,
upload_failed: 0,
staged: 0,
retired: 0,
thumbnails: 0,
}
}
@@ -698,12 +828,18 @@ pub fn describe(outcome: &Outcome) -> String {
}
if outcome.upload_failed > 0 {
out.push_str(&format!(
", {} still only on this computer",
", {} waiting to go up — the next import sends them",
outcome.upload_failed
));
}
}
// The card. Said plainly, because emptying one is irreversible and the
// user should not have to infer it from a count of uploads.
if outcome.retired > 0 {
out.push_str(&format!(". {} removed from the card", outcome.retired));
}
// Worth saying: it is why the grid fills in immediately after an import
// rather than fetching a preview out of every original that just went up.
if outcome.thumbnails > 0 {
@@ -938,7 +1074,77 @@ mod tests {
};
let text = describe(&o);
assert!(text.contains("2 uploaded"), "{text}");
assert!(text.contains("1 still only on this computer"), "{text}");
// Named for what happens next rather than for what went wrong: the
// file is queued, not lost, and the next import sends it.
assert!(text.contains("1 waiting to go up"), "{text}");
}
#[test]
fn emptying_the_card_is_stated_plainly() {
// Irreversible, so the user should not have to infer it from a count
// of uploads.
let o = Outcome {
uploaded: 3,
retirable: 3,
retired: 3,
..outcome()
};
assert!(
describe(&o).contains("3 removed from the card"),
"{}",
describe(&o)
);
}
#[test]
fn a_copy_import_never_mentions_the_card() {
let o = Outcome {
uploaded: 3,
..outcome()
};
assert!(!describe(&o).contains("card"), "{}", describe(&o));
}
#[test]
fn only_confirmed_photographs_are_taken_off_the_card() {
// The rule: a card is emptied of what the *server* has, never of what
// merely reached this disk — the staging copy is removed straight
// after, so anything not uploaded would otherwise exist nowhere.
let imported: Vec<Imported> = (0..3)
.map(|i| Imported {
source: dr_types::SourceRef::Local {
root: CARD,
relative: format!("IMG_{i}.CR3"),
},
written: dr_types::SourceRef::Local {
root: LIBRARY,
relative: format!("2026/2026-08-22/IMG_{i}.CR3"),
},
folders: vec!["2026".into(), "2026-08-22".into()],
name: format!("IMG_{i}.CR3"),
digest: "x".into(),
size: 10,
dated_from: DateSource::Capture,
backup: None,
})
.collect();
// The server took the first two; the third never went up.
let sent: Vec<Sent> = (0..2)
.map(|i| Sent {
remote_path: format!("PhotosRaw/2026/2026-08-22/IMG_{i}.CR3"),
digest: "x".into(),
transferred: true,
thumbnail: None,
})
.collect();
let confirmed = confirmed_sources(&imported, &sent);
assert_eq!(confirmed.len(), 2);
assert_eq!(confirmed[0].key(), "IMG_0.CR3");
assert_eq!(confirmed[1].key(), "IMG_1.CR3");
// IMG_2 keeps its card copy, which is now the only copy of it.
assert!(!confirmed.iter().any(|s| s.key() == "IMG_2.CR3"));
}
#[test]
ui/dr-ui/src/import_ui.rs
+44 -31
View File
@@ -188,7 +188,9 @@ impl ImportController {
fn can_start(&self) -> bool {
!self.running.get()
&& !self.card.borrow().is_empty()
&& !self.options().destination.is_empty()
// An account, because the library is on the server and an import
// with none has nowhere to go at all.
&& !self.upload_target.borrow().is_empty()
&& self.survey.borrow().map(|(n, _)| n > 0).unwrap_or(false)
}
}
@@ -252,17 +254,9 @@ pub fn render(window: &AppWindow, ctl: &Rc<ImportController>) {
.into(),
);
window.set_import_destination(stored.destination.as_str().into());
// Empty where no account is signed in, or where the user turned the upload
// off — the page then shows one destination because there is one.
window.set_import_upload_target(
if stored.upload {
ctl.upload_target.borrow().clone()
} else {
String::new()
}
.into(),
);
// The only destination there is. Empty means no account, which is what
// keeps Import disabled — see `can_start`.
window.set_import_upload_target(ctl.upload_target.borrow().as_str().into());
window.set_import_folder_template(stored.folder_template.as_str().into());
window.set_import_template_preview(preview(&stored, crate::library::now_secs()).into());
@@ -333,13 +327,6 @@ where
// does: another instance may have written it since.
*ctl.settings.settings.borrow_mut() = ctl.settings.store.load();
// A destination the user has never chosen defaults to somewhere
// that exists, rather than to an empty field they must decode.
if ctl.options().destination.is_empty() {
if let Some(home) = default_destination() {
ctl.edit(|s| s.destination = home.display().to_string());
}
}
*ctl.upload_target.borrow_mut() =
context().map(|c| c.library_label).unwrap_or_default();
ctl.refresh_volumes();
@@ -505,16 +492,6 @@ where
}
}
/// Somewhere sensible for a first import to go.
fn default_destination() -> Option<PathBuf> {
let home = std::env::var_os("HOME").map(PathBuf::from)?;
let pictures = home.join("Pictures");
// Only if it exists: inventing a folder the user has not asked for, in a
// field they may not read, is how photographs end up somewhere nobody
// looks. An empty field keeps Import disabled until they choose.
pictures.exists().then_some(pictures)
}
/// Count what is on the card, without copying anything.
///
/// Runs on the UI thread, which is defensible only because it is a directory
@@ -570,14 +547,21 @@ fn start(
Some(PathBuf::from(stored.backup.trim()))
};
let Some(upload) = context.upload else {
*ctl.error.borrow_mut() =
"Sign in first — the library this imports into is on the server.".into();
render(window, ctl);
return;
};
let request = Request {
card: PathBuf::from(ctl.card.borrow().clone()),
library: PathBuf::from(&stored.destination),
staging: upload.staging.clone(),
backup,
catalog: context.catalog.clone(),
filter: context.filter.clone(),
options: ctl.ingest_options(),
upload: stored.upload.then_some(context.upload).flatten(),
upload,
};
let cancel: import::Cancel = Default::default();
@@ -745,6 +729,35 @@ mod tests {
assert_eq!(preview(&s, AUG_22), "2026/EOS R5");
}
#[test]
fn an_import_needs_an_account_before_it_can_start() {
// The library is on the server, so "where does this go" has no local
// answer to fall back on. Import stays disabled rather than quietly
// filling a folder nothing ever shows.
let ctl = ImportController::new(
crate::settings_ui::SettingsController::new(),
crate::activity::ActivityLog::new(),
);
*ctl.card.borrow_mut() = "/run/media/duncan/EOS DIGITAL".into();
*ctl.survey.borrow_mut() = Some((12, 1234));
assert!(!ctl.can_start(), "no account, yet Import was offered");
*ctl.upload_target.borrow_mut() = "PhotosRaw".into();
assert!(ctl.can_start());
}
#[test]
fn an_empty_card_cannot_be_imported_either() {
let ctl = ImportController::new(
crate::settings_ui::SettingsController::new(),
crate::activity::ActivityLog::new(),
);
*ctl.card.borrow_mut() = "/run/media/duncan/EOS DIGITAL".into();
*ctl.upload_target.borrow_mut() = "PhotosRaw".into();
*ctl.survey.borrow_mut() = Some((0, 0));
assert!(!ctl.can_start());
}
#[test]
fn a_volume_says_why_it_is_being_offered() {
// "EOS DIGITAL" and "archive" look equally plausible in a list, and
ui/dr-ui/src/lib.rs
+1
View File
@@ -953,6 +953,7 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
// pushes, so a thumbnail made during an import is the
// one every other client gets.
thumbs: library::thumbs_dir(&session.server, &session.user_id),
staging: import::staging_dir(&session.server, &session.user_id),
}),
})
},