modification to RenderText for inheritance/SelectionModel

ui/gfx/render_text.h

Index: ui/gfx/render_text.h
===================================================================
--- ui/gfx/render_text.h	(revision 93971)
+++ ui/gfx/render_text.h	(working copy)
@@ -6,6 +6,7 @@
 #define UI_GFX_RENDER_TEXT_H_
 #pragma once
 
+#include <algorithm>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
@@ -17,7 +18,7 @@
 #include "ui/gfx/rect.h"
 #include "ui/gfx/point.h"
 
-namespace {
+namespace gfx {
 
 // Strike line width.
 const int kStrikeWidth = 2;
@@ -32,10 +33,6 @@
 const SkColor kUnfocusedSelectionColor = SK_ColorLTGRAY;
 const SkColor kCursorColor = SK_ColorBLACK;
 
-}  // namespace
-
-namespace gfx {
-
 class Canvas;
 class RenderTextTest;
 
@@ -59,6 +56,83 @@
   LINE_BREAK,
 };
 
+// SelectionModel is used to represent the logical selection and visual
+// position of cursor.
+//
+// For bidirectional text, the mappging between visual position and logical

[msw, 2011/08/01 05:02:23]

"bi-directional" and "mapping"

+// position is not one-to-one. For example, logical text "abcDEF" where capital
+// letter stands for Hebrew, the visual display is "abcFED". According to the

[msw, 2011/08/01 05:02:23]

"capital *letters stand*" or "*a* capital letter stands".
I would move this sentence with the example string below with the rest of the
example.

+// bidi editing guide:

[msw, 2011/08/01 05:02:23]

Perhaps we should formally post the bidi editing guide somewhere, otherwise this
mention is meaningless.

[xji, 2011/08/01 23:38:42]

You are right. Since the meeting note is not a completed document (and has
restricted accessibility), and the original guideline is from non-Googler and
may not be ready to public yet (besides we are modifying some of them), how
about I just have a place holder for the doc here?

[msw, 2011/08/02 01:29:06]

ok

+// 1. If pointing to the right half of the cell of a LTR character, the current
+// position must be set after this character and the caret must be displayed
+// after this character.
+// 2. If pointing to the right half of the cell of a RTL character, the current
+// position must be set before this character and the caret must be displayed
+// before this character.
+//
+// pointing to the right half of 'c' and pointing to the right half of 'D' both

[msw, 2011/08/01 05:02:23]

"*P*ointing"

+// set cursor logical position as 3. But the cursor displayed visually at

[msw, 2011/08/01 05:02:23]

"set *the* logical cursor position *to*"... "the cursor *is* displayed"

+// different places:
+// pointing right half of 'c' displays cursor right of 'c' as "abc|FED".

[msw, 2011/08/01 05:02:23]

"*P*ointing *to the* right"... "displays *the* cursor *to the* right".
Same for the next line. If you want to keep these as single lines, maybe write:
"Pointing to the right side of 'c' yields the visual cursor position "abc|FED.""

+// pointing right half of 'D' displays cursor right of 'D' as "abcFED|".
+// So, besides the logical selection start point and end point, we need extra

[msw, 2011/08/01 05:02:23]

"point*s*"

[xji, 2011/08/01 23:38:42]

change to "start and end points"?

[msw, 2011/08/02 01:29:06]

oh, i misread this. your suggestion or the current text is fine.

+// information to specify to which character and on which edge of the character
+// the visual cursor is bound to. For example, the visual cursor is bounds to

[msw, 2011/08/01 05:02:23]

"bound"

+// the trailing side of the 2nd character 'c' when point to right half of 'c'.

[msw, 2011/08/01 05:02:23]

"point*ing*"

+// And it bounds to the leading edge of the 3rd character 'D' when point to

[msw, 2011/08/01 05:02:23]

"binds" or "is bound to"... "point*ing*"

+// right of 'D'.
+class UI_API SelectionModel {
+ public:
+  enum LeadingTrailingStatus {
+    // PREVIOUS_GRAPHEME_TRAILING means cursor is visually attached to the
+    // trailing edge of previous grapheme.
+    PREVIOUS_GRAPHEME_TRAILING,

[msw, 2011/08/01 05:02:23]

We'll have to discuss how this behaves in Pango... I don't have anything similar
in Uniscribe, and I'll be implementing this manually.

[xji, 2011/08/01 23:38:42]

It is the default mode, used in insertText/deleteText. I need to think whether
this could be combined with LEADING/TRAILING, which means when insertText, we
need to set the cursor visual position in SelectionModel as
(cursor_logical_potion - 1, TRAILING). Let's discuss it more in person.

[msw, 2011/08/02 01:29:06]

ok, this is fine for now, we can work out kinks later.

+    LEADING,
+    TRAILING,
+  };
+
+  SelectionModel();
+  SelectionModel(size_t pos);

[msw, 2011/08/02 01:29:06]

Make this ctor explicit.

+  SelectionModel(size_t pos, size_t index, LeadingTrailingStatus status);
+
+  virtual ~SelectionModel();
+
+  size_t selection_start() const { return selection_start_; }
+  void set_selection_start(size_t start) { selection_start_ = start; }
+
+  size_t cursor_pos() const { return cursor_pos_; }
+  void set_cursor_pos(size_t pos) { cursor_pos_ = pos; }
+
+  size_t char_index() const { return char_index_; }
+  void set_char_index(size_t index) { char_index_ = index; }
+
+  LeadingTrailingStatus bounding_edge() const { return bounding_edge_; }
+  void set_bounding_edge(LeadingTrailingStatus edge) {
+      bounding_edge_ = edge;
+  }
+
+  virtual bool CursorChanged(const SelectionModel& sel) const {

[msw, 2011/08/01 05:02:23]

Make this an operator==

[xji, 2011/08/01 23:38:42]

But this does not check whether selection_start_ changes or not.

[msw, 2011/08/02 01:29:06]

Ah, okay. This should be renamed CusorEquals or similar, and make an explicit
note that it ignores the selection start.

+    return cursor_pos_ != sel.cursor_pos()
+           || char_index_ != sel.char_index()
+           || bounding_edge_ != sel.bounding_edge();
+  }
+
+ private:
+  void init(size_t pos, size_t index, LeadingTrailingStatus status);

[msw, 2011/08/01 05:02:23]

Capitalize "Init". Please separate these members with blank lines.

+  // Logical selection start. If there is non-empty selection, The cursor's

[msw, 2011/08/01 05:02:23]

un-capitalize "the cursor's"

+  // visual start point is the leading edge of the selection_start_.

[msw, 2011/08/01 05:02:23]

What do you mean by the "cursor's visual start point"?

[xji, 2011/08/01 23:38:42]

I am trying to say:
we only need to save cursor (selection end)'s visual position (char_index_,
bounding_edge) because the logical to visual mapping could be ambiguous.

When there is selection, the visual selection (begin to end) is not ambiguous.
The selection always starts visually at the leading edge of the
selection_start_.

Maybe I changed it to:
The selection always starts visually at the leading edge of the
selection_start_?

Any suggestions? 

[msw, 2011/08/02 01:29:06]

I think your suggested text is better, let's go with that. At some point we may
want a more complete description of the visual representation of a logical
selection range, up to you if you want to try to add that now.

+  // So, we do not need extra information for visual cursor bounding.
+  size_t selection_start_;
+  // The logical cursor position that next character will be inserted into.
+  // It is also the end of the selection.
+  size_t cursor_pos_;

[msw, 2011/08/01 05:02:23]

The dichotomy between cursor_pos_ and char_index_ is unclear from
their names. I don't have a great suggestion, but maybe rename "char_index_" as
"caret_pos_". Then we could use the terminology of cursor versus caret to
distinguish between logical and visual... I don't know...

Perhaps we could still represent the selection range as a ui::Range (using the
selection_range_.end() as the logical editing position), and use "cursor_pos_"
to mean the associated character where the cursor is drawn.

[xji, 2011/08/01 23:38:42]

Maybe change cursor_pos_ to selection_end_?
and change char_index_ to cursor_pos_?
but without using ui::Range. ( I feel a range inside a 4 data member class is a
bit overdo).

change char_index_ to caret_pos_ is fine too.

[msw, 2011/08/02 01:29:06]

Sure, let's do selection_end_ and cursor_pos_.

+  // The following 2 fields are used to guide cursor visual position.

[msw, 2011/08/01 05:02:23]

replace "2" with "two", if you must include it.

+  // The index of the character that cursor is visually attached to.
+  size_t char_index_;
+  // Cursor is attached to the leading or trailing edge of current character.

[msw, 2011/08/01 05:02:23]

"The visual placement of the cursor, relative to its associated character."

+  LeadingTrailingStatus bounding_edge_;

[msw, 2011/08/01 05:02:23]

Please rename "bounding_edge_", perhaps use "cursor_placement_" "cursor_edge_"
or "edge_binding_".

+};
+
 // TODO(msw): Implement RenderText[Win|Linux] for Uniscribe/Pango BiDi...
 
 // RenderText represents an abstract model of styled text and its corresponding
@@ -66,7 +140,6 @@
 // complex scripts, and bi-directional text. Implementations provide mechanisms
 // for rendering and translation between logical and visual data.
 class UI_API RenderText {
-
  public:
   virtual ~RenderText();
 
@@ -76,6 +149,8 @@
   const string16& text() const { return text_; }
   virtual void SetText(const string16& text);
 
+  void set_cursor_bounds_valid(bool valid) { cursor_bounds_valid_ = valid; }

[msw, 2011/08/01 05:02:23]

This should be protected.

+
   bool cursor_visible() const { return cursor_visible_; }
   void set_cursor_visible(bool visible) { cursor_visible_ = visible; }
 
@@ -89,34 +164,53 @@
   void set_default_style(StyleRange style) { default_style_ = style; }
 
   const gfx::Rect& display_rect() const { return display_rect_; }
-  void set_display_rect(const gfx::Rect& r) { display_rect_ = r; }
+  virtual void set_display_rect(const gfx::Rect& r) { display_rect_ = r; }
 
   size_t GetCursorPosition() const;
   void SetCursorPosition(const size_t position);
 
+  SelectionModel::LeadingTrailingStatus GetCursorBoundingEdge() const {
+      return cursor_and_selection_.bounding_edge();
+  }
+  void SetCursorBoundingEdge(SelectionModel::LeadingTrailingStatus edge) {
+      cursor_and_selection_.set_bounding_edge(edge);
+  }
+
+  size_t GetCursorBoundingCharIndex() const {
+      return cursor_and_selection_.char_index();
+  }
+
   // Moves the cursor left or right. Cursor movement is visual, meaning that
   // left and right are relative to screen, not the directionality of the text.
   // If |select| is false, the selection range is emptied at the new position.
   // If |break_type| is CHARACTER_BREAK, move to the neighboring character.
   // If |break_type| is WORD_BREAK, move to the nearest word boundary.
   // If |break_type| is LINE_BREAK, move to text edge as shown on screen.
-  void MoveCursorLeft(BreakType break_type, bool select);
-  void MoveCursorRight(BreakType break_type, bool select);
+  virtual void MoveCursorLeft(BreakType break_type, bool select);
+  virtual void MoveCursorRight(BreakType break_type, bool select);
 
-  // Moves the cursor to the specified logical |position|.
+  // Moves the cursor to the specified |selection|.

[msw, 2011/08/01 05:02:23]

What does this mean now? Will this function only use certain values from the
SelectionModel object (just the cursor), or take all the values of the argument
SelectionModel? How does |select| interact with the notion of the selection
being provided in the struct? I think we need to change this function name and
clarify its comment.

[xji, 2011/08/01 23:38:42]

Talked to Michael offline, will change the signature to:
bool MoveCursorTo(const SelectionModel& selection)
and introduce SetSelectionModel().

[msw, 2011/08/02 01:29:06]

Sorry, what will the revised MoveCursorTo do differently from SetSelectionModel?
Also, I suppose it ought to be called set_cursor_and_selection(), or you could
rename the data member to be selection_model_ (I think I like that more).

   // If |select| is false, the selection range is emptied at the new position.
   // Returns true if the cursor position or selection range changed.
-  bool MoveCursorTo(size_t position, bool select);
+  bool MoveCursorTo(const SelectionModel& selection, bool select);
 
   // Move the cursor to the position associated with the clicked point.
   // If |select| is false, the selection range is emptied at the new position.
   bool MoveCursorTo(const gfx::Point& point, bool select);
 
-  const ui::Range& GetSelection() const;
-  void SetSelection(const ui::Range& range);
+  size_t GetSelectionStart() const {
+      return cursor_and_selection_.selection_start();
+  }
+  size_t MinOfSelection() const {

[msw, 2011/08/01 05:02:23]

This seems unnecessary, move it to RenderTextLinux if you need it, or at least
make it private/protected with MaxOfSelection.

[xji, 2011/08/01 23:38:42]

It is used in TextfieldViewModel.
It is just a helper function. I could remove it and do the computation inside
caller. But providing it is more convenient.

[msw, 2011/08/02 01:29:06]

It was easy to do this with a ui::Range before. But fine, if we're going to
provide this convenience function, we may as well make the matching
MaxOfSelection public too...

+      return std::min(GetSelectionStart(), GetCursorPosition());
+  }
+  bool EmptySelection() const {
+      return GetSelectionStart() == GetCursorPosition();
+  }
+  void SetSelection(size_t start, size_t end);

[msw, 2011/08/01 05:02:23]

Comment on this function, especially how it effects (or doesn't effect) the
cursor. Should this be a private/protected member?

[xji, 2011/08/01 23:38:42]

Talked to Michael offline, will change this to private and add setter/getter of
SelectionModel for external callsites.

[msw, 2011/08/02 01:29:06]

good.

 
   // Returns true if the local point is over selected text.
-  bool IsPointInSelection(const gfx::Point& point) const;
+  bool IsPointInSelection(const gfx::Point& point);
 
   // Selects no text, all text, or the word at the current cursor position.
   void ClearSelection();
@@ -135,26 +229,30 @@
   base::i18n::TextDirection GetTextDirection() const;
 
   // Get the width of the entire string.
-  int GetStringWidth() const;
+  virtual int GetStringWidth();
 
   virtual void Draw(gfx::Canvas* canvas);
 
-  // TODO(msw): Deprecate this function. Logical and visual cursors are not
-  //  mapped one-to-one. See the selection_range_ TODO for more information.
-  // Get the logical cursor position from a visual point in local coordinates.
-  virtual size_t FindCursorPosition(const gfx::Point& point) const;
+  // Gets the SelectionModel from a visual point in local coordinates.
+  virtual SelectionModel FindCursorPosition(const gfx::Point& point);
 
-  // Get the visual bounds containing the logical substring within |range|.
-  // These bounds could be visually discontiguous if the logical selection range
-  // is split by an odd number of LTR/RTL level change.
+  // Get the visual bounds containing the logical substring within |from| to
+  // |to|. These bounds could be visually discontiguous if the logical
+  // selection range is split by an odd number of LTR/RTL level change.
   virtual std::vector<gfx::Rect> GetSubstringBounds(
-      const ui::Range& range) const;
+      size_t from, size_t to) const;
 
-  // Get the visual bounds describing the cursor at |position|. These bounds
+  // Get the visual bounds describing the cursor at |position| and
+  // cursor_and_selection_. These bounds

[msw, 2011/08/01 05:02:23]

This should not use cursor_and_selection_.

   // typically represent a vertical line, but if |insert_mode| is true they
   // contain the bounds of the associated glyph.
-  virtual gfx::Rect GetCursorBounds(size_t position, bool insert_mode) const;
+  // TODO(xji): do I stil need |position|? can I use cursor_and_selection_ only?

[msw, 2011/08/01 05:02:23]

This ought to take a SelectionModel, and yes, it needs to use the argument
information, not the current cursor_and_selection_. The drag and drop code, and
likely other instances will make use of this information.

[xji, 2011/08/01 23:38:42]

Make sense. I planned to introduce a private one in RenderTextLinux accept
SelectionModel.
But you are right that RenderText::GetCursorBounds() should accept
SelectionModel instead.

+  virtual gfx::Rect GetCursorBounds(size_t position, bool insert_mode);
 
+  // Compute cursor_bounds_ and update display_offset_ when necessary. Cache
+  // the values for later use and return cursor_bounds_.
+  gfx::Rect CursorBounds();
+
  protected:
   RenderText();
 
@@ -164,11 +262,15 @@
 
   // Get the cursor position that visually neighbors |position|.
   // If |move_by_word| is true, return the neighboring word delimiter position.
-  virtual size_t GetLeftCursorPosition(size_t position,
-                                       bool move_by_word) const;
-  virtual size_t GetRightCursorPosition(size_t position,
-                                        bool move_by_word) const;
+  virtual SelectionModel GetLeftCursorPosition(const SelectionModel& current,
+                                               bool move_by_word);
+  virtual SelectionModel GetRightCursorPosition(const SelectionModel& current,
+                                                bool move_by_word);
 
+  // Apply composition style (underline) to composition range and selection
+  // style (foreground) to selection range.
+  void ApplyCompositionAndSelectionStyles(StyleRanges& style_ranges) const;
+
  private:
   friend class RenderTextTest;
 
@@ -180,22 +282,27 @@
   // Clear out |style_ranges_|.
   void ClearStyleRanges();
 
+  size_t MaxOfSelection() const {

[msw, 2011/08/01 05:02:23]

This seems unnecessary, move it to RenderTextLinux if you need it. 

[xji, 2011/08/01 23:38:42]

It is used in RenderText::ApplyCompositionAndSelectionStyles() and
IsPointInSelection().

+      return std::max(GetSelectionStart(), GetCursorPosition());
+  }
+
   bool IsPositionAtWordSelectionBoundary(size_t pos);
 
+  void UpdateCursorBoundsAndDisplayOffset();
+
   // Logical UTF-16 string data to be drawn.
   string16 text_;
 
-  // TODO(msw): A single logical cursor position doesn't support two potential
-  //  visual cursor positions. For example, clicking right of 'c' & 'D' yeilds:
-  //  (visually: 'abc|FEDghi' and 'abcFED|ghi', both logically: 'abc|DEFghi').
-  //  Similarly, one visual position may have two associated logical positions.
-  //  For example, clicking the right side of 'D' and left side of 'g' yields:
-  //  (both visually: 'abcFED|ghi', logically: 'abc|DEFghi' and 'abcDEF|ghi').
-  //  Update the cursor model with a leading/trailing flag, a level association,
-  //  or a disjoint visual position to satisfy the proposed visual behavior.
-  // Logical selection range; the range end is also the logical cursor position.
-  ui::Range selection_range_;
+  // Logical selection range and visual cursor position.
+  SelectionModel cursor_and_selection_;
 
+  // The cached cursor bounds.
+  gfx::Rect cursor_bounds_;
+  // cursor_bounds_ is computed when needed and cached afterwards. And it is
+  // invalidated in operations such as SetCursorPosition, SetSelection, Font
+  // related style change, and other operations that trigger relayout.
+  bool cursor_bounds_valid_;
+
   // The cursor visibility and insert mode.
   bool cursor_visible_;
   bool insert_mode_;