diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -45,6 +45,8 @@ pub struct Dialog { /// The dialog this one was opened from (Name Manager › New…): it comes back when this closes. back: Option>, focus_initialized: bool, + /// Save/Don't Save answer on release, before quitting can move focus to another viewport. + save_changes_armed: Option, /// The formula box that takes references clicked or dragged on the grid (New Formatting Rule). pick: Option, } @@ -157,6 +159,7 @@ impl Dialog { picture_target: None, back: None, focus_initialized: false, + save_changes_armed: None, pick: None, } } @@ -178,6 +181,7 @@ impl Dialog { picture_target: None, back: None, focus_initialized: false, + save_changes_armed: None, pick: None, } } @@ -1455,15 +1459,21 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { "saveChanges" => { ui.label(format!("Do you want to save the changes you made to {}?", d.values.get("title").and_then(Json::as_str).unwrap_or("this workbook"))); let quitting = d.values.get("quit").and_then(Json::as_bool) == Some(true); + let mut answer = save_changes_shortcut(&mut d, ui.ctx()); ui.horizontal(|ui| { - for (label, answer) in + for (label, choice) in [("Don't Save", crate::SaveAnswer::DontSave), ("Cancel", crate::SaveAnswer::Cancel), ("Save", crate::SaveAnswer::Save)] { - if ui.button(label).clicked() && app.answer_save_changes(answer, quitting, ui.ctx()) { - open = false; + if ui.button(label).clicked() { + answer = Some(choice); } } }); + if let Some(answer) = answer + && app.answer_save_changes(answer, quitting, ui.ctx()) + { + open = false; + } } "start" => { ui.heading("Start a new workbook"); @@ -1573,6 +1583,42 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { } } +fn save_changes_shortcut(d: &mut Dialog, ctx: &egui::Context) -> Option { + let mut answer = None; + ctx.input_mut(|i| { + // Windows can synthesize key releases before its focus-loss event. Even if focus + // returns in this batch, those releases must not answer the question. + let focused = i.focused && !i.events.iter().any(|event| matches!(event, egui::Event::WindowFocused(false))); + if !focused { + d.save_changes_armed = None; + } + i.events.retain(|event| { + match event { + egui::Event::Key { key, pressed, repeat, modifiers, .. } => { + if matches!(key, Key::Y | Key::N) { + if *pressed { + if !repeat { + d.save_changes_armed = (focused && modifiers.is_none()).then_some(*key); + } + } else if d.save_changes_armed.take() == Some(*key) && focused && modifiers.is_none() { + answer.get_or_insert(if *key == Key::Y { crate::SaveAnswer::Save } else { crate::SaveAnswer::DontSave }); + } + } else if *key == Key::Escape && *pressed && !repeat && focused && modifiers.is_none() { + answer.get_or_insert(crate::SaveAnswer::Cancel); + } + // Consume the whole answer gesture, including repeats, before another + // workbook's question can be drawn in this frame. + return !modifiers.is_none() || !matches!(key, Key::Y | Key::N | Key::Escape); + } + egui::Event::Text(text) if matches!(text.as_str(), "y" | "Y" | "n" | "N") => return false, + _ => {} + } + true + }); + }); + answer +} + /// Paste Special as Excel lays it out: Paste and Operation each in two columns of options, Skip /// blanks and Transpose side by side, and Paste Link as a button that pastes at once. The options /// come from the dialog's `fields`, so the letter shortcuts and the control channel see the same. diff --git a/crates/ui-egui/tests/doc_windows.rs b/crates/ui-egui/tests/doc_windows.rs --- a/crates/ui-egui/tests/doc_windows.rs +++ b/crates/ui-egui/tests/doc_windows.rs @@ -16,6 +16,8 @@ use serde_json::json; thread_local! { /// Each viewport's input for the next frame (focus, close button). static INFOS: RefCell> = RefCell::new(egui::ViewportIdMap::default()); + /// Keyboard input belongs to one native viewport, as it does in eframe. + static KEYS: RefCell>> = RefCell::new(egui::ViewportIdMap::default()); /// The viewport commands the app sent this frame. static SENT: RefCell> = const { RefCell::new(Vec::new()) }; } @@ -39,12 +41,14 @@ struct Desk { impl Desk { fn new(quit_with_last_window: bool) -> Desk { INFOS.with(|i| i.borrow_mut().clear()); + KEYS.with(|i| i.borrow_mut().clear()); let ctx = Context::default(); ctx.set_embed_viewports(false); Context::set_immediate_viewport_renderer(|ctx, viewport| { let id = viewport.ids.this; let mut input = RawInput { viewport_id: id, ..RawInput::default() }; input.viewports = INFOS.with(|i| i.borrow().clone()); + input.events = KEYS.with(|i| i.borrow_mut().remove(&id).unwrap_or_default()); input.viewports.entry(id).or_default().parent = Some(viewport.ids.parent); let mut cb = viewport.viewport_ui_cb; record(ctx.run_ui(input, |ui| cb(ui))); @@ -64,6 +68,7 @@ impl Desk { SENT.with(|s| s.borrow_mut().clear()); let mut input = RawInput { viewport_id: ViewportId::ROOT, ..RawInput::default() }; input.viewports = INFOS.with(|i| i.borrow().clone()); + input.events = KEYS.with(|i| i.borrow_mut().remove(&ViewportId::ROOT).unwrap_or_default()); input.viewports.entry(ViewportId::ROOT).or_default(); let Desk { ctx, app, windows } = self; let mut act = Some(act); @@ -86,6 +91,20 @@ impl Desk { (0..n).flat_map(|_| self.frame_with(|_, _| {})).collect() } + fn answer_key(&mut self, window: u64, key: egui::Key, pressed: bool) -> Vec<(ViewportId, ViewportCommand)> { + let vp = self.viewport(window); + KEYS.with(|i| { + i.borrow_mut().entry(vp).or_default().push(egui::Event::Key { + key, + physical_key: None, + pressed, + repeat: false, + modifiers: egui::Modifiers::NONE, + }) + }); + self.frames(3) + } + fn viewport(&self, window: u64) -> ViewportId { self.windows.viewport_of(window).unwrap_or_else(|| panic!("window {window} has no native window")) } @@ -121,6 +140,74 @@ fn sent_to(sent: &[(ViewportId, ViewportCommand)], vp: ViewportId, cmd: &Viewpor sent.iter().any(|(id, c)| *id == vp && c == cmd) } +#[test] +fn held_n_discards_only_one_workbook_when_quit_moves_to_another_native_viewport() { + let mut desk = Desk::new(true); + // Keep the clean root window, so the two dirty workbooks have distinct child viewports. + for _ in 0..2 { + desk.app.run("file.new", json!({})).unwrap(); + desk.app.run("cell.set", json!({"cell": "A1", "input": "42"})).unwrap(); + } + desk.frames(3); + let (w2, w3) = (desk.window_of(1), desk.window_of(2)); + assert_ne!(desk.viewport(w2), desk.viewport(w3)); + desk.frame_with(|app, ctx| app.request_quit(ctx)); + desk.frames(3); + for _ in 0..3 { + desk.answer_key(w2, egui::Key::N, true); + assert_eq!(desk.app.session.documents().len(), 3, "holding N does not close a window"); + } + desk.answer_key(w2, egui::Key::N, false); + assert_eq!(desk.app.session.documents().len(), 2, "release discards the first dirty workbook"); + assert_eq!(desk.app.session.active_window(), Some(w3)); + assert_eq!(desk.app.dialog.as_ref().map(|d| d.name()), Some("saveChanges")); + desk.answer_key(w3, egui::Key::N, false); + assert_eq!(desk.app.session.documents().len(), 2, "a stray release cannot answer the next question"); + desk.answer_key(w3, egui::Key::N, true); + let sent = desk.answer_key(w3, egui::Key::N, false); + assert_eq!(desk.app.session.documents().len(), 1); + assert!(sent_to(&sent, ViewportId::ROOT, &ViewportCommand::Close)); +} + +#[test] +fn held_y_saves_only_one_workbook_when_quit_moves_to_another_native_viewport() { + let dir = std::env::temp_dir().join(format!("gridcraft-native-held-y-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let paths = [dir.join("First.xlsx"), dir.join("Second.xlsx")]; + let mut desk = Desk::new(true); + // Both dirty child windows have a file, so Y can save without a picker. + for path in &paths { + desk.app.run("file.new", json!({})).unwrap(); + desk.app.run("file.save", json!({"path": path})).unwrap(); + desk.app.run("cell.set", json!({"cell": "A1", "input": "42"})).unwrap(); + } + desk.frames(3); + let (w2, w3) = (desk.window_of(1), desk.window_of(2)); + assert_ne!(desk.viewport(w2), desk.viewport(w3)); + desk.frame_with(|app, ctx| app.request_quit(ctx)); + desk.frames(3); + for _ in 0..3 { + desk.answer_key(w2, egui::Key::Y, true); + assert_eq!(desk.app.session.documents().len(), 3, "holding Y must not advance to another window"); + } + desk.answer_key(w2, egui::Key::Y, false); + assert_eq!(desk.app.session.documents().len(), 2); + assert_eq!(desk.app.session.active_window(), Some(w3)); + assert!(desk.app.session.doc().unwrap().is_dirty(), "the second workbook has not been saved"); + let (saved, _) = gridcraft_engine::io::open_bytes("First.xlsx", &std::fs::read(&paths[0]).unwrap()).unwrap(); + assert_eq!(saved.active().unwrap().value(CellRef::new(0, 0)), Value::Number(42.0)); + let (untouched, _) = gridcraft_engine::io::open_bytes("Second.xlsx", &std::fs::read(&paths[1]).unwrap()).unwrap(); + assert_eq!(untouched.active().unwrap().value(CellRef::new(0, 0)), Value::Empty); + desk.answer_key(w3, egui::Key::Y, false); + assert_eq!(desk.app.session.documents().len(), 2, "a stray release does not answer the next question"); + desk.answer_key(w3, egui::Key::Y, true); + assert_eq!(desk.app.session.documents().len(), 2); + let sent = desk.answer_key(w3, egui::Key::Y, false); + assert_eq!(desk.app.session.documents().len(), 1); + assert!(sent_to(&sent, ViewportId::ROOT, &ViewportCommand::Close)); + std::fs::remove_dir_all(dir).unwrap(); +} + #[test] fn each_window_keeps_its_own_selection_and_draft() { let mut desk = Desk::new(true); diff --git a/crates/ui-egui/tests/it/file_menu.rs b/crates/ui-egui/tests/it/file_menu.rs --- a/crates/ui-egui/tests/it/file_menu.rs +++ b/crates/ui-egui/tests/it/file_menu.rs @@ -300,6 +300,198 @@ fn quitting_with_one_of_two_workbooks_unsaved_asks_once() { assert!(h.state().dialog.is_none()); } +#[test] +fn save_changes_shortcuts_save_before_closing() { + let dir = std::env::temp_dir().join(format!("gridcraft-close-y-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("Book.xlsx"); + let mut h = harness(Services::default()); + h.state_mut().run("file.save", serde_json::json!({"path": path.to_string_lossy()})).unwrap(); + h.state_mut().run("cell.set", serde_json::json!({"cell": "A1", "input": "42"})).unwrap(); + request_window_close(&mut h); + assert_eq!(h.state().dialog.as_ref().unwrap().values.get("quit"), Some(&serde_json::json!(true))); + h.key_press(egui::Key::Y); + let sent = step_commands(&mut h); + assert!(sent.contains(&egui::ViewportCommand::Close), "Y saves and continues quitting: {sent:?}"); + assert!(h.state().dialog.is_none()); + let (wb, _) = gridcraft_engine::io::open_bytes("Book.xlsx", &std::fs::read(&path).unwrap()).unwrap(); + assert_eq!(wb.active().unwrap().value(CellRef::new(0, 0)), Value::Number(42.0)); + std::fs::remove_dir_all(dir).unwrap(); +} + +#[test] +fn save_changes_shortcuts_cancel_or_discard_only_the_requested_workbook() { + let mut h = harness(Services::default()); + h.state_mut().run("cell.set", serde_json::json!({"cell": "A1", "input": "42"})).unwrap(); + h.state_mut().run("file.new", serde_json::json!({})).unwrap(); + h.state_mut().run("cell.set", serde_json::json!({"cell": "B2", "input": "7"})).unwrap(); + let kept = h.state().session.doc().unwrap().uid; + // File Close asks about this workbook without starting an application quit. + h.state_mut().run_or_alert("file.close", serde_json::json!({})); + h.run_steps(3); + h.key_press(egui::Key::Escape); + let sent = step_commands(&mut h); + assert!(!sent.contains(&egui::ViewportCommand::Close)); + assert!(h.state().dialog.is_none()); + assert_eq!(h.state().session.documents().len(), 2); + assert_eq!(h.state().session.doc().unwrap().uid, kept); + h.state_mut().run_or_alert("file.close", serde_json::json!({})); + h.run_steps(3); + h.key_press(egui::Key::N); + let sent = step_commands(&mut h); + assert!(!sent.contains(&egui::ViewportCommand::Close)); + assert_eq!(h.state().session.documents().len(), 1); + assert_ne!(h.state().session.doc().unwrap().uid, kept); + assert!(h.state().dialog.is_none()); +} + +#[test] +fn save_changes_shortcuts_ignore_modified_answers_and_held_n() { + let mut h = harness(Services::default()); + for _ in 0..3 { + h.state_mut().run("cell.set", serde_json::json!({"cell": "A1", "input": "42"})).unwrap(); + h.state_mut().run("file.new", serde_json::json!({})).unwrap(); + } + request_window_close(&mut h); + let count = h.state().session.documents().len(); + for modifiers in [egui::Modifiers::CTRL, egui::Modifiers::COMMAND, egui::Modifiers::ALT, egui::Modifiers::SHIFT, egui::Modifiers::MAC_CMD] { + for key in [egui::Key::Y, egui::Key::N] { + h.key_press_modifiers(modifiers, key); + step_commands(&mut h); + assert_eq!(h.state().dialog.as_ref().map(|d| d.name()), Some("saveChanges")); + assert_eq!(h.state().session.documents().len(), count); + assert!(h.state().session.doc().unwrap().is_dirty()); + } + } + // Don't discard until release: a quit can move its next question to a different native + // viewport, whose independent input state would treat a held N as a fresh press. + h.input_mut().events.push(egui::Event::Key { + key: egui::Key::N, + physical_key: None, + pressed: true, + repeat: false, + modifiers: egui::Modifiers::NONE, + }); + step_commands(&mut h); + assert_eq!(h.state().session.documents().len(), count); + let current = h.state().session.doc().unwrap().uid; + for _ in 0..3 { + h.input_mut().events.push(egui::Event::Key { + key: egui::Key::N, + physical_key: None, + pressed: true, + repeat: true, + modifiers: egui::Modifiers::NONE, + }); + let sent = step_commands(&mut h); + assert!(!sent.contains(&egui::ViewportCommand::Close)); + assert_eq!(h.state().session.documents().len(), count); + assert_eq!(h.state().session.doc().unwrap().uid, current); + assert_eq!(h.state().dialog.as_ref().map(|d| d.name()), Some("saveChanges")); + } + h.input_mut().events.push(egui::Event::Key { + key: egui::Key::N, + physical_key: None, + pressed: false, + repeat: false, + modifiers: egui::Modifiers::NONE, + }); + step_commands(&mut h); + assert_eq!(h.state().session.documents().len(), count - 1, "releasing N answers exactly one question"); + h.key_press(egui::Key::N); + step_commands(&mut h); + assert_eq!(h.state().session.documents().len(), count - 2, "a fresh press can answer the next question"); +} + +#[test] +fn save_changes_shortcuts_ignore_releases_from_focus_loss() { + let dir = std::env::temp_dir().join(format!("gridcraft-answer-focus-loss-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + for key in [egui::Key::N, egui::Key::Y] { + for regain_focus in [false, true] { + let mut h = harness(Services::default()); + let path = dir.join(format!("{key:?}-{regain_focus}.xlsx")); + h.state_mut().run("file.save", serde_json::json!({"path": path})).unwrap(); + h.state_mut().run("cell.set", serde_json::json!({"cell": "A1", "input": "42"})).unwrap(); + let uid = h.state().session.doc().unwrap().uid; + request_window_close(&mut h); + h.key_down(key); + h.run_steps(1); + // Windows synthesizes releases before reporting focus loss. Focus may also + // return before the next frame; neither batch represents a physical answer. + h.input_mut().focused = regain_focus; + h.input_mut().events.extend([ + egui::Event::Key { key, physical_key: None, pressed: false, repeat: false, modifiers: egui::Modifiers::NONE }, + egui::Event::WindowFocused(false), + ]); + if regain_focus { + h.input_mut().events.push(egui::Event::WindowFocused(true)); + } + let sent = step_commands(&mut h); + assert!(!sent.contains(&egui::ViewportCommand::Close), "{key:?} must not answer on focus loss"); + assert_eq!(h.state().session.doc().unwrap().uid, uid); + assert!(h.state().session.doc().unwrap().is_dirty()); + assert_eq!(h.state().dialog.as_ref().map(|d| d.name()), Some("saveChanges")); + h.input_mut().focused = true; + h.input_mut().events.push(egui::Event::WindowFocused(true)); + h.run_steps(1); + h.key_press(key); + let sent = step_commands(&mut h); + assert!(sent.contains(&egui::ViewportCommand::Close), "a fresh {key:?} gesture after focus returns still works"); + } + } + std::fs::remove_dir_all(dir).unwrap(); +} + +#[test] +fn save_changes_shortcuts_keep_unsaved_work_after_picker_cancel_or_save_failure() { + for fail in [false, true] { + let dir = std::env::temp_dir().join(format!("gridcraft-close-y-failure-{}.xlsx", std::process::id())); + // A directory is an invalid file destination on every supported platform. + std::fs::create_dir_all(&dir).unwrap(); + let target = dir.clone(); + let picked = Arc::new(Mutex::new(0)); + let capture = picked.clone(); + let mut h = harness(Services { + pick_save: Some(Box::new(move |_| { + *capture.lock().unwrap() += 1; + fail.then(|| target.to_string_lossy().into_owned()) + })), + ..Default::default() + }); + h.state_mut().run("cell.set", serde_json::json!({"cell": "A1", "input": "42"})).unwrap(); + let uid = h.state().session.doc().unwrap().uid; + request_window_close(&mut h); + h.key_press(egui::Key::Y); + let sent = step_commands(&mut h); + assert_eq!(*picked.lock().unwrap(), 1, "Y attempts Save As"); + assert!(!sent.contains(&egui::ViewportCommand::Close)); + assert_eq!(h.state().session.doc().unwrap().uid, uid); + assert!(h.state().session.doc().unwrap().is_dirty()); + assert_eq!(a1(&h, 0), Value::Number(42.0)); + assert_eq!(h.state().message.is_some(), fail); + std::fs::remove_dir_all(dir).unwrap(); + } +} + +#[test] +fn save_changes_shortcuts_do_not_answer_other_dialogs_or_steal_cell_text() { + let mut h = harness(Services::default()); + h.input_mut().events.push(egui::Event::Text("yn".into())); + h.run_steps(2); + assert_eq!(h.state().editor.as_ref().map(|ed| ed.text.as_str()), Some("yn")); + h.key_press(egui::Key::Enter); + h.run_steps(2); + h.state_mut().open_dialog("about", serde_json::json!({})); + h.run_steps(3); + for key in [egui::Key::Y, egui::Key::N] { + h.key_press(key); + h.run_steps(3); + assert_eq!(h.state().dialog.as_ref().map(|d| d.name()), Some("about")); + assert_eq!(h.state().session.documents().len(), 1); + } +} + /// Book1 has a cell being typed when File › New runs (the macOS menu runs the command directly). fn typing_in_book1_then_new(h: &mut egui_kittest::Harness<'static, SheetApp>) -> u64 { h.input_mut().events.push(egui::Event::Text("hello".into())); diff --git a/docs/ui-parity.md b/docs/ui-parity.md --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -1,6 +1,6 @@ # UI parity -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-25 · **Change:** minor (AutoSum over a selected row puts its total to the right) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-26 · **Change:** minor (single-key answers in Save Changes) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Interaction, controls and feel, against Excel. Part of [target-app-parity.md](target-app-parity.md). **UI/UX fidelity: ~55% ready, 50–80 h** (estimated). Ribbon command coverage is measured @@ -104,6 +104,7 @@ otherwise. Number-format and border keys match the physical key (Ctrl+Shift+4, w |---|---|---|---| | Ctrl+C / X / V, Ctrl+Z / Y, Ctrl+A, Ctrl+B / I / U | same with Cmd | clipboard, undo/redo, select all, bold/italic/underline | present | | Ctrl+S, Ctrl+Shift+S, Ctrl+O, Ctrl+N, Ctrl+W | Cmd | save, save as, open, new, close | present | +| Y / N / Esc in Save Changes | same | Save / Don't Save / Cancel | added (#2572); Y/N require no modifiers and answer on release so holding a key cannot answer successive workbooks; focus loss cancels the gesture; a cancelled or failed save keeps the workbook open | | Ctrl+F, Ctrl+H, Ctrl+G, Ctrl+K, Ctrl+T | Cmd (Ctrl+H) | find, replace, go to, hyperlink, table | present | | Ctrl+D, Ctrl+R, Ctrl+E, Ctrl+1, Ctrl+9 / 0, Ctrl+Shift+9 / 0 | Cmd (Ctrl+E on Mac: see below) | fill down/right, Flash Fill, Format Cells, hide/unhide | present | | Ctrl+- , Ctrl+Shift+= | Cmd | Delete / Insert cells dialog | present | @@ -187,6 +188,7 @@ scrollbar bugs 2–4 · pixel/feel pass against observed Excel 7–12. | Date | Change | Summary | |---|---|---| +| 2026-10-26 | minor | Save Changes accepts unmodified Y (Save) and N (Don't Save) on release through the existing button actions; Esc still cancels. Held keys cannot answer successive workbooks, and focus loss cancels the gesture; cancelling Save As or a failed save keeps the workbook open (#2572) | | 2026-10-25 | minor | AutoSum (Alt+=, Home › AutoSum) over a selected row puts its total to the right of the row, or in its last cell when that is empty, as in Excel; it wrote a one-cell total under each cell. A selection's empty last column takes each row's total and its empty last row each column's, with the block's total in the corner when both are empty | | 2026-10-25 | minor | Paste Name lays out only the names in view, so a workbook with thousands of names opens it at once; the arrow keys still scroll the chosen name into view. Go To matches a `Sheet2!Name` reference and hides a workbook name behind the active sheet's own name in any case and script (`Größe` = `GRÖSSE`), as formulas do. The two Page Layout view rows are one; the 0.7 in side margin used when Align with Page Margins is off is marked unverified | | 2026-10-24 | minor | Go To lists the named ranges on every sheet, as Excel's does ("The Go to popup window shows named ranges on every worksheet in your workbook"): a name scoped to another sheet appears as `Sheet2!Name`, and hidden names, constants and formulas are left out. It keeps the references entered (newest first, ten at most), and a double-click on an entry goes there. The Reference box and `edit.goTo` take several names or references separated by commas (`Price, Type`, `B14:C22,F19:G30,H21:H29`) and select them all; `edit.goTo {list: true}` returns the list |