From 3e6eac4222ec14d399c6ec84cc060cc33a57b1a8 Mon Sep 17 00:00:00 2001 From: Fubeo <83821420+Fubeo@users.noreply.github.com> Date: Sun, 25 Oct 2026 01:31:19 +0200 Subject: [PATCH] fix(engine): editing a note or comment keeps its thread Writing a note or comment on a cell that already has one of the same kind replaced the whole thread: Edit Comment lost the replies, the resolved state and the dates, and Edit Note the box read from the file. Now only the text (and the author, when the command names one) changes, as Excel's Edit Note does; a note written over a comment still replaces it. --- crates/engine/src/cmd/insert.rs | 15 +++++++++++++-- crates/engine/src/tests.rs | 29 +++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 2388d1f25..f7cbc5ab0 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -1316,9 +1316,20 @@ fn comment(s: &mut Session, p: &Json, threaded: bool) -> Result { let sheet = target_sheet(s, p)?; let at = cell_param_on(s, p, "cell", sheet)?.unwrap_or(s.doc()?.selection.active); let text = str_param(p, "text").unwrap_or("").to_string(); - let author = str_param(p, "author").map(str::to_string).unwrap_or_else(|| s.prefs.user_name.clone()); + let named = str_param(p, "author").map(str::to_string); + let author = named.clone().unwrap_or_else(|| s.prefs.user_name.clone()); edit(s, |cx| { - cx.sheet_mut(sheet)?.comments.insert( + let comments = &mut cx.sheet_mut(sheet)?.comments; + // Writing a note on a cell that has one edits it, as Excel's Edit Note does: the box, the + // replies, the dates and who wrote it stay. A note replaces a comment and vice versa. + if let Some(c) = comments.get_mut(&at).filter(|c| c.threaded == threaded) { + c.text = text.clone(); + if let Some(author) = named.clone() { + c.author = author; + } + return Ok(json!({"cell": at.a1()})); + } + comments.insert( at, Comment { author: author.clone(), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index f4c47bb62..63f96962b 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3659,6 +3659,35 @@ fn notes_comments_and_links_survive_a_json_save() { assert_eq!(after.cells, before.cells); } +/// Writing a note or a comment on a cell that has one of the same kind edits its text: the +/// replies, the resolved state, the dates and the note's box stay. `comment` replaced the whole +/// thread, so editing a comment dropped its replies. +#[test] +fn editing_a_note_or_a_comment_keeps_its_thread() { + let mut s = s(); + s.execute("review.newComment", json!({"cell": "B2", "text": "first", "author": "Ada"})).unwrap(); + s.execute("review.replyComment", json!({"cell": "B2", "text": "reply", "author": "Bob"})).unwrap(); + s.execute("review.resolveComment", json!({"cell": "B2", "resolved": true})).unwrap(); + s.execute("review.newComment", json!({"cell": "B2", "text": "edited"})).unwrap(); + s.execute("review.newNote", json!({"cell": "A1", "text": "note", "author": "Ada"})).unwrap(); + s.execute("review.showNote", json!({"cell": "A1"})).unwrap(); + s.execute("review.newNote", json!({"cell": "A1", "text": "changed"})).unwrap(); + let d = s.doc().unwrap(); + let sh = d.wb.active().unwrap(); + let thread = &sh.comments[&CellRef::parse("B2").unwrap()]; + assert_eq!((thread.author.as_str(), thread.text.as_str()), ("Ada", "edited")); + assert_eq!(thread.replies, vec![("Bob".to_string(), "reply".to_string())]); + assert!(thread.resolved && thread.threaded); + let note = &sh.comments[&CellRef::parse("A1").unwrap()]; + assert_eq!((note.author.as_str(), note.text.as_str()), ("Ada", "changed")); + assert!(note.visible && !note.threaded); + // A note written over a comment replaces it. + s.execute("review.newNote", json!({"cell": "B2", "text": "plain"})).unwrap(); + let d = s.doc().unwrap(); + let replaced = &d.wb.active().unwrap().comments[&CellRef::parse("B2").unwrap()]; + assert!(!replaced.threaded && replaced.replies.is_empty()); +} + /// A cut moves the block's notes, hyperlinks and merged areas with it. The source's cells and /// pictures were removed but the block's merges and notes were not, so one merged block became /// two (an empty one left at the old place) and a note ended up on both the old and the new