Adjust GetLeft/RightSelectionModel return, cursor on edit.

ui/gfx/render_text.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/render_text.h"
 
 #include <algorithm>
 
 #include "base/i18n/break_iterator.h"
 #include "base/logging.h"
@@ skipping 164 matching lines @@
 }
 
 size_t RenderText::GetCursorPosition() const {
   return selection_model_.selection_end();
 }
 
 void RenderText::SetCursorPosition(const size_t position) {
   SelectionModel sel(selection_model());
   sel.set_selection_start(position);
   sel.set_selection_end(position);
+  sel.set_caret_pos(GetPreviousGrapheme(position));
+  sel.set_caret_placement(SelectionModel::TRAILING);

[xji, 2011/08/08 23:56:13]

RenderText::ClearSelection() probably wont work after the change?

[msw, 2011/08/09 01:07:42]

Done.

   SetSelectionModel(sel);
 }
 
 void RenderText::MoveCursorLeft(BreakType break_type, bool select) {
-  if (break_type == LINE_BREAK) {
-    SelectionModel selection(GetSelectionStart(), 0,
-                             0, SelectionModel::LEADING);
-    if (!select)
-      selection.set_selection_start(selection.selection_end());
-    MoveCursorTo(selection);
-    return;
-  }
-  SelectionModel position = selection_model_;
+  SelectionModel position = SelectionModel(GetCursorPosition(),
+      GetCursorPosition(), SelectionModel::LEADING);

[xji, 2011/08/08 23:56:13]

should the position be selection_model_?
or selection_model_ except selection_start set to selection_end?

Moving the logic of LINE_BREAK inside GetLeftCursorPosition() seems make the
code cleaner.
but does it make sense to have the LINE_BREAK code go through the selection
bounds leftness checking? 

[msw, 2011/08/09 01:07:42]

Done.

   // Cancelling a selection moves to the edge of the selection.
   if (!EmptySelection() && !select) {
     // Use the selection start if it is left of the selection end.
     SelectionModel selection_start(GetSelectionStart(), GetSelectionStart(),
-        GetSelectionStart(), SelectionModel::LEADING);
+                                   SelectionModel::LEADING);
     if (GetCursorBounds(selection_start, false).x() <
         GetCursorBounds(position, false).x())
       position = selection_start;
-    // If |move_by_word|, use the nearest word boundary left of the selection.
-    if (break_type == WORD_BREAK)
-      position = GetLeftCursorPosition(position, true);
+    // For word and line breaks, the cursor moves beyond the selection edge.
+    if (break_type != CHARACTER_BREAK)
+      position = GetLeftSelectionModel(position, break_type);
   } else {
-    position = GetLeftCursorPosition(position, break_type == WORD_BREAK);
+    position = GetLeftSelectionModel(position, break_type);
   }
-  if (!select)
-    position.set_selection_start(position.selection_end());
+  if (select)
+    position.set_selection_start(GetSelectionStart());
   MoveCursorTo(position);
 }
 
 void RenderText::MoveCursorRight(BreakType break_type, bool select) {
-  if (break_type == LINE_BREAK) {
-    SelectionModel selection(GetSelectionStart(), text().length(),
-        text().length(), SelectionModel::PREVIOUS_GRAPHEME_TRAILING);
-    if (!select)
-      selection.set_selection_start(selection.selection_end());
-    MoveCursorTo(selection);
-    return;
-  }
-  SelectionModel position = selection_model_;
+  SelectionModel position = SelectionModel(GetCursorPosition(),
+      GetCursorPosition(), SelectionModel::LEADING);

[xji, 2011/08/08 23:56:13]

ditto

[msw, 2011/08/09 01:07:42]

Done.

   // Cancelling a selection moves to the edge of the selection.
   if (!EmptySelection() && !select) {
     // Use the selection start if it is right of the selection end.
     SelectionModel selection_start(GetSelectionStart(), GetSelectionStart(),
-        GetSelectionStart(), SelectionModel::LEADING);
+                                   SelectionModel::LEADING);
     if (GetCursorBounds(selection_start, false).x() >
         GetCursorBounds(position, false).x())
       position = selection_start;
-    // If |move_by_word|, use the nearest word boundary right of the selection.
-    if (break_type == WORD_BREAK)
-      position = GetRightCursorPosition(position, true);
+    // For word and line breaks, the cursor moves beyond the selection edge.
+    if (break_type != CHARACTER_BREAK)
+      position = GetRightSelectionModel(position, break_type);
   } else {
-    position = GetRightCursorPosition(position, break_type == WORD_BREAK);
+    position = GetRightSelectionModel(position, break_type);
   }
-  if (!select)
-    position.set_selection_start(position.selection_end());
+  if (select)
+    position.set_selection_start(GetSelectionStart());
   MoveCursorTo(position);
 }
 
 bool RenderText::MoveCursorTo(const SelectionModel& selection) {
   bool changed = !selection.Equals(selection_model_);
   SetSelectionModel(selection);
   return changed;
 }
 
 bool RenderText::MoveCursorTo(const Point& point, bool select) {
   SelectionModel selection = FindCursorPosition(point);
   if (select)
     selection.set_selection_start(GetSelectionStart());
-  else
-    selection.set_selection_start(selection.selection_end());
   return MoveCursorTo(selection);
 }
 
 bool RenderText::IsPointInSelection(const Point& point) {
   // TODO(xji): should this check whether the point is inside the visual
   // selection bounds? In case of "abcFED", if "ED" is selected, |point| points
   // to the right half of 'c', is the point in selection?
   size_t pos = FindCursorPosition(point).selection_end();
   return (pos >= MinOfSelection() && pos < MaxOfSelection());
 }
@@ skipping 228 matching lines @@
       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.selection_end();
-  SelectionModel left = current;
-  if (!move_by_word) {
-    left.set_selection_end(std::max(static_cast<long>(position - 1),
-                                    static_cast<long>(0)));
-    return left;
-  }
+SelectionModel RenderText::GetLeftSelectionModel(const SelectionModel& current,
+                                                 BreakType break_type) {
+  if (break_type == LINE_BREAK)
+    return SelectionModel(0, 0, SelectionModel::LEADING);
+  size_t pos = std::max(static_cast<long>(current.selection_end() - 1),
+                        static_cast<long>(0));
+  if (break_type == CHARACTER_BREAK)
+    return SelectionModel(pos, pos, SelectionModel::LEADING);
+
   // 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
   // middle of string and advance backwards.
   base::i18n::BreakIterator iter(text(), base::i18n::BreakIterator::BREAK_WORD);
   bool success = iter.Init();
   DCHECK(success);
-  if (!success) {
-    left.set_selection_end(position);
-    return left;
-  }
-  int last = 0;
+  if (!success)
+    return current;
   while (iter.Advance()) {
     if (iter.IsWord()) {
       size_t begin = iter.pos() - iter.GetString().length();
-      if (begin == position) {
+      if (begin == current.selection_end()) {
         // The cursor is at the beginning of a word.
         // Move to previous word.
         break;
-      } else if (iter.pos() >= position) {
+      } else if (iter.pos() >= current.selection_end()) {
         // The cursor is in the middle or at the end of a word.
         // Move to the top of current word.
-        last = begin;
+        pos = begin;
         break;
       } else {
-        last = iter.pos() - iter.GetString().length();
+        pos = iter.pos() - iter.GetString().length();
       }
     }
   }
 
-  left.set_selection_end(last);
-  return left;
+  return SelectionModel(pos, pos, SelectionModel::LEADING);
 }
 
-SelectionModel RenderText::GetRightCursorPosition(const SelectionModel& current,
-                                                  bool move_by_word) {
-  size_t position = current.selection_end();
-  SelectionModel right = current;
-
-  if (!move_by_word) {
-    right.set_selection_end(std::min(position + 1, text().length()));
-    return right;
-  }
+SelectionModel RenderText::GetRightSelectionModel(const SelectionModel& current,
+                                                  BreakType break_type) {
+  if (break_type == LINE_BREAK)
+    return SelectionModel(text().length(), text().length(),
+                          SelectionModel::LEADING);

[xji, 2011/08/08 23:56:13]

this will return the rightmost visual extreme of the line. which is not in
parity with GetLeftSelectionModel(...LINE_BREAK).
the parity one should be: 
SelectionModel(text().length(), GetPreviousGrapheme(),
SelectionModel::TRAILING);

[msw, 2011/08/09 01:07:42]

Done.

+  size_t pos = std::min(current.selection_end() + 1, text().length());
+  if (break_type == CHARACTER_BREAK)
+    return SelectionModel(pos, pos, SelectionModel::LEADING);
 
   base::i18n::BreakIterator iter(text(), base::i18n::BreakIterator::BREAK_WORD);
   bool success = iter.Init();
   DCHECK(success);
-  if (!success) {
-    right.set_selection_end(position);
-    return right;
-  }
-  size_t pos = 0;
+  if (!success)
+    return current;
   while (iter.Advance()) {
     pos = iter.pos();
-    if (iter.IsWord() && pos > position) {
+    if (iter.IsWord() && pos > current.selection_end())
       break;
-    }
   }
-  right.set_selection_end(pos);
-  return right;
+  return SelectionModel(pos, pos, SelectionModel::LEADING);
+}
+
+size_t RenderText::GetPreviousGrapheme(size_t position) const {
+  // TODO(msw): Handle complex script.
+  return std::max(static_cast<int>(position - 1), static_cast<int>(0));
 }
 
 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;
@@ skipping 27 matching lines @@
   } 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