Panics · P21 Documents: stale block keys, cells and kinds change nothing; replaced text drops visual presses; nested images, versions and find hits hold

src/documents/blocks/content.rs

@@ -61,7 +61,12 @@ impl BlockEditor {
                 let check = Checkbox::new((id.clone(), format!("todo-{key}")), done).on_change(
                     move |on, _, cx| {
                         entity.update(cx, |editor, cx| {
-                            editor.set_kind(key, BlockKind::Todo(on), cx)
+                            if editor
+                                .kind_of(key)
+                                .is_some_and(|kind| matches!(kind, BlockKind::Todo(_)))
+                            {
+                                editor.set_kind(key, BlockKind::Todo(on), cx)
+                            }
                         })
                     },
                 );
@@ -106,7 +111,9 @@ impl BlockEditor {
                     .on_mouse_down(MouseButton::Left, |_, window, _| window.prevent_default())
                     .on_click(move |_, _, cx| {
                         entity.update(cx, |editor, cx| {
-                            editor.set_kind(key, BlockKind::Toggle(!open), cx)
+                            if editor.kind_of(key) == Some(&BlockKind::Toggle(open)) {
+                                editor.set_kind(key, BlockKind::Toggle(!open), cx)
+                            }
                         })
                     })
                     .child(Disclosure::new(

src/documents/blocks/edit.rs

@@ -7,7 +7,9 @@ use super::{Block, BlockData, BlockEditor, BlockEvent, BlockKind, Snapshot};
 impl BlockEditor {
     /// Splits a prose block at the caret: the text after it moves to a new block of the kind that follows.
     pub(crate) fn split(&mut self, key: u64, window: &mut Window, cx: &mut Context<Self>) {
-        let ix = self.index(key);
+        let Some(ix) = self.index(key) else {
+            return;
+        };
         let kind = self.blocks[ix].kind.clone();
         let field = self.blocks[ix].fields[0].clone();
         if field.read(cx).is_empty()
@@ -43,7 +45,9 @@ impl BlockEditor {
 
     /// Backspace at a prose block's start: another kind turns back into text; text joins the prose above, or an empty block goes.
     pub(crate) fn join(&mut self, key: u64, window: &mut Window, cx: &mut Context<Self>) {
-        let ix = self.index(key);
+        let Some(ix) = self.index(key) else {
+            return;
+        };
         if self.blocks[ix].kind != BlockKind::Paragraph {
             self.turn_into(key, BlockKind::Paragraph, window, cx);
             return;

src/documents/blocks/keys.rs

@@ -9,6 +9,35 @@ fn paragraph() -> BlockData {
 }
 
 impl BlockEditor {
+    /// Where `key` sits, logged and none when the block has left.
+    pub(super) fn found(&self, key: u64, what: &str) -> Option<usize> {
+        let at = self.blocks.iter().position(|block| block.key == key);
+        if at.is_none() {
+            log::error!("block editor: no block {key} to {what}");
+        }
+        at
+    }
+
+    /// Where `key` sits now; a listener painted for a block since gone finds none.
+    pub(crate) fn index(&self, key: u64) -> Option<usize> {
+        self.found(key, "act on")
+    }
+
+    /// What `key` is now; a box painted for it acts only while it still is that.
+    pub(crate) fn kind_of(&self, key: u64) -> Option<&BlockKind> {
+        Some(&self.blocks[self.index(key)?].kind)
+    }
+
+    /// The block holding `key`'s field `field` now, when it still has that field.
+    pub(crate) fn place(&self, key: u64, field: usize) -> Option<usize> {
+        let ix = self.index(key)?;
+        let held = field < self.blocks[ix].fields.len();
+        if !held {
+            log::error!("block editor: block {key} has no field {field} now");
+        }
+        held.then_some(ix)
+    }
+
     /// The fields in reading order, as `(block, field)`: a closed toggle hides its body.
     fn reading_order(&self) -> Vec<(u64, usize)> {
         self.blocks
@@ -40,7 +69,10 @@ impl BlockEditor {
         window: &mut Window,
         cx: &mut Context<Self>,
     ) -> bool {
-        let kind = self.blocks[self.index(key)].kind.clone();
+        let Some(ix) = self.place(key, field) else {
+            return true;
+        };
+        let kind = self.blocks[ix].kind.clone();
         match kind {
             kind if kind.is_prose() => self.split(key, window, cx),
             BlockKind::Toggle(open) if field == 0 => {
@@ -54,7 +86,7 @@ impl BlockEditor {
             }
             BlockKind::Table { columns, merged } => {
                 let below = field + columns;
-                if below >= self.blocks[self.index(key)].fields.len() {
+                if below >= self.blocks[ix].fields.len() {
                     self.add_row(key, window, cx);
                 }
                 let shown = shown_cell(&merged, columns, below);
@@ -73,7 +105,10 @@ impl BlockEditor {
         window: &mut Window,
         cx: &mut Context<Self>,
     ) -> bool {
-        let block = &self.blocks[self.index(key)];
+        let Some(ix) = self.place(key, field) else {
+            return true;
+        };
+        let block = &self.blocks[ix];
         let input = block.fields[field].read(cx);
         if input.cursor() != 0 || !input.selection().is_empty() {
             return false;
@@ -101,7 +136,10 @@ impl BlockEditor {
         window: &mut Window,
         cx: &mut Context<Self>,
     ) -> bool {
-        let block = &self.blocks[self.index(key)];
+        let Some(ix) = self.place(key, field) else {
+            return true;
+        };
+        let block = &self.blocks[ix];
         if !at_edge(block.fields[field].read(cx), down) {
             return false;
         }
@@ -118,10 +156,10 @@ impl BlockEditor {
             }
         }
         let order = self.reading_order();
-        let at = order
-            .iter()
-            .position(|place| *place == (key, field))
-            .expect("a focused field is in reading order");
+        let Some(at) = order.iter().position(|place| *place == (key, field)) else {
+            log::error!("block editor: field {field} of block {key} is not shown now");
+            return true;
+        };
         let target = if down {
             order[at + 1..].iter().find(|(other, _)| *other != key)
         } else {
@@ -134,7 +172,10 @@ impl BlockEditor {
             }
             return false;
         };
-        let end = self.blocks[self.index(target)].fields[target_field]
+        let Some(found) = self.index(target) else {
+            return true;
+        };
+        let end = self.blocks[found].fields[target_field]
             .read(cx)
             .text()
             .len();

src/documents/blocks/media.rs

@@ -43,28 +43,44 @@ pub(crate) fn share(align: Align, column: Bounds<Pixels>, x: Pixels) -> f32 {
     (reach / width).clamp(NARROWEST, 1.0)
 }
 
-fn media_of(kind: &mut BlockKind) -> &mut Media {
+/// A media block's media; a block turned into something else since its controls painted has none.
+fn media_of(kind: &mut BlockKind) -> Option<&mut Media> {
     match kind {
-        BlockKind::Image(media) | BlockKind::Video(media) | BlockKind::Embed(media) => media,
-        other => unreachable!("{} holds no media", other.label()),
+        BlockKind::Image(media) | BlockKind::Video(media) | BlockKind::Embed(media) => Some(media),
+        other => {
+            log::error!("block editor: a {} block holds no media now", other.label());
+            None
+        }
     }
 }
 
 impl BlockEditor {
     /// Sets a picture's width while its edge is dragged; the drag's start records undo.
-    fn resize(&mut self, key: u64, width: f32, first: bool, cx: &mut Context<Self>) {
+    pub(super) fn resize(&mut self, key: u64, width: f32, first: bool, cx: &mut Context<Self>) {
+        let Some(ix) = self.index(key) else {
+            return;
+        };
+        let mut kind = self.blocks[ix].kind.clone();
+        let Some(media) = media_of(&mut kind) else {
+            return;
+        };
+        media.width = width;
         if first {
             self.before_change(cx);
         }
-        let ix = self.index(key);
-        media_of(&mut self.blocks[ix].kind).width = width;
+        self.blocks[ix].kind = kind;
         self.after_change(cx);
     }
 
     pub(crate) fn align(&mut self, key: u64, align: Align, cx: &mut Context<Self>) {
-        let ix = self.index(key);
+        let Some(ix) = self.index(key) else {
+            return;
+        };
         let mut kind = self.blocks[ix].kind.clone();
-        media_of(&mut kind).align = align;
+        let Some(media) = media_of(&mut kind) else {
+            return;
+        };
+        media.align = align;
         self.set_kind(key, kind, cx);
     }
 }

src/documents/blocks/mod.rs

@@ -228,7 +228,9 @@ impl BlockEditor {
         window: &mut Window,
         cx: &mut Context<Self>,
     ) {
-        let ix = self.index(key);
+        let Some(ix) = self.index(key) else {
+            return;
+        };
         let field = Self::field(&self.blocks[ix].kind, "", window, cx);
         let watched = Self::watch(key, &field, window, cx);
         let block = &mut self.blocks[ix];
@@ -238,19 +240,14 @@ impl BlockEditor {
     }
 
     pub(crate) fn remove_field(&mut self, key: u64, field: usize) {
-        let ix = self.index(key);
+        let Some(ix) = self.index(key) else {
+            return;
+        };
         let block = &mut self.blocks[ix];
         block.fields.remove(field);
         drop(block._changes.remove(field));
     }
 
-    pub(crate) fn index(&self, key: u64) -> usize {
-        self.blocks
-            .iter()
-            .position(|block| block.key == key)
-            .unwrap_or_else(|| panic!("block {key} left the editor"))
-    }
-
     fn on_field(
         &mut self,
         key: u64,
@@ -259,7 +256,9 @@ impl BlockEditor {
         window: &mut Window,
         cx: &mut Context<Self>,
     ) {
-        let ix = self.index(key);
+        let Some(ix) = self.index(key) else {
+            return;
+        };
         match event {
             InputEvent::Changed => {
                 let typed = field.focus_handle(cx).is_focused(window);
@@ -342,8 +341,10 @@ impl BlockEditor {
         window: &mut Window,
         cx: &mut App,
     ) {
-        let block = &self.blocks[self.index(key)];
-        let Some(field) = block.fields.get(field) else {
+        let Some(ix) = self.index(key) else {
+            return;
+        };
+        let Some(field) = self.blocks[ix].fields.get(field) else {
             return;
         };
         if let Some(at) = at {
@@ -360,10 +361,15 @@ impl BlockEditor {
         window: &mut Window,
         cx: &mut Context<Self>,
     ) -> u64 {
+        let at = match after {
+            None => 0,
+            Some(after) => self
+                .found(after, "insert after")
+                .map_or(self.blocks.len(), |ix| ix + 1),
+        };
         self.before_change(cx);
         let key = self.fresh_key();
         let block = self.build(data, key, window, cx);
-        let at = after.map_or(0, |after| self.index(after) + 1);
         log::info!("block editor: {} block at {at}", block.kind.label());
         let end = block
             .fields
@@ -383,8 +389,10 @@ impl BlockEditor {
         window: &mut Window,
         cx: &mut Context<Self>,
     ) {
+        let Some(ix) = self.found(key, "turn") else {
+            return;
+        };
         self.before_change(cx);
-        let ix = self.index(key);
         let first = self.blocks[ix]
             .fields
             .first()
@@ -403,7 +411,9 @@ impl BlockEditor {
 
     /// Changes a block's kind in place, as a to-do's box or a toggle's state does.
     pub fn set_kind(&mut self, key: u64, kind: BlockKind, cx: &mut Context<Self>) {
-        let ix = self.index(key);
+        let Some(ix) = self.found(key, "set the kind of") else {
+            return;
+        };
         assert!(
             kind.accepts(self.blocks[ix].fields.len()),
             "set_kind keeps the fields; turn_into changes them"
@@ -414,8 +424,10 @@ impl BlockEditor {
     }
 
     pub fn remove(&mut self, key: u64, window: &mut Window, cx: &mut Context<Self>) {
+        let Some(ix) = self.found(key, "remove") else {
+            return;
+        };
         self.before_change(cx);
-        let ix = self.index(key);
         log::info!("block editor: block {key} removed");
         self.blocks.remove(ix);
         self.after_change(cx);
@@ -430,13 +442,22 @@ impl BlockEditor {
     }
 
     pub fn duplicate(&mut self, key: u64, window: &mut Window, cx: &mut Context<Self>) {
-        let ix = self.index(key);
+        let Some(ix) = self.found(key, "duplicate") else {
+            return;
+        };
         let data = self.snapshot(cx).blocks[ix].1.clone();
         self.insert(Some(key), data, window, cx);
     }
 
     /// Moves the block at `from` to place `to`.
     pub fn move_block(&mut self, from: usize, to: usize, cx: &mut Context<Self>) {
+        if from >= self.blocks.len() {
+            log::error!(
+                "block editor: no block at {from} of {} to move",
+                self.blocks.len()
+            );
+            return;
+        }
         self.before_change(cx);
         log::info!("block editor: block moved from {from} to {to}");
         let block = self.blocks.remove(from);

src/documents/blocks/table.rs

@@ -10,12 +10,16 @@ use crate::{
     theme::ControlSize,
 };
 
-/// A table's shape: columns, rows, and the cells that fold into their left neighbor.
-fn shape(kind: &BlockKind, fields: usize) -> (usize, usize, &[(usize, usize)]) {
+/// Columns, rows, and the cells folded into their left neighbor.
+type Shape<'a> = (usize, usize, &'a [(usize, usize)]);
+
+/// A table's shape; none once the block is no table.
+fn shape(kind: &BlockKind, fields: usize) -> Option<Shape<'_>> {
     let BlockKind::Table { columns, merged } = kind else {
-        unreachable!("a table block is a table")
+        log::error!("block editor: a {} block is no table now", kind.label());
+        return None;
     };
-    (*columns, fields / columns, merged)
+    Some((*columns, fields / columns, merged))
 }
 
 /// How many columns the cell at `(row, column)` spans: itself and every cell folded into it.
@@ -55,8 +59,13 @@ pub(crate) fn without_row(merged: &[(usize, usize)], row: usize) -> Vec<(usize,
 impl BlockEditor {
     /// Adds a row of empty cells at the table's foot.
     pub(crate) fn add_row(&mut self, key: u64, window: &mut Window, cx: &mut Context<Self>) {
-        let ix = self.index(key);
-        let (columns, _, _) = shape(&self.blocks[ix].kind, self.blocks[ix].fields.len());
+        let Some(ix) = self.index(key) else {
+            return;
+        };
+        let Some((columns, _, _)) = shape(&self.blocks[ix].kind, self.blocks[ix].fields.len())
+        else {
+            return;
+        };
         self.before_change(cx);
         for _ in 0..columns {
             self.add_field(key, None, window, cx);
@@ -67,8 +76,14 @@ impl BlockEditor {
 
     /// Adds a column of empty cells at the table's right.
     pub(crate) fn add_column(&mut self, key: u64, window: &mut Window, cx: &mut Context<Self>) {
-        let ix = self.index(key);
-        let (columns, rows, merged) = shape(&self.blocks[ix].kind, self.blocks[ix].fields.len());
+        let Some(ix) = self.index(key) else {
+            return;
+        };
+        let Some((columns, rows, merged)) =
+            shape(&self.blocks[ix].kind, self.blocks[ix].fields.len())
+        else {
+            return;
+        };
         let merged = merged.to_vec();
         self.before_change(cx);
         for row in (0..rows).rev() {
@@ -90,8 +105,18 @@ impl BlockEditor {
         column: bool,
         cx: &mut Context<Self>,
     ) {
-        let ix = self.index(key);
-        let (columns, rows, merged) = shape(&self.blocks[ix].kind, self.blocks[ix].fields.len());
+        let Some(ix) = self.index(key) else {
+            return;
+        };
+        let Some((columns, rows, merged)) =
+            shape(&self.blocks[ix].kind, self.blocks[ix].fields.len())
+        else {
+            return;
+        };
+        if cell >= self.blocks[ix].fields.len() {
+            log::error!("table {key}: cell {cell} left the table");
+            return;
+        }
         let merged = merged.to_vec();
         let (row, at) = (cell / columns, cell % columns);
         if (column && columns == 1) || (!column && rows <= 2) {
@@ -129,8 +154,17 @@ impl BlockEditor {
 
     /// Folds the cell right of `cell` into it, moving its text over; or, when already merged, parts them again.
     pub(crate) fn merge_right(&mut self, key: u64, cell: usize, cx: &mut Context<Self>) {
-        let ix = self.index(key);
-        let (columns, _, merged) = shape(&self.blocks[ix].kind, self.blocks[ix].fields.len());
+        let Some(ix) = self.index(key) else {
+            return;
+        };
+        let Some((columns, _, merged)) = shape(&self.blocks[ix].kind, self.blocks[ix].fields.len())
+        else {
+            return;
+        };
+        if cell >= self.blocks[ix].fields.len() {
+            log::error!("table {key}: cell {cell} left the table");
+            return;
+        }
         let (row, column) = (cell / columns, cell % columns);
         let spanned = span(merged, columns, row, column);
         if column + spanned >= columns && spanned == 1 {
@@ -168,7 +202,8 @@ pub(super) fn table(
     window: &mut Window,
     cx: &mut Context<BlockEditor>,
 ) -> AnyElement {
-    let (columns, rows, merged) = shape(&block.kind, block.fields.len());
+    let (columns, rows, merged) =
+        shape(&block.kind, block.fields.len()).expect("a table block draws as a table");
     let merged = merged.to_vec();
     let (key, id) = (block.key, BlockEditor::id(cx));
     let focused = editor

src/documents/blocks/tests/mod.rs

@@ -3,6 +3,8 @@ use gpui::{AppContext as _, Entity, Focusable, TestAppContext, VisualTestContext
 use super::{BlockData, BlockEditor, BlockKind};
 use crate::{forms, theme::Theme};
 
+mod stale;
+
 fn open(
     blocks: Vec<BlockData>,
     cx: &mut TestAppContext,

src/documents/blocks/tests/stale.rs

@@ -0,0 +1,141 @@
+use gpui::{Focusable, TestAppContext};
+
+use super::{blocks, caret, open, settle, text};
+use crate::{
+    documents::blocks::{Align, BlockData, BlockKind, Media},
+    forms,
+};
+
+#[gpui::test]
+fn a_key_the_user_deleted_changes_nothing(cx: &mut TestAppContext) {
+    let paragraph = |text: &str| BlockData::new(BlockKind::Paragraph, [text]);
+    let (editor, cx) = open(vec![paragraph("one"), paragraph("two")], cx);
+    cx.update(|window, cx| {
+        editor.update(cx, |editor, cx| {
+            let gone = 999;
+            editor.set_kind(gone, BlockKind::Bullet, cx);
+            editor.turn_into(gone, BlockKind::Bullet, window, cx);
+            editor.remove(gone, window, cx);
+            editor.duplicate(gone, window, cx);
+            editor.move_block(9, 0, cx);
+            editor.insert(Some(gone), paragraph("three"), window, cx);
+        })
+    });
+    let texts: Vec<String> = editor.read_with(cx, |editor, cx| {
+        editor
+            .blocks(cx)
+            .into_iter()
+            .map(|block| block.texts[0].clone())
+            .collect()
+    });
+    assert_eq!(
+        texts,
+        ["one", "two", "three"],
+        "an insert after a gone block lands at the end"
+    );
+}
+
+#[gpui::test]
+fn keys_in_a_field_whose_block_left_change_nothing(cx: &mut TestAppContext) {
+    let paragraph = |text: &str| BlockData::new(BlockKind::Paragraph, [text]);
+    let (editor, cx) = open(vec![paragraph("one"), paragraph("two")], cx);
+    caret(&editor, 0, 0, cx);
+    cx.update(|window, cx| {
+        let key = editor.read(cx).blocks[0].key;
+        editor.update(cx, |editor, cx| editor.remove(key, window, cx));
+        window.dispatch_action(Box::new(forms::Enter), cx);
+    });
+    settle(cx);
+    assert_eq!(blocks(&editor, cx), [text(BlockKind::Paragraph, "two")]);
+}
+
+#[gpui::test]
+fn up_from_a_body_folded_away_changes_nothing(cx: &mut TestAppContext) {
+    let toggle = BlockData::new(BlockKind::Toggle(true), ["title", "body"]);
+    let (editor, cx) = open(vec![toggle], cx);
+    cx.update(|window, cx| {
+        let body = editor.read(cx).blocks[0].fields[1].clone();
+        window.focus(&body.focus_handle(cx), cx);
+    });
+    settle(cx);
+    cx.update(|window, cx| {
+        let key = editor.read(cx).blocks[0].key;
+        editor.update(cx, |editor, cx| {
+            editor.set_kind(key, BlockKind::Toggle(false), cx)
+        });
+        window.dispatch_action(Box::new(forms::Up), cx);
+    });
+    settle(cx);
+    assert_eq!(blocks(&editor, cx)[0].0, BlockKind::Toggle(false));
+}
+
+#[gpui::test]
+fn table_and_media_tools_on_a_block_turned_paragraph_change_nothing(cx: &mut TestAppContext) {
+    let table = BlockKind::Table {
+        columns: 2,
+        merged: vec![],
+    };
+    let image = BlockKind::Image(Media::new("missing.png"));
+    let (editor, cx) = open(
+        vec![
+            BlockData::new(table, ["h1", "h2", "a", "b"]),
+            BlockData::new(image, ["caption"]),
+        ],
+        cx,
+    );
+    cx.update(|window, cx| {
+        editor.update(cx, |editor, cx| {
+            let (table, image) = (editor.blocks[0].key, editor.blocks[1].key);
+            editor.turn_into(table, BlockKind::Paragraph, window, cx);
+            editor.turn_into(image, BlockKind::Paragraph, window, cx);
+            editor.add_row(table, window, cx);
+            editor.add_column(table, window, cx);
+            editor.remove_line(table, 0, true, cx);
+            editor.merge_right(table, 0, cx);
+            editor.align(image, Align::Left, cx);
+        })
+    });
+    settle(cx);
+    let kinds: Vec<BlockKind> = blocks(&editor, cx)
+        .into_iter()
+        .map(|(kind, _)| kind)
+        .collect();
+    assert_eq!(kinds, [BlockKind::Paragraph, BlockKind::Paragraph]);
+}
+
+#[gpui::test]
+fn a_cell_whose_row_left_is_not_removed_or_merged_again(cx: &mut TestAppContext) {
+    let table = BlockKind::Table {
+        columns: 2,
+        merged: vec![],
+    };
+    let cells = ["h1", "h2", "a", "b", "c", "d", "e", "f"];
+    let (editor, cx) = open(vec![BlockData::new(table, cells)], cx);
+    cx.update(|_, cx| {
+        editor.update(cx, |editor, cx| {
+            let key = editor.blocks[0].key;
+            editor.remove_line(key, 6, false, cx);
+            editor.remove_line(key, 6, false, cx);
+            editor.merge_right(key, 6, cx);
+        })
+    });
+    settle(cx);
+    let texts: Vec<String> = blocks(&editor, cx).remove(0).1;
+    assert_eq!(texts, ["h1", "h2", "a", "b", "c", "d"]);
+}
+
+#[gpui::test]
+fn a_drag_on_a_picture_turned_paragraph_leaves_undo_alone(cx: &mut TestAppContext) {
+    let image = BlockKind::Image(Media::new("missing.png"));
+    let (editor, cx) = open(vec![BlockData::new(image.clone(), ["caption"])], cx);
+    cx.update(|window, cx| {
+        editor.update(cx, |editor, cx| {
+            let key = editor.blocks[0].key;
+            editor.turn_into(key, BlockKind::Paragraph, window, cx);
+            editor.resize(key, 0.5, true, cx);
+            editor.undo(window, cx);
+        })
+    });
+    settle(cx);
+    assert_eq!(blocks(&editor, cx)[0].0, image);
+}

src/documents/blocks/view.rs

@@ -54,7 +54,9 @@ impl BlockEditor {
         window: &mut Window,
         cx: &mut Context<Self>,
     ) {
-        let ix = self.index(key);
+        let Some(ix) = self.index(key) else {
+            return;
+        };
         let empty = self.blocks[ix]
             .fields
             .first()
@@ -216,7 +218,10 @@ impl Render for BlockEditor {
                 .on_mouse_down(
                     MouseButton::Left,
                     cx.listener(move |editor, _, window, cx| {
-                        let end = editor.blocks[editor.index(key)]
+                        let Some(ix) = editor.index(key) else {
+                            return;
+                        };
+                        let end = editor.blocks[ix]
                             .fields
                             .first()
                             .map(|field| field.read(cx).text().len());