mirror of
https://github.com/storytold/gridcraft.git
synced 2026-10-26 11:46:49 -06:00
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
c5a8cf28fe
commit
c67931cff6
@@ -777,16 +777,27 @@ fn insert_shape(s: &mut Session, p: &Json) -> Result<Json> {
|
||||
})
|
||||
}
|
||||
|
||||
/// 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<Json> {
|
||||
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();
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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),
|
||||
);
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user