fix(ui): a second file dialog waits for the open one instead of being refused (fixes #2150)

crates/ui-egui/src/pickers.rs

@@ -71,10 +71,25 @@ impl std::fmt::Debug for Picked {
     }
 }
 
+/// A request that came while another picker was showing (#2150).
+struct Waiting {
+    ask: Ask,
+    target: Option<PickTarget>,
+    then: Then,
+}
+
+impl std::fmt::Debug for Waiting {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("Waiting").field("target", &self.target).finish_non_exhaustive()
+    }
+}
+
 /// Pickers in flight and the picks they finished.
-#[derive(Clone, Debug, Default)]
+#[derive(Debug, Default)]
 pub struct Pickers {
-    /// A picker is showing, or its pick is waiting for the frame; a second request is refused
+    /// Requests that show, in order, once the picker before them is answered.
+    waiting: std::collections::VecDeque<Waiting>,
+    /// A picker is showing, or its pick is waiting for the frame; a second request waits
     /// until the pick's closure starts.
     showing: Arc<AtomicBool>,
     done: Arc<Mutex<Vec<Picked>>>,
@@ -169,8 +184,9 @@ impl PdfCraftApp {
     }
 
     /// Show `ask` without blocking the frame, and call `then` with the chosen paths on a later
-    /// frame ([`Self::process_picked`]). Returns false, after telling the user, when no picker
-    /// could be shown: another one is still open, or its worker couldn't start.
+    /// frame ([`Self::process_picked`]). While another picker is open, `ask` waits and shows
+    /// when that one is answered (the user is told). Returns false, after telling the user,
+    /// when no picker could be shown because its worker couldn't start.
     ///
     /// `then` is not called when the picker is cancelled, or when `target` is given and by then
     /// that document is no longer the active one or has been edited (the user is told). Use
@@ -180,12 +196,19 @@ impl PdfCraftApp {
     /// `pick_override` answers instead of a native panel (tests and automation); the answer
     /// still arrives on a later frame, like a real pick.
     pub(crate) fn ask(&mut self, ask: Ask, target: Option<DocId>, then: impl FnOnce(&mut PdfCraftApp, Vec<PathBuf>) + Send + 'static) -> bool {
-        if self.pickers.showing.swap(true, Ordering::SeqCst) {
-            self.notify_tr("Another file dialog is still open. Finish with it first.");
-            return false;
-        }
         let target = self.pick_target(target);
         let then: Then = Box::new(then);
+        if self.pickers.showing.swap(true, Ordering::SeqCst) {
+            // Refusing would drop what asked, such as a finished Reduce File Size copy.
+            self.pickers.waiting.push_back(Waiting { ask, target, then });
+            self.notify_tr("Another file dialog is still open. This one will open when it closes.");
+            return true;
+        }
+        self.show_picker(ask, target, then)
+    }
+
+    /// [`Self::ask`] once `showing` is claimed for it.
+    fn show_picker(&mut self, ask: Ask, target: Option<PickTarget>, then: Then) -> bool {
         if let Some(paths) = self.pick_override.clone() {
             self.pickers.deliver(target, paths.into_iter().map(PathBuf::from).collect(), then);
             return true;
@@ -256,6 +279,13 @@ impl PdfCraftApp {
                 self.notify_fmt("That didn't work: an internal error stopped it ({m}).", &[("m", m.as_str())]);
             }
         }
+        // After the closures, so a picker one of them asked for comes before the waiting ones.
+        if !self.pickers.waiting.is_empty()
+            && !self.pickers.showing.swap(true, Ordering::SeqCst)
+            && let Some(Waiting { ask, target, then }) = self.pickers.waiting.pop_front()
+        {
+            self.show_picker(ask, target, then);
+        }
     }
 }
 
@@ -359,34 +389,79 @@ mod tests {
     }
 
     #[test]
-    fn a_second_picker_is_refused_while_one_is_showing() {
+    fn a_second_picker_waits_for_the_one_showing_and_opens_when_it_closes() {
+        let mut app = PdfCraftApp::new();
+        app.pick_override = Some(vec!["c.pdf".into()]);
+        let got = Arc::new(Mutex::new(Vec::new()));
+        let first = got.clone();
+        assert!(app.ask(Ask::Save(rfd::AsyncFileDialog::new()), None, move |_, _| first.lock().unwrap().push("first")));
+        let second = got.clone();
+        assert!(app.ask(Ask::Folder(rfd::AsyncFileDialog::new()), None, move |_, _| second.lock().unwrap().push("second")));
+        assert!(app.toast.as_ref().is_some_and(|(m, _)| m.contains("will open when it closes")), "the user is told it will follow");
+        app.process_picked();
+        assert_eq!(*got.lock().unwrap(), vec!["first"], "the waiting picker shows only once the first one is answered");
+        assert!(app.pickers.showing.load(Ordering::SeqCst), "the waiting picker owns the flag now");
+        app.process_picked();
+        assert_eq!(*got.lock().unwrap(), vec!["first", "second"]);
+        assert!(!app.pickers.showing.load(Ordering::SeqCst));
+    }
+
+    #[test]
+    fn a_waiting_picker_still_opens_when_the_one_showing_is_cancelled() {
         let mut app = PdfCraftApp::new();
         app.pick_override = Some(vec!["c.pdf".into()]);
         app.pickers.showing.store(true, Ordering::SeqCst);
         let got = Arc::new(Mutex::new(Vec::new()));
         let into = got.clone();
-        assert!(!app.ask(Ask::File(rfd::AsyncFileDialog::new()), None, move |_, paths| into.lock().unwrap().extend(paths)));
+        assert!(app.ask(Ask::File(rfd::AsyncFileDialog::new()), None, move |_, paths| into.lock().unwrap().extend(paths)));
         app.process_picked();
-        assert!(got.lock().unwrap().is_empty());
-        assert!(app.pickers.showing.load(Ordering::SeqCst), "the showing picker still owns the flag");
-        assert!(app.toast.as_ref().is_some_and(|(m, _)| m.contains("Another file dialog")), "the user is told");
+        assert!(got.lock().unwrap().is_empty(), "the showing picker still owns the flag");
+        app.pickers.deliver(None, vec![], Box::new(|_, _| {}));
+        app.process_picked();
+        app.process_picked();
+        assert_eq!(*got.lock().unwrap(), vec![PathBuf::from("c.pdf")]);
+    }
+
+    /// #2150: Reduce File Size finishing under an open Save As panel used to drop its bytes.
+    #[test]
+    fn a_reduced_copy_finished_under_another_dialog_is_saved_once_it_closes() {
+        let dir = std::env::temp_dir().join(format!("pdfcraft-ui-pickers-{}-reduce", std::process::id()));
+        let _ = std::fs::remove_dir_all(&dir);
+        std::fs::create_dir_all(&dir).unwrap();
+        let out = dir.join("notes (reduced).pdf");
+        let mut app = PdfCraftApp::new();
+        app.set_option("language", "en").unwrap();
+        app.open_bytes("notes.txt", None, b"hello".to_vec()).unwrap();
+        let (_, doc) = app.active_ids().unwrap();
+        app.pick_override = Some(vec![out.to_string_lossy().into_owned()]);
+        // Save As is still open on another tab when the optimizer finishes.
+        app.pickers.showing.store(true, Ordering::SeqCst);
+        let reduced = Arc::new(b"%PDF-1.7 reduced".to_vec());
+        app.save_optimized(doc, crate::optimize_ui::OptimizeKind::Reduce, Ok((reduced.clone(), String::new())));
+        app.pickers.deliver(None, vec![], Box::new(|_, _| {}));
+        app.process_picked();
+        app.process_picked();
+        assert_eq!(std::fs::read(&out).ok(), Some(reduced.to_vec()), "the reduced copy is saved where the user chose");
+        let _ = std::fs::remove_dir_all(&dir);
     }
 
     #[test]
-    fn a_follow_up_that_asks_again_gets_its_picker_even_if_another_request_came_first() {
+    fn a_follow_up_that_asks_again_gets_its_picker_before_a_request_that_came_first() {
         let mut app = PdfCraftApp::new();
         app.pick_override = Some(vec!["first.fdf".into()]);
         let got = Arc::new(Mutex::new(Vec::new()));
         let into = got.clone();
         assert!(app.ask(Ask::Files(rfd::AsyncFileDialog::new()), None, move |app, _| {
             // Merge data: the spreadsheet's save picker, asked from the first pick's closure.
-            assert!(app.ask(Ask::Save(rfd::AsyncFileDialog::new()), None, move |_, paths| into.lock().unwrap().extend(paths)));
+            assert!(app.ask(Ask::Save(rfd::AsyncFileDialog::new()), None, move |_, _| into.lock().unwrap().push("chained")));
         }));
-        // Another command asks for a picker before the frame uses the first pick: refused.
-        assert!(!app.ask(Ask::File(rfd::AsyncFileDialog::new()), None, |_, _| panic!("must not run")));
-        app.process_picked();
-        app.process_picked();
-        assert_eq!(*got.lock().unwrap(), vec![PathBuf::from("first.fdf")], "the chained picker answered");
+        // Another command asks for a picker before the frame uses the first pick: it waits.
+        let other = got.clone();
+        assert!(app.ask(Ask::File(rfd::AsyncFileDialog::new()), None, move |_, _| other.lock().unwrap().push("other")));
+        for _ in 0..4 {
+            app.process_picked();
+        }
+        assert_eq!(*got.lock().unwrap(), vec!["chained", "other"], "the chained picker answers first");
     }
 
     #[test]