Simplify handling of BiDi cursor movement

ui/gfx/render_text.h

Index: ui/gfx/render_text.h
diff --git a/ui/gfx/render_text.h b/ui/gfx/render_text.h
index db98b846057abbc7a8a0b63e0b1f7ce81953b3a6..6eb846721f5b655ad6130e50daab0df520b0c8de 100644
--- a/ui/gfx/render_text.h
+++ b/ui/gfx/render_text.h
@@ -90,23 +90,6 @@ enum HorizontalAlignment {
   ALIGN_RIGHT,
 };
 
-// VisualCursorDirection and LogicalCursorDirection represent directions of
-// motion of the cursor in BiDi text. The combinations that make sense are:
-//
-//  base::i18n::TextDirection  VisualCursorDirection  LogicalCursorDirection
-//       LEFT_TO_RIGHT             CURSOR_LEFT           CURSOR_BACKWARD
-//       LEFT_TO_RIGHT             CURSOR_RIGHT          CURSOR_FORWARD
-//       RIGHT_TO_LEFT             CURSOR_RIGHT          CURSOR_BACKWARD
-//       RIGHT_TO_LEFT             CURSOR_LEFT           CURSOR_FORWARD
-enum VisualCursorDirection {
-  CURSOR_LEFT,
-  CURSOR_RIGHT
-};
-enum LogicalCursorDirection {
-  CURSOR_BACKWARD,
-  CURSOR_FORWARD
-};
-
 // RenderText represents an abstract model of styled text and its corresponding
 // visual layout. Support is built in for a cursor, a selection, simple styling,
 // complex scripts, and bi-directional text. Implementations provide mechanisms
@@ -160,11 +143,7 @@ class UI_EXPORT RenderText {
   void set_fade_tail(bool fade_tail) { fade_tail_ = fade_tail; }
   bool fade_tail() const { return fade_tail_; }
 
-  // This cursor position corresponds to SelectionModel::selection_end. In
-  // addition to representing the selection end, it's also where logical text
-  // edits take place, and doesn't necessarily correspond to
-  // SelectionModel::caret_pos.
-  size_t GetCursorPosition() const;
+  size_t cursor_position() const { return selection_model_.caret_pos(); }

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

is this style only used for getter of data members? for others,  should be
CamelCase.

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

The style guide says this_style is for members that "do so little work they're
effectively free". The cost of these methods is the same as the cost of a simple
getter (since selection_model_ is bodily contained in RenderText).

   void SetCursorPosition(size_t position);
 
   // Moves the cursor left or right. Cursor movement is visual, meaning that
@@ -175,11 +154,10 @@ class UI_EXPORT RenderText {
                   bool select);
 
   // Set the selection_model_ to the value of |selection|.
-  // The selection model components are modified if invalid.
+  // The selection range is clamped to text().length() if out of range.
   // Returns true if the cursor position or selection range changed.
-  // If |selectin_start_| or |selection_end_| or |caret_pos_| in
-  // |selection_model| is not a cursorable position (not on grapheme boundary),
-  // it is a NO-OP and returns false.
+  // If any index in |selection_model| is not a cursorable position (not on a
+  // grapheme boundary), it is a no-op and returns false.
   bool MoveCursorTo(const SelectionModel& selection_model);
 
   // Move the cursor to the position associated with the clicked point.
@@ -194,18 +172,7 @@ class UI_EXPORT RenderText {
   // boundary), it is a NO-OP and returns false. Otherwise, returns true.
   bool SelectRange(const ui::Range& range);
 
-  size_t GetSelectionStart() const {
-    return selection_model_.selection_start();
-  }
-  size_t MinOfSelection() const {
-    return std::min(GetSelectionStart(), GetCursorPosition());
-  }
-  size_t MaxOfSelection() const {
-    return std::max(GetSelectionStart(), GetCursorPosition());
-  }
-  bool EmptySelection() const {
-    return GetSelectionStart() == GetCursorPosition();
-  }
+  const ui::Range& selection() const { return selection_model_.selection(); }

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

ditto

 
   // Returns true if the local point is over selected text.
   bool IsPointInSelection(const Point& point);
@@ -232,8 +199,13 @@ class UI_EXPORT RenderText {
   // |GetTextDirection()|, not the direction of a particular run.
   VisualCursorDirection GetVisualDirectionOfLogicalEnd();
 
-  // Get the width of the entire string.
-  virtual int GetStringWidth() = 0;
+  // Get the size in pixels of the entire string.
+  virtual Size GetStringSize() = 0;
+
+  // Get the height and horizontal bounds of the glyph at the given index in the
+  // current text. If the glyph is RTL then xspan->is_reversed(). This does not
+  // return a Rect because a Rect can't have a negative width.
+  virtual void GetGlyphBounds(size_t index, ui::Range* xspan, int* height) = 0;
 
   void Draw(Canvas* canvas);
 
@@ -245,8 +217,7 @@ class UI_EXPORT RenderText {
   // bounds of the associated glyph. These bounds are in local coordinates, but
   // may be outside the visible region if the text is longer than the textfield.
   // Subsequent text, cursor, or bounds changes may invalidate returned values.
-  virtual Rect GetCursorBounds(const SelectionModel& selection,
-                               bool insert_mode) = 0;
+  Rect GetCursorBounds(const SelectionModel& selection, bool insert_mode);
 
   // Compute the current cursor bounds, panning the text to show the cursor in
   // the display rect if necessary. These bounds are in local coordinates.
@@ -297,19 +268,18 @@ class UI_EXPORT RenderText {
 
   // Get the SelectionModels corresponding to visual text ends.
   // The returned value represents a cursor/caret position without a selection.
-  virtual SelectionModel EdgeSelectionModel(
-      VisualCursorDirection direction) = 0;
+  SelectionModel EdgeSelectionModel(VisualCursorDirection direction);
 
   // Sets the selection model, the argument is assumed to be valid.
   virtual void SetSelectionModel(const SelectionModel& model);
 
-  // Get the visual bounds containing the logical substring within |from| to
-  // |to|. If |from| equals |to|, the result is empty. These bounds could be
-  // visually discontinuous if the substring is split by a LTR/RTL level change.
+  // Get the visual bounds containing the logical substring within the |range|.
+  // If |range| is empty, the result is empty. These bounds could be visually
+  // discontinuous if the substring is split by a LTR/RTL level change.
   // These bounds are in local coordinates, but may be outside the visible
   // region if the text is longer than the textfield. Subsequent text, cursor,
   // or bounds changes may invalidate returned values.
-  virtual std::vector<Rect> GetSubstringBounds(size_t from, size_t to) = 0;
+  virtual std::vector<Rect> GetSubstringBounds(ui::Range range) = 0;
 
   // Return true if cursor can appear in front of the character at |position|,
   // which means it is a grapheme boundary or the first character in the text.
@@ -350,6 +320,12 @@ class UI_EXPORT RenderText {
   // Applies fade effects to |renderer|.
   void ApplyFadeEffects(internal::SkiaTextRenderer* renderer);
 
+  // A convenience function to check whether the glyph attached to the caret
+  // is within the given range.
+  static bool RangeContainsCaret(const ui::Range& range,
+                                 size_t caret_pos,
+                                 LogicalCursorDirection caret_affinity);
+
  private:
   friend class RenderTextTest;
 
@@ -358,7 +334,7 @@ class UI_EXPORT RenderText {
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, ApplyStyleRange);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, StyleRangesAdjust);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, GraphemePositions);
-  FRIEND_TEST_ALL_PREFIXES(RenderTextTest, SelectionModels);
+  FRIEND_TEST_ALL_PREFIXES(RenderTextTest, EdgeSelectionModels);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, OriginForSkiaDrawing);
 
   // Set the cursor to |position|, with the caret trailing the previous