Merge PR #2701 (review round e2210ed14)

crates/ui-egui/src/menubar.rs

@@ -705,7 +705,7 @@ pub(crate) fn expanded(app: &LightcraftApp, name: &str) -> Option<Vec<MenuNode>>
                     let d = std::path::Path::new(r);
                     // a library a check found missing (moved, drive disconnected) is greyed out; the
                     // check runs on a worker (`RecentChecks`), never here: the menu is rebuilt every frame
-                    let gone = app.recent_checks.reason(r).is_some();
+                    let gone = app.recent_checks.exists(r) == Some(false);
                     item("app.openLibrary", json!({"path": r}), label(r), None, can && !gone, Some(open.as_deref() == Some(d)))
                 })
                 .collect();

crates/ui-egui/src/panels/library_problem.rs

@@ -198,6 +198,13 @@ pub enum Unavailable {
 }
 
 impl Unavailable {
+    /// The folder isn't a library that is there to open (gone, a file, not a library any more, or
+    /// unlistable): what greys out File ▸ Open Recent Library and makes `app.recentLibraries` say
+    /// `exists: false`. An unreadable catalog or a check that failed doesn't say the library is gone.
+    pub fn is_missing(&self) -> bool {
+        matches!(self, Unavailable::NotFound | Unavailable::NotAFolder | Unavailable::NoLibrary | Unavailable::Unreadable(_))
+    }
+
     /// In the interface language.
     pub fn text(&self) -> String {
         match self {
@@ -252,6 +259,16 @@ impl RecentChecks {
         self.known.get(path).and_then(|a| a.why.as_ref())
     }
 
+    /// Whether the library at `path` is there: `Some(false)` when a check found it missing
+    /// ([`Unavailable::is_missing`]), `Some(true)` when one found it, `None` when no check could tell.
+    pub fn exists(&self, path: &str) -> Option<bool> {
+        match self.known.get(path)?.why.as_ref() {
+            None => Some(true),
+            Some(why) if why.is_missing() => Some(false),
+            Some(_) => None,
+        }
+    }
+
     /// Whether a check of `path` has answered.
     pub fn is_known(&self, path: &str) -> bool {
         self.known.contains_key(path)
@@ -282,9 +299,11 @@ impl RecentChecks {
         self.known.insert(path.to_string(), Answer { why: None, at: web_time::Instant::now() });
     }
 
-    /// Opening `path` failed: what the open said is its answer until the next check.
-    pub fn failed(&mut self, path: &str, error: &str) {
-        self.known.insert(path.to_string(), Answer { why: Some(Unavailable::CheckFailed(error.to_string())), at: web_time::Instant::now() });
+    /// Opening `path` from the launch window failed. That says nothing about whether it is there
+    /// (another program may have it open): its answer is dropped, so it is checked again on the next
+    /// frame. The launch list keeps the open's error on that row for as long as it shows.
+    pub fn failed(&mut self, path: &str) {
+        self.known.remove(path);
     }
 
     /// `path` left the list: forget its answer.
@@ -429,7 +448,7 @@ fn open(app: &mut LightcraftApp, path: Option<String>) {
             // a failed switch reopens the library that was open behind the launch list
             let generation = app.session.library.as_ref().map(|_| app.session.library_generation());
             if let Some(path) = path.as_deref() {
-                app.recent_checks.failed(path, &e);
+                app.recent_checks.failed(path);
             }
             if let Some(p) = app.library_problem.as_mut() {
                 if let Some(c) = p.chooser.as_mut() {

crates/ui-egui/src/panels/settings.rs

@@ -1456,8 +1456,9 @@ pub(crate) fn free_name(location: &std::path::Path, base: &str) -> std::path::Pa
 
 /// `app.recentLibraries {remove?: path, clear?: bool}`: the recently opened libraries, newest
 /// first, as `[{path, name, exists, checking, open, lastOpened}]`. `exists` is what the last
-/// background check found (`false`: moved, on a disconnected drive or no longer a library;
-/// `null`: not checked yet, `checking` then says whether a check is running): the command never
+/// background check found (`false`: moved, on a disconnected drive, a folder that can't be listed
+/// or no longer a library; `null`: not checked yet, or the check couldn't tell — an unreadable catalog, a check
+/// that failed; `checking` says whether a check is running): the command never
 /// looks at a folder itself, as one on an offline network share can take seconds to answer.
 /// `lastOpened`: ISO 8601 time, `null` when not known. `remove` forgets that
 /// one entry (an error if the list doesn't hold it) and `clear` forgets them all; neither touches
@@ -1486,7 +1487,7 @@ pub fn recent_libraries(app: &mut LightcraftApp, p: &Value) -> Result<Value, Str
         .iter()
         .map(|r| {
             let d = std::path::Path::new(r);
-            let exists = checks.is_known(r).then(|| checks.reason(r).is_none());
+            let exists = checks.exists(r);
             json!({"path": r, "name": library_name(d), "exists": exists, "checking": checks.is_checking(r), "open": open.as_deref() == Some(d), "lastOpened": settings.recent_opened.get(r)})
         })
         .collect();

crates/ui-egui/src/tests_launch_library.rs

@@ -775,7 +775,11 @@ fn opening_a_library_clears_an_old_missing_answer() {
     let a = make_library(&dir.join("A"));
     let mut app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), Services::default());
     app.ui.settings.recent_libraries = vec![a.clone()];
-    app.recent_checks.failed(&a, "the drive was disconnected");
+    app.recent_checks = crate::panels::library_problem::RecentChecks::with_check(|_| Some(crate::panels::library_problem::Unavailable::NotFound));
+    let ctx = egui::Context::default();
+    crate::panels::library_problem::logic(&mut app, &ctx);
+    assert!(crate::tasks::wait(&mut app, &ctx, T), "checked");
+    assert_eq!(app.recent_checks.exists(&a), Some(false), "found missing (the drive was disconnected)");
     let mut h = Headless::new(app, [1300.0, 900.0], 1.0);
     h.app.run("app.openLibrary", json!({"path": a})).unwrap();
     for _ in 0..4 {
@@ -785,3 +789,46 @@ fn opening_a_library_clears_an_old_missing_answer() {
     assert_eq!(h.app.recent_checks.reason(&a), None);
     let _ = std::fs::remove_dir_all(&dir);
 }
+
+/// Review of #2701: a library that fails to open from the launch window because another program
+/// has it is there all the same. The row shows the open's error, but the menu keeps it enabled and
+/// `app.recentLibraries` says it exists; an unreadable catalog doesn't say the library is gone
+/// either (`exists: null`).
+#[test]
+fn a_failed_open_or_an_unreadable_catalog_doesnt_mark_a_library_missing() {
+    use crate::menubar::MenuNode;
+    let dir = temp("held");
+    let held = make_library(&dir.join("Held"));
+    let mut app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), Services::default());
+    app.ui.settings.recent_libraries = vec![held.clone()];
+    app.ui.settings.library_path = held.clone();
+    app.library_problem = Some(LibraryProblem::choose_later(&app.ui.settings));
+    let mut h = Headless::new(app, [1300.0, 900.0], 1.0);
+    let answered =
+        |h: &Headless| h.app.library_problem.as_ref().and_then(|p| p.chooser.as_ref()).is_some_and(|c| c.libraries.iter().all(|l| !l.checking));
+    assert!(h.step_until(T, answered));
+    assert_eq!(h.app.recent_checks.exists(&held), Some(true));
+    // another program has it: Open from the launch window fails
+    let lock = lightcraft_catalog::LibraryLock::acquire(Path::new(&held), "another LightCraft").unwrap();
+    settle(&mut h);
+    let r = h.request("ui.clickWidget", json!({"id": "button:libraryOpen"}), T);
+    assert_eq!(r["ok"], true, "{r} {:?}", h.app.widgets.iter().map(|(w, _)| w.clone()).filter(|w| w.contains("library")).collect::<Vec<_>>());
+    settle(&mut h);
+    let c = h.app.library_problem.as_ref().and_then(|p| p.chooser.clone()).unwrap();
+    assert!(c.libraries[0].unavailable.is_some(), "the row shows why it didn't open: {c:?}");
+    assert!(h.step_until(T, |h| !h.app.recent_checks.is_running() && h.app.recent_checks.is_known(&held)), "checked again");
+    assert_eq!(h.app.recent_checks.exists(&held), Some(true), "a failed open doesn't mark it missing");
+    let list = h.app.run("app.recentLibraries", json!({})).unwrap();
+    assert_eq!(list[0]["exists"], true, "{list}");
+    assert!(matches!(open_recent(&h.app).first(), Some(MenuNode::Item { enabled: true, .. })), "{:?}", open_recent(&h.app));
+    drop(lock);
+
+    // an unreadable catalog: not known to be gone
+    h.app.recent_checks = crate::panels::library_problem::RecentChecks::with_check(|_| {
+        Some(crate::panels::library_problem::Unavailable::CatalogUnreadable("denied".into()))
+    });
+    assert!(h.step_until(T, |h| h.app.recent_checks.is_known(&held) && !h.app.recent_checks.is_running()));
+    assert_eq!(h.app.run("app.recentLibraries", json!({})).unwrap()[0]["exists"], serde_json::Value::Null);
+    assert!(matches!(open_recent(&h.app).first(), Some(MenuNode::Item { enabled: true, .. })));
+    let _ = std::fs::remove_dir_all(&dir);
+}

docs/control-protocol.md

@@ -187,10 +187,12 @@ Enter opens it and Escape quits (over an open library: closes the window and kee
 opening a library ends it. While it shows, the settings are saved as usual (a removal is kept).
 
 `app.recentLibraries` returns `[{path, name, exists, checking, open, lastOpened}]`. `exists` is what the last
-background check found (`false`: moved, on a disconnected drive or no longer a library; `null`: not checked yet) and
-`checking` whether one is running: the command never looks at a folder itself. Recent libraries are checked in the
+background check found (`false`: moved, on a disconnected drive, a folder that can't be listed or no longer a
+library; `null`: not checked yet, or the check couldn't tell, e.g. an unreadable catalog) and `checking` whether one
+is running: the command never looks at a folder itself. A failed open never marks a library missing (another program
+may have it). Recent libraries are checked in the
 background when the app starts, again after 30 s, and when the launch window opens; File ▸ Open Recent Library
-greys out only an entry a check found missing. `lastOpened`: ISO 8601, `null` when not known; kept in `ui.json` →
+greys out only an entry a check found missing (an unreadable catalog or a failed open doesn't grey it out). `lastOpened`: ISO 8601, `null` when not known; kept in `ui.json` →
 `settings.recentLibrariesOpened`. `{remove: path}` forgets one entry (an error if the list
 doesn't hold it) and `{clear: true}` (or `app.clearRecentLibraries`) all of them; nothing on disk is touched.
 

docs/whats-new.md

@@ -23,7 +23,7 @@
   it. One that is gone (moved, renamed, on a disconnected drive), can't be read or no longer holds a library is
   greyed out with the reason. Each one is checked in the background, showing *Checking…* until it answers (and that
   the drive isn't answering after 5 s), so the list never waits on a network share that is offline or a disk that
-  has to wake up; File ▸ Open Recent Library uses the same answers. Making the list only looks: it never locks or writes anything in your libraries, so one
+  has to wake up; File ▸ Open Recent Library uses the same answers (it greys out only libraries that are gone). Making the list only looks: it never locks or writes anything in your libraries, so one
   that another program has open is reported, with who has it, when you press **Open**, and the window stays. The
   last library is selected; **Open** (or Return, or a double-click) opens it,
   **Open Another…** asks for a folder, **New Library…** makes one, **Quit** (or Esc) quits. ↑ and ↓ move through