modification to RenderText for inheritance/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 362 matching lines @@
   render_text_->MoveCursorLeft(break_type, select);
 }
 
 void TextfieldViewsModel::MoveCursorRight(gfx::BreakType break_type,
                                           bool select) {
   if (HasCompositionText())
     ConfirmCompositionText();
   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) {
   if (HasCompositionText())
     ConfirmCompositionText();
   return render_text_->MoveCursorTo(point, select);
 }
 
 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() {
   if (HasCompositionText())
     ConfirmCompositionText();
   render_text_->SelectAll();
 }
 
 void TextfieldViewsModel::SelectWord() {
   if (HasCompositionText())
@@ skipping 63 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.
-    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;
   }
   return false;
 }
 
 void TextfieldViewsModel::Copy() {
   if (!HasCompositionText() && HasSelection()) {
     ui::ScopedClipboardWriter(views::ViewsDelegate::views_delegate
         ->GetClipboard()).WriteText(GetSelectedText());
   }
 }
 
 bool TextfieldViewsModel::Paste() {
   string16 result;
   views::ViewsDelegate::views_delegate->GetClipboard()
       ->ReadText(ui::Clipboard::BUFFER_STANDARD, &result);
   if (!result.empty()) {
     InsertTextInternal(result, false);
     return true;
   }
   return false;
 }
 
 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(
     const string16& text, size_t position) {
   if (HasCompositionText())
     CancelCompositionText();
   ExecuteAndRecordReplace(DO_NOT_MERGE,
                           GetCursorPosition(),
                           position + text.length(),
                           text,
@@ skipping 20 matching lines @@
   if (composition.text.empty())
     return;
 
   size_t cursor = GetCursorPosition();
   string16 new_text = GetText();
   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())
-    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() {
   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
   // sure exactly how this should work. Find out and fix if necessary.
   AddOrMergeEditHistory(new InsertEdit(false, text, range.start()));
   render_text_->SetCursorPosition(range.end());
@@ skipping 47 matching lines @@
     ExecuteAndRecordInsert(text, mergeable);
   }
 }
 
 void TextfieldViewsModel::ReplaceTextInternal(const string16& text,
                                               bool mergeable) {
   if (HasCompositionText()) {
     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);
 }
 
 void TextfieldViewsModel::ClearEditHistory() {
   STLDeleteContainerPointers(edit_history_.begin(),
                              edit_history_.end());
   edit_history_.clear();
   current_edit_ = edit_history_.end();
@@ skipping 10 matching lines @@
   delete_start++;
   STLDeleteContainerPointers(delete_start, edit_history_.end());
   edit_history_.erase(delete_start, edit_history_.end());
 }
 
 void TextfieldViewsModel::ExecuteAndRecordDelete(size_t from,
                                                  size_t to,
                                                  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);
   edit->Redo(this);
   if (delete_edit)
     delete edit;
 }
 
 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(),
                           new_cursor_pos,
                           new_text,
                           new_text_start);
 }
 
 void TextfieldViewsModel::ExecuteAndRecordReplace(MergeType merge_type,
                                                   size_t old_cursor_pos,
                                                   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,
                                old_text_start,
                                backward,
                                new_cursor_pos,
                                new_text,
                                new_text_start);
   bool delete_edit = AddOrMergeEditHistory(edit);
   edit->Redo(this);
@@ skipping 41 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