mirror of
https://github.com/storytold/lightcraft.git
synced 2026-10-24 10:09:01 -06:00
LRC-LIB-CATALOG-SWITCH: a failed library switch keeps the current library open; library names valid on Windows
Check the target (Lightroom catalog, file, unreadable folder, locked by another program) with Session::check_library before closing the open library, and if opening still fails, reopen the previous one (both errors reported if that fails too). New Library names reject Windows-reserved characters and trailing dots. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKWB9TCnvzJHV6xMLAj2J4
This commit is contained in:
@@ -272,6 +272,32 @@ impl SettingsLoad {
|
||||
}
|
||||
|
||||
impl Session {
|
||||
/// Check, without closing the library open now, that [`Session::open_library`] can be expected
|
||||
/// to open `dir`: not a Lightroom catalog, not some other file, a readable folder (or one not
|
||||
/// made yet), not locked by another program. Returns the library folder ([`resolve_dir`]).
|
||||
/// Nothing is created; a library folder that exists gets its lock taken and released again.
|
||||
/// Opening can still fail afterwards (a damaged catalog, a disk going away).
|
||||
pub fn check_library(&self, dir: impl AsRef<Path>) -> Result<PathBuf> {
|
||||
let dir = dir.as_ref().to_path_buf();
|
||||
if let Some(place) = crate::lightroom_folder::lightroom_catalog_at(&dir) {
|
||||
return Err(EngineError::LightroomCatalog { path: dir, catalog: place.catalog });
|
||||
}
|
||||
let dir = resolve_dir(&dir)?;
|
||||
if !dir.exists() {
|
||||
return Ok(dir);
|
||||
}
|
||||
if !dir.is_dir() {
|
||||
return Err(EngineError::Other(format!("{} is not a folder", dir.display())));
|
||||
}
|
||||
std::fs::read_dir(&dir).map_err(|e| EngineError::Other(format!("can't open library {}: {e}", dir.display())))?;
|
||||
// the library open in this session keeps its lock when reopened; any other must be free
|
||||
let current = self.library.as_ref().is_some_and(|l| l.on_disk && same_dir(&l.dir, &dir));
|
||||
if !current && is_library(&dir) {
|
||||
drop(LibraryLock::acquire(&dir, &program_name()).map_err(|e| EngineError::LibraryInUse(e.to_string()))?);
|
||||
}
|
||||
Ok(dir)
|
||||
}
|
||||
|
||||
/// Open (or create) the library at `dir` into this session, replacing its catalog. With
|
||||
/// `seed_demo`, a newly created library starts with the procedural demo photos.
|
||||
///
|
||||
@@ -721,4 +747,32 @@ mod resolve_tests {
|
||||
assert!(!e.contains("File exists"), "{e}");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
/// `check_library` refuses what `open_library` would, without touching the open library.
|
||||
#[test]
|
||||
fn check_library_refuses_locked_libraries_files_and_lightroom_catalogs() {
|
||||
let dir = std::env::temp_dir().join(format!("lc-check-lib-{}", std::process::id()));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
let (a, b) = (dir.join("a"), dir.join("b"));
|
||||
std::fs::create_dir_all(&b).unwrap();
|
||||
let mut s = Session::new().with_fs();
|
||||
s.open_library(&b, false).unwrap();
|
||||
s.close_library().unwrap();
|
||||
s.open_library(&a, false).unwrap();
|
||||
assert_eq!(s.check_library(&a).unwrap(), a, "the open library keeps its lock");
|
||||
assert_eq!(s.check_library(&b).unwrap(), b);
|
||||
assert_eq!(s.check_library(dir.join("new")).unwrap(), dir.join("new"));
|
||||
assert!(!dir.join("new").exists(), "nothing created");
|
||||
let held = LibraryLock::acquire(&b, "other").unwrap();
|
||||
assert!(matches!(s.check_library(&b), Err(EngineError::LibraryInUse(_))));
|
||||
drop(held);
|
||||
assert!(s.check_library(&b).is_ok(), "the probe released its lock");
|
||||
std::fs::write(dir.join("notes.txt"), b"x").unwrap();
|
||||
assert!(s.check_library(dir.join("notes.txt")).is_err());
|
||||
std::fs::write(dir.join("Cat.lrcat"), b"x").unwrap();
|
||||
assert!(matches!(s.check_library(dir.join("Cat.lrcat")), Err(EngineError::LightroomCatalog { .. })));
|
||||
assert!(s.library.as_ref().is_some_and(|l| l.dir == a));
|
||||
drop(s);
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -673,17 +673,27 @@ pub fn open_library(app: &mut LightcraftApp, p: &Value) -> Result<Value, String>
|
||||
}
|
||||
|
||||
/// Close the current library and open the one at `path`. If it can't be opened (locked by
|
||||
/// another program, unreadable, a file), the error is returned and the current library stays open.
|
||||
/// another program, unreadable, a file), the error is returned and the current library stays open:
|
||||
/// the target is checked before anything is closed, and if opening it still fails the previous
|
||||
/// library is opened again (both errors are reported if that fails too).
|
||||
fn switch_library(app: &mut LightcraftApp, path: &str) -> Result<Value, String> {
|
||||
let dir = lightcraft_engine::library::resolve_dir(std::path::Path::new(path)).map_err(|e| e.to_string())?;
|
||||
let dir = app.session.check_library(std::path::Path::new(path)).map_err(|e| format!("{path}: {e}"))?;
|
||||
let path = dir.to_string_lossy().into_owned();
|
||||
// the library being left goes in the list too (also when it was opened at launch)
|
||||
let previous = app.session.library.as_ref().filter(|l| l.on_disk).map(|l| l.dir.to_string_lossy().into_owned());
|
||||
let previous_dir = app.session.library.as_ref().filter(|l| l.on_disk).map(|l| l.dir.clone());
|
||||
let previous = previous_dir.as_ref().map(|d| d.to_string_lossy().into_owned());
|
||||
app.session.close_library().map_err(|e| e.to_string())?;
|
||||
app.session.open_library(&dir, false).map_err(|e| format!("{path}: {e}"))?;
|
||||
let opened = app.session.open_library(&dir, false).map(|_| ()).map_err(|e| format!("{path}: {e}"));
|
||||
app.lightroom_last = None;
|
||||
app.renderer.forget_all();
|
||||
app.ui.compare = None;
|
||||
if let Err(err) = opened {
|
||||
let Some(prev) = previous_dir else { return Err(err) };
|
||||
return match app.session.open_library(&prev, false) {
|
||||
Ok(_) => Err(err),
|
||||
Err(e) => Err(format!("{err}; reopening the previous library {} failed too: {e}", prev.display())),
|
||||
};
|
||||
}
|
||||
app.ui.settings.library_path = path.clone();
|
||||
if let Some(prev) = previous.filter(|p| p != &path && !app.ui.settings.recent_libraries.contains(p)) {
|
||||
remember_library(&mut app.ui.settings.recent_libraries, &prev);
|
||||
@@ -768,10 +778,19 @@ pub fn new_library(app: &mut LightcraftApp, p: &Value) -> Result<Value, String>
|
||||
switch_library(app, &dir.to_string_lossy())
|
||||
}
|
||||
|
||||
/// Characters a folder name can't hold on Windows (and `/`, which no system allows).
|
||||
const RESERVED_NAME_CHARS: [char; 9] = ['<', '>', ':', '"', '/', '\\', '|', '?', '*'];
|
||||
|
||||
fn valid_library_name(s: &str) -> Result<&str, String> {
|
||||
let n = s.trim();
|
||||
if n.is_empty() || n == "." || n == ".." || n.contains(['/', '\\']) || n.chars().any(char::is_control) {
|
||||
return Err(format!("app.newLibrary: {s:?} can't be a library name (a folder name, without / or \\)"));
|
||||
// Windows drops trailing dots and spaces from a folder name: refuse them rather than
|
||||
// creating a folder whose name differs from the one asked for (spaces around the name
|
||||
// are trimmed first)
|
||||
let bad = n.is_empty() || n == "." || n == ".." || n.ends_with(['.', ' ']) || n.contains(RESERVED_NAME_CHARS) || n.chars().any(char::is_control);
|
||||
if bad {
|
||||
return Err(format!(
|
||||
"app.newLibrary: {s:?} can't be a library name (a folder name without < > : \" / \\ | ? * and not ending in a dot or space)"
|
||||
));
|
||||
}
|
||||
Ok(n)
|
||||
}
|
||||
|
||||
@@ -162,3 +162,41 @@ fn missing_recent_libraries_are_greyed_out_and_pruned_and_the_list_is_capped() {
|
||||
assert!(app.ui.settings.recent_libraries.is_empty());
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
/// A library another program has open is refused before the current one is closed: the current
|
||||
/// library stays open with its photos, and the window title keeps its name.
|
||||
#[test]
|
||||
fn switching_to_a_locked_library_keeps_the_current_one_open() {
|
||||
let dir = temp("lockedlib");
|
||||
let mut app = app_in(&dir);
|
||||
app.run("app.newLibrary", json!({"path": dir.to_string_lossy(), "name": "Other"})).unwrap();
|
||||
app.run("app.openLibrary", json!({"path": dir.join("first").to_string_lossy()})).unwrap();
|
||||
// "another program" holds Other's lock
|
||||
let other = dir.join("Other");
|
||||
let held = lightcraft_catalog::LibraryLock::acquire(&other, "someone else").unwrap();
|
||||
let e = app.run("app.openLibrary", json!({"path": other.to_string_lossy()})).unwrap_err();
|
||||
assert!(e.contains("already open"), "{e}");
|
||||
assert!(app.session.library.as_ref().unwrap().dir.ends_with("first"), "the current library stays");
|
||||
assert!(app.session.library.as_ref().unwrap().on_disk);
|
||||
assert!(app.ui.settings.library_path.ends_with("first"));
|
||||
drop(held);
|
||||
app.run("app.openLibrary", json!({"path": other.to_string_lossy()})).unwrap();
|
||||
assert!(app.session.library.as_ref().unwrap().dir.ends_with("Other"));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
/// Library names that can't be folder names everywhere (Windows' reserved characters, a
|
||||
/// trailing dot) are refused; spaces around a name are trimmed.
|
||||
#[test]
|
||||
fn library_names_must_work_as_folder_names_on_every_system() {
|
||||
let dir = temp("libnames");
|
||||
let mut app = app_in(&dir);
|
||||
for bad in ["a<b", "a>b", "a:b", "a\"b", "a/b", "a\\b", "a|b", "a?b", "a*b", "name.", "..", " ", "a\u{7}"] {
|
||||
let e = app.run("app.newLibrary", json!({"path": dir.to_string_lossy(), "name": bad})).unwrap_err();
|
||||
assert!(e.contains("library name"), "{bad:?}: {e}");
|
||||
}
|
||||
assert!(app.session.library.as_ref().unwrap().dir.ends_with("first"));
|
||||
app.run("app.newLibrary", json!({"path": dir.to_string_lossy(), "name": " Trip 2026 "})).unwrap();
|
||||
assert_eq!(app.session.library.as_ref().unwrap().dir, dir.join("Trip 2026"));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user