Simplify handling of BiDi cursor movement

ui/views/controls/textfield/native_textfield_views.cc

Index: ui/views/controls/textfield/native_textfield_views.cc
diff --git a/ui/views/controls/textfield/native_textfield_views.cc b/ui/views/controls/textfield/native_textfield_views.cc
index 8da7db6bf39db3ba702cd835aaffa34d8c5273d5..01ddaa023f90de7bef5bddf88996639bd2d65ba6 100644
--- a/ui/views/controls/textfield/native_textfield_views.cc
+++ b/ui/views/controls/textfield/native_textfield_views.cc
@@ -186,8 +186,7 @@ int NativeTextfieldViews::OnPerformDrop(const DropTargetEvent& event) {
   OnBeforeUserAction();
   skip_input_method_cancel_composition_ = true;
 
-  // TODO(msw): Remove final reference to FindCursorPosition.
-  gfx::SelectionModel drop_destination =
+  gfx::SelectionModel drop_destination_model =
       GetRenderText()->FindCursorPosition(event.location());
   string16 text;
   event.data().GetString(&text);
@@ -199,20 +198,12 @@ int NativeTextfieldViews::OnPerformDrop(const DropTargetEvent& event) {
     gfx::SelectionModel selected;
     model_->GetSelectionModel(&selected);
     // Adjust the drop destination if it is on or after the current selection.
-    size_t max_of_selected_range = std::max(selected.selection_start(),
-                                            selected.selection_end());
-    size_t min_of_selected_range = std::min(selected.selection_start(),
-                                            selected.selection_end());
-    size_t selected_range_length = max_of_selected_range -
-                                   min_of_selected_range;
-    size_t drop_destination_end = drop_destination.selection_end();
-    if (max_of_selected_range <= drop_destination_end)
-      drop_destination_end -= selected_range_length;
-    else if (min_of_selected_range <= drop_destination_end)
-      drop_destination_end = min_of_selected_range;
-    model_->DeleteSelectionAndInsertTextAt(text, drop_destination_end);
+    size_t drop_destination = drop_destination_model.caret_pos();
+    drop_destination -=
+        selected.selection().Intersect(ui::Range(0, drop_destination)).length();
+    model_->DeleteSelectionAndInsertTextAt(text, drop_destination);
   } else {
-    model_->MoveCursorTo(drop_destination);
+    model_->MoveCursorTo(drop_destination_model);
     // Drop always inserts text even if the textfield is not in insert mode.
     model_->InsertText(text);
   }
@@ -248,18 +239,18 @@ void NativeTextfieldViews::SelectRect(const gfx::Point& start,
   if (GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE)
     return;
 
-  gfx::SelectionModel start_pos = GetRenderText()->FindCursorPosition(start);
-  gfx::SelectionModel end_pos = GetRenderText()->FindCursorPosition(end);
+  size_t start_pos = GetRenderText()->FindCursorPosition(start).caret_pos();

[xji, 2012/02/16 02:38:29]

s/caret_pos()/selection_start()/

[benrg, 2012/02/16 09:20:55]

FindCursorPosition returns a caret position with an empty selection. I think
caret_pos() makes more sense.

+  gfx::SelectionModel end_model = GetRenderText()->FindCursorPosition(end);
+  size_t end_pos = end_model.caret_pos();
 
   OnBeforeUserAction();
   // Merge selection models of "start_pos" and "end_pos" so that
   // selection start is the value from "start_pos", while selection end,
   // caret position, and caret placement are values from "end_pos".
-  if (start_pos.selection_start() == end_pos.selection_end())
-    model_->SelectSelectionModel(end_pos);
+  if (start_pos == end_pos)
+    model_->SelectSelectionModel(end_model);

[msw, 2012/02/16 00:09:21]

nit: Optionally set the selection start on |end_model| and always call
model_->SelectSelectionModel(end_model);

[benrg, 2012/02/16 09:20:55]

This is another place where I was just trying to preserve existing behavior, but
thinking about it now, I think your change will preserve the observable behavior
even though it sometimes leads to a different SelectionModel.

I used set_selection_start(), which necessitated making it public. I don't know
if that's a problem. The default assignment operator was public already.

Actually, I think this method may be dead. It's only called from
touch_selection_controller_impl.cc, which isn't listed in any GYP file.

[xji, 2012/02/16 19:52:55]

I remember the reason to make SelectionModel::set_selection_start() is that we
do not want upper layers (except RenderText) to change that value arbitrarily
without considering its impact to caret_affinity.

for example, text "abcDEF", visually as "abcFED", if original selection is (1,
3), the caret affinity is BACKWARD,
if set_selection_start as 5, the selection range changes to (5, 3), the affinity
should be changed to FORWARD.
touch_selection_controller_impl.cc is disabled for now. I think varanjain will
enable the functionality, but I do not know whether the code will remain the
same.

[benrg, 2012/02/21 23:21:45]

I changed it back, but I'm not sure I believe that the affinity should depend on
the selection in the way you describe. Consider abcF|ED -> Shift+Left -> abc|*ED
-> Shift+Left -> ab|*F** -> Shift+Right -> ???. Currently it goes to abc|F**
because we don't set the affinity according to the selection. If we did, the
cursor would jump from ab|*F** to abcF**|.

In practice it doesn't matter because we don't draw the cursor when there's a
selection. Also, if my selection proposal (currently private) is adopted, this
will basically be irrelevant.

[xji, 2012/02/22 01:30:34]

You are right in this case, in which we have explicitly set the (caret_pos,
caret_affinity) of the selection's start and end point.

when cursor is at "abcF|ED", the selection start's selection model is (4,
BACKWARD) or (5, FORWARD).
when shift+right from ab|*F**, the selection end is attached to the character it
just passed over, so the selection model is (3, BACKWARD).

But currently, we have SelectRange() which upper layer calls to just set the
selection range without any affinity. That is what I meant the default affinity
would take place.

As you mentioned, SelectRange() is problematic. Ideally, we should never just
set selection's start/end position without affinity. In UI, mouse or
keyboard/cursor operation always indicate affinity. But I am not sure whether
all of our upper layer code could be changed to calculate and pass in affinity
for both selection start and selection end.
It matters when we collapse selection, for example, press left (or right) arrow
when there is selection.
the proposal you emailed Aharon?

   else
-    model_->SelectRange(ui::Range(start_pos.selection_start(),
-                                  end_pos.selection_end()));
+    model_->SelectRange(ui::Range(start_pos, end_pos));
 
   OnCaretBoundsChanged();
   SchedulePaint();
@@ -738,8 +729,7 @@ bool NativeTextfieldViews::GetSelectionRange(ui::Range* range) {
 
   gfx::SelectionModel sel;
   model_->GetSelectionModel(&sel);
-  range->set_start(sel.selection_start());
-  range->set_end(sel.selection_end());
+  *range = sel.selection();
   return true;
 }