From 312b4c056e9b3851f2573a8bbda6dee71fd85a3f Mon Sep 17 00:00:00 2001 From: lloyd Date: Sun, 11 Oct 2026 23:22:58 +0200 Subject: [PATCH] UI: password dialog fixes from review (#11) OK on the untouched field says the password is incorrect instead of asking again; the Backstage closes once the workbook opens; app.open on the control channel takes a password and never opens the dialog. Control protocol docs cover file.open {password}. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/control.rs | 17 ++++++--- crates/ui-egui/src/dialogs.rs | 6 ++- crates/ui-egui/src/lib.rs | 6 ++- crates/ui-egui/tests/open_encrypted.rs | 52 ++++++++++++++++++++++++++ docs/control-protocol.md | 9 ++++- 5 files changed, 81 insertions(+), 9 deletions(-) diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 3ec3f72..6f6280e 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -14,7 +14,7 @@ //! - `ui.edit.begin {text?}`, `ui.edit.commit {move?}`, `ui.edit.cancel` //! - `ui.screenshot {path?}` — PNG of the window (base64 when no path); animations are settled //! - `ui.resize {width, height}`, `ui.set {dark?, formulaBar?, ribbonCollapsed?, animations?}` -//! - `app.open {path}`, `app.save {path?}`, `app.quit` +//! - `app.open {path, password?}`, `app.save {path?}`, `app.quit` use std::sync::mpsc::Sender; @@ -351,10 +351,17 @@ pub fn handle(app: &mut SheetApp, ctx: &egui::Context, method: &str, p: &Json) - "ui.screenshot" => return Outcome::Screenshot { path: s("path").map(str::to_string) }, "app.open" => { let Some(path) = s("path") else { return err("missing `path`") }; - app.open_path(path); - match app.message.take() { - Some((_, m)) => err(m), - None => ok(json!({"title": app.session.active().map(|d| d.display_title())})), + // Not `open_path`: that asks for a missing password in a dialog, and programmatic + // calls never open one. A password-protected workbook takes `password` instead. + let r = app.run("file.open", json!({"path": path, "password": s("password")})); + if let Ok(r) = &r { + app.after_open(Some(path), r); + } + app.after_engine(); + match (r, app.message.take()) { + (Err(e), _) => err(crate::clean_error(&e)), + (Ok(_), Some((_, m))) => err(m), + (Ok(_), None) => ok(json!({"title": app.session.active().map(|d| d.display_title())})), } } "app.save" => wrap(app.run("file.save", json!({"path": s("path")}))), diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index de04998..5e859c6 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -241,12 +241,16 @@ impl Dialog { // on the web) and goes back to `file.open` with the password. let source = p("path").as_str().or(p("name").as_str()).map(str::to_string).unwrap_or_default(); let file = std::path::Path::new(&source).file_name().and_then(|n| n.to_str()).unwrap_or("This workbook").to_string(); + // An empty password from the start, so OK on the untouched field says "incorrect" + // rather than asking for the password again. + let mut values = params.clone(); + values["password"] = json!(""); Dialog::form( "openPassword", "Password", "file.open", vec![Note(format!("'{file}' is protected.")), Password { key: "password", label: "Password:" }], - params.clone(), + values, ) } "unprotectSheet" => Dialog::form( diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 0ad05a6..60a5afd 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -632,9 +632,11 @@ impl SheetApp { self.after_engine(); } - /// What follows a successful `file.open`: the Recent list (for a file on disk) and the open - /// warnings notice. Shared by File › Open and the password dialog. + /// What follows a successful `file.open`: the workbook shows (not the Backstage), the Recent + /// list (for a file on disk) and the open warnings notice. Shared by File › Open, the + /// password dialog and the control channel. pub fn after_open(&mut self, path: Option<&str>, r: &Json) { + self.backstage = None; if let Some(path) = path { self.ui.recent.retain(|p| p != path); self.ui.recent.insert(0, path.to_string()); diff --git a/crates/ui-egui/tests/open_encrypted.rs b/crates/ui-egui/tests/open_encrypted.rs index 6baeac1..fabbd56 100644 --- a/crates/ui-egui/tests/open_encrypted.rs +++ b/crates/ui-egui/tests/open_encrypted.rs @@ -118,3 +118,55 @@ fn a_file_picked_on_the_web_asks_for_its_password_too() { assert_eq!(a1(&h), Value::Number(42.0)); assert_eq!(h.state().session.active().unwrap().path, None); } + +#[test] +fn an_untouched_password_field_is_incorrect_not_required() { + let path = encrypted_file("untouched"); + let mut h = harness(Services::default()); + h.state_mut().open_path(&path.to_string_lossy()); + h.run_steps(3); + h.get_by_label(" OK ").click(); + h.run_steps(3); + let msg = h.state().message.as_ref().map(|(_, m)| m.clone()).unwrap_or_default(); + assert!(msg.contains(gridcraft_engine::io::WRONG_PASSWORD), "{msg}"); + assert_eq!(dialog(&h).as_deref(), Some("openPassword")); + std::fs::remove_dir_all(path.parent().unwrap()).unwrap(); +} + +#[test] +fn the_backstage_closes_once_the_password_opens_the_workbook() { + let path = encrypted_file("backstage"); + let mut h = harness(Services::default()); + h.state_mut().backstage = Some(gridcraft_ui_egui::backstage::Backstage::new(gridcraft_ui_egui::backstage::Page::Open)); + h.state_mut().open_path(&path.to_string_lossy()); + h.run_steps(3); + h.state_mut().dialog.as_mut().unwrap().values.insert("password".into(), json!(PW)); + h.get_by_label(" OK ").click(); + h.run_steps(3); + assert_eq!(a1(&h), Value::Number(42.0)); + assert!(h.state().backstage.is_none(), "the opened workbook shows, not the Backstage"); + std::fs::remove_dir_all(path.parent().unwrap()).unwrap(); +} + +#[test] +fn app_open_over_the_control_channel_never_opens_a_dialog() { + use gridcraft_ui_egui::control::{Outcome, handle}; + let path = encrypted_file("control"); + let p = path.to_string_lossy().to_string(); + let mut h = harness(Services::default()); + let ctx = h.ctx.clone(); + let reply = |o: Outcome| match o { + Outcome::Done(v) => v, + Outcome::Screenshot { .. } => json!(null), + }; + let r = reply(handle(h.state_mut(), &ctx, "app.open", &json!({"path": p}))); + assert_eq!(r["ok"], json!(false), "{r}"); + assert!(r["error"].as_str().unwrap_or_default().contains(gridcraft_engine::io::PASSWORD_REQUIRED), "{r}"); + assert_eq!(dialog(&h), None, "programmatic calls never open dialogs"); + let r = reply(handle(h.state_mut(), &ctx, "app.open", &json!({"path": p, "password": PW}))); + assert_eq!(r["ok"], json!(true), "{r}"); + assert_eq!(r["result"]["title"], json!("Secret.xlsx")); + assert_eq!(a1(&h), Value::Number(42.0)); + assert_eq!(h.state().ui.recent.first(), Some(&p)); + std::fs::remove_dir_all(path.parent().unwrap()).unwrap(); +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index c142ebf..a16208a 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -36,7 +36,7 @@ Errors come back as `{"id": …, "ok": false, "error": "…"}`. | `ui.screenshot` | `{path?}` | PNG of the window (base64 when no path). Animations are settled first: the picture shows the final state | | `ui.set` | `{dark?, formulaBar?, ribbonCollapsed?, animations?}` | UI preferences (`formulaBar` runs `view.formulaBar`) | | `ui.resize` / `ui.focus` | `{width, height}` | Window control | -| `app.open` / `app.save` / `app.quit` | `{path}` / `{path?}` | Files and lifetime | +| `app.open` / `app.save` / `app.quit` | `{path, password?}` / `{path?}` | Files and lifetime. `password` opens a password-protected workbook; without it, such a workbook returns an error (never a dialog) | Any other method name that is a command id (e.g. `home.bold`) runs that command. @@ -56,5 +56,12 @@ the View ▸ Animations checkbox, saved with the other UI preferences and on by document text. The choice is saved in `ui.json`; with nothing saved the desktop app follows the system language. +`{command: "file.open", params: {path: "Secret.xlsx", password: "…"}}` opens a password-protected +workbook (Agile or Standard encryption). Without `password` it returns an error containing +"is password-protected. Enter its password to open it." and never opens a dialog; a wrong +password returns "The password is incorrect." The password is left out of the journal and never +quoted in an error. The opened workbook has no save path, so Save asks where to save it, without +the password, and never replaces the encrypted original. + For a headless equivalent (no window), use `gridcraft-cli mcp` or `gridcraft-cli run`; see [`mcp.md`](mcp.md) and [`cli.md`](cli.md).