Reverse geocoding: surface download failures, cache the gazetteer, e2e download test

- A failed, cancelled or finished place-names download is reported once
  as a toast (failures as an error toast that says what to do), and
  metadata.placeNamesStatus reports downloading / lastDownload / lastError.
- The parsed GeoNames data is cached in the session and re-read only when
  the cities file's size or modification time changes.
- End-to-end download test against a fake HTTP server on 127.0.0.1:
  a synthetic zip, a garbage zip, a truncated body and 404s.
- Info panel: Fill Location gets its own row (it was clipped beside
  Show on Map in a narrow panel).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Andrew Frahn
2026-10-11 21:09:53 +11:00
co-authored by Claude Opus 5.5
parent b28b05c475
commit 23e046daba
15 changed files with 257 additions and 40 deletions
+203 -21
View File
@@ -5,7 +5,9 @@
//!
//! Sources: GeoNames dump readme (prose); own design.
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use std::sync::{Arc, Mutex, PoisonError};
use std::time::SystemTime;
use serde_json::{Value, json};
@@ -21,12 +23,79 @@ fn dataset_dir(p: &Value) -> Option<PathBuf> {
str_param(p, "dataset").map(PathBuf::from).or_else(|| crate::config::config_dir().map(|d| d.join("geonames")))
}
fn status(_s: &mut Session, p: &Value) -> Result<Value> {
/// How a place-names download ended.
#[derive(Clone, Debug, PartialEq)]
pub enum DownloadOutcome {
/// Done: this many places.
Done(usize),
/// Failed, in words the user can act on.
Failed(String),
Cancelled,
}
#[derive(Default)]
struct DownloadShared {
running: bool,
last: Option<DownloadOutcome>,
/// The last outcome, until the UI has shown it ([`PlaceNames::take_outcome`]).
unreported: Option<DownloadOutcome>,
}
/// The session's place names: the parsed gazetteer (kept while its file is unchanged) and the download state.
#[derive(Default)]
pub struct PlaceNames {
shared: Arc<Mutex<DownloadShared>>,
/// (cities file, its size, its modification time, the parsed names).
cache: Option<(PathBuf, u64, Option<SystemTime>, Arc<GeoNames>)>,
}
impl PlaceNames {
fn lock(&self) -> std::sync::MutexGuard<'_, DownloadShared> {
self.shared.lock().unwrap_or_else(PoisonError::into_inner)
}
/// A download that ended since the last call (the UI shows it once: an error stays readable as a toast).
pub fn take_outcome(&self) -> Option<DownloadOutcome> {
self.lock().unreported.take()
}
pub fn downloading(&self) -> bool {
self.lock().running
}
/// The gazetteer in `dir`, parsed once and re-read only when the cities file's size or time changes.
fn load(&mut self, dir: &Path) -> std::result::Result<Arc<GeoNames>, lightcraft_geocode::GeocodeError> {
let file = dir.join(GEONAMES_FILES[0]);
let meta = std::fs::metadata(&file).ok();
let (len, mtime) = (meta.as_ref().map_or(0, |m| m.len()), meta.as_ref().and_then(|m| m.modified().ok()));
if let Some((f, l, t, g)) = &self.cache
&& meta.is_some()
&& *f == file
&& *l == len
&& *t == mtime
{
return Ok(g.clone());
}
let g = Arc::new(GeoNames::load(dir)?);
self.cache = Some((file, len, mtime, g.clone()));
Ok(g)
}
}
fn status(s: &mut Session, p: &Value) -> Result<Value> {
let dir = dataset_dir(p);
let size = dir.as_ref().and_then(|d| std::fs::metadata(d.join(GEONAMES_FILES[0])).ok()).map(|m| m.len());
Ok(
json!({"downloaded": size.is_some(), "bytes": size, "path": dir, "attribution": GEONAMES_ATTRIBUTION, "source": lightcraft_geocode::GEONAMES_BASE}),
)
let sh = s.place_names.lock();
let (last, error) = match &sh.last {
None => (Value::Null, Value::Null),
Some(DownloadOutcome::Done(n)) => (json!({"places": n}), Value::Null),
Some(DownloadOutcome::Failed(e)) => (json!("failed"), json!(e)),
Some(DownloadOutcome::Cancelled) => (json!("cancelled"), Value::Null),
};
Ok(json!({
"downloaded": size.is_some(), "bytes": size, "path": dir, "downloading": sh.running, "lastDownload": last, "lastError": error,
"attribution": GEONAMES_ATTRIBUTION, "source": lightcraft_geocode::GEONAMES_BASE,
}))
}
/// Which place fields to write: a field is filled when it is empty, or always with `overwrite`.
@@ -59,7 +128,7 @@ fn reverse(s: &mut Session, p: &Value) -> Result<Value> {
return Err(bad(C, "none of these photos has a GPS position"));
}
let dir = dataset_dir(p).ok_or_else(|| bad(C, "no settings folder: give `dataset`"))?;
let names = GeoNames::load(&dir).map_err(|e| bad(C, format!("{e}: download the place names first (Photo ▸ Download Place Names)")))?;
let names = s.place_names.load(&dir).map_err(|e| bad(C, format!("{e}: download the place names first (Photo ▸ Download Place Names)")))?;
// a large selection shows in the activity stack and can be stopped
let task = (todo.len() > 1).then(|| s.activity.start("geocode", "Looking up places", Cancel::Yes));
let mut ops = Vec::new();
@@ -145,25 +214,43 @@ mod download {
if !(base.starts_with("https://") || base.starts_with("http://127.0.0.1")) {
return Err(bad(C, "`base` must be an https:// address"));
}
let shared = s.place_names.shared.clone();
{
let mut sh = s.place_names.lock();
if sh.running {
return Err(bad(C, "the place names are already downloading"));
}
sh.running = true;
}
let cancel = Arc::new(AtomicBool::new(false));
let task = s.activity.start("download", "Downloading place names", Cancel::Flag(cancel.clone()));
task.set_unit(Unit::Bytes);
task.detail("GeoNames");
let handle = task.handle();
let thread_dir = dir.clone();
std::thread::Builder::new()
.name("place-names-download".into())
.spawn(move || {
// the activity row goes when the thread ends, however it ends
let _task = task;
let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| run(&base, &thread_dir, &cancel, &handle)));
match outcome {
Ok(Ok(n)) => log::info!("place names: {n} places downloaded"),
Ok(Err(e)) => log::warn!("place names download failed: {e}"),
Err(_) => log::warn!("place names download stopped unexpectedly"),
}
})
.map_err(|e| bad(C, format!("could not start the download: {e}")))?;
let thread_shared = shared.clone();
let spawned = std::thread::Builder::new().name("place-names-download".into()).spawn(move || {
// the activity row goes when the thread ends, however it ends
let _task = task;
let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| run(&base, &thread_dir, &cancel, &handle)));
let outcome = match outcome {
Ok(Ok(n)) => DownloadOutcome::Done(n),
_ if cancel.load(std::sync::atomic::Ordering::Relaxed) => DownloadOutcome::Cancelled,
Ok(Err(e)) => DownloadOutcome::Failed(format!("Could not download the place names: {e}. Check the connection and try again.")),
Err(_) => DownloadOutcome::Failed("The place names download stopped unexpectedly. Try again.".into()),
};
if let DownloadOutcome::Failed(e) = &outcome {
log::warn!("{e}");
}
let mut sh = thread_shared.lock().unwrap_or_else(PoisonError::into_inner);
sh.running = false;
sh.last = Some(outcome.clone());
sh.unreported = Some(outcome);
});
if let Err(e) = spawned {
shared.lock().unwrap_or_else(PoisonError::into_inner).running = false;
return Err(bad(C, format!("could not start the download: {e}")));
}
Ok(json!({"started": true, "path": dir, "attribution": GEONAMES_ATTRIBUTION}))
}
}
@@ -263,10 +350,105 @@ mod tests {
assert_eq!(st["downloaded"], false);
}
#[test]
fn the_gazetteer_is_cached_until_its_file_changes() {
let dir = dataset("cache");
let mut names = PlaceNames::default();
let a = names.load(&dir).unwrap();
assert!(Arc::ptr_eq(&a, &names.load(&dir).unwrap()), "unchanged file: no re-read");
// a different size invalidates it (a new download)
std::fs::write(dir.join(GEONAMES_FILES[0]), "1\tRome\tRome\t\t41.9\t12.5\tP\tP\tIT\t\t07\n").unwrap();
let b = names.load(&dir).unwrap();
assert!(!Arc::ptr_eq(&a, &b));
assert_eq!(b.reverse(LatLon::new(41.9, 12.5)).unwrap().city, "Rome");
std::fs::remove_file(dir.join(GEONAMES_FILES[0])).unwrap();
assert!(names.load(&dir).is_err(), "a deleted file is not served from the cache");
let _ = std::fs::remove_dir_all(&dir);
}
/// A local HTTP server; the first path segment picks the behaviour: `good` (the synthetic dump), `garbage` (a zip
/// that isn't one), `short` (the body ends before its Content-Length), anything else 404.
#[cfg(not(target_arch = "wasm32"))]
fn serve() -> String {
use std::io::{BufRead, BufReader, Write};
let l = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
let port = l.local_addr().unwrap().port();
let zip = lightcraft_geocode::unzip::make(GEONAMES_FILES[0], CITIES.as_bytes());
std::thread::spawn(move || {
for conn in l.incoming() {
let Ok(mut c) = conn else { continue };
let mut r = BufReader::new(c.try_clone().unwrap());
let mut line = String::new();
let _ = r.read_line(&mut line);
loop {
let mut h = String::new();
if r.read_line(&mut h).unwrap_or(0) == 0 || h.trim().is_empty() {
break;
}
}
let path = line.split(' ').nth(1).unwrap_or("").to_string();
let (status, body, len): (&str, Vec<u8>, Option<usize>) = match (path.split('/').nth(1).unwrap_or(""), path.ends_with(".zip")) {
("good", true) => ("200 OK", zip.clone(), None),
("good", false) if path.contains("countryInfo") => ("200 OK", b"FR\tFRA\t250\tFR\tFrance\n".to_vec(), None),
("garbage", true) => ("200 OK", b"<html>not a zip</html>".to_vec(), None),
("short", true) => ("200 OK", zip[..zip.len() / 2].to_vec(), Some(zip.len())),
_ => ("404 Not Found", b"nope".to_vec(), None),
};
let n = len.unwrap_or(body.len());
let _ = c.write_all(format!("HTTP/1.1 {status}\r\nContent-Length: {n}\r\nConnection: close\r\n\r\n").as_bytes());
let _ = c.write_all(&body);
}
});
format!("http://127.0.0.1:{port}")
}
#[cfg(not(target_arch = "wasm32"))]
fn download(s: &mut Session, base: &str, dir: &Path) -> DownloadOutcome {
s.execute("metadata.downloadPlaceNames", &json!({"dataset": dir, "base": base})).unwrap();
assert!(s.place_names.downloading() || s.place_names.lock().unreported.is_some());
let started = std::time::Instant::now();
loop {
if let Some(o) = s.place_names.take_outcome() {
// the activity row ends with the thread
while !s.activity.list().is_empty() && started.elapsed().as_secs() < 30 {
std::thread::sleep(std::time::Duration::from_millis(10));
}
assert!(s.activity.list().is_empty());
return o;
}
assert!(started.elapsed().as_secs() < 60, "the download never ended");
std::thread::sleep(std::time::Duration::from_millis(10));
}
}
#[cfg(not(target_arch = "wasm32"))]
#[test]
fn download_refuses_plain_http() {
fn download_end_to_end_and_its_failures() {
let mut s = Session::with_demo();
assert!(s.execute("metadata.downloadPlaceNames", &json!({"dataset": "/nonexistent", "base": "http://example.com/"})).is_err());
let ids = s.visible_cloned();
let base = serve();
let dir = std::env::temp_dir().join(format!("lightcraft-geocode-dl-{}", std::process::id()));
let _ = std::fs::remove_dir_all(&dir);
assert!(s.execute("metadata.downloadPlaceNames", &json!({"dataset": dir, "base": "http://example.com/"})).is_err());
for bad in ["missing", "garbage", "short"] {
let o = download(&mut s, &format!("{base}/{bad}/"), &dir);
let DownloadOutcome::Failed(e) = o else { panic!("{bad}: {o:?}") };
assert!(e.contains("try again"), "{bad}: {e}");
let st = s.execute("metadata.placeNamesStatus", &json!({"dataset": dir})).unwrap();
assert_eq!((st["downloaded"].as_bool(), st["lastDownload"].as_str()), (Some(false), Some("failed")), "{bad}: {st}");
assert_eq!(st["lastError"].as_str(), Some(e.as_str()));
assert!(s.place_names.take_outcome().is_none(), "reported once");
}
assert_eq!(download(&mut s, &format!("{base}/good/"), &dir), DownloadOutcome::Done(2));
let st = s.execute("metadata.placeNamesStatus", &json!({"dataset": dir})).unwrap();
assert_eq!((st["downloaded"].as_bool(), st["lastError"].is_null(), st["lastDownload"]["places"].as_u64()), (Some(true), true, Some(2)));
set_gps(&mut s, ids[0], Some((48.86, 2.34)), "");
s.execute("metadata.reverseGeocode", &json!({"ids": [ids[0].0], "dataset": dir})).unwrap();
let m = s.catalog.photo(ids[0]).unwrap().meta.clone();
// admin1 was a 404 (optional): the state stays blank, the rest is filled
assert_eq!((m.city.as_str(), m.state.as_str(), m.country.as_str()), ("Paris", "", "France"));
let _ = std::fs::remove_dir_all(&dir);
}
}
+1 -1
View File
@@ -19,7 +19,7 @@ pub(crate) mod face_detect;
mod face_models;
mod face_recognize;
pub mod filters;
mod geocode;
pub mod geocode;
pub mod keywords;
pub mod library;
pub mod lut_profiles;
+3
View File
@@ -184,6 +184,8 @@ pub struct Session {
pub face_models_dir: Option<std::path::PathBuf>,
/// Face model downloads started this session (the staged files wait in `<face_models_dir>/.downloads`).
pub face_downloads: face_download::Downloads,
/// Reverse geocoding: the parsed place names and their download (`metadata.*PlaceNames`).
pub place_names: cmd::geocode::PlaceNames,
/// The user's own list of models to download (`catalog.json` in the models folder), as last read.
pub(crate) face_catalog: lightcraft_faces::catalog::Catalog,
/// The loaded recognition model and the face embeddings made with it.
@@ -310,6 +312,7 @@ impl Session {
denoise: Default::default(),
face_models_dir: None,
face_downloads: Default::default(),
place_names: Default::default(),
face_catalog: Default::default(),
#[cfg(not(target_arch = "wasm32"))]
faces: Default::default(),
+3 -2
View File
@@ -68,8 +68,9 @@ pub fn extract(zip: &[u8], name: &str, max: usize) -> Result<Vec<u8>, GeocodeErr
Err(GeocodeError::Invalid(format!("{name} is not in the archive")))
}
#[cfg(test)]
pub(crate) fn make(name: &str, data: &[u8]) -> Vec<u8> {
/// A one-file deflated ZIP (test fixtures: the engine's download test serves one).
#[doc(hidden)]
pub fn make(name: &str, data: &[u8]) -> Vec<u8> {
let c = miniz_oxide::deflate::compress_to_vec(data, 6);
let mut z = Vec::new();
z.extend_from_slice(b"PK\x03\x04");
+3 -1
View File
@@ -2046,5 +2046,7 @@
"Fill Location": "Ort ausfüllen",
"Fill empty City, State and Country from GPS (offline GeoNames place names)": "Leere Felder Stadt, Bundesland und Land aus GPS ausfüllen (GeoNames-Ortsnamen, offline)",
"Looking up places": "Orte werden nachgeschlagen",
"Downloading place names": "Ortsnamen werden heruntergeladen"
"Downloading place names": "Ortsnamen werden heruntergeladen",
"Place names downloaded": "Ortsnamen heruntergeladen",
"Place names download cancelled": "Download der Ortsnamen abgebrochen"
}
+3 -1
View File
@@ -1891,5 +1891,7 @@
"Fill Location": "Rellenar ubicación",
"Fill empty City, State and Country from GPS (offline GeoNames place names)": "Rellenar Ciudad, Estado y País vacíos desde el GPS (nombres de lugares de GeoNames, sin conexión)",
"Looking up places": "Buscando lugares",
"Downloading place names": "Descargando nombres de lugares"
"Downloading place names": "Descargando nombres de lugares",
"Place names downloaded": "Nombres de lugares descargados",
"Place names download cancelled": "Descarga de nombres de lugares cancelada"
}
+3 -1
View File
@@ -1891,5 +1891,7 @@
"Fill Location": "Remplir le lieu",
"Fill empty City, State and Country from GPS (offline GeoNames place names)": "Remplir Ville, État et Pays vides depuis le GPS (noms de lieux GeoNames, hors ligne)",
"Looking up places": "Recherche des lieux",
"Downloading place names": "Téléchargement des noms de lieux"
"Downloading place names": "Téléchargement des noms de lieux",
"Place names downloaded": "Noms de lieux téléchargés",
"Place names download cancelled": "Téléchargement des noms de lieux annulé"
}
+3 -1
View File
@@ -1893,5 +1893,7 @@
"Fill Location": "場所を入力",
"Fill empty City, State and Country from GPS (offline GeoNames place names)": "空の市区町村・都道府県・国を GPS から入力 (GeoNames の地名、オフライン)",
"Looking up places": "場所を検索中",
"Downloading place names": "地名データをダウンロード中"
"Downloading place names": "地名データをダウンロード中",
"Place names downloaded": "地名データをダウンロードしました",
"Place names download cancelled": "地名データのダウンロードを中止しました"
}
+3 -1
View File
@@ -1891,5 +1891,7 @@
"Fill Location": "Preencher local",
"Fill empty City, State and Country from GPS (offline GeoNames place names)": "Preencher Cidade, Estado e País vazios pelo GPS (nomes de lugares GeoNames, offline)",
"Looking up places": "Procurando lugares",
"Downloading place names": "Baixando nomes de lugares"
"Downloading place names": "Baixando nomes de lugares",
"Place names downloaded": "Nomes de lugares baixados",
"Place names download cancelled": "Download dos nomes de lugares cancelado"
}
+3 -1
View File
@@ -1605,5 +1605,7 @@
"Fill Location": "Заполнить место",
"Fill empty City, State and Country from GPS (offline GeoNames place names)": "Заполнить пустые поля «Город», «Регион» и «Страна» по GPS (названия мест GeoNames, офлайн)",
"Looking up places": "Поиск мест",
"Downloading place names": "Загрузка названий мест"
"Downloading place names": "Загрузка названий мест",
"Place names downloaded": "Названия мест загружены",
"Place names download cancelled": "Загрузка названий мест отменена"
}
+3 -1
View File
@@ -2085,5 +2085,7 @@
"Fill Location": "Заповнити місце",
"Fill empty City, State and Country from GPS (offline GeoNames place names)": "Заповнити порожні поля «Місто», «Регіон» і «Країна» за GPS (назви місць GeoNames, офлайн)",
"Looking up places": "Пошук місць",
"Downloading place names": "Завантаження назв місць"
"Downloading place names": "Завантаження назв місць",
"Place names downloaded": "Назви місць завантажено",
"Place names download cancelled": "Завантаження назв місць скасовано"
}
+3 -1
View File
@@ -1891,5 +1891,7 @@
"Fill Location": "填写位置",
"Fill empty City, State and Country from GPS (offline GeoNames place names)": "根据 GPS 填写空白的城市、省/州和国家(GeoNames 地名,离线)",
"Looking up places": "正在查找地点",
"Downloading place names": "正在下载地名数据"
"Downloading place names": "正在下载地名数据",
"Place names downloaded": "地名数据已下载",
"Place names download cancelled": "已取消下载地名数据"
}
+3 -1
View File
@@ -1891,5 +1891,7 @@
"Fill Location": "填寫位置",
"Fill empty City, State and Country from GPS (offline GeoNames place names)": "根據 GPS 填寫空白的城市、州/省和國家(GeoNames 地名,離線)",
"Looking up places": "正在查找地點",
"Downloading place names": "正在下載地名資料"
"Downloading place names": "正在下載地名資料",
"Place names downloaded": "地名資料已下載",
"Place names download cancelled": "已取消下載地名資料"
}
+13
View File
@@ -547,6 +547,18 @@ impl LightcraftApp {
self.ui.toast = Some((text.into(), t + secs, None));
}
/// Reverse geocoding: say how a place-names download ended (a failure stays readable, with what to do).
fn place_names_outcome(&mut self, ctx: &egui::Context) {
use lightcraft_engine::cmd::geocode::DownloadOutcome;
match self.session.place_names.take_outcome() {
Some(DownloadOutcome::Done(_)) => self.toast(ctx, crate::i18n::tr("Place names downloaded")),
Some(DownloadOutcome::Cancelled) => self.toast(ctx, crate::i18n::tr("Place names download cancelled")),
Some(DownloadOutcome::Failed(e)) => self.toast_error(ctx, e),
None if self.session.place_names.downloading() => ctx.request_repaint_after(std::time::Duration::from_millis(500)),
None => {}
}
}
/// AI masks: apply finished background requests (clicks, descriptions, detail passes) and
/// show their errors; start a zoomed-in detail pass once the clicking stops
/// (`ui.detail_due`); watch the model download. Never waits for the model.
@@ -1024,6 +1036,7 @@ impl LightcraftApp {
// panels set it again this frame while the pointer rests on a preset or profile
self.hover_preview = None;
self.ai_mask_detail(&ctx);
self.place_names_outcome(&ctx);
if self.ui.fullscreen {
// full-screen preview: the photo alone on black
egui::CentralPanel::default().frame(egui::Frame::NONE.fill(egui::Color32::BLACK)).show(ui, |ui| panels::detail::show(self, ui));
+7 -7
View File
@@ -668,14 +668,14 @@ fn info(app: &mut LightcraftApp, ui: &mut egui::Ui, id: PhotoId) {
app.toast(ui.ctx(), e);
}
}
if text_button(ui, "fillLocation", crate::i18n::tr("Fill Location"), false)
.on_hover_text(crate::i18n::tr("Fill empty City, State and Country from GPS (offline GeoNames place names)"))
.clicked()
&& let Err(e) = app.run("metadata.reverseGeocode", json!({}))
{
app.toast(ui.ctx(), e);
}
});
if text_button(ui, "fillLocation", crate::i18n::tr("Fill Location"), false)
.on_hover_text(crate::i18n::tr("Fill empty City, State and Country from GPS (offline GeoNames place names)"))
.clicked()
&& let Err(e) = app.run("metadata.reverseGeocode", json!({}))
{
app.toast_error(ui.ctx(), e);
}
}
if let Some(a) = p.analysis {
let mut line = crate::i18n::tr_format!("Focus {:.0}", a.sharpness);