fix: selection and highlight for mixed bidi text

Fixes two issues:
- When writing RTL text in an LTR line the carret disappears since the
last glyph is not necessarily at the end of the line
- `highlight()` used to return just one (x_left, x_width) but in Bidi
text you can't draw the selection rectangle with only one rectangle.
This issue wasn't visible before because Editor was drawing the
selection rectangle per glyph and not using `highlight()` method

src/buffer.rs

@@ -40,44 +40,131 @@ pub struct LayoutRun<'a> {
 }
 
 impl LayoutRun<'_> {
-    /// Return the pixel span `Some((x_left, x_width))` of the highlighted area between `cursor_start`
+    /// Return an iterator of `(x_left, x_width)` pixel spans for the highlighted areas
-    /// and `cursor_end` within this run, or None if the cursor range does not intersect this run.
+    /// between `cursor_start` and `cursor_end` within this run.
-    /// This may return widths of zero if `cursor_start == cursor_end`, if the run is empty, or if the
+    ///
-    /// region's left start boundary is the same as the cursor's end boundary or vice versa.
+    /// For pure LTR or pure RTL runs this yields at most one span. For mixed BiDi runs
-    #[allow(clippy::missing_panics_doc)]
+    /// (where selected and unselected glyphs interleave visually) it yields multiple
-    pub fn highlight(&self, cursor_start: Cursor, cursor_end: Cursor) -> Option<(f32, f32)> {
+    /// disjoint spans.
-        let mut x_start = None;
+    ///
-        let mut x_end = None;
+    /// Returns an empty iterator if the cursor range does not intersect this run.
-        let rtl_factor = if self.rtl { 1. } else { 0. };
+    pub fn highlight(
-        let ltr_factor = 1. - rtl_factor;
+        &self,
+        cursor_start: Cursor,
+        cursor_end: Cursor,
+    ) -> impl Iterator<Item = (f32, f32)> {
+        let line_i = self.line_i;
+        let mut results = Vec::new();
+        let mut range_opt: Option<(f32, f32)> = None;
+
         for glyph in self.glyphs {
-            let cursor = self.cursor_from_glyph_left(glyph);
+            let cluster = &self.text[glyph.start..glyph.end];
-            if cursor >= cursor_start && cursor <= cursor_end {
+            let total = cluster.grapheme_indices(true).count().max(1);
-                if x_start.is_none() {
+            let c_w = glyph.w / total as f32;
-                    x_start = Some(glyph.x + glyph.w.mul_add(rtl_factor, 0.0));
+            let mut c_x = glyph.x;
+
+            for (i, c) in cluster.grapheme_indices(true) {
+                let c_start = glyph.start + i;
+                let c_end = glyph.start + i + c.len();
+
+                let is_selected = (cursor_start.line != line_i || c_end > cursor_start.index)
+                    && (cursor_end.line != line_i || c_start < cursor_end.index);
+
+                if is_selected {
+                    range_opt = Some(match range_opt {
+                        Some((min, max)) => (min.min(c_x), max.max(c_x + c_w)),
+                        None => (c_x, c_x + c_w),
+                    });
+                } else if let Some((min_x, max_x)) = range_opt.take() {
+                    let width = max_x - min_x;
+                    if width > 0.0 {
+                        results.push((min_x, width));
                     }
-                x_end = Some(glyph.x + glyph.w.mul_add(rtl_factor, 0.0));
-            }
-            let cursor = self.cursor_from_glyph_right(glyph);
-            if cursor >= cursor_start && cursor <= cursor_end {
-                if x_start.is_none() {
-                    x_start = Some(glyph.x + glyph.w.mul_add(ltr_factor, 0.0));
-                }
-                x_end = Some(glyph.x + glyph.w.mul_add(ltr_factor, 0.0));
-            }
-        }
-        x_start.map(|x_start| {
-            let x_end = x_end.expect("end of cursor not found");
-            let (x_start, x_end) = if x_start < x_end {
-                (x_start, x_end)
-            } else {
-                (x_end, x_start)
-            };
-            (x_start, x_end - x_start)
-        })
                 }
 
-    const fn cursor_from_glyph_left(&self, glyph: &LayoutGlyph) -> Cursor {
+                c_x += c_w;
+            }
+        }
+
+        // Flush remaining highlighted region
+        if let Some((min_x, max_x)) = range_opt {
+            let width = max_x - min_x;
+            if width > 0.0 {
+                results.push((min_x, width));
+            }
+        }
+
+        results.into_iter()
+    }
+
+    /// Returns the visual x position (in pixels) of `cursor` within this run,
+    /// or `None` if the cursor does not belong to this run.
+    ///
+    /// For RTL glyphs the cursor is placed at the right edge minus the offset;
+    /// for LTR glyphs it is placed at the left edge plus the offset.
+    pub fn cursor_position(&self, cursor: &Cursor) -> Option<f32> {
+        let (glyph_idx, glyph_offset) = self.cursor_glyph(cursor)?;
+        let x = self.glyphs.get(glyph_idx).map_or_else(
+            || {
+                // Past-the-end: position after the last glyph
+                self.glyphs.last().map_or(0.0, |glyph| {
+                    if glyph.level.is_rtl() {
+                        glyph.x
+                    } else {
+                        glyph.x + glyph.w
+                    }
+                })
+            },
+            |glyph| {
+                if glyph.level.is_rtl() {
+                    glyph.x + glyph.w - glyph_offset
+                } else {
+                    glyph.x + glyph_offset
+                }
+            },
+        );
+        Some(x)
+    }
+
+    /// Find which glyph in this run contains `cursor`, returning
+    /// `(glyph_index, pixel_offset_within_glyph)`, or `None` if the cursor
+    /// is not on this run.
+    pub fn cursor_glyph(&self, cursor: &Cursor) -> Option<(usize, f32)> {
+        if cursor.line != self.line_i {
+            return None;
+        }
+        for (glyph_i, glyph) in self.glyphs.iter().enumerate() {
+            if cursor.index == glyph.start {
+                return Some((glyph_i, 0.0));
+            } else if cursor.index > glyph.start && cursor.index < glyph.end {
+                // Guess x offset based on graphemes within the cluster
+                let cluster = &self.text[glyph.start..glyph.end];
+                let mut before = 0;
+                let mut total = 0;
+                for (i, _) in cluster.grapheme_indices(true) {
+                    if glyph.start + i < cursor.index {
+                        before += 1;
+                    }
+                    total += 1;
+                }
+                let offset = glyph.w * (before as f32) / (total as f32);
+                return Some((glyph_i, offset));
+            }
+        }
+        // in mixed BiDi the last logical glyph may not be the last visual glyph.
+        for (glyph_i, glyph) in self.glyphs.iter().enumerate() {
+            if cursor.index == glyph.end {
+                return Some((glyph_i, glyph.w));
+            }
+        }
+        if self.glyphs.is_empty() {
+            return Some((0, 0.0));
+        }
+        None
+    }
+
+    /// Get the left-edge cursor position of a glyph, accounting for paragraph direction.
+    pub const fn cursor_from_glyph_left(&self, glyph: &LayoutGlyph) -> Cursor {
         if self.rtl {
             Cursor::new_with_affinity(self.line_i, glyph.end, Affinity::Before)
         } else {
@@ -85,7 +172,8 @@ impl LayoutRun<'_> {
         }
     }
 
-    const fn cursor_from_glyph_right(&self, glyph: &LayoutGlyph) -> Cursor {
+    /// Get the right-edge cursor position of a glyph, accounting for paragraph direction.
+    pub const fn cursor_from_glyph_right(&self, glyph: &LayoutGlyph) -> Cursor {
         if self.rtl {
             Cursor::new_with_affinity(self.line_i, glyph.start, Affinity::After)
         } else {

src/edit/editor.rs

@@ -27,64 +27,9 @@ pub struct Editor<'buffer> {
     change: Option<Change>,
 }
 
-fn cursor_glyph_opt(cursor: &Cursor, run: &LayoutRun) -> Option<(usize, f32)> {
-    if cursor.line == run.line_i {
-        for (glyph_i, glyph) in run.glyphs.iter().enumerate() {
-            if cursor.index == glyph.start {
-                return Some((glyph_i, 0.0));
-            } else if cursor.index > glyph.start && cursor.index < glyph.end {
-                // Guess x offset based on characters
-                let mut before = 0;
-                let mut total = 0;
-
-                let cluster = &run.text[glyph.start..glyph.end];
-                for (i, _) in cluster.grapheme_indices(true) {
-                    if glyph.start + i < cursor.index {
-                        before += 1;
-                    }
-                    total += 1;
-                }
-
-                let offset = glyph.w * (before as f32) / (total as f32);
-                return Some((glyph_i, offset));
-            }
-        }
-        match run.glyphs.last() {
-            Some(glyph) => {
-                if cursor.index == glyph.end {
-                    return Some((run.glyphs.len(), 0.0));
-                }
-            }
-            None => {
-                return Some((0, 0.0));
-            }
-        }
-    }
-    None
-}
-
 fn cursor_position(cursor: &Cursor, run: &LayoutRun) -> Option<(i32, i32)> {
-    let (cursor_glyph, cursor_glyph_offset) = cursor_glyph_opt(cursor, run)?;
+    let x = run.cursor_position(cursor)?;
-    let x = run.glyphs.get(cursor_glyph).map_or_else(
+    Some((x as i32, run.line_top as i32))
-        || {
-            run.glyphs.last().map_or(0, |glyph| {
-                if glyph.level.is_rtl() {
-                    glyph.x as i32
-                } else {
-                    (glyph.x + glyph.w) as i32
-                }
-            })
-        },
-        |glyph| {
-            if glyph.level.is_rtl() {
-                (glyph.x + glyph.w - cursor_glyph_offset) as i32
-            } else {
-                (glyph.x + cursor_glyph_offset) as i32
-            }
-        },
-    );
-
-    Some((x, run.line_top as i32))
 }
 
 impl<'buffer> Editor<'buffer> {
@@ -149,53 +94,34 @@ impl<'buffer> Editor<'buffer> {
                 // Highlight selection
                 if let Some((start, end)) = selection_bounds {
                     if line_i >= start.line && line_i <= end.line {
-                        let mut range_opt = None;
+                        let highlights: Vec<(f32, f32)> = run.highlight(start, end).collect();
-                        for glyph in run.glyphs {
+
-                            // Guess x offset based on characters
+                        if highlights.is_empty() && run.glyphs.is_empty() && end.line > line_i {
-                            let cluster = &run.text[glyph.start..glyph.end];
+                            // Highlight all of internal empty lines
-                            let total = cluster.grapheme_indices(true).count();
+                            let max = buffer.size().0.unwrap_or(0.0) as i32;
-                            let mut c_x = glyph.x;
-                            let c_w = glyph.w / total as f32;
-                            for (i, c) in cluster.grapheme_indices(true) {
-                                let c_start = glyph.start + i;
-                                let c_end = glyph.start + i + c.len();
-                                if (start.line != line_i || c_end > start.index)
-                                    && (end.line != line_i || c_start < end.index)
-                                {
-                                    range_opt = match range_opt.take() {
-                                        Some((min, max)) => Some((
-                                            cmp::min(min, c_x as i32),
-                                            cmp::max(max, (c_x + c_w) as i32),
-                                        )),
-                                        None => Some((c_x as i32, (c_x + c_w) as i32)),
-                                    };
-                                } else if let Some((min, max)) = range_opt.take() {
                             renderer.rectangle(
-                                        min,
+                                0,
                                 line_top as i32,
-                                        cmp::max(0, max - min) as u32,
+                                max as u32,
                                 line_height as u32,
                                 selection_color,
                             );
-                                }
+                        } else {
-                                c_x += c_w;
+                            let len = highlights.len();
-                            }
+                            for (idx, (x, width)) in highlights.into_iter().enumerate() {
-                        }
+                                let mut min = x as i32;
+                                let mut max = (x + width) as i32;
 
-                        if run.glyphs.is_empty() && end.line > line_i {
+                                // Extend the last rect to the line edge for
-                            // Highlight all of internal empty lines
+                                // multi-line selections
-                            range_opt = Some((0, buffer.size().0.unwrap_or(0.0) as i32));
+                                if idx == len - 1 && end.line > line_i {
-                        }
-
-                        if let Some((mut min, mut max)) = range_opt.take() {
-                            if end.line > line_i {
-                                // Draw to end of line
                                     if run.rtl {
                                         min = 0;
                                     } else {
                                         max = buffer.size().0.unwrap_or(0.0) as i32;
                                     }
                                 }
+
                                 renderer.rectangle(
                                     min,
                                     line_top as i32,
@@ -206,6 +132,7 @@ impl<'buffer> Editor<'buffer> {
                             }
                         }
                     }
+                }
 
                 // Draw cursor
                 if let Some((x, y)) = cursor_position(&self.cursor, &run) {