Merge the deferred import review fixes (francoisgn/lightcraft#2) into round3a-integration

crates/ui-egui/src/import.rs

@@ -91,6 +91,10 @@ pub struct ImportDialog {
     /// The candidate clicked last: where a Shift-click range starts ([`ImportDialog::click`]).
     #[serde(skip)]
     pub last_clicked: Option<usize>,
+    /// The library the scan was made in ([`lightcraft_engine::Session::library_generation`]): its duplicate
+    /// decisions and album ids belong to that catalog, so the review is not shown or confirmed in another.
+    #[serde(skip)]
+    pub library: Option<u64>,
 }
 
 /// A candidate's file type, as the review groups them: its format, or its extension when the
@@ -114,6 +118,11 @@ impl ImportDialog {
         let checked = candidates.iter().map(|c| c.duplicate.is_none() && c.error.is_none()).collect();
         ImportDialog { candidates, checked, on_deleted: "restore".into(), ..Default::default() }
     }
+
+    /// Whether the library the scan was made in is still the open one (a review that names none counts).
+    fn in_library(&self, app: &LightcraftApp) -> bool {
+        self.library.is_none_or(|g| g == app.session.library_generation())
+    }
     /// Mark the candidates whose photo is in Recently Deleted. They are checked when the choice
     /// for them (`on_deleted`) imports them — by default it restores them, so a photo deleted by
     /// mistake comes back by importing its file again (issue #706).
@@ -471,6 +480,9 @@ pub fn open(app: &mut LightcraftApp, paths: Vec<String>) -> Result<Value, String
     if app.scan.is_some() {
         return Err("a scan is already running".into());
     }
+    if app.review_pending.is_some() {
+        return Err("an import review is waiting for the open dialog to close".into());
+    }
     let sources = paths.clone();
     let (input, paths) = ScanInput::new(&mut app.session, &paths);
     let progress = std::sync::Arc::new(ScanProgress::default());
@@ -627,16 +639,24 @@ pub fn poll_scan(app: &mut LightcraftApp, ctx: &egui::Context) {
     // only an album the review offers (it may have been deleted while the scan ran)
     d.album = task.album.filter(|id| app.session.catalog.album(lightcraft_catalog::AlbumId(*id)).is_some_and(|a| !a.folder && !a.is_smart()));
     d.sources = task.sources;
+    d.library = Some(app.session.library_generation());
     app.review_pending = Some(crate::state::Dialog::Import { opts: Box::new(d) });
     show_pending_review(app, ctx);
 }
 
 /// Open the review of a finished scan once no other dialog is open (a face model's licence, Settings…
-/// opened while the scan ran stays), checked again shortly while it waits.
+/// opened while the scan ran stays), checked again shortly while it waits. A review whose library was
+/// closed meanwhile (Settings can open another) is dropped.
 fn show_pending_review(app: &mut LightcraftApp, ctx: &egui::Context) {
     if app.review_pending.is_none() {
         return;
     }
+    if let Some(crate::state::Dialog::Import { opts }) = &app.review_pending
+        && !opts.in_library(app)
+    {
+        app.review_pending = None;
+        return;
+    }
     if app.ui.dialog.is_none() {
         app.ui.dialog = app.review_pending.take();
     } else {
@@ -660,10 +680,11 @@ pub(crate) fn stall_scan_of(app: &mut LightcraftApp, browse: bool) -> Box<dyn st
     Box::new(tx)
 }
 
-/// Whether an import or any scan (the review's, or a Local folder browse) is running, which a drop of
-/// photos must not replace: `open` refuses while a scan runs, and a browse that finishes replaces `app.import`.
+/// Whether an import or any scan (the review's, or a Local folder browse) is running, or a finished scan's
+/// review waits for a dialog to close, which a drop of photos must not replace: `open` refuses while a scan
+/// runs, and a browse that finishes replaces `app.import`.
 pub fn running(app: &LightcraftApp) -> bool {
-    app.import.is_some() || app.scan.is_some()
+    app.import.is_some() || app.scan.is_some() || app.review_pending.is_some()
 }
 
 impl ScanTask {
@@ -673,9 +694,17 @@ impl ScanTask {
     }
 }
 
-/// Start importing the dialog's checked files (the dialog's OK / `ui.dialog.confirm`).
+/// Start importing the dialog's checked files (the dialog's OK / `ui.dialog.confirm`). Refused while another
+/// import runs: replacing `app.import` would stop its worker.
 pub fn start(app: &mut LightcraftApp, d: &ImportDialog) -> Result<Value, String> {
     busy_synchronizing(app)?;
+    if !d.in_library(app) {
+        return Err("another library was opened since the photos were scanned: nothing was imported".into());
+    }
+    // a browse scan that finishes starts its own import, replacing this one
+    if app.import.is_some() || app.scan.as_ref().is_some_and(|t| t.browse) {
+        return Err(crate::i18n::tr("An import is running").to_string());
+    }
     let queue = d.selected_paths();
     if queue.is_empty() {
         return Err("no photos selected".into());

crates/ui-egui/src/tests_drop_import.rs

@@ -166,6 +166,60 @@ fn a_finished_scan_waits_for_the_open_dialog_before_opening_the_review() {
     assert_eq!(opts.candidates.len(), 2, "the scan's photos were kept");
 }
 
+/// A scan that finishes behind Settings, which then opens another library: the review of the first
+/// library's scan doesn't open over the second one once Settings closes, and nothing is imported into it.
+#[test]
+fn a_waiting_review_is_dropped_when_another_library_opens() {
+    let dir = Scratch::new("switch");
+    let mut h = app(&dir);
+    crate::import::open(&mut h.app, vec![dir.path("trip")]).unwrap();
+    h.app.ui.dialog = Some(Dialog::Settings { tab: "general".into() });
+    assert!(h.step_until(SETTLE, |h| h.app.scan.is_none()), "the scan ends");
+    h.step();
+    let stale = h.app.review_pending.clone().expect("the review waits behind Settings");
+    h.app.run("app.openLibrary", json!({"path": dir.path("other")})).unwrap();
+    h.app.ui.dialog = None;
+    for _ in 0..10 {
+        h.step();
+    }
+    assert!(h.app.ui.dialog.is_none(), "no review from the other library opens");
+    assert!(h.app.review_pending.is_none());
+    // a review confirmed after the switch (an agent holding it) imports nothing either
+    assert!(crate::panels::dialogs::confirm_dialog(&mut h.app, &stale).is_err());
+    for _ in 0..10 {
+        h.step();
+    }
+    assert!(h.app.import.is_none(), "nothing is imported");
+    assert_eq!(photos(&h), 0);
+}
+
+/// A review waiting behind another dialog counts as an import in progress: a direct drop meanwhile is
+/// refused, and the review's OK never replaces an import that runs when it is confirmed.
+#[test]
+fn a_waiting_review_never_replaces_a_running_import() {
+    let dir = Scratch::new("pending-running");
+    let mut h = app(&dir);
+    h.app.ui.settings.import_dropped_directly = true;
+    write_png(&dir.path("more/c.png"), 3);
+    crate::import::open(&mut h.app, vec![dir.path("trip")]).unwrap();
+    h.app.ui.dialog = Some(Dialog::WhatsNew);
+    assert!(h.step_until(SETTLE, |h| h.app.scan.is_none()), "the scan ends");
+    h.step();
+    assert!(h.app.review_pending.is_some(), "the review waits behind the dialog");
+    assert!(crate::import::start_paths(&mut h.app, vec![dir.path("more")]).is_err(), "no second import starts");
+    assert!(h.app.import.is_none());
+    h.app.ui.dialog = None;
+    h.step();
+    let Some(review) = h.app.ui.dialog.clone() else { panic!("the review opens once the dialog is closed") };
+    assert!(matches!(review, Dialog::Import { .. }));
+    // an import that started anyway (a Local folder browse, say) is the one that runs
+    h.app.import = Some(crate::import::ImportTask::new(vec![dir.path("more")], json!({"mode": "add"}), 0, false));
+    assert!(crate::panels::dialogs::confirm_dialog(&mut h.app, &review).is_err(), "the review's OK is refused");
+    assert_eq!(h.app.import.as_ref().map(|t| t.total), Some(1), "the running import stays");
+    assert!(h.step_until(SETTLE, |h| h.app.import.is_none()), "it runs to the end");
+    assert_eq!(photos(&h), 1, "only the running import's photo arrives");
+}
+
 /// A drop while an import runs would open a review whose OK replaces the running import: it is
 /// refused instead, and the card says so.
 #[test]