Simplify handling of BiDi cursor movement

ui/gfx/render_text_linux.cc

Index: ui/gfx/render_text_linux.cc
diff --git a/ui/gfx/render_text_linux.cc b/ui/gfx/render_text_linux.cc
index 133bd4eb63d435ff7daf451b8f08f9fb4a889b0d..ede30e4accf931c8dacd8d9614bd45fcf79bf5b7 100644
--- a/ui/gfx/render_text_linux.cc
+++ b/ui/gfx/render_text_linux.cc
@@ -78,130 +78,93 @@ base::i18n::TextDirection RenderTextLinux::GetTextDirection() {
   return base::i18n::LEFT_TO_RIGHT;
 }
 
-int RenderTextLinux::GetStringWidth() {
+Size RenderTextLinux::GetStringSize() {
   EnsureLayout();
-  int width;
-  pango_layout_get_pixel_size(layout_, &width, NULL);
-  return width;
+  int width, height;

[msw, 2012/02/16 00:09:21]

Please explicitly init these locals.

[benrg, 2012/02/16 09:20:55]

I'm not sure what to initialize them to.

[msw, 2012/02/21 21:13:39]

I see that you updated them to 0; that's better than garbage, thanks.
(Just saying "Done" would have saved us both a little time)

+  pango_layout_get_pixel_size(layout_, &width, &height);
+  return Size(width, height);
+}
+
+void RenderTextLinux::GetGlyphBounds(size_t index,
+                                     ui::Range* xspan,
+                                     int* height) {
+  PangoRectangle pos;
+  pango_layout_index_to_pos(layout_, Utf16IndexToUtf8Index(index), &pos);
+  *xspan = ui::Range(PANGO_PIXELS(pos.x), PANGO_PIXELS(pos.x + pos.width));
+  *height = PANGO_PIXELS(pos.height);
 }
 
 SelectionModel RenderTextLinux::FindCursorPosition(const Point& point) {
   EnsureLayout();
 
   if (text().empty())
-    return SelectionModel(0, 0, SelectionModel::LEADING);
+    return SelectionModel(0, CURSOR_FORWARD);
 
   Point p(ToTextPoint(point));
 
   // When the point is outside of text, return HOME/END position.
   if (p.x() < 0)
     return EdgeSelectionModel(CURSOR_LEFT);
-  else if (p.x() > GetStringWidth())
+  else if (p.x() > GetStringSize().width())
     return EdgeSelectionModel(CURSOR_RIGHT);
 
   int caret_pos, trailing;
   pango_layout_xy_to_index(layout_, p.x() * PANGO_SCALE, p.y() * PANGO_SCALE,
                            &caret_pos, &trailing);
 
-  size_t selection_end = caret_pos;
-  if (trailing > 0) {
-    const char* ch = g_utf8_offset_to_pointer(layout_text_ + caret_pos,
-                                              trailing);
-    DCHECK_GE(ch, layout_text_);
-    DCHECK_LE(ch, layout_text_ + layout_text_len_);
-    selection_end = ch - layout_text_;
+  DCHECK(trailing >= 0);

[msw, 2012/02/16 00:09:21]

Use DCHECK_GE.

+  if (trailing) {

[msw, 2012/02/16 00:09:21]

Why remove the explicit check for "> 0"?

[benrg, 2012/02/16 09:20:55]

No reason. Reverted.

+    caret_pos = g_utf8_offset_to_pointer(layout_text_ + caret_pos,
+                                         trailing) - layout_text_;
+    DCHECK_LE(static_cast<size_t>(caret_pos), layout_text_len_);
   }
 
   return SelectionModel(
-      Utf8IndexToUtf16Index(selection_end),
       Utf8IndexToUtf16Index(caret_pos),

[msw, 2012/02/16 00:09:21]

nit: move to the line above and fix the next line's indent.

-      trailing > 0 ? SelectionModel::TRAILING : SelectionModel::LEADING);
-}
-
-Rect RenderTextLinux::GetCursorBounds(const SelectionModel& selection,
-                                      bool insert_mode) {
-  EnsureLayout();
-
-  size_t caret_pos = insert_mode ? selection.caret_pos() :
-                                   selection.selection_end();
-  PangoRectangle pos;
-  pango_layout_index_to_pos(layout_, Utf16IndexToUtf8Index(caret_pos), &pos);
-
-  SelectionModel::CaretPlacement caret_placement = selection.caret_placement();
-  int x = pos.x;
-  if ((insert_mode && caret_placement == SelectionModel::TRAILING) ||
-      (!insert_mode && pos.width < 0))
-    x += pos.width;
-  x = PANGO_PIXELS(x);
-
-  int h = std::min(display_rect().height(), PANGO_PIXELS(pos.height));
-  Rect bounds(x, (display_rect().height() - h) / 2, 0, h);
-  bounds.set_origin(ToViewPoint(bounds.origin()));
-
-  if (!insert_mode)
-    bounds.set_width(PANGO_PIXELS(std::abs(pos.width)));
-
-  return bounds;
+      trailing ? CURSOR_BACKWARD : CURSOR_FORWARD);

[xji, 2012/02/16 02:38:29]

"trailing > 0" here too.

 }
 
-// Assume caret_pos in |current| is n, 'l' represents leading in
-// caret_placement and 't' represents trailing in caret_placement. Following
-// is the calculation from (caret_pos, caret_placement) in |current| to
-// (selection_end, caret_pos, caret_placement) when moving cursor left/right by
-// one grapheme (for simplicity, assume each grapheme is one character).
-// If n is in LTR (if moving left) or RTL (if moving right) run,
-// (n, t) --> (n, n, l).
-// (n, l) --> (n-1, n-1, l) if n is inside run (not at boundary).
-// (n, l) --> goto across run case if n is at run boundary.
-// Otherwise,
-// (n, l) --> (n+1, n, t).
-// (n, t) --> (n+2, n+1, t) if n is inside run.
-// (n, t) --> goto across run case if n is at run boundary.
-// If n is at run boundary, get its visually left/right run,
-// If left/right run is LTR/RTL run,
-// (n, t) --> (left/right run's end, left/right run's end, l).
-// Otherwise,
-// (n, t) --> (left/right run's begin + 1, left/right run's begin, t).
 SelectionModel RenderTextLinux::AdjacentCharSelectionModel(
     const SelectionModel& selection,
     VisualCursorDirection direction) {
   size_t caret = selection.caret_pos();
-  SelectionModel::CaretPlacement caret_placement = selection.caret_placement();
-  GSList* run = GetRunContainingPosition(caret);
-  DCHECK(run);
-
-  PangoLayoutRun* layout_run = reinterpret_cast<PangoLayoutRun*>(run->data);
-  PangoItem* item = layout_run->item;
-  size_t run_start = Utf8IndexToUtf16Index(item->offset);
-  size_t run_end = Utf8IndexToUtf16Index(item->offset + item->length);
-
-  if (!IsForwardMotion(direction, item)) {
-    if (caret_placement == SelectionModel::TRAILING)
-      return SelectionModel(caret, caret, SelectionModel::LEADING);
-    else if (caret > run_start) {
-      caret = IndexOfAdjacentGrapheme(caret, CURSOR_BACKWARD);
-      return SelectionModel(caret, caret, SelectionModel::LEADING);
-    }
+
+  GSList* run = GetRunContainingCaret(selection);
+  if (!run) {
+    // Cursor not in any run: we're at the visual & logical edge.

[msw, 2012/02/16 00:09:21]

grammar nit: "The cursor is not". Also, use "and" instead of "&".

+    SelectionModel edge = EdgeSelectionModel(direction);
+    if (text().empty() ||
+        edge == SelectionModel(caret, selection.caret_affinity()))

[msw, 2012/02/16 00:09:21]

Use |selection| instead of constructing an identical SelectionModel.

[benrg, 2012/02/16 09:20:55]

It's not identical since |selection| might have a nonempty selection range. But
actually a much simpler test works here.

+      return edge;
+    else if (direction == CURSOR_RIGHT)

[xji, 2012/02/16 02:38:29]

why need to check the condition at line 136/137?
and in which cases it reaches 'else if' and 'else'?

[benrg, 2012/02/16 09:20:55]

if (!run) then the caret is either at (0, backward) or (text_length, forward).
If it's the correct side of the text then we're done (first case), but if it's
the opposite side then we need to cursor into the text.

[xji, 2012/02/16 19:52:55]

Ha, I realized that. Thanks for the explanation.

+      run = current_line_->runs;
+    else
+      run = g_slist_last(current_line_->runs);
   } else {
-    if (caret_placement == SelectionModel::LEADING) {
-      size_t cursor = IndexOfAdjacentGrapheme(caret, CURSOR_FORWARD);
-      return SelectionModel(cursor, caret, SelectionModel::TRAILING);
-    } else if (selection.selection_end() < run_end) {
-      caret = IndexOfAdjacentGrapheme(caret, CURSOR_FORWARD);
-      size_t cursor = IndexOfAdjacentGrapheme(caret, CURSOR_FORWARD);
-      return SelectionModel(cursor, caret, SelectionModel::TRAILING);
+    // If the cursor is moving within the current run, just move it by one
+    // grapheme in the appropriate direction.
+    PangoItem* item = reinterpret_cast<PangoLayoutRun*>(run->data)->item;
+    if (IsForwardMotion(direction, item)) {
+      size_t run_end = Utf8IndexToUtf16Index(item->offset + item->length);
+      if (caret < run_end) {

[msw, 2012/02/16 00:09:21]

nit: inline the |run_end| value calc here.

+        caret = IndexOfAdjacentGrapheme(caret, CURSOR_FORWARD);
+        return SelectionModel(caret, CURSOR_BACKWARD);
+      }
+    } else {
+      size_t run_start = Utf8IndexToUtf16Index(item->offset);
+      if (caret > run_start) {

[msw, 2012/02/16 00:09:21]

nit: inline the |run_start| value calc here and use "else if" above.

[benrg, 2012/02/16 09:20:55]

The then and else clauses here are mirror images. I think it's clearer if they
have the same indentation.

+        caret = IndexOfAdjacentGrapheme(caret, CURSOR_BACKWARD);
+        return SelectionModel(caret, CURSOR_FORWARD);
+      }
     }
+    // The cursor is at the edge of a run; move to the visually adjacent run.
+    // TODO(xji): Keep a vector of runs to avoid using a singly-linked list.
+    run = (direction == CURSOR_RIGHT) ?
+        run->next : GSListPrevious(current_line_->runs, run);
+    if (!run)
+      return EdgeSelectionModel(direction);
   }
-
-  // The character is at the edge of its run; advance to the adjacent visual
-  // run.
-  // TODO(xji): Keep a vector of runs to avoid using a singly-linked list.
-  GSList* adjacent_run = (direction == CURSOR_RIGHT) ?
-      run->next : GSListPrevious(current_line_->runs, run);
-  if (!adjacent_run)
-    return EdgeSelectionModel(direction);
-
-  item = reinterpret_cast<PangoLayoutRun*>(adjacent_run->data)->item;
+  PangoItem* item = reinterpret_cast<PangoLayoutRun*>(run->data)->item;
   return IsForwardMotion(direction, item) ?
       FirstSelectionModelInsideRun(item) : LastSelectionModelInsideRun(item);
 }
@@ -215,69 +178,41 @@ SelectionModel RenderTextLinux::AdjacentWordSelectionModel(
   if (!success)
     return selection;
 
-  SelectionModel end = EdgeSelectionModel(direction);
   SelectionModel cur(selection);
-  while (!cur.Equals(end)) {
+  for (;;) {
     cur = AdjacentCharSelectionModel(cur, direction);
-    size_t caret = cur.caret_pos();
-    GSList* run = GetRunContainingPosition(caret);
-    DCHECK(run);
+    GSList* run = GetRunContainingCaret(cur);
+    if (!run)
+      break;
     PangoItem* item = reinterpret_cast<PangoLayoutRun*>(run->data)->item;
-    size_t cursor = cur.selection_end();
+    size_t cursor = cur.caret_pos();
     if (IsForwardMotion(direction, item) ?
         iter.IsEndOfWord(cursor) : iter.IsStartOfWord(cursor))
-      return cur;
+      break;
   }
 
-  return end;
-}
-
-SelectionModel RenderTextLinux::EdgeSelectionModel(
-    VisualCursorDirection direction) {
-  if (direction == GetVisualDirectionOfLogicalEnd()) {
-    // Advance to the logical end of the text.
-    GSList* run = current_line_->runs;
-    if (direction == CURSOR_RIGHT)
-      run = g_slist_last(run);
-    if (run) {
-      PangoLayoutRun* end_run = reinterpret_cast<PangoLayoutRun*>(run->data);
-      PangoItem* item = end_run->item;
-      if (IsForwardMotion(direction, item)) {
-        size_t caret = Utf8IndexToUtf16Index(
-            Utf8IndexOfAdjacentGrapheme(item->offset + item->length,
-                                        CURSOR_BACKWARD));
-        return SelectionModel(text().length(), caret, SelectionModel::TRAILING);
-      } else {
-        size_t caret = Utf8IndexToUtf16Index(item->offset);
-        return SelectionModel(text().length(), caret, SelectionModel::LEADING);
-      }
-    }
-  }
-  return SelectionModel(0, 0, SelectionModel::LEADING);
+  return cur;
 }
 
 void RenderTextLinux::SetSelectionModel(const SelectionModel& model) {
-  if (GetSelectionStart() != model.selection_start() ||
-      GetCursorPosition() != model.selection_end()) {
+  if (selection() != model.selection())
     selection_visual_bounds_.clear();
-  }
 
   RenderText::SetSelectionModel(model);
 }
 
-std::vector<Rect> RenderTextLinux::GetSubstringBounds(size_t from, size_t to) {
-  DCHECK(from <= text().length());
-  DCHECK(to <= text().length());
+std::vector<Rect> RenderTextLinux::GetSubstringBounds(ui::Range range) {
+  DCHECK(range.GetMax() <= text().length());

[msw, 2012/02/16 00:09:21]

DCHECK_LE

 
-  if (from == to)
+  if (range.is_empty())
     return std::vector<Rect>();
 
   EnsureLayout();
 
-  if (from == GetSelectionStart() && to == GetCursorPosition())
+  if (range == selection())
     return GetSelectionBounds();
   else
-    return CalculateSubstringBounds(from, to);
+    return CalculateSubstringBounds(range);

[msw, 2012/02/16 00:09:21]

Move this return out of the else block.

 }
 
 bool RenderTextLinux::IsCursorablePosition(size_t position) {
@@ -473,14 +408,15 @@ size_t RenderTextLinux::IndexOfAdjacentGrapheme(
       Utf8IndexOfAdjacentGrapheme(Utf16IndexToUtf8Index(index), direction));
 }
 
-GSList* RenderTextLinux::GetRunContainingPosition(size_t position) const {
+GSList* RenderTextLinux::GetRunContainingCaret(
+    const SelectionModel& caret) const {
+  size_t position = Utf16IndexToUtf8Index(caret.caret_pos());
+  LogicalCursorDirection affinity = caret.caret_affinity();
   GSList* run = current_line_->runs;
   while (run) {
     PangoItem* item = reinterpret_cast<PangoLayoutRun*>(run->data)->item;
-    size_t run_start = Utf8IndexToUtf16Index(item->offset);
-    size_t run_end = Utf8IndexToUtf16Index(item->offset + item->length);
-
-    if (position >= run_start && position < run_end)
+    if (RangeContainsCaret(ui::Range(item->offset, item->offset + item->length),

[xji, 2012/02/16 02:38:29]

hm... what happens if you continuously press left/right arrow after reach visual
boundary?

[xji, 2012/02/16 02:40:51]

forgot to update the comment. It is handled in moving left right function.

+                           position, affinity))
       return run;
     run = run->next;
   }
@@ -515,17 +451,16 @@ size_t RenderTextLinux::Utf8IndexOfAdjacentGrapheme(
 
 SelectionModel RenderTextLinux::FirstSelectionModelInsideRun(
     const PangoItem* item) const {
-  size_t caret = Utf8IndexToUtf16Index(item->offset);
   size_t cursor = Utf8IndexToUtf16Index(
       Utf8IndexOfAdjacentGrapheme(item->offset, CURSOR_FORWARD));
-  return SelectionModel(cursor, caret, SelectionModel::TRAILING);
+  return SelectionModel(cursor, CURSOR_BACKWARD);
 }
 
 SelectionModel RenderTextLinux::LastSelectionModelInsideRun(
     const PangoItem* item) const {
   size_t caret = Utf8IndexToUtf16Index(Utf8IndexOfAdjacentGrapheme(
       item->offset + item->length, CURSOR_BACKWARD));
-  return SelectionModel(caret, caret, SelectionModel::LEADING);
+  return SelectionModel(caret, CURSOR_FORWARD);
 }
 
 void RenderTextLinux::ResetLayout() {
@@ -575,16 +510,13 @@ size_t RenderTextLinux::Utf8IndexToUtf16Index(size_t index) const {
   return utf16_index;
 }
 
-std::vector<Rect> RenderTextLinux::CalculateSubstringBounds(size_t from,
-                                                            size_t to) {
+std::vector<Rect> RenderTextLinux::CalculateSubstringBounds(ui::Range range) {
   int* ranges;
   int n_ranges;
-  size_t from_in_utf8 = Utf16IndexToUtf8Index(from);
-  size_t to_in_utf8 = Utf16IndexToUtf8Index(to);
   pango_layout_line_get_x_ranges(
       current_line_,
-      std::min(from_in_utf8, to_in_utf8),
-      std::max(from_in_utf8, to_in_utf8),
+      Utf16IndexToUtf8Index(range.GetMin()),
+      Utf16IndexToUtf8Index(range.GetMax()),
       &ranges,
       &n_ranges);
 
@@ -607,8 +539,7 @@ std::vector<Rect> RenderTextLinux::CalculateSubstringBounds(size_t from,
 
 std::vector<Rect> RenderTextLinux::GetSelectionBounds() {
   if (selection_visual_bounds_.empty())
-    selection_visual_bounds_ =
-        CalculateSubstringBounds(GetSelectionStart(), GetCursorPosition());
+    selection_visual_bounds_ = CalculateSubstringBounds(selection());
   return selection_visual_bounds_;
 }