Merge pull request #654 from zed-industries/remove-movment-results

Keith Simmons created 

Remove results from movement functions

Change summary

crates/editor/src/display_map.rs | 16 ++++-----
crates/editor/src/editor.rs      | 34 ++++++-------------
crates/editor/src/movement.rs    | 58 ++++++++++++++-------------------
crates/editor/src/test.rs        | 26 +++++++++++++++
4 files changed, 68 insertions(+), 66 deletions(-)

Detailed changes

crates/editor/src/display_map.rs 🔗 

@@ -714,7 +714,7 @@ mod tests {
 
                 log::info!("Moving from point {:?}", point);
 
-                let moved_right = movement::right(&snapshot, point).unwrap();
+                let moved_right = movement::right(&snapshot, point);
                 log::info!("Right {:?}", moved_right);
                 if point < max_point {
                     assert!(moved_right > point);
@@ -728,7 +728,7 @@ mod tests {
                     assert_eq!(moved_right, point);
                 }
 
-                let moved_left = movement::left(&snapshot, point).unwrap();
+                let moved_left = movement::left(&snapshot, point);
                 log::info!("Left {:?}", moved_left);
                 if point > min_point {
                     assert!(moved_left < point);
@@ -786,15 +786,15 @@ mod tests {
             DisplayPoint::new(1, 0)
         );
         assert_eq!(
-            movement::right(&snapshot, DisplayPoint::new(0, 7)).unwrap(),
+            movement::right(&snapshot, DisplayPoint::new(0, 7)),
             DisplayPoint::new(1, 0)
         );
         assert_eq!(
-            movement::left(&snapshot, DisplayPoint::new(1, 0)).unwrap(),
+            movement::left(&snapshot, DisplayPoint::new(1, 0)),
             DisplayPoint::new(0, 7)
         );
         assert_eq!(
-            movement::up(&snapshot, DisplayPoint::new(1, 10), SelectionGoal::None).unwrap(),
+            movement::up(&snapshot, DisplayPoint::new(1, 10), SelectionGoal::None),
             (DisplayPoint::new(0, 7), SelectionGoal::Column(10))
         );
         assert_eq!(
@@ -802,8 +802,7 @@ mod tests {
                 &snapshot,
                 DisplayPoint::new(0, 7),
                 SelectionGoal::Column(10)
-            )
-            .unwrap(),
+            ),
             (DisplayPoint::new(1, 10), SelectionGoal::Column(10))
         );
         assert_eq!(
@@ -811,8 +810,7 @@ mod tests {
                 &snapshot,
                 DisplayPoint::new(1, 10),
                 SelectionGoal::Column(10)
-            )
-            .unwrap(),
+            ),
             (DisplayPoint::new(2, 4), SelectionGoal::Column(10))
         );

crates/editor/src/editor.rs 🔗 

@@ -2663,7 +2663,6 @@ impl Editor {
                 let old_head = selection.head();
                 let mut new_head =
                     movement::left(&display_map, old_head.to_display_point(&display_map))
-                        .unwrap()
                         .to_point(&display_map);
                 if let Some((buffer, line_buffer_range)) = display_map
                     .buffer_snapshot
@@ -2695,9 +2694,7 @@ impl Editor {
         for selection in &mut selections {
             if selection.is_empty() {
                 let head = selection.head().to_display_point(&display_map);
-                let cursor = movement::right(&display_map, head)
-                    .unwrap()
-                    .to_point(&display_map);
+                let cursor = movement::right(&display_map, head).to_point(&display_map);
                 selection.set_head(cursor);
                 selection.goal = SelectionGoal::None;
             }
@@ -3315,9 +3312,7 @@ impl Editor {
             if start != end {
                 selection.end = selection.start.clone();
             } else {
-                let cursor = movement::left(&display_map, start)
-                    .unwrap()
-                    .to_point(&display_map);
+                let cursor = movement::left(&display_map, start).to_point(&display_map);
                 selection.start = cursor.clone();
                 selection.end = cursor;
             }
@@ -3332,9 +3327,7 @@ impl Editor {
         let mut selections = self.local_selections::<Point>(cx);
         for selection in &mut selections {
             let head = selection.head().to_display_point(&display_map);
-            let cursor = movement::left(&display_map, head)
-                .unwrap()
-                .to_point(&display_map);
+            let cursor = movement::left(&display_map, head).to_point(&display_map);
             selection.set_head(cursor);
             selection.goal = SelectionGoal::None;
         }
@@ -3351,9 +3344,7 @@ impl Editor {
             if start != end {
                 selection.start = selection.end.clone();
             } else {
-                let cursor = movement::right(&display_map, end)
-                    .unwrap()
-                    .to_point(&display_map);
+                let cursor = movement::right(&display_map, end).to_point(&display_map);
                 selection.start = cursor;
                 selection.end = cursor;
             }
@@ -3368,9 +3359,7 @@ impl Editor {
         let mut selections = self.local_selections::<Point>(cx);
         for selection in &mut selections {
             let head = selection.head().to_display_point(&display_map);
-            let cursor = movement::right(&display_map, head)
-                .unwrap()
-                .to_point(&display_map);
+            let cursor = movement::right(&display_map, head).to_point(&display_map);
             selection.set_head(cursor);
             selection.goal = SelectionGoal::None;
         }
@@ -3402,7 +3391,7 @@ impl Editor {
                 selection.goal = SelectionGoal::None;
             }
 
-            let (start, goal) = movement::up(&display_map, start, selection.goal).unwrap();
+            let (start, goal) = movement::up(&display_map, start, selection.goal);
             let cursor = start.to_point(&display_map);
             selection.start = cursor;
             selection.end = cursor;
@@ -3417,7 +3406,7 @@ impl Editor {
         let mut selections = self.local_selections::<Point>(cx);
         for selection in &mut selections {
             let head = selection.head().to_display_point(&display_map);
-            let (head, goal) = movement::up(&display_map, head, selection.goal).unwrap();
+            let (head, goal) = movement::up(&display_map, head, selection.goal);
             let cursor = head.to_point(&display_map);
             selection.set_head(cursor);
             selection.goal = goal;
@@ -3448,7 +3437,7 @@ impl Editor {
                 selection.goal = SelectionGoal::None;
             }
 
-            let (start, goal) = movement::down(&display_map, end, selection.goal).unwrap();
+            let (start, goal) = movement::down(&display_map, end, selection.goal);
             let cursor = start.to_point(&display_map);
             selection.start = cursor;
             selection.end = cursor;
@@ -3463,7 +3452,7 @@ impl Editor {
         let mut selections = self.local_selections::<Point>(cx);
         for selection in &mut selections {
             let head = selection.head().to_display_point(&display_map);
-            let (head, goal) = movement::down(&display_map, head, selection.goal).unwrap();
+            let (head, goal) = movement::down(&display_map, head, selection.goal);
             let cursor = head.to_point(&display_map);
             selection.set_head(cursor);
             selection.goal = goal;
@@ -6149,13 +6138,12 @@ mod tests {
     #[gpui::test]
     fn test_selection_with_mouse(cx: &mut gpui::MutableAppContext) {
         populate_settings(cx);
-        let buffer = MultiBuffer::build_simple("aaaaaa\nbbbbbb\ncccccc\ndddddd\n", cx);
-        let (_, editor) = cx.add_window(Default::default(), |cx| build_editor(buffer, cx));
 
+        let buffer = MultiBuffer::build_simple("aaaaaa\nbbbbbb\ncccccc\nddddddd\n", cx);
+        let (_, editor) = cx.add_window(Default::default(), |cx| build_editor(buffer, cx));
         editor.update(cx, |view, cx| {
             view.begin_selection(DisplayPoint::new(2, 2), false, 1, cx);
         });
-
         assert_eq!(
             editor.update(cx, |view, cx| view.selected_display_ranges(cx)),
             [DisplayPoint::new(2, 2)..DisplayPoint::new(2, 2)]

crates/editor/src/movement.rs 🔗 

@@ -1,20 +1,19 @@
 use super::{Bias, DisplayPoint, DisplaySnapshot, SelectionGoal, ToDisplayPoint};
 use crate::{char_kind, CharKind, ToPoint};
-use anyhow::Result;
 use language::Point;
 use std::ops::Range;
 
-pub fn left(map: &DisplaySnapshot, mut point: DisplayPoint) -> Result<DisplayPoint> {
+pub fn left(map: &DisplaySnapshot, mut point: DisplayPoint) -> DisplayPoint {
     if point.column() > 0 {
         *point.column_mut() -= 1;
     } else if point.row() > 0 {
         *point.row_mut() -= 1;
         *point.column_mut() = map.line_len(point.row());
     }
-    Ok(map.clip_point(point, Bias::Left))
+    map.clip_point(point, Bias::Left)
 }
 
-pub fn right(map: &DisplaySnapshot, mut point: DisplayPoint) -> Result<DisplayPoint> {
+pub fn right(map: &DisplaySnapshot, mut point: DisplayPoint) -> DisplayPoint {
     let max_column = map.line_len(point.row());
     if point.column() < max_column {
         *point.column_mut() += 1;
@@ -22,14 +21,14 @@ pub fn right(map: &DisplaySnapshot, mut point: DisplayPoint) -> Result<DisplayPo
         *point.row_mut() += 1;
         *point.column_mut() = 0;
     }
-    Ok(map.clip_point(point, Bias::Right))
+    map.clip_point(point, Bias::Right)
 }
 
 pub fn up(
     map: &DisplaySnapshot,
     start: DisplayPoint,
     goal: SelectionGoal,
-) -> Result<(DisplayPoint, SelectionGoal)> {
+) -> (DisplayPoint, SelectionGoal) {
     let mut goal_column = if let SelectionGoal::Column(column) = goal {
         column
     } else {
@@ -54,17 +53,17 @@ pub fn up(
         Bias::Right
     };
 
-    Ok((
+    (
         map.clip_point(point, clip_bias),
         SelectionGoal::Column(goal_column),
-    ))
+    )
 }
 
 pub fn down(
     map: &DisplaySnapshot,
     start: DisplayPoint,
     goal: SelectionGoal,
-) -> Result<(DisplayPoint, SelectionGoal)> {
+) -> (DisplayPoint, SelectionGoal) {
     let mut goal_column = if let SelectionGoal::Column(column) = goal {
         column
     } else {
@@ -86,10 +85,10 @@ pub fn down(
         Bias::Right
     };
 
-    Ok((
+    (
         map.clip_point(point, clip_bias),
         SelectionGoal::Column(goal_column),
-    ))
+    )
 }
 
 pub fn line_beginning(
@@ -267,7 +266,7 @@ pub fn surrounding_word(map: &DisplaySnapshot, position: DisplayPoint) -> Range<
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::{Buffer, DisplayMap, MultiBuffer};
+    use crate::{test::marked_text, Buffer, DisplayMap, MultiBuffer};
     use language::Point;
 
     #[gpui::test]
@@ -487,50 +486,49 @@ mod tests {
             );
             multibuffer
         });
-
         let display_map =
             cx.add_model(|cx| DisplayMap::new(multibuffer, 2, font_id, 14.0, None, 2, 2, cx));
-
         let snapshot = display_map.update(cx, |map, cx| map.snapshot(cx));
+
         assert_eq!(snapshot.text(), "\n\nabc\ndefg\n\n\nhijkl\nmn");
 
         // Can't move up into the first excerpt's header
         assert_eq!(
-            up(&snapshot, DisplayPoint::new(2, 2), SelectionGoal::Column(2)).unwrap(),
+            up(&snapshot, DisplayPoint::new(2, 2), SelectionGoal::Column(2)),
             (DisplayPoint::new(2, 0), SelectionGoal::Column(0)),
         );
         assert_eq!(
-            up(&snapshot, DisplayPoint::new(2, 0), SelectionGoal::None).unwrap(),
+            up(&snapshot, DisplayPoint::new(2, 0), SelectionGoal::None),
             (DisplayPoint::new(2, 0), SelectionGoal::Column(0)),
         );
 
         // Move up and down within first excerpt
         assert_eq!(
-            up(&snapshot, DisplayPoint::new(3, 4), SelectionGoal::Column(4)).unwrap(),
+            up(&snapshot, DisplayPoint::new(3, 4), SelectionGoal::Column(4)),
             (DisplayPoint::new(2, 3), SelectionGoal::Column(4)),
         );
         assert_eq!(
-            down(&snapshot, DisplayPoint::new(2, 3), SelectionGoal::Column(4)).unwrap(),
+            down(&snapshot, DisplayPoint::new(2, 3), SelectionGoal::Column(4)),
             (DisplayPoint::new(3, 4), SelectionGoal::Column(4)),
         );
 
         // Move up and down across second excerpt's header
         assert_eq!(
-            up(&snapshot, DisplayPoint::new(6, 5), SelectionGoal::Column(5)).unwrap(),
+            up(&snapshot, DisplayPoint::new(6, 5), SelectionGoal::Column(5)),
             (DisplayPoint::new(3, 4), SelectionGoal::Column(5)),
         );
         assert_eq!(
-            down(&snapshot, DisplayPoint::new(3, 4), SelectionGoal::Column(5)).unwrap(),
+            down(&snapshot, DisplayPoint::new(3, 4), SelectionGoal::Column(5)),
             (DisplayPoint::new(6, 5), SelectionGoal::Column(5)),
         );
 
         // Can't move down off the end
         assert_eq!(
-            down(&snapshot, DisplayPoint::new(7, 0), SelectionGoal::Column(0)).unwrap(),
+            down(&snapshot, DisplayPoint::new(7, 0), SelectionGoal::Column(0)),
             (DisplayPoint::new(7, 2), SelectionGoal::Column(2)),
         );
         assert_eq!(
-            down(&snapshot, DisplayPoint::new(7, 2), SelectionGoal::Column(2)).unwrap(),
+            down(&snapshot, DisplayPoint::new(7, 2), SelectionGoal::Column(2)),
             (DisplayPoint::new(7, 2), SelectionGoal::Column(2)),
         );
     }
@@ -540,15 +538,7 @@ mod tests {
         text: &str,
         cx: &mut gpui::MutableAppContext,
     ) -> (DisplaySnapshot, Vec<DisplayPoint>) {
-        let mut marked_offsets = Vec::new();
-        let chunks = text.split('|');
-        let mut text = String::new();
-
-        for chunk in chunks {
-            text.push_str(chunk);
-            marked_offsets.push(text.len());
-        }
-        marked_offsets.pop();
+        let (unmarked_text, markers) = marked_text(text);
 
         let tab_size = 4;
         let family_id = cx.font_cache().load_family(&["Helvetica"]).unwrap();
@@ -558,15 +548,15 @@ mod tests {
             .unwrap();
         let font_size = 14.0;
 
-        let buffer = MultiBuffer::build_simple(&text, cx);
+        let buffer = MultiBuffer::build_simple(&unmarked_text, cx);
         let display_map = cx
             .add_model(|cx| DisplayMap::new(buffer, tab_size, font_id, font_size, None, 1, 1, cx));
         let snapshot = display_map.update(cx, |map, cx| map.snapshot(cx));
-        let marked_display_points = marked_offsets
+        let markers = markers
             .into_iter()
             .map(|offset| offset.to_display_point(&snapshot))
             .collect();
 
-        (snapshot, marked_display_points)
+        (snapshot, markers)
     }
 }

crates/editor/src/test.rs 🔗 

@@ -1,3 +1,5 @@
+use collections::HashMap;
+
 #[cfg(test)]
 #[ctor::ctor]
 fn init_logger() {
@@ -5,3 +7,27 @@ fn init_logger() {
         env_logger::init();
     }
 }
+
+pub fn marked_text_by(
+    marked_text: &str,
+    markers: Vec<char>,
+) -> (String, HashMap<char, Vec<usize>>) {
+    let mut extracted_markers: HashMap<char, Vec<usize>> = Default::default();
+    let mut unmarked_text = String::new();
+
+    for char in marked_text.chars() {
+        if markers.contains(&char) {
+            let char_offsets = extracted_markers.entry(char).or_insert(Vec::new());
+            char_offsets.push(unmarked_text.len());
+        } else {
+            unmarked_text.push(char);
+        }
+    }
+
+    (unmarked_text, extracted_markers)
+}
+
+pub fn marked_text(marked_text: &str) -> (String, Vec<usize>) {
+    let (unmarked_text, mut markers) = marked_text_by(marked_text, vec!['|']);
+    (unmarked_text, markers.remove(&'|').unwrap_or_else(Vec::new))
+}