From 7daa4b5b042735a2252f678311a8e65f54ad8afd Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 12 Jan 2022 09:14:48 +0100 Subject: [PATCH 1/3] Don't return a `Result` in test-only method `select_display_ranges` --- crates/editor/src/editor.rs | 143 ++++++++++++------------------------ 1 file changed, 47 insertions(+), 96 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 06f76ff127..40682eb9f7 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -1128,11 +1128,7 @@ impl Editor { } #[cfg(test)] - fn select_display_ranges<'a, T>( - &mut self, - ranges: T, - cx: &mut ViewContext, - ) -> anyhow::Result<()> + fn select_display_ranges<'a, T>(&mut self, ranges: T, cx: &mut ViewContext) where T: IntoIterator>, { @@ -1158,7 +1154,6 @@ impl Editor { }) .collect(); self.update_selections(selections, None, cx); - Ok(()) } pub fn handle_input(&mut self, action: &Input, cx: &mut ViewContext) { @@ -4205,8 +4200,7 @@ mod tests { }); view.update(cx, |view, cx| { - view.select_display_ranges(&[DisplayPoint::new(8, 0)..DisplayPoint::new(12, 0)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(8, 0)..DisplayPoint::new(12, 0)], cx); view.fold(&Fold, cx); assert_eq!( view.display_text(cx), @@ -4325,8 +4319,7 @@ mod tests { &[DisplayPoint::new(0, 0)..DisplayPoint::new(0, 0)] ); - view.select_display_ranges(&[DisplayPoint::new(0, 1)..DisplayPoint::new(0, 2)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(0, 1)..DisplayPoint::new(0, 2)], cx); view.select_to_beginning(&SelectToBeginning, cx); assert_eq!( view.selected_display_ranges(cx), @@ -4452,9 +4445,7 @@ mod tests { build_editor(buffer.clone(), settings, cx) }); view.update(cx, |view, cx| { - view.select_display_ranges(&[empty_range(0, "ⓐⓑⓒⓓⓔ".len())], cx) - .unwrap(); - + view.select_display_ranges(&[empty_range(0, "ⓐⓑⓒⓓⓔ".len())], cx); view.move_down(&MoveDown, cx); assert_eq!( view.selected_display_ranges(cx), @@ -4505,8 +4496,7 @@ mod tests { DisplayPoint::new(1, 4)..DisplayPoint::new(1, 4), ], cx, - ) - .unwrap(); + ); }); view.update(cx, |view, cx| { @@ -4647,8 +4637,7 @@ mod tests { DisplayPoint::new(2, 4)..DisplayPoint::new(2, 4), ], cx, - ) - .unwrap(); + ); }); view.update(cx, |view, cx| { @@ -4787,8 +4776,7 @@ mod tests { "use one::{\n two::three::\n four::five\n};" ); - view.select_display_ranges(&[DisplayPoint::new(1, 7)..DisplayPoint::new(1, 7)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(1, 7)..DisplayPoint::new(1, 7)], cx); view.move_to_next_word_boundary(&MoveToNextWordBoundary, cx); assert_eq!( @@ -4845,8 +4833,7 @@ mod tests { DisplayPoint::new(0, 9)..DisplayPoint::new(0, 12), ], cx, - ) - .unwrap(); + ); view.delete_to_previous_word_boundary(&DeleteToPreviousWordBoundary, cx); }); @@ -4861,8 +4848,7 @@ mod tests { DisplayPoint::new(0, 9)..DisplayPoint::new(0, 10), ], cx, - ) - .unwrap(); + ); view.delete_to_next_word_boundary(&DeleteToNextWordBoundary, cx); }); @@ -4885,8 +4871,7 @@ mod tests { DisplayPoint::new(1, 6)..DisplayPoint::new(1, 6), ], cx, - ) - .unwrap(); + ); view.newline(&Newline, cx); assert_eq!(view.text(cx), "aa\naa\n \n bb\n bb\n"); @@ -4909,8 +4894,7 @@ mod tests { DisplayPoint::new(0, 6)..DisplayPoint::new(0, 9), ], cx, - ) - .unwrap(); + ); // indent from mid-tabstop to full tabstop view.tab(&Tab, cx); @@ -4935,8 +4919,7 @@ mod tests { ); // select across line ending - view.select_display_ranges(&[DisplayPoint::new(1, 1)..DisplayPoint::new(2, 0)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(1, 1)..DisplayPoint::new(2, 0)], cx); // indent and outdent affect only the preceding line view.tab(&Tab, cx); @@ -4953,8 +4936,7 @@ mod tests { ); // Ensure that indenting/outdenting works when the cursor is at column 0. - view.select_display_ranges(&[DisplayPoint::new(1, 0)..DisplayPoint::new(1, 0)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(1, 0)..DisplayPoint::new(1, 0)], cx); view.tab(&Tab, cx); assert_eq!(view.text(cx), "one two\n three\n four"); assert_eq!( @@ -4962,8 +4944,7 @@ mod tests { &[DisplayPoint::new(1, 4)..DisplayPoint::new(1, 4)] ); - view.select_display_ranges(&[DisplayPoint::new(1, 0)..DisplayPoint::new(1, 0)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(1, 0)..DisplayPoint::new(1, 0)], cx); view.outdent(&Outdent, cx); assert_eq!(view.text(cx), "one two\nthree\n four"); assert_eq!( @@ -4993,8 +4974,7 @@ mod tests { DisplayPoint::new(2, 6)..DisplayPoint::new(3, 0), ], cx, - ) - .unwrap(); + ); view.backspace(&Backspace, cx); }); @@ -5024,8 +5004,7 @@ mod tests { DisplayPoint::new(2, 6)..DisplayPoint::new(3, 0), ], cx, - ) - .unwrap(); + ); view.delete(&Delete, cx); }); @@ -5048,8 +5027,7 @@ mod tests { DisplayPoint::new(3, 0)..DisplayPoint::new(3, 0), ], cx, - ) - .unwrap(); + ); view.delete_line(&DeleteLine, cx); assert_eq!(view.display_text(cx), "ghi"); assert_eq!( @@ -5065,8 +5043,7 @@ mod tests { let buffer = MultiBuffer::build_simple("abc\ndef\nghi\n", cx); let (_, view) = cx.add_window(Default::default(), |cx| build_editor(buffer, settings, cx)); view.update(cx, |view, cx| { - view.select_display_ranges(&[DisplayPoint::new(2, 0)..DisplayPoint::new(0, 1)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(2, 0)..DisplayPoint::new(0, 1)], cx); view.delete_line(&DeleteLine, cx); assert_eq!(view.display_text(cx), "ghi\n"); assert_eq!( @@ -5090,8 +5067,7 @@ mod tests { DisplayPoint::new(3, 0)..DisplayPoint::new(3, 0), ], cx, - ) - .unwrap(); + ); view.duplicate_line(&DuplicateLine, cx); assert_eq!(view.display_text(cx), "abc\nabc\ndef\ndef\nghi\n\n"); assert_eq!( @@ -5115,8 +5091,7 @@ mod tests { DisplayPoint::new(1, 2)..DisplayPoint::new(2, 1), ], cx, - ) - .unwrap(); + ); view.duplicate_line(&DuplicateLine, cx); assert_eq!(view.display_text(cx), "abc\ndef\nghi\nabc\ndef\nghi\n"); assert_eq!( @@ -5151,8 +5126,7 @@ mod tests { DisplayPoint::new(5, 0)..DisplayPoint::new(5, 2), ], cx, - ) - .unwrap(); + ); assert_eq!( view.display_text(cx), "aa…bbb\nccc…eeee\nfffff\nggggg\n…i\njjjjj" @@ -5312,8 +5286,7 @@ mod tests { DisplayPoint::new(2, 0)..DisplayPoint::new(2, 1), ], cx, - ) - .unwrap(); + ); view.cut(&Cut, cx); assert_eq!( view.display_text(cx), @@ -5331,8 +5304,7 @@ mod tests { DisplayPoint::new(2, 2)..DisplayPoint::new(2, 3), ], cx, - ) - .unwrap(); + ); view.paste(&Paste, cx); assert_eq!( view.display_text(cx), @@ -5350,8 +5322,7 @@ mod tests { // Copy with a single cursor only, which writes the whole line into the clipboard. view.update(cx, |view, cx| { - view.select_display_ranges(&[DisplayPoint::new(0, 1)..DisplayPoint::new(0, 1)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(0, 1)..DisplayPoint::new(0, 1)], cx); view.copy(&Copy, cx); }); @@ -5365,8 +5336,7 @@ mod tests { DisplayPoint::new(2, 1)..DisplayPoint::new(2, 1), ], cx, - ) - .unwrap(); + ); view.paste(&Paste, cx); assert_eq!( view.display_text(cx), @@ -5411,8 +5381,7 @@ mod tests { DisplayPoint::new(4, 2)..DisplayPoint::new(4, 2), ], cx, - ) - .unwrap(); + ); view.select_line(&SelectLine, cx); assert_eq!( view.selected_display_ranges(cx), @@ -5465,8 +5434,7 @@ mod tests { DisplayPoint::new(4, 4)..DisplayPoint::new(4, 4), ], cx, - ) - .unwrap(); + ); assert_eq!(view.display_text(cx), "aa…bbb\nccc…eeee\nfffff\nggggg\n…i"); }); @@ -5488,8 +5456,7 @@ mod tests { }); view.update(cx, |view, cx| { - view.select_display_ranges(&[DisplayPoint::new(5, 0)..DisplayPoint::new(0, 1)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(5, 0)..DisplayPoint::new(0, 1)], cx); view.split_selection_into_lines(&SplitSelectionIntoLines, cx); assert_eq!( view.display_text(cx), @@ -5518,8 +5485,7 @@ mod tests { let (_, view) = cx.add_window(Default::default(), |cx| build_editor(buffer, settings, cx)); view.update(cx, |view, cx| { - view.select_display_ranges(&[DisplayPoint::new(1, 3)..DisplayPoint::new(1, 3)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(1, 3)..DisplayPoint::new(1, 3)], cx); }); view.update(cx, |view, cx| { view.add_selection_above(&AddSelectionAbove, cx); @@ -5574,8 +5540,7 @@ mod tests { }); view.update(cx, |view, cx| { - view.select_display_ranges(&[DisplayPoint::new(1, 4)..DisplayPoint::new(1, 3)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(1, 4)..DisplayPoint::new(1, 3)], cx); }); view.update(cx, |view, cx| { view.add_selection_below(&AddSelectionBelow, cx); @@ -5616,8 +5581,7 @@ mod tests { }); view.update(cx, |view, cx| { - view.select_display_ranges(&[DisplayPoint::new(0, 1)..DisplayPoint::new(1, 4)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(0, 1)..DisplayPoint::new(1, 4)], cx); view.add_selection_below(&AddSelectionBelow, cx); assert_eq!( view.selected_display_ranges(cx), @@ -5655,8 +5619,7 @@ mod tests { }); view.update(cx, |view, cx| { - view.select_display_ranges(&[DisplayPoint::new(4, 3)..DisplayPoint::new(1, 1)], cx) - .unwrap(); + view.select_display_ranges(&[DisplayPoint::new(4, 3)..DisplayPoint::new(1, 1)], cx); }); view.update(cx, |view, cx| { view.add_selection_above(&AddSelectionAbove, cx); @@ -5715,8 +5678,7 @@ mod tests { DisplayPoint::new(3, 18)..DisplayPoint::new(3, 18), ], cx, - ) - .unwrap(); + ); view.select_larger_syntax_node(&SelectLargerSyntaxNode, cx); }); assert_eq!( @@ -5928,8 +5890,7 @@ mod tests { DisplayPoint::new(1, 0)..DisplayPoint::new(1, 0), ], cx, - ) - .unwrap(); + ); view.handle_input(&Input("{".to_string()), cx); view.handle_input(&Input("{".to_string()), cx); view.handle_input(&Input("{".to_string()), cx); @@ -5980,8 +5941,7 @@ mod tests { DisplayPoint::new(3, 0)..DisplayPoint::new(3, 0), ], cx, - ) - .unwrap(); + ); view.handle_input(&Input("*".to_string()), cx); assert_eq!( view.text(cx), @@ -6023,15 +5983,13 @@ mod tests { view.update(&mut cx, |editor, cx| { // If multiple selections intersect a line, the line is only // toggled once. - editor - .select_display_ranges( - &[ - DisplayPoint::new(1, 3)..DisplayPoint::new(2, 3), - DisplayPoint::new(3, 5)..DisplayPoint::new(3, 6), - ], - cx, - ) - .unwrap(); + editor.select_display_ranges( + &[ + DisplayPoint::new(1, 3)..DisplayPoint::new(2, 3), + DisplayPoint::new(3, 5)..DisplayPoint::new(3, 6), + ], + cx, + ); editor.toggle_comments(&ToggleComments, cx); assert_eq!( editor.text(cx), @@ -6047,9 +6005,7 @@ mod tests { // The comment prefix is inserted at the same column for every line // in a selection. - editor - .select_display_ranges(&[DisplayPoint::new(1, 3)..DisplayPoint::new(3, 6)], cx) - .unwrap(); + editor.select_display_ranges(&[DisplayPoint::new(1, 3)..DisplayPoint::new(3, 6)], cx); editor.toggle_comments(&ToggleComments, cx); assert_eq!( editor.text(cx), @@ -6064,9 +6020,7 @@ mod tests { ); // If a selection ends at the beginning of a line, that line is not toggled. - editor - .select_display_ranges(&[DisplayPoint::new(2, 0)..DisplayPoint::new(3, 0)], cx) - .unwrap(); + editor.select_display_ranges(&[DisplayPoint::new(2, 0)..DisplayPoint::new(3, 0)], cx); editor.toggle_comments(&ToggleComments, cx); assert_eq!( editor.text(cx), @@ -6117,8 +6071,7 @@ mod tests { DisplayPoint::new(1, 0)..DisplayPoint::new(1, 0), ], cx, - ) - .unwrap(); + ); view.handle_input(&Input("X".to_string()), cx); assert_eq!(view.text(cx), "Xaaaa\nXbbbb"); @@ -6170,8 +6123,7 @@ mod tests { DisplayPoint::new(2, 3)..DisplayPoint::new(2, 3), ], cx, - ) - .unwrap(); + ); view.handle_input(&Input("X".to_string()), cx); assert_eq!(view.text(cx), "aaaa\nbXbbXb\nbXbbXb\ncccc"); @@ -6241,8 +6193,7 @@ mod tests { DisplayPoint::new(4, 4)..DisplayPoint::new(4, 4), ], cx, - ) - .unwrap(); + ); view.newline(&Newline, cx); assert_eq!( From b768a3977c2c597c3330014ac33ec70128fa2425 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 12 Jan 2022 09:22:15 +0100 Subject: [PATCH 2/3] Add unit test reproducing a panic when refreshing selections --- crates/editor/src/editor.rs | 77 +++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 40682eb9f7..fb7053b272 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -6147,6 +6147,83 @@ mod tests { }); } + #[gpui::test] + fn test_refresh_selections(cx: &mut gpui::MutableAppContext) { + let settings = EditorSettings::test(cx); + let buffer = cx.add_model(|cx| Buffer::new(0, sample_text(3, 4, 'a'), cx)); + let mut excerpt1_id = None; + let multibuffer = cx.add_model(|cx| { + let mut multibuffer = MultiBuffer::new(0); + excerpt1_id = Some(multibuffer.push_excerpt( + ExcerptProperties { + buffer: &buffer, + range: Point::new(0, 0)..Point::new(1, 4), + }, + cx, + )); + multibuffer.push_excerpt( + ExcerptProperties { + buffer: &buffer, + range: Point::new(1, 0)..Point::new(2, 4), + }, + cx, + ); + multibuffer + }); + assert_eq!( + multibuffer.read(cx).read(cx).text(), + "aaaa\nbbbb\nbbbb\ncccc" + ); + let (_, editor) = cx.add_window(Default::default(), |cx| { + let mut editor = build_editor(multibuffer.clone(), settings, cx); + editor.select_display_ranges( + &[ + DisplayPoint::new(1, 3)..DisplayPoint::new(1, 3), + DisplayPoint::new(2, 1)..DisplayPoint::new(2, 1), + ], + cx, + ); + editor + }); + + // Refreshing selections is a no-op when excerpts haven't changed. + editor.update(cx, |editor, cx| { + editor.refresh_selections(cx); + assert_eq!( + editor.selected_display_ranges(cx), + [ + DisplayPoint::new(1, 3)..DisplayPoint::new(1, 3), + DisplayPoint::new(2, 1)..DisplayPoint::new(2, 1), + ] + ); + }); + + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.remove_excerpts([&excerpt1_id.unwrap()], cx); + }); + editor.update(cx, |editor, cx| { + // Removing an excerpt causes the first selection to become degenerate. + assert_eq!( + editor.selected_display_ranges(cx), + [ + DisplayPoint::new(0, 0)..DisplayPoint::new(0, 0), + DisplayPoint::new(0, 1)..DisplayPoint::new(0, 1) + ] + ); + + // Refreshing selections will relocate the first selection to the original buffer + // location. + editor.refresh_selections(cx); + assert_eq!( + editor.selected_display_ranges(cx), + [ + DisplayPoint::new(0, 1)..DisplayPoint::new(0, 1), + DisplayPoint::new(0, 3)..DisplayPoint::new(0, 3) + ] + ); + }); + } + #[gpui::test] async fn test_extra_newline_insertion(mut cx: gpui::TestAppContext) { let settings = cx.read(EditorSettings::test); From 6fbbbab7ba631ad9c27f209300965cea7aab4ada Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 12 Jan 2022 09:28:09 +0100 Subject: [PATCH 3/3] Process selection anchors in a sorted fashion when refreshing them --- crates/editor/src/editor.rs | 14 +++++++------- crates/editor/src/multi_buffer.rs | 27 ++++++++++++++------------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index fb7053b272..a7cf2365a9 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -3293,17 +3293,17 @@ impl Editor { .flat_map(|selection| [&selection.start, &selection.end]), ); let offsets = - snapshot.summaries_for_anchors::(anchors_with_status.iter().map(|a| &a.0)); + snapshot.summaries_for_anchors::(anchors_with_status.iter().map(|a| &a.1)); let offsets = offsets.chunks(2); - let statuses = anchors_with_status.chunks(2).map(|a| (a[0].1, a[1].1)); + let statuses = anchors_with_status + .chunks(2) + .map(|a| (a[0].0 / 2, a[0].2, a[1].2)); let mut selections_with_lost_position = HashMap::default(); - let new_selections = self - .selections - .iter() - .zip(offsets) + let new_selections = offsets .zip(statuses) - .map(|((selection, offsets), (kept_start, kept_end))| { + .map(|(offsets, (selection_ix, kept_start, kept_end))| { + let selection = &self.selections[selection_ix]; let kept_head = if selection.reversed { kept_start } else { diff --git a/crates/editor/src/multi_buffer.rs b/crates/editor/src/multi_buffer.rs index 5adbc8c9a1..cd4a3207df 100644 --- a/crates/editor/src/multi_buffer.rs +++ b/crates/editor/src/multi_buffer.rs @@ -1394,14 +1394,14 @@ impl MultiBufferSnapshot { summaries } - pub fn refresh_anchors<'a, I>(&'a self, anchors: I) -> Vec<(Anchor, bool)> + pub fn refresh_anchors<'a, I>(&'a self, anchors: I) -> Vec<(usize, Anchor, bool)> where I: 'a + IntoIterator, { - let mut anchors = anchors.into_iter().peekable(); + let mut anchors = anchors.into_iter().enumerate().peekable(); let mut cursor = self.excerpts.cursor::>(); let mut result = Vec::new(); - while let Some(anchor) = anchors.peek() { + while let Some((_, anchor)) = anchors.peek() { let old_excerpt_id = &anchor.excerpt_id; // Find the location where this anchor's excerpt should be. @@ -1414,12 +1414,13 @@ impl MultiBufferSnapshot { let prev_excerpt = cursor.prev_item(); // Process all of the anchors for this excerpt. - while let Some(&anchor) = anchors.peek() { + while let Some((_, anchor)) = anchors.peek() { if anchor.excerpt_id != *old_excerpt_id { break; } let mut kept_position = false; - let mut anchor = anchors.next().unwrap().clone(); + let (anchor_ix, anchor) = anchors.next().unwrap(); + let mut anchor = anchor.clone(); // Leave min and max anchors unchanged. if *old_excerpt_id == ExcerptId::max() || *old_excerpt_id == ExcerptId::min() { @@ -1469,9 +1470,10 @@ impl MultiBufferSnapshot { }; } - result.push((anchor, kept_position)); + result.push((anchor_ix, anchor, kept_position)); } } + result.sort_unstable_by(|a, b| a.1.cmp(&b.1, self).unwrap()); result } @@ -2588,8 +2590,8 @@ mod tests { assert_eq!( refresh, &[ - (snapshot_2.anchor_before(0), false), - (snapshot_2.anchor_after(0), false), + (0, snapshot_2.anchor_before(0), false), + (1, snapshot_2.anchor_after(0), false), ] ); @@ -2627,11 +2629,11 @@ mod tests { let new_anchors = snapshot_3.refresh_anchors(&anchors); assert_eq!( - new_anchors.iter().map(|a| a.1).collect::>(), - &[true, true, true, true] + new_anchors.iter().map(|a| (a.0, a.2)).collect::>(), + &[(0, true), (1, true), (2, true), (3, true)] ); assert_eq!( - snapshot_3.summaries_for_anchors::(new_anchors.iter().map(|a| &a.0)), + snapshot_3.summaries_for_anchors::(new_anchors.iter().map(|a| &a.1)), &[0, 2, 7, 13] ); } @@ -2693,9 +2695,8 @@ mod tests { anchors = multibuffer .refresh_anchors(&anchors) .into_iter() - .map(|a| a.0) + .map(|a| a.1) .collect(); - anchors.sort_by(|a, b| a.cmp(&b, &multibuffer).unwrap()); } _ => { let buffer_handle = if buffers.is_empty() || rng.gen_bool(0.4) {