Fixed cursor positioning bugs in multi-byte charcters. Still have at least one though :/

Mikayla Maki created

Change summary

crates/editor/src/element.rs            |  20 ++--
crates/terminal/src/terminal.rs         |   5 
crates/terminal/src/terminal_element.rs | 107 ++++++++++++++++++--------
3 files changed, 86 insertions(+), 46 deletions(-)

Detailed changes

crates/editor/src/element.rs 🔗

@@ -520,7 +520,7 @@ impl EditorElement {
                         cursors.push(Cursor {
                             color: selection_style.cursor,
                             block_width,
-                            origin: content_origin + vec2f(x, y),
+                            origin: vec2f(x, y),
                             line_height: layout.line_height,
                             shape: self.cursor_shape,
                             block_text,
@@ -546,13 +546,12 @@ impl EditorElement {
 
         cx.scene.push_layer(Some(bounds));
         for cursor in cursors {
-            cursor.paint(cx);
+            cursor.paint(content_origin, cx);
         }
         cx.scene.pop_layer();
 
         if let Some((position, context_menu)) = layout.context_menu.as_mut() {
             cx.scene.push_stacking_context(None);
-
             let cursor_row_layout = &layout.line_layouts[(position.row() - start_row) as usize];
             let x = cursor_row_layout.x_for_index(position.column() as usize) - scroll_left;
             let y = (position.row() + 1) as f32 * layout.line_height - scroll_top;
@@ -1658,14 +1657,15 @@ impl Cursor {
         }
     }
 
-    pub fn paint(&self, cx: &mut PaintContext) {
+    pub fn paint(&self, origin: Vector2F, cx: &mut PaintContext) {
         let bounds = match self.shape {
-            CursorShape::Bar => RectF::new(self.origin, vec2f(2.0, self.line_height)),
-            CursorShape::Block => {
-                RectF::new(self.origin, vec2f(self.block_width, self.line_height))
-            }
+            CursorShape::Bar => RectF::new(self.origin + origin, vec2f(2.0, self.line_height)),
+            CursorShape::Block => RectF::new(
+                self.origin + origin,
+                vec2f(self.block_width, self.line_height),
+            ),
             CursorShape::Underscore => RectF::new(
-                self.origin + Vector2F::new(0.0, self.line_height - 2.0),
+                self.origin + origin + Vector2F::new(0.0, self.line_height - 2.0),
                 vec2f(self.block_width, 2.0),
             ),
         };
@@ -1678,7 +1678,7 @@ impl Cursor {
         });
 
         if let Some(block_text) = &self.block_text {
-            block_text.paint(self.origin, bounds, self.line_height, cx);
+            block_text.paint(self.origin + origin, bounds, self.line_height, cx);
         }
     }
 }

crates/terminal/src/terminal.rs 🔗

@@ -464,7 +464,7 @@ fn to_alac_rgb(color: Color) -> AlacRgb {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::terminal_element::build_chunks;
+    use crate::terminal_element::{build_chunks, BuiltChunks};
     use gpui::TestAppContext;
 
     ///Basic integration test, can we get the terminal to show up, execute a command,
@@ -481,9 +481,10 @@ mod tests {
         terminal
             .condition(cx, |terminal, _cx| {
                 let term = terminal.term.clone();
-                let (chunks, _) = build_chunks(
+                let BuiltChunks { chunks, .. } = build_chunks(
                     term.lock().renderable_content().display_iter,
                     &Default::default(),
+                    Default::default(),
                 );
                 let content = chunks.iter().map(|e| e.0.trim()).collect::<String>();
                 content.contains("7")

crates/terminal/src/terminal_element.rs 🔗

@@ -78,7 +78,7 @@ pub struct LayoutState {
     lines: Vec<Line>,
     line_height: LineHeight,
     em_width: CellWidth,
-    cursor: Option<(Vector2F, Color, Option<Line>)>,
+    cursor: Option<Cursor>,
     cur_size: SizeInfo,
     background_color: Color,
     background_rects: Vec<(RectF, Color)>, //Vec index == Line index for the LineSpan
@@ -120,18 +120,18 @@ impl Element for TerminalEl {
         let terminal_theme = &(cx.global::<Settings>()).theme.terminal;
         let term = view_handle.read(cx).term.lock();
 
-        // let cursor_char = term.grid().cursor_cell().c.to_string();
-
-        let cursor_text = {
-            let grid = term.grid();
-            let cursor_point = grid.cursor.point;
-            grid[cursor_point.line][cursor_point.column].c.to_string()
-        };
+        let grid = term.grid();
+        let cursor_point = grid.cursor.point;
+        let cursor_text = grid[cursor_point.line][cursor_point.column].c.to_string();
 
         let content = term.renderable_content();
 
         //And we're off! Begin layouting
-        let (chunks, line_count) = build_chunks(content.display_iter, &terminal_theme);
+        let BuiltChunks {
+            chunks,
+            line_count,
+            cursor_index,
+        } = build_chunks(content.display_iter, &terminal_theme, cursor_point);
 
         let shaped_lines = layout_highlighted_chunks(
             chunks
@@ -163,13 +163,30 @@ impl Element for TerminalEl {
                 },
             )],
         );
+
         let cursor = get_cursor_position(
-            content.cursor.point,
+            content.cursor.point.line.0 as usize,
+            cursor_index,
             &shaped_lines,
             content.display_offset,
             &line_height,
         )
-        .map(|cursor_rect| (cursor_rect, terminal_theme.cursor, Some(block_text)));
+        .map(move |(cursor_position, block_width)| {
+            let block_width = if block_width != 0.0 {
+                block_width
+            } else {
+                cell_width.0
+            };
+
+            Cursor::new(
+                cursor_position,
+                block_width,
+                line_height.0,
+                terminal_theme.cursor,
+                CursorShape::Block,
+                Some(block_text.clone()),
+            )
+        });
 
         (
             constraint.max,
@@ -239,20 +256,11 @@ impl Element for TerminalEl {
         cx.scene.pop_layer();
 
         //Draw cursor
-        cx.scene.push_layer(Some(visible_bounds));
-        if let Some((c, color, block_text)) = &layout.cursor {
-            let editor_cursor = Cursor::new(
-                origin + *c,
-                layout.em_width.0,
-                layout.line_height.0,
-                *color,
-                CursorShape::Block,
-                block_text.clone(), //TODO fix this
-            );
-
-            editor_cursor.paint(cx);
+        if let Some(cursor) = &layout.cursor {
+            cx.scene.push_layer(Some(visible_bounds));
+            cursor.paint(origin, cx);
+            cx.scene.pop_layer();
         }
-        cx.scene.pop_layer();
 
         #[cfg(debug_assertions)]
         if DEBUG_GRID {
@@ -339,21 +347,30 @@ fn make_new_size(
     )
 }
 
+pub struct BuiltChunks {
+    pub chunks: Vec<(String, Option<HighlightStyle>, RectSpan)>,
+    pub line_count: usize,
+    pub cursor_index: usize,
+}
+
 ///In a single pass, this function generates the background and foreground color info for every item in the grid.
 pub(crate) fn build_chunks(
     grid_iterator: GridIterator<Cell>,
     theme: &TerminalStyle,
-) -> (Vec<(String, Option<HighlightStyle>, RectSpan)>, usize) {
+    cursor_point: Point,
+) -> BuiltChunks {
     let mut line_count: usize = 0;
+    let mut cursor_index: usize = 0;
     //Every `group_by()` -> `into_iter()` pair needs to be seperated by a local variable so
     //rust knows where to put everything.
     //Start by grouping by lines
     let lines = grid_iterator.group_by(|i| i.point.line.0);
     let result = lines
         .into_iter()
-        .map(|(_, line)| {
+        .map(|(_line_grid_index, line)| {
             line_count += 1;
             let mut col_index = 0;
+            //Setup a variable
 
             //Then group by style
             let chunks = line.group_by(|i| cell_style(&i, theme));
@@ -361,9 +378,20 @@ pub(crate) fn build_chunks(
                 .into_iter()
                 .map(|(style, fragment)| {
                     //And assemble the styled fragment into it's background and foreground information
-                    let str_fragment = fragment.map(|indexed| indexed.c).collect::<String>();
+                    let mut str_fragment = String::new();
+                    for indexed_cell in fragment {
+                        if cursor_point.line.0 == indexed_cell.point.line.0
+                            && indexed_cell.point.column < cursor_point.column.0
+                        {
+                            cursor_index += indexed_cell.c.to_string().len();
+                        }
+                        str_fragment.push(indexed_cell.c);
+                    }
+
                     let start = col_index;
                     let end = start + str_fragment.len() as i32;
+
+                    //munge it here
                     col_index = end;
                     (
                         str_fragment,
@@ -378,7 +406,12 @@ pub(crate) fn build_chunks(
         .flatten()
         //We have a Vec<Vec<>> (Vec of lines of styled chunks), flatten to just Vec<> (the styled chunks)
         .collect::<Vec<(String, Option<HighlightStyle>, RectSpan)>>();
-    (result, line_count)
+
+    BuiltChunks {
+        chunks: result,
+        line_count,
+        cursor_index,
+    }
 }
 
 ///Convert a RectSpan in terms of character offsets, into RectFs of exact offsets
@@ -408,17 +441,23 @@ fn make_background_rects(
         .collect::<Vec<(RectF, Color)>>()
 }
 
-///Create the rectangle for a cursor, exactly positioned according to the text
+// Compute the cursor position and expected block width, may return a zero width if x_for_index returns
+// the same position for sequential indexes. Use em_width instead
 fn get_cursor_position(
-    cursor_point: Point,
+    line: usize,
+    line_index: usize,
     shaped_lines: &Vec<Line>,
     display_offset: usize,
     line_height: &LineHeight,
-) -> Option<Vector2F> {
-    let cursor_line = cursor_point.line.0 as usize + display_offset;
+) -> Option<(Vector2F, f32)> {
+    let cursor_line = line + display_offset;
     shaped_lines.get(cursor_line).map(|layout_line| {
-        let cursor_x = layout_line.x_for_index(cursor_point.column.0);
-        vec2f(cursor_x, cursor_line as f32 * line_height.0)
+        let cursor_x = layout_line.x_for_index(line_index);
+        let next_char_x = layout_line.x_for_index(line_index + 1);
+        (
+            vec2f(cursor_x, cursor_line as f32 * line_height.0),
+            next_char_x - cursor_x,
+        )
     })
 }