Merge PR #1389

crates/engine/src/cmd/denoise.rs

@@ -303,9 +303,20 @@ fn inspect(s: &mut Session, p: &Value) -> Result<Value> {
     if !src.extension().is_some_and(|e| e.eq_ignore_ascii_case("onnx")) {
         return Err(bad(C, "choose an .onnx file with denoise-model.json beside it"));
     }
-    let bytes = read_capped(&src.with_file_name("denoise-model.json"), MAX_MANIFEST_BYTES)
-        .ok_or_else(|| bad(C, "denoise-model.json is missing or too large"))?;
-    let m = manifest::parse(&bytes).map_err(|e| bad(C, e.to_string()))?;
+    let sibling = src.with_file_name("denoise-model.json");
+    let Some(bytes) = read_capped(&sibling, MAX_MANIFEST_BYTES) else {
+        let why = if sibling.is_file() {
+            "denoise-model.json could not be read or is too large"
+        } else if src.with_file_name("model.json").is_file() {
+            // other apps ship their models with a model.json of their own (#1381)
+            "No denoise-model.json beside this .onnx file. The model.json there is another app's description, which LightCraft \
+             does not read: it needs its own denoise-model.json (see docs/denoise.md)"
+        } else {
+            "No denoise-model.json beside this .onnx file: LightCraft needs one that says how to use the model (see docs/denoise.md)"
+        };
+        return Err(bad(C, why));
+    };
+    let m = manifest::parse(&bytes).map_err(|e| bad(C, format!("denoise-model.json: {e} (see docs/denoise.md)")))?;
     let dir = models_dir(s, C)?;
     Ok(json!({"kind": "model", "domain": "denoise", "path": path, "fileName": src.file_name().map(|n| n.to_string_lossy()), "model": m,
         "sizeBytes": std::fs::metadata(src).ok().map(|m| m.len()), "alreadyInstalled": dir.join(&m.id).join("model.onnx").is_file()}))

crates/engine/src/tests_denoise.rs

@@ -277,6 +277,29 @@ fn a_manifest_that_does_not_validate_or_name_the_file_is_refused() {
     let _ = std::fs::remove_dir_all(&x.dir);
 }
 
+/// Another app's model folder (#1381): an `.onnx` with no manifest, then with that app's `model.json` beside it, then
+/// with the `model.json` renamed. Each refusal says what is wrong and where LightCraft's manifest is described.
+#[test]
+fn inspecting_another_apps_model_folder_says_what_is_missing() {
+    let dir = temp_dir("denoise-foreign");
+    let mut s = Session::new().with_fs();
+    s.set_denoise_models_dir(Some(dir.join("denoise-models")));
+    let src = dir.join("rawdenoise");
+    std::fs::create_dir_all(&src).unwrap();
+    let onnx = src.join("rawdenoise.onnx");
+    std::fs::write(&onnx, b"not an onnx file").unwrap();
+    let mut inspect = || s.execute("denoise.models.inspect", &json!({"path": onnx.to_string_lossy()})).unwrap_err().to_string();
+    let e = inspect();
+    assert!(e.contains("No denoise-model.json beside") && e.contains("docs/denoise.md") && !e.contains("model.json there"), "{e}");
+    std::fs::write(src.join("model.json"), json!({"name": "rawdenoise", "input": {"channels": 4}, "scale": 2}).to_string()).unwrap();
+    let e = inspect();
+    assert!(e.contains("No denoise-model.json beside") && e.contains("The model.json there") && e.contains("docs/denoise.md"), "{e}");
+    std::fs::rename(src.join("model.json"), src.join("denoise-model.json")).unwrap();
+    let e = inspect();
+    assert!(e.contains("not a denoise-model manifest") && e.contains("docs/denoise.md"), "{e}");
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
 #[test]
 fn a_photo_is_denoised_in_the_background_and_the_amount_mixes_it_in() {
     let mut x = setup("background", true);

crates/ui-egui/src/headless.rs

@@ -508,6 +508,45 @@ mod tests {
         let _ = std::fs::remove_dir_all(&dir);
     }
 
+    /// Install from file… with a model the engine rejects says why in the AI Denoise section (#1381), instead of
+    /// leaving the button looking dead.
+    #[test]
+    fn a_rejected_model_file_says_why_in_settings() {
+        use crate::state::Dialog;
+        let mut h = demo([1400.0, 900.0]);
+        let t = Duration::from_secs(10);
+        let dir = std::env::temp_dir().join(format!("lc-ui-denoise-rejected-{}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&dir);
+        std::fs::create_dir_all(&dir).unwrap();
+        h.app.session.set_denoise_models_dir(Some(dir.join("denoise-models")));
+        // without the runtime the button is disabled (the workspace build turns it on through the desktop app)
+        if h.app.session.execute("denoise.models.list", &json!({})).unwrap()["runtime"] != true {
+            let _ = std::fs::remove_dir_all(&dir);
+            return;
+        }
+        // another app's model folder: the .onnx with that app's model.json, and no denoise-model.json
+        let onnx = dir.join("rawdenoise.onnx");
+        std::fs::write(&onnx, b"not a model").unwrap();
+        std::fs::write(dir.join("model.json"), br#"{"name": "rawdenoise", "scale": 2}"#).unwrap();
+        let picked = onnx.to_string_lossy().into_owned();
+        h.app.services.pick_denoise_model = Some(Box::new(move || vec![picked.clone()]));
+        h.app.ui.dialog = Some(Dialog::Settings { tab: "denoise".into() });
+        h.settle(SETTLE);
+        let r = h.request("ui.clickWidget", json!({"id": "denoise:installFile"}), t);
+        assert_eq!(r["ok"], true, "{r}");
+        h.step();
+        h.step();
+        assert_eq!(h.app.ui.dialog, Some(Dialog::Settings { tab: "denoise".into() }));
+        let painted = h.painted_text();
+        assert!(
+            painted
+                .iter()
+                .any(|s| s.starts_with("No denoise-model.json beside this .onnx file. The model.json there") && s.ends_with("(see docs/denoise.md)")),
+            "{painted:?}"
+        );
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
     use super::*;
 
     /// The pretend host has a clipboard, as a desktop does: text the UI copies lands in it, and a

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

@@ -35,6 +35,8 @@ pub struct Ui {
     /// Models the user pressed Download for that have not been installed yet.
     dl_watch: Vec<String>,
     list: Option<(u64, f64, Value)>,
+    /// Why the last file picked with Install from file… was refused, shown under the button.
+    rejected: Option<String>,
 }
 
 /// Called every frame: keeps the engine's denoise work going and asks for a redraw when a photo's picture appears (the
@@ -247,9 +249,17 @@ pub fn settings_tab(app: &mut LightcraftApp, ui: &mut egui::Ui, t: &Tokens) {
     register(ui.ctx(), "denoise:installFile", r.rect);
     if r.clicked()
         && let Some(path) = app.services.pick_denoise_model.as_mut().and_then(|f| f().into_iter().next())
-        && let Ok(info) = app.run("denoise.models.inspect", json!({"path": path}))
     {
-        app.ui.dialog = Some(crate::state::Dialog::DenoiseModel { info, accepted: false });
+        match app.session.execute("denoise.models.inspect", &json!({"path": path})) {
+            Ok(info) => {
+                app.caches.denoise.rejected = None;
+                app.ui.dialog = Some(crate::state::Dialog::DenoiseModel { info, accepted: false });
+            }
+            Err(e) => app.caches.denoise.rejected = Some(plain(&e)),
+        }
+    }
+    if let Some(why) = &app.caches.denoise.rejected {
+        hint(ui, t, why);
     }
     if all.is_empty() {
         hint(ui, t, "Install your own ONNX model with denoise-model.json beside it. This build offers no model downloads.");
@@ -550,6 +560,14 @@ pub fn install(app: &mut LightcraftApp, info: &Value, accepted: bool) -> Result<
     r
 }
 
+/// What an engine error says, without the command it came from (the install job's errors read the same way).
+fn plain(e: &lightcraft_engine::EngineError) -> String {
+    match e {
+        lightcraft_engine::EngineError::BadParams { msg, .. } => msg.clone(),
+        other => other.to_string(),
+    }
+}
+
 /// "1 photo" / "3 photos": the count with the right noun.
 fn count(n: u64, one: &str, many: &str) -> String {
     tr_format!("{n} {noun}", n = n, noun = tr(if n == 1 { one } else { many }))