From 628732ef5811a5a933ba314effb15ec39b763dac Mon Sep 17 00:00:00 2001 From: harrisonoconnorhover <130776285+harrisonoconnorhover@users.noreply.github.com> Date: Fri, 9 Oct 2026 17:47:58 -0400 Subject: [PATCH 1/2] Enable inline editing of worksheet text boxes (#56) --- apps/gridcraft/src/native_menu.rs | 25 +++ crates/engine/src/cmd/insert.rs | 21 +++ crates/engine/src/tests.rs | 74 ++++++++ crates/ui-egui/src/chartview.rs | 7 +- crates/ui-egui/src/grid.rs | 23 ++- crates/ui-egui/src/lib.rs | 40 +++- crates/ui-egui/src/ribbon.rs | 2 +- crates/ui-egui/src/text_box.rs | 165 ++++++++++++++++ crates/ui-egui/tests/text_box_editing.rs | 230 +++++++++++++++++++++++ 9 files changed, 577 insertions(+), 10 deletions(-) create mode 100644 crates/ui-egui/src/text_box.rs create mode 100644 crates/ui-egui/tests/text_box_editing.rs diff --git a/apps/gridcraft/src/native_menu.rs b/apps/gridcraft/src/native_menu.rs index 43e57a4..0bdce38 100644 --- a/apps/gridcraft/src/native_menu.rs +++ b/apps/gridcraft/src/native_menu.rs @@ -226,6 +226,31 @@ impl NativeMenu { pub fn poll(&self, app: &mut SheetApp, ctx: &egui::Context) { while let Ok(ev) = self.events.try_recv() { let id = ev.id.as_ref().to_string(); + // Text selection belongs to the inline editor, not the worksheet clipboard/undo. + if app.text_box_editor.is_some() { + let clipboard = match id.as_str() { + "edit.copy" => Some(egui::ViewportCommand::RequestCopy), + "edit.cut" => Some(egui::ViewportCommand::RequestCut), + "edit.paste" => Some(egui::ViewportCommand::RequestPaste), + _ => None, + }; + if let Some(command) = clipboard { + ctx.send_viewport_cmd(command); + continue; + } + if id == "edit.undo" || id == "edit.redo" { + ctx.input_mut(|i| { + i.events.push(egui::Event::Key { + key: egui::Key::Z, + physical_key: None, + pressed: true, + repeat: false, + modifiers: egui::Modifiers { command: true, mac_cmd: true, shift: id == "edit.redo", ..Default::default() }, + }); + }); + continue; + } + } if let Some(url) = id.strip_prefix("url:") { if let Some(open) = &app.services.open_url { open(url); diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index e4b5bc9..d2f0e35 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -122,6 +122,7 @@ pub fn specs() -> Vec { s, &with(p, "kind", json!("textBox")) )), + cmd!("shape.setText", "Edit Text Box", [], None, "{id: textBox id, text: string}", has_doc, shape_set_text), cmd!("object.delete", "Delete Object", [], None, "{kind: chart|image|shape, id}", has_doc, delete_object), cmd!("object.move", "Move Object", [], None, "{kind: chart|image|shape, id, at?: \"C3\", dx?, dy?, width?, height?}", has_doc, move_object), cmd!( @@ -776,6 +777,26 @@ fn insert_shape(s: &mut Session, p: &Json) -> Result { }) } +fn shape_set_text(s: &mut Session, p: &Json) -> Result { + let id = + p.get("id").and_then(Json::as_u64).and_then(|id| u32::try_from(id).ok()).ok_or_else(|| bad("shape.setText", "`id` must be a text box id"))?; + let text = str_param(p, "text").ok_or_else(|| bad("shape.setText", "`text` must be a string"))?; + let d = s.doc()?; + let sheet = d.wb.active_sheet; + let shape = d.wb.active().and_then(|sh| sh.shapes.iter().find(|shape| shape.id == id)).ok_or_else(|| bad("shape.setText", "no such text box"))?; + if shape.kind != ShapeKind::TextBox { + return Err(bad("shape.setText", "the shape is not a text box")); + } + if shape.text == text { + return ok(); + } + edit(s, |cx| { + let shape = cx.sheet_mut(sheet)?.shapes.iter_mut().find(|shape| shape.id == id).ok_or_else(|| bad("shape.setText", "no such text box"))?; + shape.text = text.into(); + Ok(Json::Null) + }) +} + fn delete_object(s: &mut Session, p: &Json) -> Result { let id = u32_param(p, "id").ok_or_else(|| bad("object.delete", "missing `id`"))?; let kind = str_param(p, "kind").unwrap_or("").to_string(); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index f3e36cd..bb25512 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -39,6 +39,80 @@ fn undo_redo() { assert!(s.doc().unwrap().is_dirty()); } +#[test] +fn shape_text_edit_undo_redo_and_noop() { + let mut s = s(); + let id = s.execute("insert.textBox", json!({"at": "C4", "width": 210, "height": 90, "text": "Original"})).unwrap()["shape"].clone(); + let original = s.doc().unwrap().wb.active().unwrap().shapes[0].clone(); + let mut changed = original.clone(); + changed.text = "First line\n\nSecond line & ".into(); + let undo_len = s.doc().unwrap().undo.len(); + s.execute("shape.setText", json!({"id": id, "text": changed.text})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().shapes[0], changed); + assert_eq!(s.doc().unwrap().undo.len(), undo_len + 1); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().shapes[0], original); + + let before_noop = s.doc().unwrap().wb.clone(); + let revision = s.doc().unwrap().revision; + s.execute("shape.setText", json!({"id": id, "text": "Original"})).unwrap(); + assert!(std::sync::Arc::ptr_eq(&s.doc().unwrap().wb, &before_noop)); + assert_eq!(s.doc().unwrap().revision, revision); + assert_eq!(s.doc().unwrap().undo.len(), undo_len); + assert_eq!(s.doc().unwrap().redo.len(), 1); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().shapes[0], changed); + + s.execute("shape.setText", json!({"id": id, "text": ""})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().shapes[0].text, ""); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().shapes[0], changed); +} + +#[test] +fn shape_text_rejects_invalid_targets_and_params() { + let mut s = s(); + let id = s.execute("insert.textBox", json!({"text": "Original"})).unwrap()["shape"].clone(); + let rectangle = s.execute("insert.shape", json!({"kind": "rectangle", "text": "Rectangle"})).unwrap()["shape"].clone(); + let before = s.doc().unwrap().wb.clone(); + let undo_len = s.doc().unwrap().undo.len(); + for params in [ + json!({}), + json!({"id": id}), + json!({"id": id, "text": 12}), + json!({"id": "1", "text": "bad id"}), + json!({"id": -1, "text": "bad id"}), + json!({"id": 1.5, "text": "bad id"}), + json!({"id": u64::from(u32::MAX) + 1, "text": "bad id"}), + json!({"id": 999, "text": "unknown"}), + json!({"id": rectangle, "text": "not a text box"}), + ] { + assert!(matches!(s.execute("shape.setText", params), Err(crate::EngineError::BadParams { .. }))); + assert!(std::sync::Arc::ptr_eq(&s.doc().unwrap().wb, &before)); + assert_eq!(s.doc().unwrap().undo.len(), undo_len); + } + s.execute("home.insertSheet", json!({})).unwrap(); + assert!(matches!(s.execute("shape.setText", json!({"id": id, "text": "wrong sheet"})), Err(crate::EngineError::BadParams { .. }))); + assert_eq!(s.doc().unwrap().wb.sheet(0).unwrap().shapes[0].text, "Original"); + assert!(s.doc().unwrap().wb.active().unwrap().shapes.is_empty()); +} + +#[test] +fn shape_text_edit_survives_xlsx_roundtrip() { + let mut s = s(); + s.execute("insert.textBox", json!({"text": "Imported"})).unwrap(); + let saved = s.execute("file.saveBytes", json!({"format": "xlsx"})).unwrap(); + s.execute("file.open", json!({"name": "text-box.xlsx", "base64": saved["base64"]})).unwrap(); + let id = s.doc().unwrap().wb.active().unwrap().shapes[0].id; + let text = "Edited & \n\nLast line\n"; + s.execute("shape.setText", json!({"id": id, "text": text})).unwrap(); + let saved = s.execute("file.saveBytes", json!({"format": "xlsx"})).unwrap(); + s.execute("file.open", json!({"name": "edited.xlsx", "base64": saved["base64"]})).unwrap(); + let shape = &s.doc().unwrap().wb.active().unwrap().shapes[0]; + assert_eq!(shape.kind, gridcraft_model::ShapeKind::TextBox); + assert_eq!(shape.text, text); +} + #[test] fn copy_paste_shifts_formulas() { let mut s = s(); diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index 082be11..adeeddb 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -180,7 +180,7 @@ fn area_mesh(v: &[Pos2], col: Color32) -> egui::Mesh { mesh } -fn anchor_rect(geo: &Geo, sh: &Sheet, a: &Anchor) -> Rect { +pub(crate) fn anchor_rect(geo: &Geo, sh: &Sheet, a: &Anchor) -> Rect { let x = geo.x(sh, a.cell.col) + a.dx * geo.z; let y = geo.y(sh, a.cell.row) + a.dy * geo.z; Rect::from_min_size(pos2(x, y), vec2(a.width * geo.z, a.height.max(1.0) * geo.z)) @@ -243,7 +243,7 @@ pub fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workbook, si: } } } - if !sp.text.is_empty() { + if !sp.text.is_empty() && !app.editing_text_box(sp.id) { let job = egui::text::LayoutJob::simple( sp.text.clone(), FontId::new(14.0 * geo.z, egui::FontFamily::Name(theme::CELL.into())), @@ -254,6 +254,9 @@ pub fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workbook, si: let pos = if sp.kind == ShapeKind::TextBox { r.min + vec2(4.0, 4.0) } else { r.center() - g.size() / 2.0 }; p.galley(pos, g, Color32::BLACK); } + if sp.kind == ShapeKind::TextBox && app.selected_chart == Some(sp.id) { + selection_frame(p, r); + } } for im in &sh.images { let r = anchor_rect(geo, sh, &im.anchor); diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 4b326cc..a3e0fb7 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -323,7 +323,7 @@ pub fn show(app: &mut SheetApp, ui: &mut egui::Ui) { let mut view = app.view(); // Scrolling. let (scroll_delta, zoom_delta, modifiers) = ui.input(|i| (i.smooth_scroll_delta, i.zoom_delta(), i.modifiers)); - if resp.hovered() || resp.contains_pointer() { + if (resp.hovered() || resp.contains_pointer()) && !crate::text_box::contains_pointer(app, ui.ctx()) { if zoom_delta != 1.0 { let z = (sh.zoom as f32 * zoom_delta).clamp(10.0, 400.0).round(); let _ = app.session.run("view.zoom", json!({"percent": z})); @@ -417,6 +417,7 @@ pub fn show(app: &mut SheetApp, ui: &mut egui::Ui) { } // Interaction. interact(app, ui, &resp, &geo, sh, &wb); + crate::text_box::show(app, ui, &geo, sh); if app.editor.as_ref().is_some_and(|e| !e.from_formula_bar) { in_cell_editor(app, ui, &geo, sh, &wb); } @@ -982,6 +983,10 @@ fn paint_headers(p: &Painter, geo: &Geo, sh: &Sheet, sel: &gridcraft_engine::Sel // ------------------------------------------------------------------ interaction fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: &Geo, sh: &Sheet, wb: &Workbook) { + // Text selection and editing must not also move the object or edit worksheet cells. + if app.text_box_editor.is_some() { + return; + } let ctx = ui.ctx().clone(); let pointer = ctx.input(|i| i.pointer.clone()); let mods = ctx.input(|i| i.modifiers); @@ -1064,6 +1069,13 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & && let Some((kind, id, rect)) = crate::chartview::hit(app, geo, sh, p) { app.selected_chart = Some(id); + if kind == "shape" + && resp.double_clicked() + && sh.shapes.iter().any(|s| s.id == id && s.kind == gridcraft_engine::model::ShapeKind::TextBox) + { + app.begin_text_box_edit(id); + return; + } if kind == "chart" && resp.double_clicked() { app.grid.pane = Some("formatChart".into()); } @@ -1503,12 +1515,13 @@ fn select_ranges(app: &mut SheetApp, r: RangeRef, add: bool, active: CellRef) { /// Grid keyboard handling when no text field has focus. fn keyboard(app: &mut SheetApp, ctx: &egui::Context, resp: &egui::Response, geo: &Geo, sh: &Sheet) { // Focus left on an editor that no longer exists goes back to the grid. - let stale = [egui::Id::new("gridcraft.cell_editor"), egui::Id::new("gridcraft.formula_bar")]; - if app.editor.is_none() && ctx.memory(|m| m.focused()).is_some_and(|f| stale.contains(&f)) { + let stale = [egui::Id::new("gridcraft.cell_editor"), egui::Id::new("gridcraft.formula_bar"), crate::text_box::editor_id()]; + let editing = app.editor.is_some() || app.text_box_editor.is_some(); + if !editing && ctx.memory(|m| m.focused()).is_some_and(|f| stale.contains(&f)) { resp.request_focus(); } - let other_focus = ctx.memory(|m| m.focused()).is_some_and(|f| f != resp.id && !(app.editor.is_none() && stale.contains(&f))); - if app.editor.is_some() || app.dialog.is_some() || other_focus || app.message.is_some() { + let other_focus = ctx.memory(|m| m.focused()).is_some_and(|f| f != resp.id && (editing || !stale.contains(&f))); + if editing || app.dialog.is_some() || other_focus || app.message.is_some() { return; } if ctx.memory(|m| m.focused()).is_none() { diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 8eb1eca..36a0fed 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -18,6 +18,7 @@ pub mod panes; pub mod pivot_pane; pub mod ribbon; pub mod tabs; +pub mod text_box; pub mod theme; pub mod widgets; @@ -93,6 +94,7 @@ pub struct SheetApp { pub ui: UiState, pub views: HashMap<(u64, usize), SheetView>, pub editor: Option, + pub text_box_editor: Option, pub dialog: Option, pub message: Option<(String, String)>, pub toast: Option<(String, f64)>, @@ -118,6 +120,7 @@ impl SheetApp { ui: UiState::default(), views: HashMap::new(), editor: None, + text_box_editor: None, dialog: None, message: None, toast: None, @@ -148,6 +151,29 @@ impl SheetApp { /// Runs an engine command (or a UI command) and handles UI requests it makes. pub fn run(&mut self, id: &str, params: Json) -> Result { + if id.starts_with("file.") + || matches!( + id, + "window.activate" + | "sheet.activate" + | "sheet.next" + | "sheet.previous" + | "sheet.move" + | "sheet.hide" + | "sheet.unhide" + | "view.newWindow" + | "view.hideWindow" + | "view.unhideWindow" + | "home.insertSheet" + | "home.deleteSheet" + | "edit.undo" + | "edit.redo" + | "object.delete" + | "object.move" + ) + { + self.commit_text_box_edit()?; + } if let Some(r) = self.run_ui_command(id, ¶ms) { return r; } @@ -218,6 +244,10 @@ impl SheetApp { } pub fn open_dialog(&mut self, name: &str, params: Json) { + if let Err(e) = self.commit_text_box_edit() { + self.message = Some(("Text Box".into(), clean_error(&e))); + return; + } match name { "open" => { if let Some(start) = &self.services.open_async { @@ -248,7 +278,7 @@ impl SheetApp { } pub fn open_path(&mut self, path: &str) { - match self.session.run("file.open", json!({"path": path})) { + match self.run("file.open", json!({"path": path})) { Ok(_) => { self.ui.recent.retain(|p| p != path); self.ui.recent.insert(0, path.to_string()); @@ -274,6 +304,10 @@ impl SheetApp { /// Starts editing the active cell. `text` replaces the content (typing) or pre-fills it. pub fn begin_edit(&mut self, text: Option, from_formula_bar: bool) { + if let Err(e) = self.commit_text_box_edit() { + self.message = Some(("Text Box".into(), clean_error(&e))); + return; + } let Some(d) = self.session.active() else { return }; let Some(sh) = d.wb.active() else { return }; let at = d.selection.active; @@ -375,7 +409,7 @@ impl SheetApp { .unwrap_or_default(); for (name, bytes) in arrived { let b64 = gridcraft_engine::io::base64_encode(&bytes); - if let Err(e) = self.session.run("file.open", json!({"name": name, "base64": b64})) { + if let Err(e) = self.run("file.open", json!({"name": name, "base64": b64})) { self.message = Some(("GridCraft".into(), clean_error(&e))); } self.after_engine(); @@ -399,6 +433,7 @@ impl SheetApp { return; } let t0 = now_ms(); + text_box::before_ui(self, &ctx); let t = theme::Tokens::get(&ctx); ribbon::title_bar(self, ui); ribbon::show(self, ui); @@ -436,6 +471,7 @@ impl SheetApp { // AutoSave: save workbooks that have a file shortly after they change. if self.grid.autosave && self.editor.is_none() + && self.text_box_editor.is_none() && now - self.grid.last_autosave > 2000.0 && self.session.active().is_some_and(|d| d.is_dirty() && d.path.as_deref().is_some_and(|p| p.ends_with(".xlsx"))) { diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 47c5432..87e33dd 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -210,7 +210,7 @@ fn contextual_tabs(app: &SheetApp) -> Vec<&'static str> { if sh.table_at(d.selection.active).is_some() { v.push("Table Design"); } - if app.selected_chart.is_some() { + if app.selected_chart.is_some_and(|id| sh.charts.iter().any(|c| c.id == id)) { v.push("Chart Design"); } } diff --git a/crates/ui-egui/src/text_box.rs b/crates/ui-egui/src/text_box.rs new file mode 100644 index 0000000..f9d8dec --- /dev/null +++ b/crates/ui-egui/src/text_box.rs @@ -0,0 +1,165 @@ +//! In-place editing of a text box. The draft becomes one undoable engine command. + +use egui::{Color32, FontId, Key, Modifiers, Rect, TextEdit}; +use gridcraft_engine::{Mode, model::ShapeKind}; +use serde_json::json; + +use crate::{SheetApp, grid::Geo, theme}; + +#[derive(Clone, Debug)] +pub struct EditState { + pub id: u32, + pub text: String, + uid: u64, + sheet: usize, + original: String, + request_focus: bool, + rect: Option, +} + +pub(crate) fn editor_id() -> egui::Id { + egui::Id::new("gridcraft.text_box_editor") +} + +impl SheetApp { + pub fn begin_text_box_edit(&mut self, id: u32) -> bool { + if !self.commit_edit(0, 0, false, false) { + return false; + } + if let Err(e) = self.commit_text_box_edit() { + self.message = Some(("Text Box".into(), crate::clean_error(&e))); + return false; + } + let Some(d) = self.session.active() else { return false }; + let Some(shape) = d.wb.active().and_then(|sh| sh.shapes.iter().find(|s| s.id == id && s.kind == ShapeKind::TextBox)) else { + return false; + }; + self.text_box_editor = Some(EditState { + id, + text: shape.text.clone(), + uid: d.uid, + sheet: d.wb.active_sheet, + original: shape.text.clone(), + request_focus: true, + rect: None, + }); + self.selected_chart = Some(id); + self.grid.drag = crate::grid::Drag::None; + self.grid.drag_select = false; + self.session.mode = Mode::Edit; + true + } + + pub fn commit_text_box_edit(&mut self) -> Result<(), String> { + let Some(ed) = self.text_box_editor.take() else { return Ok(()) }; + let result = if self.view_key() != Some((ed.uid, ed.sheet)) { + Err("The text box's worksheet is no longer active.".into()) + } else if ed.text == ed.original { + Ok(()) + } else { + // Call the engine directly: app.run flushes this draft before saving/switching. + self.session.run("shape.setText", json!({"id": ed.id, "text": ed.text})).map(|_| ()) + }; + match result { + Ok(()) => { + self.session.mode = Mode::Ready; + self.after_engine(); + Ok(()) + } + Err(e) => { + self.text_box_editor = Some(ed); + Err(e) + } + } + } + + pub fn cancel_text_box_edit(&mut self) { + self.text_box_editor = None; + self.session.mode = Mode::Ready; + } + + pub(crate) fn editing_text_box(&self, id: u32) -> bool { + self.text_box_editor.as_ref().is_some_and(|ed| ed.id == id && self.view_key() == Some((ed.uid, ed.sheet))) + } +} + +/// Resolve outside clicks before the ribbon/tabs can save or switch the workbook. +pub(crate) fn before_ui(app: &mut SheetApp, ctx: &egui::Context) { + let Some(ed) = &app.text_box_editor else { return }; + if ctx.input_mut(|i| i.consume_key(Modifiers::NONE, Key::Escape)) { + app.cancel_text_box_edit(); + ctx.memory_mut(|m| m.surrender_focus(editor_id())); + return; + } + let outside = ctx.input(|i| i.pointer.any_pressed() && i.pointer.interact_pos().is_some_and(|p| ed.rect.is_none_or(|r| !r.contains(p)))); + if outside { + match app.commit_text_box_edit() { + Ok(()) => ctx.memory_mut(|m| m.surrender_focus(editor_id())), + Err(e) => app.message = Some(("Text Box".into(), crate::clean_error(&e))), + } + } +} + +pub(crate) fn contains_pointer(app: &SheetApp, ctx: &egui::Context) -> bool { + app.text_box_editor.as_ref().and_then(|ed| ed.rect).is_some_and(|r| ctx.input(|i| i.pointer.hover_pos().is_some_and(|p| r.contains(p)))) +} + +pub(crate) fn show(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, sh: &gridcraft_engine::model::Sheet) { + let Some(mut ed) = app.text_box_editor.take() else { return }; + if app.view_key() != Some((ed.uid, ed.sheet)) { + app.text_box_editor = Some(ed); + return; + } + let Some(shape) = sh.shapes.iter().find(|s| s.id == ed.id && s.kind == ShapeKind::TextBox) else { + app.session.mode = Mode::Ready; + return; + }; + let save = ui.input_mut(|i| { + if i.consume_key(Modifiers::COMMAND | Modifiers::SHIFT, Key::S) { + Some("file.saveAs") + } else if i.consume_key(Modifiers::COMMAND, Key::S) { + Some("file.save") + } else { + None + } + }); + let rect = crate::chartview::anchor_rect(geo, sh, &shape.anchor).shrink(4.0).intersect(geo.cells); + if !rect.is_positive() { + ed.rect = None; + app.text_box_editor = Some(ed); + if let Some(command) = save { + app.run_or_alert(command, json!({})); + } + return; + } + ed.rect = Some(rect); + let id = editor_id(); + if ed.request_focus { + // A new editing session must not reuse the previous box's caret/undo history. + egui::text_edit::TextEditState::default().store(ui.ctx(), id); + } + let mut child = ui.new_child(egui::UiBuilder::new().id_salt(id).max_rect(rect)); + child.set_clip_rect(rect); + egui::ScrollArea::vertical().id_salt(id).max_height(rect.height()).auto_shrink([false, false]).show(&mut child, |ui| { + let response = ui.add( + TextEdit::multiline(&mut ed.text) + .id(id) + .font(FontId::new(14.0 * geo.z, egui::FontFamily::Name(theme::CELL.into()))) + .text_color(Color32::BLACK) + .frame(egui::Frame::NONE) + .margin(egui::Margin::ZERO) + .desired_width(rect.width()) + .desired_rows(1) + .min_size(rect.size()) + .lock_focus(true), + ); + if ed.request_focus { + response.request_focus(); + ed.request_focus = false; + } + }); + app.text_box_editor = Some(ed); + if let Some(command) = save { + app.run_or_alert(command, json!({})); + } +} diff --git a/crates/ui-egui/tests/text_box_editing.rs b/crates/ui-egui/tests/text_box_editing.rs new file mode 100644 index 0000000..64e4ccb --- /dev/null +++ b/crates/ui-egui/tests/text_box_editing.rs @@ -0,0 +1,230 @@ +//! Text-box editing through real pointer and keyboard input, including document boundaries. + +use egui::{Event, Key, Modifiers, PointerButton, Pos2, Rect, pos2, vec2}; +use gridcraft_engine::Session; +use gridcraft_engine::core::{CellRef, Value}; +use gridcraft_engine::model::{Shape, Workbook}; +use gridcraft_ui_egui::{SheetApp, grid::Geo}; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +const ORIGINAL: &str = "First line\nSecond line"; + +fn at(s: &str) -> CellRef { + CellRef::parse(s).unwrap() +} + +fn fixture() -> (Harness, u32) { + let mut session = Session::new(); + session.new_workbook(); + session.execute("cell.set", json!({"cell": "B2", "input": "cell stays"})).unwrap(); + session.execute("selection.set", json!({"cell": "B2"})).unwrap(); + let id = session.execute("insert.textBox", json!({"at": "B2", "width": 240, "height": 120, "text": ORIGINAL})).unwrap()["shape"].as_u64().unwrap() + as u32; + let mut h = egui_kittest::Harness::builder() + .with_size(vec2(1200.0, 800.0)) + .with_step_dt(1.0 / 60.0) + .with_os(egui::os::OperatingSystem::Mac) + .build_ui_state( + |ui, app: &mut SheetApp| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + SheetApp::new(session, Default::default()), + ); + h.run_steps(4); + (h, id) +} + +fn shape(h: &Harness, id: u32) -> &Shape { + h.state().session.doc().unwrap().wb.active().unwrap().shapes.iter().find(|s| s.id == id).unwrap() +} + +fn shape_text(wb: &Workbook, sheet: usize, id: u32) -> &str { + &wb.sheet(sheet).unwrap().shapes.iter().find(|s| s.id == id).unwrap().text +} + +fn shape_rect(h: &Harness, id: u32) -> Rect { + let app = h.state(); + let sh = app.session.doc().unwrap().wb.active().unwrap(); + let geo = Geo::new(sh, app.grid.rect.unwrap(), app.view().scroll); + let a = shape(h, id).anchor; + Rect::from_min_size(pos2(geo.x(sh, a.cell.col) + a.dx * geo.z, geo.y(sh, a.cell.row) + a.dy * geo.z), vec2(a.width, a.height) * geo.z) +} + +fn cell_center(h: &Harness, address: &str) -> Pos2 { + let app = h.state(); + let sh = app.session.doc().unwrap().wb.active().unwrap(); + Geo::new(sh, app.grid.rect.unwrap(), app.view().scroll).cell_rect(sh, at(address)).center() +} + +fn pointer(h: &mut Harness, p: Pos2, pressed: bool) { + h.input_mut().events.push(Event::PointerMoved(p)); + h.input_mut().events.push(Event::PointerButton { pos: p, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }); + h.step(); +} + +fn click(h: &mut Harness, p: Pos2) { + pointer(h, p, true); + pointer(h, p, false); + h.run_steps(2); +} + +fn edit_box(h: &mut Harness, id: u32) { + // Separate this gesture from egui's 600 ms triple-click window as well. + h.run_steps(45); + let p = shape_rect(h, id).center(); + click(h, p); + click(h, p); +} + +fn key(h: &mut Harness, key: Key, modifiers: Modifiers) { + h.input_mut().events.push(Event::ModifiersChanged(modifiers)); + h.input_mut().events.push(Event::Key { key, physical_key: None, pressed: true, repeat: false, modifiers }); + h.input_mut().events.push(Event::Key { key, physical_key: None, pressed: false, repeat: false, modifiers }); + h.step(); + h.input_mut().events.push(Event::ModifiersChanged(Modifiers::NONE)); + h.step(); +} + +fn event(h: &mut Harness, event: Event) { + h.input_mut().events.push(event); + h.run_steps(2); +} + +fn replace(h: &mut Harness, text: &str) { + key(h, Key::A, Modifiers::COMMAND); + event(h, Event::Paste(text.into())); +} + +fn assert_cell_unchanged(h: &Harness) { + assert_eq!(h.state().session.doc().unwrap().wb.active().unwrap().value(at("B2")), Value::text("cell stays")); + assert!(h.state().editor.is_none(), "text-box input must not start a cell editor"); +} + +#[test] +fn double_click_edits_multiline_text_and_commits_on_outside_click() { + let (mut h, id) = fixture(); + edit_box(&mut h, id); + assert_eq!(h.state().selected_chart, Some(id)); + let anchor = shape(&h, id).anchor; + let start = shape_rect(&h, id).min + vec2(5.0, 10.0); + drag(&mut h, start, start + vec2(60.0, 0.0)); + assert_eq!(shape(&h, id).anchor, anchor, "dragging inside the editor selects text instead of moving the box"); + h.input_mut().events.push(Event::Copy); + h.step(); + assert!(h.output().platform_output.commands.iter().any(|c| matches!(c, egui::OutputCommand::CopyText(text) if !text.is_empty()))); + key(&mut h, Key::A, Modifiers::COMMAND); + h.input_mut().events.push(Event::Copy); + h.step(); + assert!(h.output().platform_output.commands.iter().any(|c| matches!(c, egui::OutputCommand::CopyText(text) if text == ORIGINAL))); + event(&mut h, Event::Cut); + event(&mut h, Event::Paste("New first line".into())); + key(&mut h, Key::Enter, Modifiers::NONE); + event(&mut h, Event::Text("New second line".into())); + assert_cell_unchanged(&h); + assert_eq!(h.state().session.doc().unwrap().selection.active, at("B2")); + + let outside = cell_center(&h, "J15"); + click(&mut h, outside); + assert_eq!(shape(&h, id).text, "New first line\nNew second line"); + assert_cell_unchanged(&h); +} + +fn drag(h: &mut Harness, from: Pos2, to: Pos2) { + pointer(h, from, true); + // Cross egui's drag threshold while still inside the object/resize handle. + h.input_mut().events.push(Event::PointerMoved(from + vec2(5.0, 5.0))); + h.step(); + h.input_mut().events.push(Event::PointerMoved(to)); + h.step(); + pointer(h, to, false); + h.run_steps(2); +} + +#[test] +fn escape_discards_text_and_the_box_still_moves_and_resizes() { + let (mut h, id) = fixture(); + edit_box(&mut h, id); + replace(&mut h, "discard this edit"); + key(&mut h, Key::Escape, Modifiers::NONE); + assert_eq!(shape(&h, id).text, ORIGINAL); + assert_cell_unchanged(&h); + h.run_steps(30); + + let before = shape_rect(&h, id); + drag(&mut h, before.center(), before.center() + vec2(40.0, 30.0)); + let moved = shape_rect(&h, id); + assert!(moved.left() > before.left() && moved.top() > before.top(), "ordinary dragging still moves the box"); + assert!((moved.size() - before.size()).abs().max_elem() < 0.1); + + let handle = moved.right_bottom() - vec2(7.0, 7.0); + drag(&mut h, handle, handle + vec2(45.0, 25.0)); + let resized = shape_rect(&h, id); + assert!(resized.width() > moved.width() && resized.height() > moved.height(), "the corner still resizes the box"); + assert_eq!(shape(&h, id).text, ORIGINAL); + assert_cell_unchanged(&h); + + edit_box(&mut h, id); + replace(&mut h, "Offscreen draft"); + h.state_mut().view_mut().unwrap().scroll = vec2(900.0, 700.0); + h.run_steps(2); + assert_eq!(h.state().text_box_editor.as_ref().unwrap().text, "Offscreen draft"); + key(&mut h, Key::Escape, Modifiers::NONE); + assert_eq!(shape(&h, id).text, ORIGINAL); + assert_cell_unchanged(&h); +} + +#[test] +fn saving_and_switching_context_commit_to_the_original_text_box() { + let (mut h, id) = fixture(); + edit_box(&mut h, id); + replace(&mut h, "Saved draft\nSecond paragraph"); + h.state_mut().run("sheet.read", json!({})).unwrap(); + assert_eq!(shape(&h, id).text, ORIGINAL, "inspecting the sheet must not commit a draft"); + assert!(h.state().text_box_editor.is_some()); + let saved = h.state_mut().run("file.saveBytes", json!({"format": "xlsx"})).unwrap(); + let bytes = gridcraft_engine::io::base64_decode(saved["base64"].as_str().unwrap()).unwrap(); + let (reopened, _) = gridcraft_engine::io::open_bytes("edited.xlsx", &bytes).unwrap(); + assert_eq!(shape_text(&reopened, 0, id), "Saved draft\nSecond paragraph"); + assert_eq!(reopened.sheet(0).unwrap().value(at("B2")), Value::text("cell stays")); + + // Save and Save As must work while the editor still owns keyboard focus. + let downloaded = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let sink = downloaded.clone(); + h.state_mut().services.download = Some(Box::new(move |_, bytes| *sink.lock().unwrap() = bytes.to_vec())); + for modifiers in [Modifiers::COMMAND, Modifiers::COMMAND | Modifiers::SHIFT] { + edit_box(&mut h, id); + replace(&mut h, "Saved from the text editor"); + key(&mut h, Key::S, modifiers); + let (saved, _) = gridcraft_engine::io::open_bytes("shortcut.xlsx", &downloaded.lock().unwrap()).unwrap(); + assert_eq!(shape_text(&saved, 0, id), "Saved from the text editor"); + downloaded.lock().unwrap().clear(); + } + + h.state_mut().run("home.insertSheet", json!({"name": "Other"})).unwrap(); + let other_id = h.state_mut().run("insert.textBox", json!({"at": "B2", "text": "Other sheet"})).unwrap()["shape"].as_u64().unwrap() as u32; + h.state_mut().run("sheet.activate", json!({"sheet": 0})).unwrap(); + h.run_steps(3); + edit_box(&mut h, id); + replace(&mut h, "Original sheet only"); + h.state_mut().run("sheet.activate", json!({"sheet": 1})).unwrap(); + let wb = &h.state().session.doc().unwrap().wb; + assert_eq!(shape_text(wb, 0, id), "Original sheet only"); + assert_eq!(shape_text(wb, 1, other_id), "Other sheet"); + + h.state_mut().run("file.new", json!({})).unwrap(); + let other_doc_id = h.state_mut().run("insert.textBox", json!({"at": "B2", "text": "Other workbook"})).unwrap()["shape"].as_u64().unwrap() as u32; + h.state_mut().run("window.activate", json!({"index": 0})).unwrap(); + h.state_mut().run("sheet.activate", json!({"sheet": 0})).unwrap(); + h.run_steps(3); + edit_box(&mut h, id); + replace(&mut h, "Original workbook only"); + h.state_mut().run("window.activate", json!({"index": 1})).unwrap(); + let docs = h.state().session.documents(); + assert_eq!(shape_text(&docs[0].wb, 0, id), "Original workbook only"); + assert_eq!(shape_text(&docs[1].wb, 0, other_doc_id), "Other workbook"); + assert_eq!(docs[0].wb.sheet(0).unwrap().value(at("B2")), Value::text("cell stays")); +} From c67931cff650ac9a3ae62a3328a3aa8abe0ce9db Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sat, 10 Oct 2026 12:23:51 -0400 Subject: [PATCH 2/2] Review fix-up: protect and cap text box edits shape.setText refuses edits on a protected sheet (drawing objects are locked; there is no edit-objects allowance yet) and text longer than 32,767 characters, like a cell. The editor doesn't open on a protected sheet and its TextEdit carries the same character limit. Tests for both. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/insert.rs | 13 +++++++++- crates/engine/src/tests.rs | 30 ++++++++++++++++++++++++ crates/ui-egui/src/text_box.rs | 10 +++++++- crates/ui-egui/tests/text_box_editing.rs | 11 +++++++++ 4 files changed, 62 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index d2f0e35..9c383fa 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -777,16 +777,27 @@ fn insert_shape(s: &mut Session, p: &Json) -> Result { }) } +/// A text box holds at most as many characters as a cell. +const SHAPE_TEXT_MAX_CHARS: usize = 32_767; + fn shape_set_text(s: &mut Session, p: &Json) -> Result { let id = p.get("id").and_then(Json::as_u64).and_then(|id| u32::try_from(id).ok()).ok_or_else(|| bad("shape.setText", "`id` must be a text box id"))?; let text = str_param(p, "text").ok_or_else(|| bad("shape.setText", "`text` must be a string"))?; + if text.chars().nth(SHAPE_TEXT_MAX_CHARS).is_some() { + return Err(bad("shape.setText", "`text` is longer than 32,767 characters")); + } let d = s.doc()?; let sheet = d.wb.active_sheet; - let shape = d.wb.active().and_then(|sh| sh.shapes.iter().find(|shape| shape.id == id)).ok_or_else(|| bad("shape.setText", "no such text box"))?; + let sh = d.wb.active().ok_or_else(|| bad("shape.setText", "no such text box"))?; + let shape = sh.shapes.iter().find(|shape| shape.id == id).ok_or_else(|| bad("shape.setText", "no such text box"))?; if shape.kind != ShapeKind::TextBox { return Err(bad("shape.setText", "the shape is not a text box")); } + // Sheet protection locks drawing objects (there is no "edit objects" allowance yet). + if sh.is_protected() { + return Err(EngineError::Other(PROTECTED.into())); + } if shape.text == text { return ok(); } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 5ecaea7..b6111a2 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -168,6 +168,36 @@ fn shape_text_rejects_invalid_targets_and_params() { assert!(s.doc().unwrap().wb.active().unwrap().shapes.is_empty()); } +#[test] +fn shape_text_respects_sheet_protection() { + let mut s = s(); + let id = s.execute("insert.textBox", json!({"text": "Original"})).unwrap()["shape"].clone(); + s.execute("review.protectSheet", json!({})).unwrap(); + let before = s.doc().unwrap().wb.clone(); + let undo_len = s.doc().unwrap().undo.len(); + assert!(s.execute("shape.setText", json!({"id": id, "text": "Changed"})).is_err()); + assert!(std::sync::Arc::ptr_eq(&s.doc().unwrap().wb, &before)); + assert_eq!(s.doc().unwrap().undo.len(), undo_len); + s.execute("review.unprotectSheet", json!({})).unwrap(); + s.execute("shape.setText", json!({"id": id, "text": "Changed"})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().shapes[0].text, "Changed"); +} + +#[test] +fn shape_text_is_capped_like_a_cell() { + let mut s = s(); + let id = s.execute("insert.textBox", json!({"text": "Original"})).unwrap()["shape"].clone(); + // The cap counts characters, not bytes. + let longest = "é".repeat(32_767); + s.execute("shape.setText", json!({"id": id, "text": longest})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().shapes[0].text.chars().count(), 32_767); + let undo_len = s.doc().unwrap().undo.len(); + let too_long = "x".repeat(32_768); + assert!(matches!(s.execute("shape.setText", json!({"id": id, "text": too_long})), Err(crate::EngineError::BadParams { .. }))); + assert_eq!(s.doc().unwrap().wb.active().unwrap().shapes[0].text.chars().count(), 32_767); + assert_eq!(s.doc().unwrap().undo.len(), undo_len); +} + #[test] fn shape_text_edit_survives_xlsx_roundtrip() { let mut s = s(); diff --git a/crates/ui-egui/src/text_box.rs b/crates/ui-egui/src/text_box.rs index f9d8dec..1e83f62 100644 --- a/crates/ui-egui/src/text_box.rs +++ b/crates/ui-egui/src/text_box.rs @@ -31,9 +31,15 @@ impl SheetApp { return false; } let Some(d) = self.session.active() else { return false }; - let Some(shape) = d.wb.active().and_then(|sh| sh.shapes.iter().find(|s| s.id == id && s.kind == ShapeKind::TextBox)) else { + let Some(sh) = d.wb.active() else { return false }; + let Some(shape) = sh.shapes.iter().find(|s| s.id == id && s.kind == ShapeKind::TextBox) else { return false; }; + // shape.setText refuses edits on a protected sheet; don't open an editor that can't commit. + if sh.is_protected() { + self.message = Some(("Text Box".into(), "The cell or chart you're trying to change is on a protected sheet.".into())); + return false; + } self.text_box_editor = Some(EditState { id, text: shape.text.clone(), @@ -150,6 +156,8 @@ pub(crate) fn show(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, sh: &gridcr .margin(egui::Margin::ZERO) .desired_width(rect.width()) .desired_rows(1) + // shape.setText's limit (a cell's). + .char_limit(32_767) .min_size(rect.size()) .lock_focus(true), ); diff --git a/crates/ui-egui/tests/text_box_editing.rs b/crates/ui-egui/tests/text_box_editing.rs index 64e4ccb..1d3b053 100644 --- a/crates/ui-egui/tests/text_box_editing.rs +++ b/crates/ui-egui/tests/text_box_editing.rs @@ -228,3 +228,14 @@ fn saving_and_switching_context_commit_to_the_original_text_box() { assert_eq!(shape_text(&docs[1].wb, 0, other_doc_id), "Other workbook"); assert_eq!(docs[0].wb.sheet(0).unwrap().value(at("B2")), Value::text("cell stays")); } + +#[test] +fn protected_sheet_refuses_to_open_the_text_box_editor() { + let (mut h, id) = fixture(); + h.state_mut().session.execute("review.protectSheet", json!({})).unwrap(); + edit_box(&mut h, id); + assert!(h.state().text_box_editor.is_none(), "a protected sheet's text box must not open an editor"); + assert!(h.state().message.as_ref().is_some_and(|(_, m)| m.contains("protected sheet"))); + assert_eq!(shape(&h, id).text, ORIGINAL); + assert_cell_unchanged(&h); +}