MacViews: Fix behavior of move and select commands when selection direction changes.

ui/gfx/render_text.cc

 // Copyright (c) 2012 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 "ui/gfx/render_text.h"
 
 #include <limits.h>
 
 #include <algorithm>
 #include <climits>
@@ skipping 189 matching lines @@
     range = break_list->GetRange(current_break);
     if (range.end() < break_list->max() &&
         !render_text->IsValidCursorIndex(range.end())) {
       range.set_end(
           render_text->IndexOfAdjacentGrapheme(range.end(), CURSOR_FORWARD));
       break_list->ApplyValue(current_break->second, range);
     }
   }
 }
 
+// Merges |range1| and |range2|, with the directionality of the result same as
+// |range2|. |range1| and |range2| must at least share a point.
+gfx::Range MergeRangesWithDirection(const Range& range1, const Range& range2) {
+  DCHECK(!(range1.GetMax() < range2.GetMin() ||

[tapted, 2016/08/11 03:44:07]

DCHECK(range1.Intersects(range2))?

[karandeepb, 2016/08/16 10:24:51]

Removed this function. Also Range::Intersects is weird. The function is not
symmetric, A intersects B does not imply B intersects A.

+           range2.GetMax() < range1.GetMin()));
+  const uint32_t min = std::min(range1.GetMin(), range2.GetMin());
+  const uint32_t max = std::max(range1.GetMax(), range2.GetMax());
+
+  // If |range2| is empty, use the direction opposition to the one of |range1|.
+  bool reversed =
+      range2.is_empty() ? !range1.is_reversed() : range2.is_reversed();
+  return reversed ? Range(max, min) : Range(min, max);
+}

[tapted, 2016/08/11 03:44:07]

ui/gfx OWNERS might be interested in a Range::Union() method (to go with
Intersects..), but I wouldn't add it unless they suggest it

+
 }  // namespace
 
 namespace internal {
 
 // Value of |underline_thickness_| that indicates that underline metrics have
 // not been set explicitly.
 const SkScalar kUnderlineMetricsNotSet = -1.0f;
 
 SkiaTextRenderer::SkiaTextRenderer(Canvas* canvas)
     : canvas_(canvas),
@@ skipping 386 matching lines @@
     }
   }
 }
 
 void RenderText::SetCursorPosition(size_t position) {
   MoveCursorTo(position, false);
 }
 
 void RenderText::MoveCursor(BreakType break_type,
                             VisualCursorDirection direction,
-                            bool select) {
+                            bool select,
+                            SelectionReversedBehavior select_behavior) {
   SelectionModel cursor(cursor_position(), selection_model_.caret_affinity());
   // Cancelling a selection moves to the edge of the selection.
   if (break_type != LINE_BREAK && !selection().is_empty() && !select) {
     SelectionModel selection_start = GetSelectionModelForSelectionStart();
     int start_x = GetCursorBounds(selection_start, true).x();
     int cursor_x = GetCursorBounds(cursor, true).x();
     // Use the selection start if it is left (when |direction| is CURSOR_LEFT)
     // or right (when |direction| is CURSOR_RIGHT) of the selection end.
     if (direction == CURSOR_RIGHT ? start_x > cursor_x : start_x < cursor_x)
       cursor = selection_start;
     // Use the nearest word boundary in the proper |direction| for word breaks.
     if (break_type == WORD_BREAK)
       cursor = GetAdjacentSelectionModel(cursor, break_type, direction);
     // Use an adjacent selection model if the cursor is not at a valid position.
     if (!IsValidCursorIndex(cursor.caret_pos()))
       cursor = GetAdjacentSelectionModel(cursor, CHARACTER_BREAK, direction);
   } else {
     cursor = GetAdjacentSelectionModel(cursor, break_type, direction);
   }
-  if (select)
+  if (select) {
+    // |select_behavior| is not relevant for CHARACTER_BREAK. Verify
+    // |select_behavior| has the default value for this case.
+    DCHECK(break_type != CHARACTER_BREAK ||
+           select_behavior == SELECTION_DEFAULT);
+
+    // SELECTION_EXTEND is only relevant for LINE_BREAK.
+    DCHECK(select_behavior != SELECTION_EXTEND || break_type == LINE_BREAK);

[tapted, 2016/08/11 03:44:07]

What's the behaviour if these DCHECKs fail? (is it bad? can we just ignore?). If
we put these assertions in, they need to be documented in the header too with a
reason (and the use-case that explains why we can't just infer
|select_behaviour| from |break_type| itself. Also don't be scared of putting
DCHECKs in statements. E.g. I think this would be clearer with something like

switch (break_type) {
  case CHARACTER_BREAK:
    // Direction can't change when moving by single characters.
    EXPECT_EQ(SELECTION_DEFAULT, select_behavior)
    break;
  case WORD_BREAK:
    // <Explain why it's not relevant - not just that it isn't - is it just
because Mac doesn't do it?>.
    EXPECT_NE(SELECTION_EXTEND, select_behavior);
    break;
  case LINE_BREAK:
    // Anything goes.
}



and... _sometimes_ (maybe not here) for cases like this there's a way to design
the API so that the compiler can enforce the precondition. E.g. by extending the
BreakType enum or turning it into a bitfield  (and keeping `bool select`).

E.g. 

CHARACTER_BREAK
WORD_BREAK_SWAP_SELECTION
WORD_BREAK_COLLAPSE_SELECTION
LINE_BREAK_SWAP_SELECTION
LINE_BREAK_EXTEND_SELECTION
LINE_BREAK_COLLAPSE_SELECTION

[karandeepb, 2016/08/16 10:24:51]

Removed the DCHECK. The behavior is now correct and intuitive even for the cases
not being used, like (CHARACTER_BREAK/WORD_BREAK, SELECTION_EXTEND).

+
     cursor.set_selection_start(selection().start());
+
+    // cursor now corresponds to the new selection. Take |select_behavior| into

[tapted, 2016/08/11 03:44:07]

cursor -> Cursor, or "The cursor"

[karandeepb, 2016/08/16 10:24:51]

Changed to |cursor| since I want to refer to the local variable.

+    // account.
+    bool selection_direction_changed =
+        !selection().is_empty() && !cursor.selection().is_empty() &&
+        selection().is_reversed() != cursor.selection().is_reversed();
+
+    if (select_behavior == SELECTION_EXTEND) {
+      cursor = SelectionModel(
+          MergeRangesWithDirection(selection(), cursor.selection()),
+          selection_model_.caret_affinity());
+    } else if (selection_direction_changed &&
+               select_behavior == SELECTION_CARET) {
+      cursor = SelectionModel(selection().start(),
+                              selection_model_.caret_affinity());
+    }
+  }
+
   MoveCursorTo(cursor);
 }
 
 bool RenderText::MoveCursorTo(const SelectionModel& model) {
   // Enforce valid selection model components.
   size_t text_length = text().length();
   Range range(std::min(model.selection().start(),
                        static_cast<uint32_t>(text_length)),
               std::min(model.caret_pos(), text_length));
   // The current model only supports caret positions at valid cursor indices.
@@ skipping 939 matching lines @@
 
   SetDisplayOffset(display_offset_.x() + delta_x);
 }
 
 void RenderText::DrawSelection(Canvas* canvas) {
   for (const Rect& s : GetSubstringBounds(selection()))
     canvas->FillRect(s, selection_background_focused_color_);
 }
 
 }  // namespace gfx