diff --git a/apps/gridcraft/src/native_menu.rs b/apps/gridcraft/src/native_menu.rs index 2c2866379..877c82fd8 100644 --- a/apps/gridcraft/src/native_menu.rs +++ b/apps/gridcraft/src/native_menu.rs @@ -283,27 +283,32 @@ impl NativeMenu { /// step with the session. fn sync_windows(&mut self, app: &SheetApp) { let windows = app.open_windows(); - if windows == self.windows { - return; + if windows != self.windows + && let Some(menu) = &self.window_menu + { + let same_docs = windows.len() == self.windows.len() && windows.iter().zip(&self.windows).all(|(a, b)| a.doc == b.doc); + if same_docs { + for (item, w) in self.window_items.iter().zip(&windows) { + item.set_text(w.label()); + } + } else { + for item in self.window_items.drain(..) { + let _ = menu.remove(&item); + } + for w in &windows { + let item = CheckMenuItem::with_id(format!("{WINDOW_ITEM}{}", w.doc), w.label(), true, w.active, None); + let _ = menu.append(&item); + self.window_items.push(item); + } + } + self.windows = windows; } - let Some(menu) = &self.window_menu else { return }; - let same_docs = windows.len() == self.windows.len() && windows.iter().zip(&self.windows).all(|(a, b)| a.doc == b.doc); - if same_docs { - for (item, w) in self.window_items.iter().zip(&windows) { - item.set_text(w.label()); + // AppKit flips a check item when it is chosen, so the active check is put back every poll. + for (item, w) in self.window_items.iter().zip(&self.windows) { + if item.is_checked() != w.active { item.set_checked(w.active); } - } else { - for item in self.window_items.drain(..) { - let _ = menu.remove(&item); - } - for w in &windows { - let item = CheckMenuItem::with_id(format!("{WINDOW_ITEM}{}", w.doc), w.label(), true, w.active, None); - let _ = menu.append(&item); - self.window_items.push(item); - } } - self.windows = windows; } pub fn poll(&mut self, app: &mut SheetApp, ctx: &egui::Context) { diff --git a/crates/ui-egui/src/editor.rs b/crates/ui-egui/src/editor.rs index eeeaf12da..3f23e8bed 100644 --- a/crates/ui-egui/src/editor.rs +++ b/crates/ui-egui/src/editor.rs @@ -45,11 +45,15 @@ pub struct EditState { pub completion: Option, /// Move the caret to the end on the next frame. pub caret_to_end: bool, + /// The cell's text when an F2-style edit began. Committing it unchanged leaves the cell alone + /// (no undo step, the workbook stays clean). `None` for an edit started by typing. + pub original: Option, } impl EditState { pub fn new(doc: u64, sheet: usize, cell: CellRef, text: String, enter_mode: bool, from_formula_bar: bool) -> EditState { let caret = text.chars().count(); + let original = (!enter_mode).then(|| text.clone()); EditState { doc, sheet, @@ -68,6 +72,7 @@ impl EditState { sync_caret: true, caret_to_end: true, completion: None, + original, } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 001abaad5..cc478168e 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -412,7 +412,7 @@ impl SheetApp { || matches!(id, "view.newWindow" | "view.hideWindow" | "view.unhideWindow") || (id == "view.switchWindows" && params.get("doc").is_some_and(|d| !d.is_null())); if id.starts_with("file.") || switches_window { - self.settle_drafts(); + self.settle_drafts()?; } if matches!(id, "file.save" | "file.saveAs" | "file.saveBytes") { self.store_view_state(); @@ -575,8 +575,11 @@ impl SheetApp { self.open_switch_windows = true; return; } - if let Err(e) = self.run(id, params) { - self.alert(e); + match self.run(id, params) { + // A refused cell draft already shows its own message (with the rule's title). + Err(e) if self.message.as_ref().is_some_and(|(_, m)| *m == e) => {} + Err(e) => self.alert(e), + Ok(_) => {} } } @@ -1013,8 +1016,8 @@ impl SheetApp { /// Commits the edit; `then` moves the selection (dr, dc) afterwards. /// /// The draft goes into the workbook it was typed in. If another workbook is showing, that - /// workbook stays active afterwards, and a draft its own workbook refuses (validation, a bad - /// formula) is dropped rather than left open over the wrong grid. + /// workbook stays active afterwards; a draft its own workbook refuses (validation, a bad + /// formula) brings that workbook back to the front with the draft still open. pub fn commit_edit(&mut self, dr: i64, dc: i64, array: bool, fill_selection: bool) -> bool { let Some(doc) = self.editor.as_ref().map(|ed| ed.doc) else { return true }; let shown = self.session.active().map(|d| d.uid); @@ -1028,25 +1031,21 @@ impl SheetApp { let back = self.session.active_index(); self.session.set_active(owner); if !self.commit_edit_here(dr, dc, array, fill_selection) { - self.cancel_edit(); + // Refused: its workbook stays in front with the draft open and the reason shown. + return false; } self.session.set_active(back); true } - /// Settles the cell and text-box drafts before a command that can show another workbook or - /// write this one to a file: each is committed as Enter would, or dropped when it can't be - /// (the refusal is still reported), so a draft never lands in another workbook. - fn settle_drafts(&mut self) { + /// Commits the cell and text-box drafts, as Enter would, before a command that can show + /// another workbook or write this one to a file, so a draft never lands in another workbook. + /// A draft its workbook refuses stays open with the reason shown, and the command doesn't run. + fn settle_drafts(&mut self) -> Result<(), String> { if !self.commit_edit(0, 0, false, false) { - self.cancel_edit(); - } - if let Err(e) = self.commit_text_box_edit() { - self.cancel_text_box_edit(); - if self.message.is_none() { - self.message = Some(("Text Box".into(), clean_error(&e))); - } + return Err(self.message.as_ref().map(|(_, m)| m.clone()).unwrap_or_else(|| "The cell being edited can't be entered.".into())); } + self.commit_text_box_edit() } fn commit_edit_here(&mut self, dr: i64, dc: i64, array: bool, fill_selection: bool) -> bool { @@ -1056,6 +1055,13 @@ impl SheetApp { Some(full) if full.to_lowercase().starts_with(&ed.text.to_lowercase()) => full.clone(), _ => ed.text.clone(), }; + // F2 then Enter (or a file command) on an untouched draft: nothing to write. + if !fill_selection && !array && ed.original.as_ref() == Some(&text) { + if dr != 0 || dc != 0 { + self.move_after_enter(dr, dc); + } + return true; + } let text = match self.session.active().map(|d| formula_locale::canonical(&text, ed.locale, &d.wb, ed.sheet, ed.cell)) { Some(Ok(text)) => text, Some(Err(error)) => { @@ -1355,7 +1361,8 @@ impl SheetApp { let t0 = now_ms(); text_box::before_ui(self, &ctx); // Another workbook came forward without going through a command (which settles drafts - // first): the draft goes into its own workbook instead of floating over this one. + // first): the draft goes into its own workbook instead of floating over this one. If that + // workbook refuses it, it comes back to the front with the draft open. if self.editor.as_ref().is_some_and(|ed| self.session.active().map(|d| d.uid) != Some(ed.doc)) { self.commit_edit(0, 0, false, false); } diff --git a/crates/ui-egui/tests/file_menu.rs b/crates/ui-egui/tests/file_menu.rs index 415e09902..5c447cb63 100644 --- a/crates/ui-egui/tests/file_menu.rs +++ b/crates/ui-egui/tests/file_menu.rs @@ -351,19 +351,49 @@ fn new_workbook_keeps_the_typed_cell_in_the_first_and_switch_windows_brings_it_b } #[test] -fn a_draft_its_workbook_refuses_is_dropped_when_switching_away() { +fn a_draft_its_workbook_refuses_stops_the_switch_and_stays_open() { let mut h = harness(Services::default()); let book1 = typing_in_book1_then_new(&mut h); + let book2 = h.state().session.doc().unwrap().uid; h.state_mut().session.execute("data.validation", serde_json::json!({"range": "A1", "type": "list", "formula1": "\"Red,Green\""})).unwrap(); h.input_mut().events.push(egui::Event::Text("Purple".into())); h.run_steps(2); - assert!(h.state().editor.is_some()); h.state_mut().run_or_alert("view.switchWindows", serde_json::json!({"doc": book1})); h.run_steps(2); - assert!(h.state().editor.is_none()); - assert_eq!(h.state().session.doc().unwrap().uid, book1, "the switch still happens"); + assert_eq!(h.state().session.doc().unwrap().uid, book2, "the switch doesn't happen"); + assert_eq!(h.state().editor.as_ref().map(|ed| ed.text.as_str()), Some("Purple"), "the draft stays open to fix"); assert_eq!(a1(&h, 1), Value::Empty, "Book2 is unchanged"); assert!(h.state().message.is_some(), "the refusal is reported"); + + // Esc drops the draft; then the switch goes through. + h.state_mut().message = None; + h.state_mut().cancel_edit(); + h.state_mut().run_or_alert("view.switchWindows", serde_json::json!({"doc": book1})); + h.run_steps(2); + assert_eq!(h.state().session.doc().unwrap().uid, book1); + assert!(h.state().message.is_none()); +} + +#[test] +fn an_untouched_f2_draft_leaves_the_workbook_clean_on_a_file_command() { + let dir = std::env::temp_dir().join(format!("gridcraft-f2-draft-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("Book.xlsx").to_string_lossy().into_owned(); + let mut h = harness(Services::default()); + h.state_mut().run("cell.set", serde_json::json!({"cell": "A1", "input": "=1+2"})).unwrap(); + h.state_mut().run("file.save", serde_json::json!({"path": &path})).unwrap(); + h.run_steps(2); + let undo = h.state().session.doc().unwrap().undo.len(); + h.key_press(egui::Key::F2); + h.run_steps(2); + assert_eq!(h.state().editor.as_ref().map(|ed| ed.text.as_str()), Some("=1+2")); + h.state_mut().run_or_alert("file.new", serde_json::json!({})); + h.run_steps(2); + assert!(h.state().editor.is_none()); + let book1 = &h.state().session.documents()[0]; + assert!(!book1.is_dirty(), "nothing was typed, so nothing changed"); + assert_eq!(book1.undo.len(), undo, "and no undo step was added"); + let _ = std::fs::remove_dir_all(&dir); } fn pdf_picker_harness(dir: &std::path::Path) -> (Arc>>, egui_kittest::Harness<'static, SheetApp>) {