Simplify handling of BiDi cursor movement

ui/gfx/selection_model.h

 // 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.
 
 #ifndef UI_GFX_SELECTION_MODEL_H_
 #define UI_GFX_SELECTION_MODEL_H_
 #pragma once
 
-#include <stdlib.h>
+#include <iosfwd>
 
+#include "ui/base/range/range.h"
 #include "ui/base/ui_export.h"
 
 namespace gfx {
 
-// TODO(xji): publish bidi-editing guide line and replace the place holder.
-// SelectionModel is used to represent the logical selection and visual
-// position of cursor.
+// VisualCursorDirection and LogicalCursorDirection represent directions of
+// motion of the cursor in BiDi text. The combinations that make sense are:
 //
-// For bi-directional text, the mapping between visual position and logical
-// position is not one-to-one. For example, logical text "abcDEF" where capital
-// letters stand for Hebrew, the visual display is "abcFED". According to the
-// bidi editing guide (http://bidi-editing-guideline):
-// 1. If pointing to the right half of the cell of a LTR character, the current
-// position must be set after this character and the caret must be displayed
-// after this character.
-// 2. If pointing to the right half of the cell of a RTL character, the current
-// position must be set before this character and the caret must be displayed
-// before this character.
-//
-// Pointing to the right half of 'c' and pointing to the right half of 'D' both
-// set the logical cursor position to 3. But the cursor displayed visually at
-// different places:
-// Pointing to the right half of 'c' displays the cursor right of 'c' as
-// "abc|FED".
-// Pointing to the right half of 'D' displays the cursor right of 'D' as
-// "abcFED|".
-// So, besides the logical selection start point and end point, we need extra
-// information to specify to which character and on which edge of the character
-// the visual cursor is bound to. For example, the visual cursor is bound to
-// the trailing side of the 2nd character 'c' when pointing to right half of
-// 'c'. And it is bound to the leading edge of the 3rd character 'D' when
-// pointing to right of 'D'.
+//  base::i18n::TextDirection  VisualCursorDirection  LogicalCursorDirection
+//       LEFT_TO_RIGHT             CURSOR_LEFT           CURSOR_BACKWARD
+//       LEFT_TO_RIGHT             CURSOR_RIGHT          CURSOR_FORWARD
+//       RIGHT_TO_LEFT             CURSOR_RIGHT          CURSOR_BACKWARD
+//       RIGHT_TO_LEFT             CURSOR_LEFT           CURSOR_FORWARD
+enum VisualCursorDirection {
+  CURSOR_LEFT,
+  CURSOR_RIGHT
+};
+enum LogicalCursorDirection {
+  CURSOR_BACKWARD,
+  CURSOR_FORWARD
+};
+
 class UI_EXPORT SelectionModel {

[xji, 2012/02/24 20:16:52]

Please add comments.

[benrg, 2012/03/07 01:04:44]

I restored your original comment because actually it seems fine as a description
of the new SelectionModel.

  public:
-  enum CaretPlacement {
-    LEADING,
-    TRAILING,
-  };
+  // Create a default SelectionModel to be overwritten later.
+  SelectionModel();
+  // Create a SelectionModel representing a caret |position| without a
+  // selection. The |affinity| is meaningful only when the caret is positioned
+  // between bidi runs that are not visually contiguous: in that case, it
+  // indicates the run to which the caret is attached for display purposes.
+  SelectionModel(size_t position, LogicalCursorDirection affinity);
+  // Create a SelectionModel representing a selection (which may be empty).
+  // The caret position is the end of the range.
+  SelectionModel(ui::Range selection, LogicalCursorDirection affinity);
 
-  SelectionModel();
-  explicit SelectionModel(size_t pos);
-  SelectionModel(size_t end, size_t pos, CaretPlacement status);
-  SelectionModel(size_t start, size_t end, size_t pos, CaretPlacement status);
+  const ui::Range& selection() const { return selection_; }
+  size_t caret_pos() const { return selection_.end(); }
+  LogicalCursorDirection caret_affinity() const { return caret_affinity_; }
 
-  virtual ~SelectionModel();
-
-  size_t selection_start() const { return selection_start_; }
-  size_t selection_end() const { return selection_end_; }
-  size_t caret_pos() const { return caret_pos_; }
-  CaretPlacement caret_placement() const { return caret_placement_; }
-
-  bool Equals(const SelectionModel& sel) const;
+  bool operator==(const SelectionModel& sel) const;
+  bool operator!=(const SelectionModel& sel) { return !(*this == sel); }
 
  private:
   friend class RenderText;
 
-  void Init(size_t start, size_t end, size_t pos, CaretPlacement status);
+  void set_selection_start(size_t pos) { selection_.set_start(pos); }

[msw, 2012/02/28 22:51:32]

Add a comment about why this is private, in case we forget again.

[benrg, 2012/03/07 01:04:44]

I added a comment, but to be honest I'm not sure I get it. It was supposed to be
because you shouldn't set the selection start without also updating the caret
affinity, but the two uses of this method in RenderText *do* set the selection
start without updating the affinity. If that's wrong, as the comment implies,
then we ought to fix it, after which this method could be removed.

For that matter, should SelectionModel(range, affinity) be public? Right now
anybody can simulate sel.set_selection_start(foo) by doing sel =
SelectionModel(Range(foo, sel.caret_pos()), sel.caret_affinity()). Even
RenderText could do that...

[msw, 2012/03/08 18:07:19]

That's correct; users should flip the affinity when the selection directionality
changes *and* the selection start is at a BiDi level change.  Add this
requirement as a comment with Xiaomei's following example:
Given "abcDEF" (visually "abcFED"), with original selection (1, 3, BACKWARD):
set_selection_start(5) should also change the affinity to yield (5, 3, FORWARD).

RenderText::MoveCursor[To] (4 total functions, we should disambiguate the
overrides...) should be audited, it appears they fail to do this. Your point
about the ctor is legitimate too, and I don't see any uses in
NativeTextfieldViews nor TextfieldViewsModel, so it should be easy to make that
ctor private, and give it the same note. We should also write tests to ensure
this is happening correctly.

[msw, 2012/03/08 18:09:36]

Gah, correction: when the "selection *end* is at a BiDi level change".

[xji, 2012/03/08 20:30:02]

What I mean is that if we explicitly know where the caret_affinity should be
(such as when mouse click), we do not change it in set_selectiion_start(). Those
should be the cases in RenderText.

If we do not, such as in SelectRange() (which we already agreed that it has
design flaw, and it should never happen), caret_affinity is determined by
selection_start/end range.
We do not want to expose set_selection_start to be public because we do not want
users to call it randomly without knowing what he is doing.

(And I think we already agreed that we need (caret_pos, caret_affinity) for both
selection_end and selection_start, so there wont be this problem, and the issue
of SelectionRange()).

Looking at all the usage of set_selection_start() in RenderText:
1. In MoveCursor(BreakType, VisualCursorDirection, bool),
the carate_pos and caret_affinity should be determined by
either GetAdjacentSelectionModel() or , GetSelectionModelForSelectionStart().
So, set_selection_start() at the end should not affect them.

2. in MoveCursorTo(const SelectionModel&),
set_selection_start is an error handling step.

3. in MoveCursorTo(const Point&, bool)
caret_pos and caret_affinity is determined by FindCursorPosition(). And final
set_selection_start should not affect them.

4. ClearSelection() should be fine.
5. SelectAll() should be fine.
6. SetSelectionModel() should be fine.

 
-  void set_selection_start(size_t pos) { selection_start_ = pos; }
-  void set_selection_end(size_t pos) { selection_end_ = pos; }
-  void set_caret_pos(size_t pos) { caret_pos_ = pos; }
-  void set_caret_placement(CaretPlacement placement) {
-    caret_placement_ = placement;
-  }
+  // Logical selection. The logical caret position is the end of the selection.
+  ui::Range selection_;
 
-  // Logical selection start. If there is non-empty selection, if
-  // selection_start_ is less than selection_end_, the selection starts visually
-  // at the leading edge of the selection_start_. If selection_start_ is greater
-  // than selection_end_, the selection starts visually at the trailing edge of
-  // selection_start_'s previous grapheme. So, we do not need extra information
-  // for visual bounding.
-  size_t selection_start_;
+  // The logical direction from the caret position (selection_.end()) to the
+  // character it is attached to for display purposes. This matters only when
+  // the surrounding characters are not visually contiguous, which happens only
+  // in bidi text (and only at bidi run boundaries). The text is treated as
+  // though it was surrounded on both sides by runs in the dominant text
+  // direction. For example, supposing the dominant direction is LTR and the
+  // logical text is "abcDEF", where DEF is right-to-left text, the visual
+  // cursor will display as follows:
+  //    caret position    CURSOR_BACKWARD affinity    CURSOR_FORWARD affinity
+  //          0                  |abcFED                     |abcFED
+  //          1                  a|bcFED                     a|bcFED
+  //          2                  ab|cFED                     ab|cFED
+  //          3                  abc|FED                     abcFED|
+  //          4                  abcFE|D                     abcFE|D
+  //          5                  abcF|ED                     abcF|ED
+  //          6                  abc|FED                     abcFED|
+  LogicalCursorDirection caret_affinity_;
+};
 
-  // The logical cursor position that next character will be inserted into.
-  // It is also the end of the selection.
-  size_t selection_end_;
-
-  // The following two fields are used to guide cursor visual position.
-  // The index of the character that cursor is visually attached to.
-  size_t caret_pos_;
-  // The visual placement of the cursor, relative to its associated character.
-  CaretPlacement caret_placement_;
-};
+UI_EXPORT std::ostream& operator<<(std::ostream& out,
+                                   const SelectionModel& sel);
 
 }  // namespace gfx
 
 #endif  // UI_GFX_SELECTION_MODEL_H_