Clean up of SelectionModel

views/controls/textfield/textfield_views_model.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 "views/controls/textfield/textfield_views_model.h"
 
 #include <algorithm>
 
 #include "base/i18n/break_iterator.h"
 #include "base/logging.h"
@@ skipping 372 matching lines @@
   if (HasCompositionText())
     ConfirmCompositionText();
   render_text_->MoveCursorRight(break_type, select);
 }
 
 bool TextfieldViewsModel::MoveCursorTo(const gfx::SelectionModel& selection) {
   if (HasCompositionText()) {
     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);
 }
 
 bool TextfieldViewsModel::MoveCursorTo(const gfx::Point& point, bool select) {
   if (HasCompositionText())
     ConfirmCompositionText();
   return render_text_->MoveCursorTo(point, select);
 }
 
 string16 TextfieldViewsModel::GetSelectedText() const {
   return GetText().substr(render_text_->MinOfSelection(),
       (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();
 }
 
 void TextfieldViewsModel::SelectSelectionModel(const gfx::SelectionModel& sel) {
   if (HasCompositionText())
     ConfirmCompositionText();
   render_text_->MoveCursorTo(sel);
 }
 
@@ skipping 71 matching lines @@
 
 bool TextfieldViewsModel::Cut() {
   if (!HasCompositionText() && HasSelection()) {
     ui::ScopedClipboardWriter(views::ViewsDelegate::views_delegate
         ->GetClipboard()).WriteText(GetSelectedText());
     // A trick to let undo/redo handle cursor correctly.
     // Undoing CUT moves the cursor to the end of the change rather
     // 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;
   }
   return false;
 }
 
 void TextfieldViewsModel::Copy() {
   if (!HasCompositionText() && HasSelection()) {
     ui::ScopedClipboardWriter(views::ViewsDelegate::views_delegate
         ->GetClipboard()).WriteText(GetSelectedText());
@@ skipping 58 matching lines @@
   render_text_->SetText(new_text.insert(cursor, composition.text));
   ui::Range range(cursor, cursor + composition.text.length());
   render_text_->SetCompositionRange(range);
   // TODO(msw): Support multiple composition underline ranges.
 
   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);
-    render_text_->MoveCursorTo(sel);
+    render_text_->SelectRange(ui::Range(start, end));
   } else {
     render_text_->SetCursorPosition(range.end());
   }
 }
 
 void TextfieldViewsModel::ConfirmCompositionText() {
   DCHECK(HasCompositionText());
   ui::Range range = render_text_->GetCompositionRange();
   string16 text = GetText().substr(range.start(), range.length());
   // TODO(oshima): current behavior on ChromeOS is a bit weird and not
@@ skipping 50 matching lines @@
     ExecuteAndRecordInsert(text, mergeable);
   }
 }
 
 void TextfieldViewsModel::ReplaceTextInternal(const string16& text,
                                               bool mergeable) {
   if (HasCompositionText()) {
     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.

 }
 
 void TextfieldViewsModel::ClearEditHistory() {
   STLDeleteContainerPointers(edit_history_.begin(),
                              edit_history_.end());
   edit_history_.clear();
   current_edit_ = edit_history_.end();
 }
 
 void TextfieldViewsModel::ClearRedoHistory() {
@@ skipping 97 matching lines @@
   if (delete_from != delete_to)
     render_text_->SetText(text.erase(delete_from, delete_to - delete_from));
   if (!new_text.empty())
     render_text_->SetText(text.insert(new_text_insert_at, new_text));
   render_text_->SetCursorPosition(new_cursor_pos);
   // TODO(oshima): mac selects the text that is just undone (but gtk doesn't).
   // This looks fine feature and we may want to do the same.
 }
 
 }  // namespace views