modification to RenderText for inheritance/SelectionModel

ui/gfx/render_text.cc

Index: ui/gfx/render_text.cc
===================================================================
--- ui/gfx/render_text.cc	(revision 93971)
+++ ui/gfx/render_text.cc	(working copy)
@@ -85,6 +85,33 @@
       range() {
 }
 
+SelectionModel::SelectionModel() {

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

These constructors should have initialization lists.

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

Thanks for the detailed review!

For the comments that I did not reply specifically, it means "done".

On 2011/08/01 05:02:23, msw wrote:
The first 2 constructors could be just a call to the 3rd constructor. init() is
introduced to remove redundant code. If you think initialization list is better,
I could change them to use initialization list.

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

seems like a matter of taste, so you can leave it alone, but make sure to make
the ctor with one arg explicit.

+  init(0, 0, PREVIOUS_GRAPHEME_TRAILING);
+}
+
+SelectionModel::SelectionModel(size_t pos) {
+  init(pos, pos, PREVIOUS_GRAPHEME_TRAILING);
+}
+
+SelectionModel::SelectionModel(size_t pos, size_t index,
+                               LeadingTrailingStatus edge) {
+  init(pos, index, edge);
+}
+
+void SelectionModel::init(size_t pos, size_t index,
+                          LeadingTrailingStatus edge) {
+  selection_start_ = pos;
+  cursor_pos_ = pos;
+  char_index_ = index;
+  bounding_edge_ = edge;
+}
+
+SelectionModel::~SelectionModel() {
+}
+
+RenderText::~RenderText() {
+}
+
 void RenderText::SetText(const string16& text) {
   size_t old_text_length = text_.length();
   text_ = text;
@@ -115,25 +142,24 @@
 }
 
 size_t RenderText::GetCursorPosition() const {
-  return GetSelection().end();
+    return cursor_and_selection_.cursor_pos();
 }
-
 void RenderText::SetCursorPosition(const size_t position) {
-  SetSelection(ui::Range(position, position));
+    SetSelection(position, position);

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

outdent two spaces.

 }
 
 void RenderText::MoveCursorLeft(BreakType break_type, bool select) {
   if (break_type == LINE_BREAK) {
-    MoveCursorTo(0, select);
+    MoveCursorTo(SelectionModel(0), select);
     return;
   }
-  size_t position = GetCursorPosition();
+  SelectionModel position(GetCursorPosition());
   // Cancelling a selection moves to the edge of the selection.
-  if (!GetSelection().is_empty() && !select) {
+  if (!EmptySelection() && !select) {
     // Use the selection start if it is left of the selection end.
-    if (GetCursorBounds(GetSelection().start(), false).x() <
-        GetCursorBounds(position, false).x())
-      position = GetSelection().start();
+    if (GetCursorBounds(GetSelectionStart(), false).x() <
+        GetCursorBounds(GetCursorPosition(), false).x())
+      position = SelectionModel(GetSelectionStart());
     // If |move_by_word|, use the nearest word boundary left of the selection.
     if (break_type == WORD_BREAK)
       position = GetLeftCursorPosition(position, true);
@@ -145,16 +171,16 @@
 
 void RenderText::MoveCursorRight(BreakType break_type, bool select) {
   if (break_type == LINE_BREAK) {
-    MoveCursorTo(text().length(), select);
+    MoveCursorTo(SelectionModel(text().length()), select);
     return;
   }
-  size_t position = GetCursorPosition();
+  SelectionModel position(GetCursorPosition());
   // Cancelling a selection moves to the edge of the selection.
-  if (!GetSelection().is_empty() && !select) {
+  if (!EmptySelection() && !select) {
     // Use the selection start if it is right of the selection end.
-    if (GetCursorBounds(GetSelection().start(), false).x() >
-        GetCursorBounds(position, false).x())
-      position = GetSelection().start();
+    if (GetCursorBounds(GetSelectionStart(), false).x() >
+        GetCursorBounds(GetCursorPosition(), false).x())
+      position = SelectionModel(GetSelectionStart());
     // If |move_by_word|, use the nearest word boundary right of the selection.
     if (break_type == WORD_BREAK)
       position = GetRightCursorPosition(position, true);
@@ -164,51 +190,32 @@
   MoveCursorTo(position, select);
 }
 
-bool RenderText::MoveCursorTo(size_t position, bool select) {
-  bool changed = GetCursorPosition() != position ||
-                 select == GetSelection().is_empty();
+bool RenderText::MoveCursorTo(const SelectionModel& selection, bool select) {

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

This should essentially become SetSelectionModel or SetCursorAndSelection; and
we can just copy values by assigning |selection| to |cursor_and_selection_|.

+  bool changed = selection.CursorChanged(cursor_and_selection_) ||
+                 select == EmptySelection();
   if (select)
-    SetSelection(ui::Range(GetSelection().start(), position));
+    SetSelection(GetSelectionStart(), selection.cursor_pos());
   else
-    SetSelection(ui::Range(position, position));
+    SetSelection(selection.cursor_pos(), selection.cursor_pos());
+  cursor_and_selection_.set_char_index(selection.char_index());
+  cursor_and_selection_.set_bounding_edge(selection.bounding_edge());
   return changed;
 }
 
 bool RenderText::MoveCursorTo(const gfx::Point& point, bool select) {
-  // TODO(msw): Make this function support cursor placement via mouse near BiDi
-  //  level changes. The visual cursor appearance will depend on the location
-  //  clicked, not solely the resulting logical cursor position. See the TODO
-  //  note pertaining to selection_range_ for more information.
   return MoveCursorTo(FindCursorPosition(point), select);
 }
 
-const ui::Range& RenderText::GetSelection() const {
-  return selection_range_;
-}
+void RenderText::SetSelection(size_t start, size_t end) {

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

With a revised MoveCursorTo, we may want to rename or change this function as
well.

+  cursor_and_selection_.set_cursor_pos(std::min(end, text().length()));
+  cursor_and_selection_.set_selection_start(std::min(start, text().length()));
 
-void RenderText::SetSelection(const ui::Range& range) {
-  selection_range_.set_end(std::min(range.end(), text().length()));
-  selection_range_.set_start(std::min(range.start(), text().length()));
-
-  // Update |display_offset_| to ensure the current cursor is visible.
-  gfx::Rect cursor_bounds(GetCursorBounds(GetCursorPosition(), insert_mode()));
-  int display_width = display_rect_.width();
-  int string_width = GetStringWidth();
-  if (string_width < display_width) {
-    // Show all text whenever the text fits to the size.
-    display_offset_.set_x(0);
-  } else if ((display_offset_.x() + cursor_bounds.right()) > display_width) {
-    // Pan to show the cursor when it overflows to the right,
-     display_offset_.set_x(display_width - cursor_bounds.right());
-  } else if ((display_offset_.x() + cursor_bounds.x()) < 0) {
-    // Pan to show the cursor when it overflows to the left.
-    display_offset_.set_x(-cursor_bounds.x());
-  }
+  cursor_bounds_valid_ = false;
 }
 
-bool RenderText::IsPointInSelection(const gfx::Point& point) const {
-  size_t pos = FindCursorPosition(point);
-  return (pos >= GetSelection().GetMin() && pos < GetSelection().GetMax());
+bool RenderText::IsPointInSelection(const gfx::Point& point) {
+  size_t pos = FindCursorPosition(point).cursor_pos();
+  return (pos >= MinOfSelection() && pos < MaxOfSelection());
 }
 
 void RenderText::ClearSelection() {
@@ -216,11 +223,11 @@
 }
 
 void RenderText::SelectAll() {
-  SetSelection(ui::Range(0, text().length()));
+  SetSelection(0, text().length());
 }
 
 void RenderText::SelectWord() {
-  size_t selection_start = GetSelection().start();
+  size_t selection_start = GetSelectionStart();
   size_t cursor_position = GetCursorPosition();
   // First we setup selection_start_ and cursor_pos_. There are so many cases
   // because we try to emulate what select-word looks like in a gtk textfield.
@@ -255,11 +262,11 @@
       break;
   }
 
-  SetSelection(ui::Range(selection_start, cursor_position));
+  SetSelection(selection_start, cursor_position);
 }
 
 const ui::Range& RenderText::GetCompositionRange() const {
-  return composition_range_;
+    return composition_range_;

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

outdent 2 spaces

 }
 
 void RenderText::SetCompositionRange(const ui::Range& composition_range) {
@@ -279,6 +286,8 @@
 #ifndef NDEBUG
   CheckStyleRanges(style_ranges_, text_.length());
 #endif
+  // TODO(xji): only invalidate cursor_bounds if font or underline change.
+  cursor_bounds_valid_ = false;
 }
 
 void RenderText::ApplyDefaultStyle() {
@@ -294,8 +303,8 @@
   return base::i18n::LEFT_TO_RIGHT;
 }
 
-int RenderText::GetStringWidth() const {
-  return GetSubstringBounds(ui::Range(0, text_.length()))[0].width();
+int RenderText::GetStringWidth() {
+  return GetSubstringBounds(0, text_.length())[0].width();
 }
 
 void RenderText::Draw(gfx::Canvas* canvas) {
@@ -304,7 +313,8 @@
                       display_rect_.width(), display_rect_.height());
 
   // Draw the selection.
-  std::vector<gfx::Rect> selection(GetSubstringBounds(GetSelection()));
+  std::vector<gfx::Rect> selection(GetSubstringBounds(GetSelectionStart(),
+                                                      GetCursorPosition()));
   SkColor selection_color =
       focused() ? kFocusedSelectionColor : kUnfocusedSelectionColor;
   for (std::vector<gfx::Rect>::const_iterator i = selection.begin();
@@ -315,25 +325,8 @@
   }
 
   // Create a temporary copy of the style ranges for composition and selection.
-  // TODO(msw): This pattern ought to be reconsidered; what about composition
-  //            and selection overlaps, retain existing local style features?
   StyleRanges style_ranges(style_ranges_);
-  // Apply a composition style override to a copy of the style ranges.
-  if (composition_range_.IsValid() && !composition_range_.is_empty()) {
-    StyleRange composition_style(default_style_);
-    composition_style.underline = true;
-    composition_style.range.set_start(composition_range_.start());
-    composition_style.range.set_end(composition_range_.end());
-    ApplyStyleRangeImpl(style_ranges, composition_style);
-  }
-  // Apply a selection style override to a copy of the style ranges.
-  if (selection_range_.IsValid() && !selection_range_.is_empty()) {
-    StyleRange selection_style(default_style_);
-    selection_style.foreground = kSelectedTextColor;
-    selection_style.range.set_start(selection_range_.GetMin());
-    selection_style.range.set_end(selection_range_.GetMax());
-    ApplyStyleRangeImpl(style_ranges, selection_style);
-  }
+  ApplyCompositionAndSelectionStyles(style_ranges);
 
   // Draw the text.
   gfx::Rect bounds(display_rect_);
@@ -365,7 +358,7 @@
 
   // Paint cursor. Replace cursor is drawn as rectangle for now.
   if (cursor_visible() && focused()) {
-    bounds = GetCursorBounds(GetCursorPosition(), insert_mode());
+    bounds = CursorBounds();
     bounds.Offset(display_offset_);
     if (!bounds.IsEmpty())
       canvas->DrawRectInt(kCursorColor,
@@ -376,7 +369,7 @@
   }
 }
 
-size_t RenderText::FindCursorPosition(const gfx::Point& point) const {
+SelectionModel RenderText::FindCursorPosition(const gfx::Point& point) {
   const gfx::Font& font = Font();
   int left = 0;
   int left_pos = 0;
@@ -384,8 +377,8 @@
   int right_pos = text().length();
 
   int x = point.x();
-  if (x <= left) return left_pos;
-  if (x >= right) return right_pos;
+  if (x <= left) return SelectionModel(left_pos);
+  if (x >= right) return SelectionModel(right_pos);
   // binary searching the cursor position.
   // TODO(oshima): use the center of character instead of edge.
   // Binary search may not work for language like arabic.
@@ -396,19 +389,19 @@
       left = pivot;
       left_pos = pivot_pos;
     } else if (pivot == x) {
-      return pivot_pos;
+      return SelectionModel(pivot_pos);
     } else {
       right = pivot;
       right_pos = pivot_pos;
     }
   }
-  return left_pos;
+  return SelectionModel(left_pos);
 }
 
 std::vector<gfx::Rect> RenderText::GetSubstringBounds(
-    const ui::Range& range) const {
-  size_t start = range.GetMin();
-  size_t end = range.GetMax();
+    size_t from, size_t to) const {
+  size_t start = std::min(from, to);
+  size_t end = std::max(from, to);
   gfx::Font font;
   int start_x = font.GetStringWidth(text().substr(0, start));
   int end_x = font.GetStringWidth(text().substr(0, end));
@@ -417,8 +410,7 @@
   return bounds;
 }
 
-gfx::Rect RenderText::GetCursorBounds(size_t cursor_pos,
-                                      bool insert_mode) const {
+gfx::Rect RenderText::GetCursorBounds(size_t cursor_pos, bool insert_mode) {
   gfx::Font font;
   int x = font.GetStringWidth(text_.substr(0U, cursor_pos));
   DCHECK_GE(x, 0);
@@ -429,10 +421,34 @@
   return bounds;
 }
 
-size_t RenderText::GetLeftCursorPosition(size_t position,
-                                         bool move_by_word) const {
+gfx::Rect RenderText::CursorBounds() {
+  if (cursor_bounds_valid_ == false) {
+    UpdateCursorBoundsAndDisplayOffset();
+    cursor_bounds_valid_ = true;
+  }
+  return cursor_bounds_;
+}
+
+RenderText::RenderText()
+    : text_(),
+      cursor_and_selection_(),
+      cursor_bounds_(),
+      cursor_bounds_valid_(false),
+      cursor_visible_(false),
+      insert_mode_(true),
+      composition_range_(),
+      style_ranges_(),
+      default_style_(),
+      display_rect_(),
+      display_offset_() {
+}
+
+SelectionModel RenderText::GetLeftCursorPosition(const SelectionModel& current,
+                                                 bool move_by_word) {
+  size_t position = current.cursor_pos();
   if (!move_by_word)
-    return position == 0? position : position - 1;
+    return position == 0? SelectionModel(position) :

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

This code ought to be deprecated soon, but please change this to:
return SelectionModel(std::max(position - 1, 0));

+                          SelectionModel(position - 1);
   // Notes: We always iterate words from the begining.
   // This is probably fast enough for our usage, but we may
   // want to modify WordIterator so that it can start from the
@@ -441,7 +457,7 @@
   bool success = iter.Init();
   DCHECK(success);
   if (!success)
-    return position;
+    return SelectionModel(position);
   int last = 0;
   while (iter.Advance()) {
     if (iter.IsWord()) {
@@ -450,7 +466,7 @@
         // The cursor is at the beginning of a word.
         // Move to previous word.
         break;
-      } else if(iter.pos() >= position) {
+      } else if (iter.pos() >= position) {
         // The cursor is in the middle or at the end of a word.
         // Move to the top of current word.
         last = begin;
@@ -461,18 +477,19 @@
     }
   }
 
-  return last;
+  return SelectionModel(last);
 }
 
-size_t RenderText::GetRightCursorPosition(size_t position,
-                                          bool move_by_word) const {
+SelectionModel RenderText::GetRightCursorPosition(const SelectionModel& current,
+                                                  bool move_by_word) {
+  size_t position = current.cursor_pos();
   if (!move_by_word)
-    return std::min(position + 1, text().length());
+    return SelectionModel(std::min(position + 1, text().length()));
   base::i18n::BreakIterator iter(text(), base::i18n::BreakIterator::BREAK_WORD);
   bool success = iter.Init();
   DCHECK(success);
   if (!success)
-    return position;
+    return SelectionModel(position);
   size_t pos = 0;
   while (iter.Advance()) {
     pos = iter.pos();
@@ -480,27 +497,51 @@
       break;
     }
   }
-  return pos;
+  return SelectionModel(pos);
 }
 
-RenderText::RenderText()
-    : text_(),
-      selection_range_(),
-      cursor_visible_(false),
-      insert_mode_(true),
-      composition_range_(),
-      style_ranges_(),
-      default_style_(),
-      display_rect_(),
-      display_offset_() {
+void RenderText::ApplyCompositionAndSelectionStyles(
+    StyleRanges& style_ranges) const {
+  // TODO(msw): This pattern ought to be reconsidered; what about composition
+  //            and selection overlaps, retain existing local style features?
+  // Apply a composition style override to a copy of the style ranges.
+  if (composition_range_.IsValid() && !composition_range_.is_empty()) {
+    StyleRange composition_style(default_style_);
+    composition_style.underline = true;
+    composition_style.range.set_start(composition_range_.start());
+    composition_style.range.set_end(composition_range_.end());
+    ApplyStyleRangeImpl(style_ranges, composition_style);
+  }
+  // Apply a selection style override to a copy of the style ranges.
+  if (!EmptySelection()) {
+    StyleRange selection_style(default_style_);
+    selection_style.foreground = kSelectedTextColor;
+    selection_style.range.set_start(MinOfSelection());
+    selection_style.range.set_end(MaxOfSelection());
+    ApplyStyleRangeImpl(style_ranges, selection_style);
+  }
 }
 
-RenderText::~RenderText() {
-}
-
 bool RenderText::IsPositionAtWordSelectionBoundary(size_t pos) {
   return pos == 0 || (isalnum(text()[pos - 1]) && !isalnum(text()[pos])) ||
       (!isalnum(text()[pos - 1]) && isalnum(text()[pos]));
 }
 
+void RenderText::UpdateCursorBoundsAndDisplayOffset() {
+  cursor_bounds_ = GetCursorBounds(GetCursorPosition(), insert_mode());
+  // Update |display_offset_| to ensure the current cursor is visible.
+  int display_width = display_rect_.width();
+  int string_width = GetStringWidth();
+  if (string_width < display_width) {
+    // Show all text whenever the text fits to the size.
+    display_offset_.set_x(0);
+  } else if ((display_offset_.x() + cursor_bounds_.right()) > display_width) {
+    // Pan to show the cursor when it overflows to the right,
+     display_offset_.set_x(display_width - cursor_bounds_.right());
+  } else if ((display_offset_.x() + cursor_bounds_.x()) < 0) {
+    // Pan to show the cursor when it overflows to the left.
+    display_offset_.set_x(-cursor_bounds_.x());
+  }
+}
+
 }  // namespace gfx