modification to RenderText for inheritance/SelectionModel

views/controls/textfield/textfield_views_model.cc

Index: views/controls/textfield/textfield_views_model.cc
===================================================================
--- views/controls/textfield/textfield_views_model.cc	(revision 95171)
+++ views/controls/textfield/textfield_views_model.cc	(working copy)
@@ -380,10 +380,16 @@
   render_text_->MoveCursorRight(break_type, select);
 }
 
-bool TextfieldViewsModel::MoveCursorTo(size_t pos, bool select) {
-  if (HasCompositionText())
+bool TextfieldViewsModel::MoveCursorTo(gfx::SelectionModel* selection) {
+  if (HasCompositionText()) {
     ConfirmCompositionText();
-  return render_text_->MoveCursorTo(pos, select);
+    // ConfirmCompositionText() updates cursor position. Need to update

[msw, 2011/08/03 02:13:06]

This is horrible, callers won't expect their argument to be modified. We should
disregard the cursor set by confirming the composition text and pass |selection|
to RenderText::MoveCursorTo unmodified. Otherwise, pass by const ref, create a
copy to perform this unexpected (and I think undesirable) behavior, and comment
on this behavior in the header.

[xji, 2011/08/03 18:47:40]

Agree that this is terrible. Previously the MoveCursorTo() signature is
MoveCursorTo(const SelectionModel&, bool select), inside the function, if
|select| is true, selection's start remains, and cursor moves to selection's
end.

Now since we pass in SelectionModel alone, if any function (such as
ConfirmCompositionText()) changes selection start, we have to reflect it in the
SelectionModel .

There is a test case in TextfieldViewsModelTest.CompositionTextTest (Line
611-623 in patch, search for the 3rd sel.set_selection_start) to cover this:
the original text is "678", the (will-be-appended) composition text is "678"
where "8" is selected. During SetCompositionText(), the selection range becomes
(5, 6) (I do not know why it changes render_text's selection range in
SetCompositionText, should it be done in ConfirmCompositionText? But it is
orthogonal to the issue we discussed here.).
Then, we call MoveCursorTo() and pass 5 as selection_start. 
Inside MoveCursorTo(), ConfirmCompositionText() is called and the cursor
position is set to 6. We need to move the selection start to 6 as well so the
subsequent operation works correctly. The next operation is
SetCompositionText(), which will DeleteSelection if there is any. 
changing selection range from (5, 0) to (6, 0) makes difference for test
"EXPECT_STR_EQ("678", model.GetText());".


I am pass by const ref here and added comments in header.
Any suggestion is welcomed.

+    // |selection| accordingly.
+    // TODO(xji): this is not ideal. But passing |selection| to
+    // ConfirmCompositionText() does not sounds right either.
+    selection->set_selection_start(render_text_->GetSelectionStart());
+  }
+  return render_text_->MoveCursorTo(*selection);
 }
 
 bool TextfieldViewsModel::MoveCursorTo(const gfx::Point& point, bool select) {
@@ -393,22 +399,27 @@
 }
 
 std::vector<gfx::Rect> TextfieldViewsModel::GetSelectionBounds() const {
-  return render_text_->GetSubstringBounds(render_text_->GetSelection());
+  return render_text_->GetSubstringBounds(render_text_->GetSelectionStart(),
+                                          render_text_->GetCursorPosition());
 }
 
 string16 TextfieldViewsModel::GetSelectedText() const {
-  ui::Range selection = render_text_->GetSelection();
-  return GetText().substr(selection.GetMin(), selection.length());
+  return GetText().substr(render_text_->MinOfSelection(),
+      std::abs(static_cast<long>(render_text_->GetCursorPosition() -
+                                 render_text_->GetSelectionStart())));
 }
 
 void TextfieldViewsModel::GetSelectedRange(ui::Range* range) const {
-  *range = render_text_->GetSelection();
+  *range = ui::Range(render_text_->GetSelectionStart(),

[msw, 2011/08/03 02:13:06]

Optional: Change this to range->set_start() and range->set_end().

[xji, 2011/08/03 18:47:40]

Done.

+                     render_text_->GetCursorPosition());
 }
 
 void TextfieldViewsModel::SelectRange(const ui::Range& range) {
   if (HasCompositionText())
     ConfirmCompositionText();
-  render_text_->SetSelection(range);
+  gfx::SelectionModel selection(range.start(), range.end(),
+      range.end(), gfx::SelectionModel::PREVIOUS_GRAPHEME_TRAILING);
+  render_text_->SetSelectionModel(selection);
 }
 
 void TextfieldViewsModel::SelectAll() {
@@ -492,8 +503,11 @@
     // than beginning, unlike Delete/Backspace.
     // TODO(oshima): Change Delete/Backspace to use DeleteSelection,
     // update DeleteEdit and remove this trick.
-    ui::Range selection = render_text_->GetSelection();
-    render_text_->SetSelection(ui::Range(selection.end(), selection.start()));
+    gfx::SelectionModel sel(render_text_->GetCursorPosition(),
+                            render_text_->GetSelectionStart(),
+                            render_text_->GetSelectionStart(),
+                            gfx::SelectionModel::LEADING);
+    render_text_->SetSelectionModel(sel);
     DeleteSelection();
     return true;
   }
@@ -519,14 +533,14 @@
 }
 
 bool TextfieldViewsModel::HasSelection() const {
-  return !render_text_->GetSelection().is_empty();
+  return !render_text_->EmptySelection();
 }
 
 void TextfieldViewsModel::DeleteSelection() {
   DCHECK(!HasCompositionText());
   DCHECK(HasSelection());
-  ui::Range selection = render_text_->GetSelection();
-  ExecuteAndRecordDelete(selection.start(), selection.end(), false);
+  ExecuteAndRecordDelete(render_text_->GetSelectionStart(),
+                         render_text_->GetCursorPosition(), false);
 }
 
 void TextfieldViewsModel::DeleteSelectionAndInsertTextAt(
@@ -567,12 +581,17 @@
   render_text_->SetCompositionRange(range);
   // TODO(msw): Support multiple composition underline ranges.
 
-  if (composition.selection.IsValid())
-    render_text_->SetSelection(ui::Range(
-        std::min(range.start() + composition.selection.start(), range.end()),
-        std::min(range.start() + composition.selection.end(), range.end())));
-  else
+  if (composition.selection.IsValid()) {
+    size_t start = std::min(range.start() + composition.selection.start(),
+                            range.end());
+    size_t end = std::min(range.start() + composition.selection.end(),
+                          range.end());
+    gfx::SelectionModel sel(start, end, end,
+                            gfx::SelectionModel::PREVIOUS_GRAPHEME_TRAILING);
+    render_text_->SetSelectionModel(sel);
+  } else {
     render_text_->SetCursorPosition(range.end());
+  }
 }
 
 void TextfieldViewsModel::ConfirmCompositionText() {
@@ -640,7 +659,10 @@
     CancelCompositionText();
   } else if (!HasSelection()) {
     size_t cursor = GetCursorPosition();
-    render_text_->SetSelection(ui::Range(cursor + text.length(), cursor));
+    gfx::SelectionModel sel(cursor + text.length(), cursor,

[msw, 2011/08/03 02:13:06]

Try this instead:
gfx::SelectionModel sel(render_text_->selection_model());
sel.set_selection_start(cursor + text.length());
sel.set_selection_end(cursor);
...

[xji, 2011/08/03 18:47:40]

Done and seems "sel.set_selection_end(cursor);" is not needed.

+                            render_text_->GetCursorBoundingCharIndex(),
+                            render_text_->GetCursorPlacement());
+    render_text_->SetSelectionModel(sel);
   }
   // Edit history is recorded in InsertText.
   InsertTextInternal(text, mergeable);
@@ -671,7 +693,7 @@
                                                  bool mergeable) {
   size_t old_text_start = std::min(from, to);
   const string16 text = GetText().substr(old_text_start,
-                                     std::abs(static_cast<long>(from - to)));
+      std::abs(static_cast<long>(from - to)));
   bool backward = from > to;
   Edit* edit = new DeleteEdit(mergeable, text, old_text_start, backward);
   bool delete_edit = AddOrMergeEditHistory(edit);
@@ -682,7 +704,7 @@
 
 void TextfieldViewsModel::ExecuteAndRecordReplaceSelection(
     MergeType merge_type, const string16& new_text) {
-  size_t new_text_start = render_text_->GetSelection().GetMin();
+  size_t new_text_start = render_text_->MinOfSelection();
   size_t new_cursor_pos = new_text_start + new_text.length();
   ExecuteAndRecordReplace(merge_type,
                           GetCursorPosition(),
@@ -696,8 +718,9 @@
                                                   size_t new_cursor_pos,
                                                   const string16& new_text,
                                                   size_t new_text_start) {
-  size_t old_text_start = render_text_->GetSelection().GetMin();
-  bool backward = render_text_->GetSelection().is_reversed();
+  size_t old_text_start = render_text_->MinOfSelection();
+  bool backward = render_text_->GetSelectionStart() >
+                  render_text_->GetCursorPosition();
   Edit* edit = new ReplaceEdit(merge_type,
                                GetSelectedText(),
                                old_cursor_pos,