From 40b254c3067b5872f322f773845dd86ad2a79355 Mon Sep 17 00:00:00 2001 From: Max Murphy Date: Sat, 31 Oct 2026 16:52:49 +0100 Subject: [PATCH 1/2] fix(ui): refuse a locked cell's edit before a draft blocks Save (#2945) --- crates/engine/src/lib.rs | 11 +++ crates/engine/src/tests.rs | 11 +++ crates/ui-egui/src/app/editing.rs | 88 +++++++++++++++++++++++ crates/ui-egui/tests/it/main.rs | 1 + crates/ui-egui/tests/it/protected_cell.rs | 70 ++++++++++++++++++ 5 files changed, 181 insertions(+) create mode 100644 crates/ui-egui/tests/it/protected_cell.rs diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 3ae4ac95a..a470f2158 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -558,6 +558,17 @@ impl Session { pub fn doc_mut(&mut self) -> Result<&mut DocState> { self.active_mut().ok_or(EngineError::NoDocument) } + /// The warning an edit of `at` on `sheet` gets from sheet protection, or `None` when the edit + /// is allowed. Front ends ask before opening an editor, so typing into a locked cell never + /// leaves a draft that can't be entered (and so blocks saving). + pub fn locked_cell_message(&self, sheet: usize, at: CellRef) -> Option { + let d = self.active()?; + let sh = d.wb.sheet(sheet)?; + if !sh.is_protected() || !d.wb.styles.get(sh.style_id(at)).protection.locked { + return None; + } + cmd::locked_cell_error(sh, at, &d.unlocked_ranges) + } /// Opens a workbook in a new window, which becomes the active one. pub fn add_document(&mut self, d: DocState) -> usize { let uid = d.uid; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index f99f246df..04d080eea 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1400,6 +1400,17 @@ fn protection_refuses_edits_of_locked_cells() { assert_eq!(v(&s, "A1"), Value::Number(1.0)); } +#[test] +fn locked_cell_message_matches_what_an_edit_would_be_refused_with() { + let s = protected(json!({})); + let locked = CellRef::parse("A1").unwrap(); + let unlocked = CellRef::parse("D1").unwrap(); + + assert!(s.locked_cell_message(0, locked).is_some_and(|m| m.contains("protected sheet"))); + assert_eq!(s.locked_cell_message(0, unlocked), None, "an unlocked cell stays editable"); + assert_eq!(s.locked_cell_message(9, locked), None, "no such sheet"); +} + #[test] fn protection_allows_permitted_row_and_column_edits() { let mut s = protected(json!({"insertRows": true, "deleteRows": true, "insertColumns": true})); diff --git a/crates/ui-egui/src/app/editing.rs b/crates/ui-egui/src/app/editing.rs index 6890fb944..bb99ae260 100644 --- a/crates/ui-egui/src/app/editing.rs +++ b/crates/ui-egui/src/app/editing.rs @@ -15,6 +15,13 @@ impl SheetApp { let Some(d) = self.session.active() else { return }; let Some(sh) = d.wb.active() else { return }; let at = sh.merge_at(d.selection.active).map(|m| m.start).unwrap_or(d.selection.active); + // A locked cell on a protected sheet can't be entered (Excel's warning, and the engine + // refuses `cell.set` anyway): refuse before a draft exists, rather than leaving one that + // can never be committed and blocks Save (#2945). + if let Some(m) = self.session.locked_cell_message(d.wb.active_sheet(), at) { + self.message = Some(("GridCraft".into(), m)); + return; + } // A picture has no editable scalar text. Selecting it, F2, or clicking // the formula bar must not turn its fallback value into a text edit. // Typing still starts an intentional replacement. @@ -102,6 +109,13 @@ impl SheetApp { } return true; } + // A draft on a cell sheet protection now locks (the sheet was protected while it was + // open) can't be entered: drop it with the warning instead of keeping it, which would + // block Save with a draft that can never be committed (#2945). + if let Some(m) = self.session.locked_cell_message(ed.sheet, ed.cell) { + self.message = Some(("GridCraft".into(), m)); + 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)) => { @@ -209,3 +223,77 @@ impl SheetApp { self.grid.ensure_visible = true; } } + +#[cfg(test)] +mod protected_cell_tests { + use super::*; + use gridcraft_engine::Session; + use gridcraft_engine::core::{CellRef, Value}; + + /// A protected sheet whose C2 is unlocked; C2 holds "keep" as the last undo step. + fn protected_book() -> SheetApp { + let mut session = Session::new(); + session.new_workbook(); + let mut app = SheetApp::new(session, Default::default()); + app.run("cell.set", json!({"cell": "C2", "input": "keep"})).unwrap(); + app.run("home.formatCells", json!({"range": "C2", "style": {"protection": {"locked": false}}})).unwrap(); + app.run("review.protectSheet", json!({})).unwrap(); + app + } + + fn value(app: &SheetApp, a1: &str) -> Value { + let at = CellRef::parse(a1).unwrap(); + app.session.active().and_then(|d| d.wb.sheet(0)).map(|sh| sh.value(at)).unwrap_or_default() + } + + fn protected_message(app: &SheetApp) -> Option<&str> { + app.message.as_ref().map(|(_, m)| m.as_str()) + } + + #[test] + fn typing_in_a_locked_cell_does_not_leave_a_draft_that_blocks_undo_and_save() { + let mut app = protected_book(); + assert_eq!(app.session.active().unwrap().selection.active.a1(), "A1", "the locked A1 is the active cell"); + let undo = app.session.active().unwrap().undo.len(); + let dirty = app.session.active().unwrap().is_dirty(); + + app.begin_edit(Some("locked".into()), false); + + assert!(app.editor.is_none(), "a locked cell must not open an editor"); + assert!(protected_message(&app).is_some_and(|m| m.contains("protected sheet")), "the warning names the protected sheet"); + assert_eq!(app.session.active().unwrap().undo.len(), undo, "a refused edit adds no undo step"); + assert_eq!(app.session.active().unwrap().is_dirty(), dirty, "a refused edit leaves the workbook as it was"); + assert!(app.run("file.saveBytes", json!({"format": "xlsx"})).is_ok(), "Save is not blocked by a draft that can never be entered"); + } + + #[test] + fn an_unlocked_cell_on_a_protected_sheet_still_edits_undoes_and_saves() { + let mut app = protected_book(); + app.run("selection.set", json!({"cell": "C2"})).unwrap(); + app.begin_edit(Some("typed".into()), false); + assert!(app.editor.is_some(), "an unlocked cell opens its editor"); + assert!(app.commit_edit(0, 0, false, false)); + assert_eq!(value(&app, "C2"), Value::text("typed")); + + assert!(app.run("edit.undo", json!({})).is_ok()); + assert_eq!(value(&app, "C2"), Value::text("keep"), "Undo of the typed edit still works"); + assert!(app.run("file.saveBytes", json!({"format": "xlsx"})).is_ok()); + } + + #[test] + fn a_draft_that_becomes_locked_is_cancelled_instead_of_staying_open() { + let mut app = protected_book(); + app.run("selection.set", json!({"cell": "C2"})).unwrap(); + app.begin_edit(Some("typed".into()), false); + // The sheet is unprotected, C2 locked, and the sheet protected again while the draft is open. + app.run("review.unprotectSheet", json!({})).unwrap(); + app.run("home.formatCells", json!({"range": "C2", "style": {"protection": {"locked": true}}})).unwrap(); + app.run("review.protectSheet", json!({})).unwrap(); + + assert!(app.commit_edit(0, 0, false, false), "the refused draft is handled, not left open"); + assert!(app.editor.is_none()); + assert!(protected_message(&app).is_some_and(|m| m.contains("protected sheet"))); + assert_eq!(value(&app, "C2"), Value::text("keep")); + assert!(app.run("file.saveBytes", json!({"format": "xlsx"})).is_ok()); + } +} diff --git a/crates/ui-egui/tests/it/main.rs b/crates/ui-egui/tests/it/main.rs index b683f125d..9cebf6198 100644 --- a/crates/ui-egui/tests/it/main.rs +++ b/crates/ui-egui/tests/it/main.rs @@ -80,6 +80,7 @@ mod pivot_show_details; mod pivot_show_details_menu; mod popup_dismissal; mod protect_password; +mod protected_cell; mod ribbon_layout; mod right_to_left; mod scroll_position; diff --git a/crates/ui-egui/tests/it/protected_cell.rs b/crates/ui-egui/tests/it/protected_cell.rs new file mode 100644 index 000000000..15a3f5fd7 --- /dev/null +++ b/crates/ui-egui/tests/it/protected_cell.rs @@ -0,0 +1,70 @@ +//! Typing into a locked cell of a protected sheet warns before an editor opens, and the warning +//! leaves Undo, Save and typing in unlocked cells working (#2945). + +use egui::{Event, Key, Modifiers}; +use gridcraft_engine::Session; +use gridcraft_engine::core::{CellRef, Value}; +use gridcraft_ui_egui::SheetApp; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +const PROTECTED: &str = "The cell or chart you're trying to change is on a protected sheet."; + +/// A protected sheet whose C2 is unlocked and holds "keep"; A1 is locked and active. +fn harness() -> Harness { + let mut s = Session::new(); + s.new_workbook(); + s.execute("cell.set", json!({"cell": "C2", "input": "keep"})).unwrap(); + s.execute("home.formatCells", json!({"range": "C2", "style": {"protection": {"locked": false}}})).unwrap(); + s.execute("review.protectSheet", json!({})).unwrap(); + s.execute("selection.set", json!({"cell": "A1"})).unwrap(); + crate::support::harness(s, 4) +} + +fn key(h: &mut Harness, k: Key) { + h.input_mut().events.push(Event::Key { key: k, physical_key: Some(k), pressed: true, repeat: false, modifiers: Modifiers::NONE }); + h.input_mut().events.push(Event::Key { key: k, physical_key: Some(k), pressed: false, repeat: false, modifiers: Modifiers::NONE }); + h.run_steps(2); +} + +/// Types text the way a keyboard does (into whatever has focus). +fn type_text(h: &mut Harness, text: &str) { + h.input_mut().events.push(Event::Text(text.into())); + h.run_steps(2); +} + +fn value(h: &Harness, a1: &str) -> Value { + let at = CellRef::parse(a1).unwrap(); + h.state().session.active().and_then(|d| d.wb.sheet(0)).map(|sh| sh.value(at)).unwrap_or_default() +} + +#[test] +fn typing_in_a_locked_cell_warns_without_a_stuck_draft() { + let mut h = harness(); + let undo = h.state().session.active().unwrap().undo.len(); + + type_text(&mut h, "locked"); + + assert!(h.state().editor.is_none(), "no editor opened on the locked cell"); + assert_eq!(h.state().message.as_ref().map(|(_, m)| m.as_str()), Some(PROTECTED)); + assert_eq!(h.state().session.active().unwrap().undo.len(), undo, "the refused edit added no undo step"); + assert_eq!(value(&h, "A1"), Value::Empty); + // No draft exists, so Save is not blocked while the warning is up. + assert!(h.state_mut().run("file.saveBytes", json!({"format": "xlsx"})).is_ok()); + + // Enter dismisses the warning and the keyboard goes back to the grid. + key(&mut h, Key::Enter); + assert!(h.state().message.is_none(), "Enter dismisses the warning"); + + // An unlocked cell on the same sheet still edits, undoes and saves. + h.state_mut().run("selection.set", json!({"cell": "C2"})).unwrap(); + h.run_steps(1); + type_text(&mut h, "typed"); + assert_eq!(h.state().editor.as_ref().map(|e| e.text.as_str()), Some("typed")); + key(&mut h, Key::Enter); + assert_eq!(value(&h, "C2"), Value::text("typed")); + assert!(h.state_mut().run("edit.undo", json!({})).is_ok()); + assert_eq!(value(&h, "C2"), Value::text("keep"), "Undo of the unlocked cell's edit still works"); + assert!(h.state_mut().run("file.saveBytes", json!({"format": "xlsx"})).is_ok()); +} From edcb626c3c124c600eb97fb4feaaa3909525ad5f Mon Sep 17 00:00:00 2001 From: Max Murphy Date: Sat, 31 Oct 2026 20:01:48 +0100 Subject: [PATCH 2/2] contributors: add maxm11 entry (sync GitHub name) --- contributors/people.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contributors/people.toml b/contributors/people.toml index 0e5b14ca4..00f6ce76a 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -106,3 +106,6 @@ sync_github_name = true [people.Plutonium-fream] sync_github_name = true + +[people.maxm11] +sync_github_name = true