diff --git a/crates/ui-egui/src/command_error.rs b/crates/ui-egui/src/command_error.rs new file mode 100644 index 000000000..66c6bbb51 --- /dev/null +++ b/crates/ui-egui/src/command_error.rs @@ -0,0 +1,42 @@ +//! A refused or failed command as the UI handles it (#1046): the message for people, and what kind of +//! error it is for code that decides on it. Decide on [`CommandError::kind`], never on the message: +//! messages get reworded and will be translated. + +use lightcraft_engine::{EngineError, ErrorKind}; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct CommandError { + pub message: String, + pub kind: ErrorKind, +} + +impl std::fmt::Display for CommandError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.message) + } +} + +/// An error the UI raised itself (its own commands, dialogs): [`ErrorKind::Other`]. +impl From for CommandError { + fn from(message: String) -> Self { + Self { message, kind: ErrorKind::Other } + } +} + +impl From<&str> for CommandError { + fn from(message: &str) -> Self { + message.to_string().into() + } +} + +impl From for CommandError { + fn from(e: EngineError) -> Self { + Self { kind: e.kind(), message: e.to_string() } + } +} + +impl From for String { + fn from(e: CommandError) -> Self { + e.message + } +} diff --git a/crates/ui-egui/src/export_task.rs b/crates/ui-egui/src/export_task.rs index 051ad22bd..97ed3b78d 100644 --- a/crates/ui-egui/src/export_task.rs +++ b/crates/ui-egui/src/export_task.rs @@ -138,7 +138,7 @@ pub fn apple_photos_note(outcome: &Value) -> String { return crate::i18n::tr_format!("not added to Apple Photos: {e}", e = e); } if let Some(why) = outcome.get("skipped").and_then(Value::as_str) { - if why.contains("cancelled") { + if outcome.get("cancelled").and_then(Value::as_bool) == Some(true) { return crate::i18n::tr("not added to Apple Photos (cancelled)").to_string(); } return crate::i18n::tr_format!("not added to Apple Photos: {e}", e = why); @@ -342,7 +342,14 @@ mod tests { &json!({"error": "LightCraft isn't allowed to control Photos. Allow it in System Settings › Privacy & Security › Automation"}), ); assert!(denied.starts_with("not added to Apple Photos: ") && denied.contains("Automation"), "{denied}"); - assert_eq!(apple_photos_note(&json!({"skipped": "the export was cancelled"})), "not added to Apple Photos (cancelled)"); + assert_eq!(apple_photos_note(&json!({"skipped": "the export was cancelled", "cancelled": true})), "not added to Apple Photos (cancelled)"); assert_eq!(apple_photos_note(&json!({"skipped": "the export wrote no files"})), "not added to Apple Photos: the export wrote no files"); + // decided on the `cancelled` flag, not the words (#1046): reworded it is still a cancel, and a + // reason that only mentions the word is shown as it is + assert_eq!(apple_photos_note(&json!({"skipped": "Export abgebrochen", "cancelled": true})), "not added to Apple Photos (cancelled)"); + assert_eq!( + apple_photos_note(&json!({"skipped": "an earlier, cancelled request is still running"})), + "not added to Apple Photos: an earlier, cancelled request is still running" + ); } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 9a387c77c..f5553a18a 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -8,6 +8,7 @@ pub mod album_picker; pub mod brush_live; +mod command_error; pub mod control; pub mod credits; pub mod date_picker; @@ -64,6 +65,8 @@ mod tests_dialog_fit; #[cfg(test)] mod tests_dialogs; #[cfg(test)] +mod tests_error_kinds; +#[cfg(test)] mod tests_filmstrip; #[cfg(test)] mod tests_grading; @@ -124,9 +127,10 @@ mod tests_zoom_keys; use std::sync::mpsc::{Receiver, Sender}; -use lightcraft_engine::Session; +use lightcraft_engine::{AiModel, ErrorKind, Session}; use serde_json::Value; +pub use command_error::CommandError; pub use control::{ControlRequest, ControlResponse}; pub use state::UiState; @@ -487,14 +491,20 @@ impl LightcraftApp { /// Run a UI or engine command by id. The single entry point for every frontend path. pub fn run(&mut self, id: &str, params: Value) -> Result { + self.run_typed(id, params).map_err(|e| e.message) + } + + /// [`LightcraftApp::run`], keeping what kind of error a refusal is: for code that decides on it + /// (offer a model's download, treat a cancel as one) instead of reading the message. + pub fn run_typed(&mut self, id: &str, params: Value) -> Result { if let Some(result) = model_setup::intercept(self, id, ¶ms) { - return result; + return result.map_err(CommandError::from); } if let Some(r) = menus::run_ui_command(self, id, ¶ms) { if r.is_ok() { docking::reveal_command(self, id)?; } - return r; + return r.map_err(CommandError::from); } let params = if id == "crop.aspect" && params.get("aspect").and_then(Value::as_str) == Some("toggle") @@ -505,7 +515,7 @@ impl LightcraftApp { } else { params }; - let r = self.session.execute(id, ¶ms).map_err(|e| e.to_string()); + let r = self.session.execute(id, ¶ms).map_err(CommandError::from); if r.is_ok() && matches!(id, "crop.aspect" | "crop.rotateAspect") { self.ui.crop_default_aspect = if id == "crop.aspect" && params.get("aspect").and_then(Value::as_str) == Some("original") { Some(serde_json::json!("original")) @@ -553,9 +563,9 @@ impl LightcraftApp { match &r { Err(e) => { log::warn!("{id}: {e}"); - self.ui.status = e.clone(); + self.ui.status = e.message.clone(); // no detector yet: offer it (its terms first) instead of only saying so - if id == "faces.detect" && e.contains("Settings > Faces") { + if id == "faces.detect" && e.kind == ErrorKind::ModelMissing(AiModel::FaceDetector) { panels::faces::offer_detector(self); } } @@ -735,16 +745,11 @@ impl LightcraftApp { if self.ui.sam_downloading && !download.running { self.ui.sam_downloading = false; let open = matches!(self.ui.dialog, Some(state::Dialog::SamModel { .. })); - match (&download.error, open) { - (_, true) => {} - (Some(e), false) if e.contains("cancelled") => { - self.toast(ctx, crate::i18n::tr("SAM 3 download stopped: it resumes where it left off next time.")) - } - (Some(e), false) => self.toast_error(ctx, crate::i18n::tr_format!("The SAM 3 download failed: {e}", e = e)), - (None, false) if download.finished => { - self.toast_error(ctx, crate::i18n::tr("The SAM 3 model is installed: Object and Describe masks are ready.")) - } - (None, false) => {} + match sam_download_ended(&download) { + _ if open => {} + Some((text, true)) => self.toast_error(ctx, text), + Some((text, false)) => self.toast(ctx, text), + None => {} } } if let Some((due, mask)) = self.ui.detail_due { @@ -763,13 +768,14 @@ impl LightcraftApp { } /// An AI mask error: when the model isn't installed, the dialog that offers to download it - /// (`then`: the AI mask to start afterwards); otherwise a toast. - pub fn ai_error(&mut self, ctx: &egui::Context, e: impl Into, then: Option<(&str, &str)>) { + /// (`then`: the AI mask to start afterwards); otherwise a toast. Decided on the error's kind + /// ([`ErrorKind::ModelMissing`]), not its words. + pub fn ai_error(&mut self, ctx: &egui::Context, e: impl Into, then: Option<(&str, &str)>) { let e = e.into(); - if lightcraft_engine::segment::Segmenter::AVAILABLE && e.starts_with(lightcraft_engine::segment::NOT_INSTALLED) { + if lightcraft_engine::segment::Segmenter::AVAILABLE && e.kind == ErrorKind::ModelMissing(AiModel::Sam3) { self.offer_sam_download(then); } else { - self.toast_error(ctx, e); + self.toast_error(ctx, e.message); } } @@ -1377,6 +1383,19 @@ fn detect_summary(v: &Value) -> String { } } +/// What to say when a SAM 3 download ended while its dialog was closed: the text, and whether it is +/// shown as an error toast. A cancel is told by `cancelled`, not by the error's words. +fn sam_download_ended(download: &lightcraft_engine::segment::DownloadStatus) -> Option<(String, bool)> { + match &download.error { + Some(_) if download.cancelled => { + Some((crate::i18n::tr("SAM 3 download stopped: it resumes where it left off next time.").to_string(), false)) + } + Some(e) => Some((crate::i18n::tr_format!("The SAM 3 download failed: {e}", e = e), true)), + None if download.finished => Some((crate::i18n::tr("The SAM 3 model is installed: Object and Describe masks are ready.").to_string(), true)), + None => None, + } +} + /// A dropped face model file (`.onnx`). pub fn is_model_file(path: &str) -> bool { std::path::Path::new(path).extension().is_some_and(|e| e.eq_ignore_ascii_case("onnx")) diff --git a/crates/ui-egui/src/lightroom_import.rs b/crates/ui-egui/src/lightroom_import.rs index 815e98576..0d4a616ee 100644 --- a/crates/ui-egui/src/lightroom_import.rs +++ b/crates/ui-egui/src/lightroom_import.rs @@ -352,17 +352,19 @@ pub fn tick(app: &mut LightcraftApp, ctx: &egui::Context) { return; } }; + // The task's cancel flag says whether it was cancelled (the worker only stops for it), not an + // error's words: a real failure that mentions "cancelled" is still reported as a failure. match message { Message::Inspected(result) => match result { Ok(report) if !task.cancel.load(Ordering::Relaxed) => terminal(app, ctx, &task, report), Ok(_) => terminal(app, ctx, &task, json!({"cancelled": true})), - Err(e) if task.cancel.load(Ordering::Relaxed) || e.contains("cancelled") => terminal(app, ctx, &task, json!({"cancelled": true})), + Err(_) if task.cancel.load(Ordering::Relaxed) => terminal(app, ctx, &task, json!({"cancelled": true})), Err(e) => terminal(app, ctx, &task, json!({"error": e})), }, Message::Prepared(result) => { let prepared = match result { Ok(p) => p, - Err(e) if task.cancel.load(Ordering::Relaxed) || e.contains("cancelled") => { + Err(_) if task.cancel.load(Ordering::Relaxed) => { terminal(app, ctx, &task, json!({"cancelled": true})); return; } @@ -518,6 +520,30 @@ mod tests { assert!(h.app.session.activity.list().is_empty()); } + /// Whether the task was cancelled is the task's flag, not an error's words (#1046): a cancel + /// reads as one whatever the message says, and a failure that mentions "cancelled" is a failure. + #[test] + fn lightroom_cancel_is_the_flag_not_the_words() { + let mut h = demo(); + let (task, tx, cancel, _, _) = fake_import(&h); + h.app.lightroom = Some(task); + cancel.store(true, Ordering::Relaxed); + tx.send(Message::Prepared(Err("Lightroom-Import abgebrochen".into()))).unwrap(); + h.step(); + assert_eq!(h.app.lightroom_last, Some(serde_json::json!({"cancelled": true, "path": "fixture.lrcat"}))); + + for message in + [Message::Prepared(Err("the drive was ejected: copy cancelled by the system".into())), Message::Inspected(Err("cancelled".into()))] + { + let (task, tx, _, _, _) = fake_import(&h); + h.app.lightroom = Some(task); + tx.send(message).unwrap(); + h.step(); + let last = h.app.lightroom_last.clone().unwrap_or_default(); + assert!(last.get("cancelled").is_none() && last["error"].as_str().is_some_and(|e| e.contains("cancelled")), "{last}"); + } + } + #[test] fn inspect_report_keeps_empty_catalog_shape() { let report = inspect_report(CatalogImport { diff --git a/crates/ui-egui/src/panels/detail.rs b/crates/ui-egui/src/panels/detail.rs index 234476167..a7e1cd0e0 100644 --- a/crates/ui-egui/src/panels/detail.rs +++ b/crates/ui-egui/src/panels/detail.rs @@ -1808,7 +1808,7 @@ fn mask_overlay(app: &mut LightcraftApp, ui: &mut egui::Ui, resp: &egui::Respons { let n = map.norm(q); let exclude = ui.input(|i| i.modifiers.alt); - match app.run("mask.objectPoint", json!({"x": n.x, "y": n.y, "exclude": exclude})) { + match app.run_typed("mask.objectPoint", json!({"x": n.x, "y": n.y, "exclude": exclude})) { Ok(_) => { if let Some(mid) = app.session.active_mask { app.ui.detail_due = Some((ui.input(|i| i.time) + 1.0, mid)); diff --git a/crates/ui-egui/src/panels/masking.rs b/crates/ui-egui/src/panels/masking.rs index 640bc4f11..59e178d77 100644 --- a/crates/ui-egui/src/panels/masking.rs +++ b/crates/ui-egui/src/panels/masking.rs @@ -685,8 +685,11 @@ pub(crate) fn start_object(app: &mut LightcraftApp, ctx: &egui::Context, op: &st if needs_model(app, "object", op) { return; } - let r = - if op == "new" { app.run("mask.add", json!({"kind": "object"})) } else { app.run("mask.addComponent", json!({"op": op, "kind": "object"})) }; + let r = if op == "new" { + app.run_typed("mask.add", json!({"kind": "object"})) + } else { + app.run_typed("mask.addComponent", json!({"op": op, "kind": "object"})) + }; match r { Ok(_) => { app.ui.tool = "object".into(); @@ -764,9 +767,9 @@ fn describe_field(app: &mut LightcraftApp, ui: &mut egui::Ui, new_mask: bool) { app.ui.describe = Some((op.clone(), text.clone())); if submit && !text.trim().is_empty() { let r = if op == "new" { - app.run("mask.add", json!({"kind": "prompt", "text": text})) + app.run_typed("mask.add", json!({"kind": "prompt", "text": text})) } else { - app.run("mask.addComponent", json!({"op": op, "kind": "prompt", "text": text})) + app.run_typed("mask.addComponent", json!({"op": op, "kind": "prompt", "text": text})) }; match r { // the mask appears when the model has found it (a detail pass follows) diff --git a/crates/ui-egui/src/tests_error_kinds.rs b/crates/ui-egui/src/tests_error_kinds.rs new file mode 100644 index 000000000..0d99edbf4 --- /dev/null +++ b/crates/ui-egui/src/tests_error_kinds.rs @@ -0,0 +1,70 @@ +//! The UI decides on what kind of error a command returned, not on its words (#1046): messages get +//! reworded and translated, and a decision that reads them breaks silently when they do. + +use lightcraft_engine::segment::{DownloadStatus, NOT_INSTALLED, Segmenter}; +use lightcraft_engine::{AiModel, ErrorKind}; +use serde_json::json; + +use crate::headless::Headless; +use crate::state::Dialog; +use crate::{CommandError, LightcraftApp, Services}; + +fn demo() -> Headless { + let app = LightcraftApp::new(lightcraft_engine::Session::with_demo(), Services { png: None, ..Default::default() }); + Headless::new(app, [1200.0, 800.0], 1.0) +} + +/// An AI mask error offers the model on `ModelMissing(Sam3)`, whatever the message says, and a +/// message that merely starts like the engine's is a toast. +#[test] +fn ai_mask_errors_offer_the_model_by_kind() { + let mut h = demo(); + let ctx = h.view.ctx.clone(); + let missing = CommandError { message: "Das SAM-3-Modell ist nicht installiert".into(), kind: ErrorKind::ModelMissing(AiModel::Sam3) }; + h.app.ai_error(&ctx, missing, Some(("object", "new"))); + if Segmenter::AVAILABLE { + assert_eq!(h.app.ui.dialog, Some(Dialog::SamModel { then: Some(("object".into(), "new".into())), error: None })); + } else { + assert_eq!(h.app.ui.dialog, None, "a build without AI masks has nothing to offer"); + } + h.app.ui.dialog = None; + h.app.ai_error(&ctx, format!("{NOT_INSTALLED}: but this is some other failure"), None); + assert_eq!(h.app.ui.dialog, None, "decided on the kind, not the words"); + // another model missing is not SAM 3's + let face = CommandError { message: "x".into(), kind: ErrorKind::ModelMissing(AiModel::FaceDetector) }; + h.app.ai_error(&ctx, face, None); + assert_eq!(h.app.ui.dialog, None); +} + +/// `run_typed` keeps the engine's kind for the code that decides, and `run` the same message. +#[test] +fn run_typed_keeps_the_kind_and_run_the_message() { + let mut h = demo(); + let dir = std::env::temp_dir().join(format!("lc-ui-error-kinds-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + h.app.session.face_models_dir = Some(dir.join("models")); + let typed = h.app.run_typed("faces.detect", json!({})).unwrap_err(); + assert_eq!(typed.kind, ErrorKind::ModelMissing(AiModel::FaceDetector), "{typed}"); + h.app.ui.dialog = None; + let text = h.app.run("faces.detect", json!({})).unwrap_err(); + assert_eq!(text, typed.message); + let unknown = h.app.run_typed("no.such.command", json!({})).unwrap_err(); + assert_eq!(unknown.kind, ErrorKind::UnknownCommand, "{unknown}"); + // an error the UI raised itself is `Other` + assert_eq!(CommandError::from("Tick the box to accept the model's terms first".to_string()).kind, ErrorKind::Other); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A SAM 3 download that ended while its dialog was closed: a cancel is told by the status's +/// `cancelled`, not by the error's words. +#[test] +fn a_cancelled_sam_download_is_told_by_its_flag() { + let ended = |error: &str, cancelled: bool| { + crate::sam_download_ended(&DownloadStatus { error: Some(error.into()), cancelled, ..DownloadStatus::default() }).unwrap() + }; + let (text, error) = ended("Download abgebrochen", true); + assert!(!error && text.starts_with("SAM 3 download stopped"), "{text}"); + let (text, error) = ended("could not download model.safetensors: the mirror cancelled the transfer", false); + assert!(error && text.starts_with("The SAM 3 download failed: "), "{text}"); + assert_eq!(crate::sam_download_ended(&DownloadStatus::default()), None); +}