Views: Extract text selection code from Textfield.

ui/views/selection_controller.h

Index: ui/views/selection_controller.h
diff --git a/ui/views/selection_controller.h b/ui/views/selection_controller.h
new file mode 100644
index 0000000000000000000000000000000000000000..fe164338180ab747c1ded9e5be00ba3000b3ebf2
--- /dev/null
+++ b/ui/views/selection_controller.h
@@ -0,0 +1,150 @@
+// Copyright 2016 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_VIEWS_SELECTION_CONTROLLER_H_
+#define UI_VIEWS_SELECTION_CONTROLLER_H_
+
+#include "base/time/time.h"
+#include "base/timer/timer.h"
+#include "ui/gfx/geometry/point.h"
+#include "ui/gfx/range/range.h"
+#include "ui/gfx/selection_model.h"
+#include "ui/views/views_export.h"
+
+namespace gfx {
+class RenderText;
+}
+
+namespace ui {
+class MouseEvent;
+}
+
+namespace views {
+
+// Helper class used to facilitate mouse event handling and text selection. To
+// use, clients must implement the Delegate interface.
+class VIEWS_EXPORT SelectionController {
+ public:
+  class Delegate;
+
+  // |delegate| must be non-null.
+  explicit SelectionController(Delegate* delegate);
+
+  // Handle mouse events forwarded by |delegate_|. If |handled| is true, the
+  // mouse event is just used to update the internal state without making any

[msw, 2016/10/14 18:54:20]

nit: It's not entirely obvious what this means... can you clarify what other
changes or just clarify the meaning of |handled| (ie. should it be true iff the
event was already handled by the delegate, or what?). Can we re-use the existing
Event::handled bool flag or similar? Otherwise, what does it mean if the event's
flag doesn't match this flag?

It seems like we could expose TrackMouseClicks to call unconditionally from
Textfield, etc. and then only call OnMousePressed (without a handled flag) from
Textfield if the event wasn't handled by its controller, etc.

[karandeepb, 2016/10/17 07:08:15]

Yeah I meant already handled by the delegate. Have clarified the comment. 
Reading the Event::handled() documentation, it's not clear to me if this is the
right use case for Event::SetHandled/handled. 
Yeah can do that. Although, I prefer this slightly more, since it doesn't seem
right to make the delegate call both TrackMouseClicks and OnMousePressed one
after the other. This seems less error-prone, since the delegate implementation
may for example, fail to call TrackMouseClicks.

[msw, 2016/10/18 23:15:05]

Acknowledged.

+  // other changes.
+  bool OnMousePressed(const ui::MouseEvent& event, bool handled);
+  bool OnMouseDragged(const ui::MouseEvent& event);
+  void OnMouseReleased(const ui::MouseEvent& event);
+
+  // Returns the latest click location.
+  gfx::Point last_click_location() const { return last_click_location_; }
+
+ private:
+  // Helper methods to call the corresponding methods on the associated
+  // RenderText instance.
+  void SelectAll(bool reversed);
+  void SelectWordAt(const gfx::Point& point);
+  void SelectWord();
+  void SelectRange(const gfx::Range& range);
+  void MoveCursorTo(const gfx::Point& point, bool select);
+  void ClearSelection();
+
+  // Selects till the beginning/end of line in the given |direction|.
+  // Todo(karandeepb): Remove once multi-line text selection is supported.

[msw, 2016/10/14 18:54:21]

nit: TODO

[karandeepb, 2016/10/17 07:08:15]

Done.

+  void SelectTillEdge(gfx::VisualCursorDirection direction);
+
+  // Tracks the mouse clicks for single/double/triple clicks.
+  void TrackMouseClicks(const ui::MouseEvent& event);
+
+  // Returns the associated render text instance via the |delegate_|.
+  gfx::RenderText* GetRenderText();
+
+  // Helper function to update the selection on a mouse drag as per
+  // |last_drag_location_|. Can be called asynchronously, through a timer.
+  void SelectThroughLastDragLocation();
+
+  // A timer and point used to modify the selection when dragging.
+  base::RepeatingTimer drag_selection_timer_;
+  gfx::Point last_drag_location_;
+
+  // State variables used to track double and triple clicks.
+  base::TimeTicks last_click_time_;
+  gfx::Point last_click_location_;
+  size_t aggregated_clicks_;
+  gfx::Range double_click_word_;
+
+  // Weak pointer.
+  Delegate* delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(SelectionController);
+};
+
+// An interface implemented/managed by a view which uses the
+// SelectionController.
+class VIEWS_EXPORT SelectionController::Delegate {

[sky, 2016/10/14 22:29:48]

The chromium style guide encourages files like this in their own header. I also
wouldn't use an inner class, but SelectionControllerDelegate.

[karandeepb, 2016/10/17 07:08:15]

Done.

+ public:
+  // Returns the associated RenderText instance to be used for selection.
+  virtual gfx::RenderText* GetRenderTextForSelection() = 0;

[msw, 2016/10/14 18:54:20]

Is the ForSelection postfix just there to avoid a naming collision? Maybe do
ForSelectionController? (for selection makes me think that the returned value
might depend on the current selection, which doesn't make sense)

[karandeepb, 2016/10/17 07:08:15]

Changed to ForSelectionController. Didn't want to use just GetRenderText since a
view may manage multiple render text instances like in the case of Labels. And
also, to avoid name collision.

+
+  // Methods related to properties of the associated view.
+
+  // Returns true if the associated text view is read only.
+  virtual bool IsReadOnly() const = 0;
+  // Returns whether there is a drag operation originating from the associated
+  // view.
+  virtual bool HasTextBeingDragged() const = 0;
+  // Sets whether text is being dragged from the associated view.
+  virtual void SetTextBeingDragged(bool value) = 0;
+  // Returns the height of the associated view.
+  virtual int GetViewHeight() const = 0;
+  // Returns the width of the associated view.
+  virtual int GetViewWidth() const = 0;
+  // Returns the drag selection timer delay. This is the duration after which a
+  // drag selection is updated when the event location is outside the text
+  // bounds.
+  virtual int GetDragSelectionDelay() const = 0;
+
+  // Methods called to notify a mouse action which may change the associated

[msw, 2016/10/14 18:54:20]

Should these be generalized to support keyboard-based selection modification?
(blink supports SHIFT+CTRL+RIGHT to extend the selection right by word, etc.)
You could rename them On[Before|After]SelectionAction or similar.

[karandeepb, 2016/10/17 07:08:15]

But SelectionController is only managing selection modifications made through
the mouse. I can rename the class to MouseSelectionController, if it makes the
intent more clear.

[msw, 2016/10/18 23:15:05]

Yeah, but it seems reasonable to add keyboard-based selection changes soon. It's
fine as-is, at least don't rename the class to be mouse-specific.

[karandeepb, 2016/10/20 04:12:57]

Oh ok. I didn't realize that text selection could be modified using the keyboard
for labels. Keeping it as it is for the time being, since it makes the intent
more clear. This can be modified once keyboard selection support is added.

+  // view's text and/or selection.
+
+  // Called before performing a mouse action. Should not be called in succession
+  // and must always be followed by an OnAfterMouseAction call.
+  virtual void OnBeforeMouseAction() = 0;
+  // Called after the mouse action is performed. |text_changed| and
+  // |selection_changed| can be used by subclasses to make any necessary updates
+  // like redraw the text. Must always be preceeded by a OnBeforeMouseAction
+  // call.
+  virtual void OnAfterMouseAction(bool text_changed,
+                                  bool selection_changed) = 0;
+
+  // Called to notify when the current selection is updated. Must always be
+  // called during a mouse action.
+
+  // Called before the selection is updated. Not called in succession and always
+  // followed by an OnAfterSelectionUpdated call.
+  virtual void OnBeforeSelectionUpdated() = 0;
+  // Called after the selection is updated. Not called in succession and always
+  // preceeded by an OnBeforeSelectionUpdated call.
+  virtual void OnAfterSelectionUpdated() = 0;
+
+  // Selection clipboard related methods. Called only when a mouse action is in
+  // progress.
+
+  // Moves the cursor to the event location and pastes the text from the
+  // selection clipboard. The view may also request focus. NO-OP if the
+  // associated text view is read-only or the event is not a middle click.
+  virtual void PasteSelectionClipboard(const ui::MouseEvent& event) = 0;

[msw, 2016/10/14 18:54:21]

It seems odd for this to live here, on an interface that will also be
implemented by read-only views (ie. Label), instead of remaining on Textfield.
That said, I see how the logic to handle this is pretty closely tied to updating
the selection clipboard, and textfields themselves can be read-only, so it's not
unreasonable...

[karandeepb, 2016/10/17 07:08:15]

Agreed, it's a bit odd. Alternatively, we can handle selection clipboard
management in the subclasses, but it is closely tied to other code in the
SelectionController. Also, we are sharing more logic this way.

+  // Updates the selection clipboard with the currently selected text. Should
+  // empty the selection clipboard if no text is currently selected. NO-OP if
+  // the associated text view is obscured.
+  virtual void UpdateSelectionClipboard() = 0;
+
+ protected:
+  virtual ~Delegate() {}
+};
+
+}  // namespace views
+
+#endif  // UI_VIEWS_SELECTION_CONTROLLER_H_