Textfield: suppress cursor repaints when there's a selection

ui/views/controls/textfield/textfield.cc

Index: ui/views/controls/textfield/textfield.cc
diff --git a/ui/views/controls/textfield/textfield.cc b/ui/views/controls/textfield/textfield.cc
index 14736697771bda755ab2e7e08f2ad70cc1fee919..09cf937993b10417260611f29547350826c1de3b 100644
--- a/ui/views/controls/textfield/textfield.cc
+++ b/ui/views/controls/textfield/textfield.cc
@@ -255,7 +255,7 @@ Textfield::Textfield()
       text_input_flags_(0),
       performing_user_action_(false),
       skip_input_method_cancel_composition_(false),
-      cursor_visible_(false),
+      editing_cursor_visible_(false),

[Peter Kasting, 2016/09/16 18:52:27]

I still don't really know what "editing cursor visible" means:

* Does it read "editing; cursor visible"?  Or "Editing Cursor is visible"?
* If the latter, what is an "editing cursor" as distinct from a non-editing
cursor?

To me, there's just one cursor, a blinking line at the insertion point.  This
would be invisible when there's a selection or when the field has no focus. 
There's also the mouse pointer, which may look like an arrow or an i-beam in
different scenarios, and might (I guess?) be invisible sometimes, like if you
start typing maybe we hide the cursor until it moves.

From your last reply, I read you as talking about the mouse pointer, in which
I'd call this |mouse_pointer_visible_|.  But from reading the code, I think
that's not the case, in which case I'd still just call it the cursor, though it
would be nice to more clearly distinguish between "the cursor is visible in
general, i.e. blinking on and off" vs. "the cursor is currently drawn on the
screen, i.e. in the 'blink on' state".  The former could be named
|cursor_blinking_|.  The latter (which is what I think this actually means) is
the hardest to name; |cursor_visible_| is not terrible, or perhaps
|cursor_drawn_| or even |cursor_currently_drawn_|.  No matter what, I'd extend
the comment in the .h to be very precise about the meaning here.

       drop_cursor_visible_(false),
       initiating_drag_(false),
       aggregated_clicks_(0),
@@ -974,14 +974,14 @@ void Textfield::OnPaint(gfx::Canvas* canvas) {
 
 void Textfield::OnFocus() {
   GetRenderText()->set_focused(true);
-  cursor_visible_ = true;
+  editing_cursor_visible_ = true;

[Peter Kasting, 2016/09/16 18:52:27]

Why not make this mutually exclusive with HasSelection()?

Seems like part of the problem is that you have a variable that supposedly means
that your cursor is onscreen, except that when there's a selection, it doesn't. 
Just making this variable false in that case seems better.

This would mean OnBlur() wouldn't do an unnecessary repaint call when there's a
selection (and thus the cursor is not actually visible), and it would allow you
to simplify the line you touched in PaintTextAndCursor() to check fewer
conditions.

   SchedulePaint();
   if (GetInputMethod())
     GetInputMethod()->SetFocusedTextInputClient(this);
   OnCaretBoundsChanged();
 
   const size_t caret_blink_ms = Textfield::GetCaretBlinkMs();
-  if (caret_blink_ms != 0) {
+  if (caret_blink_ms != 0 && !HasSelection()) {
     cursor_repaint_timer_.Start(FROM_HERE,
         base::TimeDelta::FromMilliseconds(caret_blink_ms), this,
         &Textfield::UpdateCursor);
@@ -996,8 +996,8 @@ void Textfield::OnBlur() {
   if (GetInputMethod())
     GetInputMethod()->DetachTextInputClient(this);
   cursor_repaint_timer_.Stop();
-  if (cursor_visible_) {
-    cursor_visible_ = false;
+  if (editing_cursor_visible_) {
+    editing_cursor_visible_ = false;
     RepaintCursor();
   }
 
@@ -1787,16 +1787,33 @@ void Textfield::UpdateBackgroundColor() {
 }
 
 void Textfield::UpdateAfterChange(bool text_changed, bool cursor_changed) {
+  DCHECK(HasFocus());
   if (text_changed) {
     if (controller_)
       controller_->ContentsChanged(this, text());
     NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true);
   }
   if (cursor_changed) {
-    cursor_visible_ = true;
+    editing_cursor_visible_ = false;
     RepaintCursor();

[Peter Kasting, 2016/09/16 18:52:27]

Nit: Since RepaintCursor() merely schedules a paint, it doesn't actually matter
where it's called.  I wonder if it would be clearer to call it at the end of
this whole conditional, since where it is now, it appears as if we "force the
cursor off and erase it, then do other stuff", which is not what I think the
code actually does.

-    if (cursor_repaint_timer_.IsRunning())
+
+    // If this textfield is not focused, the timer must have been stopped in
+    // |Textfield::OnBlur|, and this method needs to avoid accidentally
+    // restarting it.
+    if (!HasFocus())
+      DCHECK(!cursor_repaint_timer_.IsRunning());

[Peter Kasting, 2016/09/16 18:52:27]

Nit: Just:

    DCHECK(HasFocus() || !cursor_repaint_timer_.IsRunning());

+
+    // Cursor repaints are suppressed whenever there is a selection or this view
+    // does not have focus. When the selection changes, the cursor moves, or
+    // this view gains/loses focus, this method will get called with
+    // |cursor_changed| set to true, and the timer will be restarted here.
+    if (HasSelection()) {

[Peter Kasting, 2016/09/16 18:52:27]

Nit: Inspired by my suggestion above to move the RepaintCursor() call, I think
it might also be clearer to eliminate the unconditional clear of
|editing_cursor_visible_| above and change the code here to:

    editing_cursor_visible_=
        enabled() && !read_only() && HasFocus() && !HasSelection();
    if (editing_cursor_visible_)
      cursor_repaint_timer_.Reset();
    else
      cursor_repaint_timer_.Stop();

Admittedly the Stop() call is not always necessary, but it's not always
necessary in the current way you wrote the code either.  This reads more like
your comment did, and makes it less likely someone later breaks something by
early-returning (or whatever) between when |editing_cursor_visible_| gets
cleared above and when it sometimes gets reset here.  Finally, it means the code
no longer says "we always want to hide the cursor -- oh no wait, keep reading, I
guess we don't"; instead it directly sets the variable to just what it
ultimately means.

+      cursor_repaint_timer_.Stop();
+    } else if (HasFocus() && enabled() && !read_only()) {
+      editing_cursor_visible_ = true;
       cursor_repaint_timer_.Reset();
+    }
+
     if (!text_changed) {
       // TEXT_CHANGED implies TEXT_SELECTION_CHANGED, so we only need to fire
       // this if only the selection changed.
@@ -1811,7 +1828,7 @@ void Textfield::UpdateAfterChange(bool text_changed, bool cursor_changed) {
 
 void Textfield::UpdateCursor() {
   const size_t caret_blink_ms = Textfield::GetCaretBlinkMs();
-  cursor_visible_ = !cursor_visible_ || (caret_blink_ms == 0);
+  editing_cursor_visible_ = !editing_cursor_visible_ || (caret_blink_ms == 0);
   RepaintCursor();
 }
 
@@ -1836,7 +1853,8 @@ void Textfield::PaintTextAndCursor(gfx::Canvas* canvas) {
   }
 
   // Draw the text, cursor, and selection.
-  render_text->set_cursor_visible(cursor_visible_ && !drop_cursor_visible_ &&
+  render_text->set_cursor_visible(editing_cursor_visible_ &&
+                                  !drop_cursor_visible_ &&
                                   !HasSelection());
   render_text->Draw(canvas);