An edit for SetText needs to be merged with previous edit for omnibox.

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 23 matching lines @@
     INSERT_EDIT,
     DELETE_EDIT,
     REPLACE_EDIT
   };
 
   virtual ~Edit() {
   }
 
   // Revert the change made by this edit in |model|.
   void Undo(TextfieldViewsModel* model) {
-    size_t old_cursor = delete_backward_ ? old_text_end() : old_text_start_;
     model->ModifyText(new_text_start_, new_text_end(),
                       old_text_, old_text_start_,
-                      old_cursor);
+                      old_cursor_pos_);
   }
 
   // Apply the change of this edit to the |model|.
   void Redo(TextfieldViewsModel* model) {
     model->ModifyText(old_text_start_, old_text_end(),
                       new_text_, new_text_start_,
-                      new_text_end());
+                      new_cursor_pos_);
   }
 
-  // Try to merge the edit into this edit. Returns true if merge was
+  // Try to merge the |edit| into this edit. Returns true if merge was
   // successful, or false otherwise. Merged edit will be deleted after
   // redo and should not be reused.
-  bool Merge(Edit* edit) {
-    return mergeable_ && edit->mergeable() && DoMerge(edit);
+  bool Merge(const Edit* edit) {
+    if (edit->merge_with_previous()) {
+      MergeSet(edit);
+      return true;
+    }
+    return mergeable() && edit->mergeable() && DoMerge(edit);
   }
 
   // Commits the edit and marks as un-mergeable.
-  void Commit() { mergeable_ = false; }
+  void Commit() { merge_type_ = DONT_MERGE; }
 
  private:
   friend class InsertEdit;
   friend class ReplaceEdit;
   friend class DeleteEdit;
 
   Edit(Type type,
-       bool mergeable,
+       MergeType merge_type,
+       size_t old_text_cursor,
        string16 old_text,
        size_t old_text_start,
        bool delete_backward,
+       size_t new_text_cursor,
        string16 new_text,
        size_t new_text_start)
       : type_(type),
-        mergeable_(mergeable),
+        merge_type_(merge_type),
+        old_cursor_pos_(old_text_cursor),

[msw, 2011/06/02 10:16:42]

nit: Rename the parameter |old_text_cursor| to |old_cursor_pos| to match its
corresponding member or vice versa. Same for new*.

[oshima, 2011/06/02 19:11:42]

Done.

         old_text_(old_text),
         old_text_start_(old_text_start),
         delete_backward_(delete_backward),
+        new_cursor_pos_(new_text_cursor),
         new_text_(new_text),
         new_text_start_(new_text_start) {
   }
 
   // A template method pattern that provides specific merge
   // implementation for each type of edit.
-  virtual bool DoMerge(Edit* edit) = 0;
+  virtual bool DoMerge(const Edit* edit) = 0;
 
-  Type type() { return type_; }
+  Type type() const { return type_; }
 
   // Can this edit be merged?
-  bool mergeable() { return mergeable_; }
+  bool mergeable() const { return merge_type_ == MERGEABLE; }
+
+  // Does this edit should be merged with previous edit?

[msw, 2011/06/02 10:16:42]

nit: comment phrasing; perhaps eliminate this function and just inline the
comparison at its only use, in Merge().

[oshima, 2011/06/02 19:11:42]

I tried it but somehow didn't like it (probably due to inconsistency with
mergeable()), but can be convinced otherwise. Let me know if you feel strongly
about it.

+  bool merge_with_previous() const {
+    return merge_type_ == MERGE_WITH_PREVIOUS;
+  }
 
   // Returns the end index of the |old_text_|.
-  size_t old_text_end() { return old_text_start_ + old_text_.length(); }
+  size_t old_text_end() const { return old_text_start_ + old_text_.length(); }
 
   // Returns the end index of the |new_text_|.
-  size_t new_text_end() { return new_text_start_ + new_text_.length(); }
+  size_t new_text_end() const { return new_text_start_ + new_text_.length(); }
+
+  // Merge the Set edit into the current edit. This is a special case to
+  // handle an omnibox setting autocomplete string after new character is
+  // typed in.
+  void MergeSet(const Edit* edit) {
+    CHECK_EQ(REPLACE_EDIT, edit->type_);
+    CHECK_EQ(0U, edit->old_text_start_);
+    CHECK_EQ(0U, edit->new_text_start_);
+    string16 old_text = edit->old_text_;
+    old_text.erase(new_text_start_, new_text_.length());
+    old_text.insert(old_text_start_, old_text_);
+    old_text_ = old_text;

[msw, 2011/06/02 10:16:42]

I thought I had this figured out before, but now I read this work around
old_text_ as a net no-op. Discussion in person, or better yet, explanatory
comments would be awesome.

[oshima, 2011/06/02 19:11:42]

comment added. Will explain in person too.

+    old_text_start_ = edit->old_text_start_;
+    delete_backward_ = false;
+
+    new_text_ = edit->new_text_;
+    new_text_start_ = edit->new_text_start_;
+    merge_type_ = DONT_MERGE;
+  }
 
   Type type_;
 
   // True if the edit can be marged.
-  bool mergeable_;
+  MergeType merge_type_;
+  // Old cursor position.
+  size_t old_cursor_pos_;
   // Deleted text by this edit.
   string16 old_text_;
   // The index of |old_text_|.
   size_t old_text_start_;
   // True if the deletion is made backward.
   bool delete_backward_;
+  // New cursor position.
+  size_t new_cursor_pos_;
   // Added text.
   string16 new_text_;
   // The index of |new_text_|
   size_t new_text_start_;
 
   DISALLOW_COPY_AND_ASSIGN(Edit);
 };
 
 class InsertEdit : public Edit {
  public:
   InsertEdit(bool mergeable, const string16& new_text, size_t at)
-      : Edit(INSERT_EDIT, mergeable, string16(), at, false, new_text, at) {
+      : Edit(INSERT_EDIT,
+             mergeable ? MERGEABLE : DONT_MERGE,
+             at  /* old cursor */,
+             string16(),
+             at,
+             false /* N/A */,

[msw, 2011/06/02 10:16:42]

nit: the other comments are preceded by two spaces, should this one be as well? 

[oshima, 2011/06/02 19:11:42]

Done.

+             at + new_text.length()  /* new cursor */,
+             new_text,
+             at) {
   }
 
   // Edit implementation.
-  virtual bool DoMerge(Edit* edit) OVERRIDE {
+  virtual bool DoMerge(const Edit* edit) OVERRIDE {
     if (edit->type() != INSERT_EDIT || new_text_end() != edit->new_text_start_)
       return false;
     // If continuous edit, merge it.
     // TODO(oshima): gtk splits edits between whitespace. Find out what
     // we want to here and implement if necessary.
     new_text_ += edit->new_text_;
+    new_cursor_pos_ = edit->new_cursor_pos_;
     return true;
   }
 };
 
 class ReplaceEdit : public Edit {
  public:
-  ReplaceEdit(bool mergeable,
+  ReplaceEdit(MergeType merge_type,
               const string16& old_text,
+              size_t old_text_cursor,

[msw, 2011/06/02 10:16:42]

nit: rename to |old_cursor_pos| to match base class ctor parameter name.

[oshima, 2011/06/02 19:11:42]

Done.

               size_t old_text_start,
               bool backward,
+              size_t new_text_cursor,
               const string16& new_text,
               size_t new_text_start)
-      : Edit(REPLACE_EDIT, mergeable,
+      : Edit(REPLACE_EDIT, merge_type,
+             old_text_cursor,
              old_text,
              old_text_start,
              backward,
+             new_text_cursor,
              new_text,
              new_text_start) {
   }
 
   // Edit implementation.
-  virtual bool DoMerge(Edit* edit) OVERRIDE {
+  virtual bool DoMerge(const Edit* edit) OVERRIDE {
     if (edit->type() == DELETE_EDIT ||
         new_text_end() != edit->old_text_start_ ||
         edit->old_text_start_ != edit->new_text_start_)
       return false;
     old_text_ += edit->old_text_;
     new_text_ += edit->new_text_;
+    new_cursor_pos_ = edit->new_cursor_pos_;
     return true;
   }
 };
 
 class DeleteEdit : public Edit {
  public:
   DeleteEdit(bool mergeable,
              const string16& text,
              size_t text_start,
              bool backward)
-      : Edit(DELETE_EDIT, mergeable,
+      : Edit(DELETE_EDIT,
+             mergeable ? MERGEABLE : DONT_MERGE,
+             (backward ? text_start + text.length() : text_start),
              text,
              text_start,
              backward,
+             text_start,
              string16(),
              text_start) {
   }
 
   // Edit implementation.
-  virtual bool DoMerge(Edit* edit) OVERRIDE {
+  virtual bool DoMerge(const Edit* edit) OVERRIDE {
     if (edit->type() != DELETE_EDIT)
       return false;
 
     if (delete_backward_) {
       // backspace can be merged only with backspace at the
       // same position.
       if (!edit->delete_backward_ || old_text_start_ != edit->old_text_end())
         return false;
       old_text_start_ = edit->old_text_start_;
       old_text_ = edit->old_text_ + old_text_;
+      new_cursor_pos_ = edit->new_cursor_pos_;
     } else {
       // delete can be merged only with delete at the same
       // position.
       if (edit->delete_backward_ || old_text_start_ != edit->old_text_start_)
         return false;
       old_text_ += edit->old_text_;
     }
     return true;
   }
 };
@@ skipping 103 matching lines @@
   DCHECK(CheckInvariant(style_ranges));
 #endif
 }
 
 }  // namespace
 
 using internal::Edit;
 using internal::DeleteEdit;
 using internal::InsertEdit;
 using internal::ReplaceEdit;
+using internal::MergeType;
+using internal::DONT_MERGE;
+using internal::MERGE_WITH_PREVIOUS;
+using internal::MERGEABLE;
 
 /////////////////////////////////////////////////////////////////
 // TextfieldViewsModel: public
 
 TextfieldViewsModel::Delegate::~Delegate() {
 }
 
 TextfieldViewsModel::TextfieldViewsModel(Delegate* delegate)
     : delegate_(delegate),
       cursor_pos_(0),
@@ skipping 62 matching lines @@
     fragments->push_back(TextFragment(current, end, kNormalStyle));
 }
 
 bool TextfieldViewsModel::SetText(const string16& text) {
   bool changed = false;
   if (HasCompositionText()) {
     ConfirmCompositionText();
     changed = true;
   }
   if (text_ != text) {
-    if (changed) // no need to remember composition.
+    if (changed)  // No need to remember composition.
       Undo();
+    size_t old_cursor = cursor_pos_;
+    size_t new_cursor = old_cursor > text.length() ? text.length() : old_cursor;
     SelectAll();
-    InsertTextInternal(text, false);
-    cursor_pos_ = 0;
+    // If there is a composition text, don't merge with previous edit.
+    // Otherwise, force merge the edits.
+    ExecuteAndRecordReplace(
+        changed ? DONT_MERGE : MERGE_WITH_PREVIOUS,
+        old_cursor,
+        new_cursor,
+        text,
+        0U);
+    cursor_pos_ = new_cursor;
   }
   ClearSelection();
   return changed;
 }
 
 void TextfieldViewsModel::Append(const string16& text) {
   if (HasCompositionText())
     ConfirmCompositionText();
   size_t save = cursor_pos_;
   MoveCursorToEnd(false);
@@ skipping 247 matching lines @@
 }
 
 bool TextfieldViewsModel::Undo() {
   if (!CanUndo())
     return false;
   DCHECK(!HasCompositionText());
   if (HasCompositionText())  // safe guard for release build.
     CancelCompositionText();
 
   string16 old = text_;
+  size_t old_cursor = cursor_pos_;
   (*current_edit_)->Commit();
   (*current_edit_)->Undo(this);
 
   if (current_edit_ == edit_history_.begin())
     current_edit_ = edit_history_.end();
   else
     current_edit_--;
-  return old != text_;
+  return old != text_ || old_cursor != cursor_pos_;
 }
 
 bool TextfieldViewsModel::Redo() {
   if (!CanRedo())
     return false;
   DCHECK(!HasCompositionText());
   if (HasCompositionText()) // safe guard for release build.
     CancelCompositionText();
 
   if (current_edit_ == edit_history_.end())
     current_edit_ = edit_history_.begin();
   else
     current_edit_ ++;
   string16 old = text_;
+  size_t old_cursor = cursor_pos_;
   (*current_edit_)->Redo(this);
-  return old != text_;
+  return old != text_ || old_cursor != cursor_pos_;
 }
 
 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.
+    std::swap(cursor_pos_, selection_start_);
     DeleteSelection();
     return true;
   }
   return false;
 }
 
 void TextfieldViewsModel::Copy() {
   if (!HasCompositionText() && HasSelection()) {
     ui::ScopedClipboardWriter(views::ViewsDelegate::views_delegate
         ->GetClipboard()).WriteText(GetSelectedText());
@@ skipping 18 matching lines @@
 void TextfieldViewsModel::DeleteSelection() {
   DCHECK(!HasCompositionText());
   DCHECK(HasSelection());
   ExecuteAndRecordDelete(selection_start_, cursor_pos_, false);
 }
 
 void TextfieldViewsModel::DeleteSelectionAndInsertTextAt(
     const string16& text, size_t position) {
   if (HasCompositionText())
     CancelCompositionText();
-  ExecuteAndRecordReplaceAt(text, position, false);
+  ExecuteAndRecordReplace(DONT_MERGE,
+                          cursor_pos_,
+                          position + text.length(),
+                          text,
+                          position);
 }
 
 string16 TextfieldViewsModel::GetTextFromRange(const ui::Range& range) const {
   if (range.IsValid() && range.GetMin() < text_.length())
     return text_.substr(range.GetMin(), range.length());
   return string16();
 }
 
 void TextfieldViewsModel::GetTextRange(ui::Range* range) const {
   *range = ui::Range(0, text_.length());
@@ skipping 123 matching lines @@
   }
   return position;
 }
 
 void TextfieldViewsModel::InsertTextInternal(const string16& text,
                                              bool mergeable) {
   if (HasCompositionText()) {
     CancelCompositionText();
     ExecuteAndRecordInsert(text, mergeable);
   } else if (HasSelection()) {
-    ExecuteAndRecordReplace(text, mergeable);
+    ExecuteAndRecordReplaceSelection(mergeable ? MERGEABLE : DONT_MERGE,
+                                     text);
   } else {
     ExecuteAndRecordInsert(text, mergeable);
   }
 }
 
 void TextfieldViewsModel::ReplaceTextInternal(const string16& text,
                                               bool mergeable) {
   if (HasCompositionText())
     CancelCompositionText();
   else if (!HasSelection())
@@ skipping 11 matching lines @@
 
 void TextfieldViewsModel::ClearRedoHistory() {
   if (edit_history_.begin() == edit_history_.end())
     return;
   if (current_edit_ == edit_history_.end()) {
     ClearEditHistory();
     return;
   }
   EditHistory::iterator delete_start = current_edit_;
   delete_start++;
-  STLDeleteContainerPointers(delete_start,
-                             edit_history_.end());
+  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 = text_.substr(old_text_start,
                                      std::abs(static_cast<long>(from - to)));
-  Edit* edit = new DeleteEdit(mergeable,
-                              text,
-                              old_text_start,
-                              (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::ExecuteAndRecordReplace(const string16& text,
-                                                  bool mergeable) {
-  size_t at = std::min(cursor_pos_, selection_start_);
-  ExecuteAndRecordReplaceAt(text, at, mergeable);
+void TextfieldViewsModel::ExecuteAndRecordReplaceSelection(
+    MergeType merge_type, const string16& new_text) {

[msw, 2011/06/02 10:16:42]

nit: I think these parameters are supposed to go on individual lines.

[oshima, 2011/06/02 19:11:42]

This is allowed if all arguments fit to single line.
It's in style guide and used in many places (see message_loop for example)

+  size_t new_text_start = std::min(cursor_pos_, selection_start_);
+  size_t new_cursor_pos = at + text.length();

[msw, 2011/06/02 10:16:42]

It looks like you need to update the |at| reference to |new_text_start|.

[oshima, 2011/06/02 19:11:42]

Yes. sorry for typos.

+  ExecuteAndRecordReplace(merge_type,
+                          cursor_pos_,
+                          new_cursor_pos,
+                          new_text,
+                          new_text_start);
 }
 
-void TextfieldViewsModel::ExecuteAndRecordReplaceAt(const string16& text,
-                                                    size_t at,
-                                                    bool mergeable) {
-  size_t text_start = std::min(cursor_pos_, selection_start_);
-  Edit* edit = new ReplaceEdit(mergeable,
+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 = std::min(cursor_pos_, selection_start_);
+  bool backward = selection_start_ > cursor_pos_;
+  Edit* edit = new ReplaceEdit(merge_type,
                                GetSelectedText(),
-                               text_start,
-                               selection_start_ > cursor_pos_,
-                               text,
-                               at);
+                               old_cursor_pos,
+                               old_text_start,
+                               backward,
+                               new_cursor_pos,
+                               new_text,
+                               new_text_start);
   bool delete_edit = AddOrMergeEditHistory(edit);
   edit->Redo(this);
   if (delete_edit)
     delete edit;
 }
 
 void TextfieldViewsModel::ExecuteAndRecordInsert(const string16& text,
                                                  bool mergeable) {
   Edit* edit = new InsertEdit(mergeable, text, cursor_pos_);
   bool delete_edit = AddOrMergeEditHistory(edit);
@@ skipping 40 matching lines @@
 }
 
 // static
 TextStyle* TextfieldViewsModel::CreateUnderlineStyle() {
   TextStyle* style = new TextStyle();
   style->set_underline(true);
   return style;
 }
 
 }  // namespace views