Clean up of SelectionModel

views/controls/textfield/textfield_views_model.cc

Index: views/controls/textfield/textfield_views_model.cc
===================================================================
--- views/controls/textfield/textfield_views_model.cc	(revision 103984)
+++ views/controls/textfield/textfield_views_model.cc	(working copy)
@@ -390,8 +390,12 @@
     ConfirmCompositionText();
     // ConfirmCompositionText() updates cursor position. Need to reflect it in
     // the SelectionModel parameter of MoveCursorTo().
-    gfx::SelectionModel sel(selection);
-    sel.set_selection_start(render_text_->GetSelectionStart());
+    if (render_text_->GetSelectionStart() != selection.selection_end())

[msw, 2011/10/07 20:54:20]

Is it always safe to just use the SelectRange statement? Could the argument
caret/placement differ from those that would be set by SelectRange?

[xji, 2011/10/07 21:42:04]

Using SelectRange() to recompute cursor's visual position is the right approach
than just resetting the selection_start. Please see detail explanation in
native_textfield_views

+      return render_text_->SelectRange(ui::Range(
+          render_text_->GetSelectionStart(), selection.selection_end()));
+    gfx::SelectionModel sel(selection.selection_end(),
+                            selection.caret_pos(),
+                            selection.caret_placement());
     return render_text_->MoveCursorTo(sel);
   }
   return render_text_->MoveCursorTo(selection);
@@ -408,6 +412,17 @@
       (render_text_->MaxOfSelection() - render_text_->MinOfSelection()));
 }
 
+void TextfieldViewsModel::GetSelectedRange(ui::Range* range) const {
+  range->set_start(render_text_->GetSelectionStart());
+  range->set_end(render_text_->GetCursorPosition());
+}
+
+void TextfieldViewsModel::SelectRange(const ui::Range& range) {
+  if (HasCompositionText())
+    ConfirmCompositionText();
+  render_text_->SelectRange(range);
+}
+
 void TextfieldViewsModel::GetSelectionModel(gfx::SelectionModel* sel) const {
   *sel = render_text_->selection_model();
 }
@@ -499,11 +514,8 @@
     // than beginning, unlike Delete/Backspace.
     // TODO(oshima): Change Delete/Backspace to use DeleteSelection,
     // update DeleteEdit and remove this trick.
-    gfx::SelectionModel sel(render_text_->GetCursorPosition(),
-                            render_text_->GetSelectionStart(),
-                            render_text_->GetSelectionStart(),
-                            gfx::SelectionModel::LEADING);
-    render_text_->MoveCursorTo(sel);
+    render_text_->SelectRange(ui::Range(render_text_->GetCursorPosition(),
+                                        render_text_->GetSelectionStart()));
     DeleteSelection();
     return true;
   }
@@ -582,8 +594,7 @@
         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);
-    render_text_->MoveCursorTo(sel);
+    render_text_->SelectRange(ui::Range(start, end));
   } else {
     render_text_->SetCursorPosition(range.end());
   }
@@ -654,9 +665,12 @@
     CancelCompositionText();
   } else if (!HasSelection()) {
     size_t cursor = GetCursorPosition();
-    gfx::SelectionModel sel(render_text_->selection_model());
-    sel.set_selection_start(render_text_->GetIndexOfNextGrapheme(cursor));
-    render_text_->MoveCursorTo(sel);
+    const gfx::SelectionModel& model = render_text_->selection_model();
+    size_t next = render_text_->GetIndexOfNextGrapheme(cursor);

[msw, 2011/10/07 20:54:20]

Sadly, I forget why we do this; can you add a comment? Is it always safe to just
use the SelectRange statement? Could the |model| caret/placement differ from
those that would be set by SelectRange?

[xji, 2011/10/07 21:42:04]

If there is non-empty selection range, we should use SelectRange() to recompute
the cursor's visual display (caret_pos/placement), and should not keep |model|'s
caret/placement. Please see the reply in native_textfield_views.cc for example.

I will add comments about 'next'

+    if (next == model.selection_end())
+      render_text_->MoveCursorTo(model);
+    else
+      render_text_->SelectRange(ui::Range(next, model.selection_end()));
   }
   // Edit history is recorded in InsertText.
   InsertTextInternal(text, mergeable);

[msw, 2011/10/07 20:54:20]

Are we doing the right thing here by setting the selection model before the text
is replaced? Could that set invalid non-grapheme-boundary positions? You can ask
Oshima if needed; he wrote the editing model.

[xji, 2011/10/07 21:42:04]

Before the text is replaced and when there is no selection, we need to set the
selection range as the range of next character (as in replace mode, which by
default is replacing next character with |text|).
Since we are using 'next=IndexOfNextGrapheme', we wont set it on
non-grapheme-boundary. There is test to test ReplaceChar() and ReplaceText() in
TextfieldViewModelTest.

[oshima, 2011/10/08 01:40:47]

Setting selection is necessary for "NO INSERT" mode editing.
When text is more than 1 char, we replace the same number (or max) of characters
that can be
replaced. This definition just happened to be this way and could be different
but i don't think
there is such real use case, so i just left that way.

So, I think just selecting one character (current to next) should be sufficient.