Improve (ViewAnchored)TextBubble logic

Improve (ViewAnchored)TextBubble logic

Make the TextBubble more robust and fix a few bugs:
- Make sure that moving the arrow actually triggers a drawable
invalidation.
- Make the ViewAnchoredTextBubble much more robust to View changes
(translate positioning, removing from hierarchy, visibility changes,
etc.).
- Make the TextBubble try to maintain it's orientation (above vs. below)
the anchor rect.
- Make the TextBubble properly swap orientations if the anchor rect
moves and requires it.
- Fix ViewAnchoredTextBubble to properly detach listeners on all dismiss
paths (instead of just an explicit call to dismiss() on the bubble).

BUG=711168
Review-Url: https://codereview.chromium.org/2818693002
Cr-Commit-Position: refs/heads/master@{#464561}
Committed: https://chromium.googlesource.com/chromium/src/+/da26a507e1367331f15769f75797fd70032a08b8

[David Trainor- moved to gerrit, 2017-04-13 06:02:15]

dtrainor@chromium.org changed reviewers:
+ tedchoc@chromium.org

[David Trainor- moved to gerrit, 2017-04-13 06:02:16]

ptal thanks!

Lots of improvements to TextBubble.  Have fun demo video if you want to see.

[Ted C, 2017-04-13 16:19:47]

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ArrowBubbleDrawable.java
(right):

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ArrowBubbleDrawable.java:124:
invalidateSelf();
Drawable#setBounds (the only caller of onBoundsChange) calls invalidateSelf, so
is this really needed?

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/TextBubble.java
(right):

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/TextBubble.java:204:
protected void onDismissed() {}
any thoughts on just making the dismiss listener a list and have all clients use
that?

[David Trainor- moved to gerrit, 2017-04-13 17:29:29]

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ArrowBubbleDrawable.java
(right):

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ArrowBubbleDrawable.java:124:
invalidateSelf();
On 2017/04/13 16:19:47, Ted C wrote:
> Drawable#setBounds (the only caller of onBoundsChange) calls invalidateSelf,
so
> is this really needed?

We need this because the bounds might not change but the internal drawables
might.  The arrow might move from side to side or above/below.

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/TextBubble.java
(right):

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/TextBubble.java:204:
protected void onDismissed() {}
On 2017/04/13 16:19:47, Ted C wrote:
> any thoughts on just making the dismiss listener a list and have all clients
use
> that?

Yeah that probably makes more sense.  Will change.

[David Trainor- moved to gerrit, 2017-04-13 17:30:26]

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ArrowBubbleDrawable.java
(right):

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ArrowBubbleDrawable.java:124:
invalidateSelf();
On 2017/04/13 17:29:29, David Trainor-ping if over 24h wrote:
> On 2017/04/13 16:19:47, Ted C wrote:
> > Drawable#setBounds (the only caller of onBoundsChange) calls invalidateSelf,
> so
> > is this really needed?
> 
> We need this because the bounds might not change but the internal drawables
> might.  The arrow might move from side to side or above/below.

Actually, I guess we should just be registering as the callback on those
drawables... then propagating the results.  That should make this do what we
want (although this drawable is by no means fully implemented).

[Ted C, 2017-04-13 17:33:48]

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ArrowBubbleDrawable.java
(right):

https://codereview.chromium.org/2818693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ArrowBubbleDrawable.java:124:
invalidateSelf();
On 2017/04/13 17:30:26, David Trainor-ping if over 24h wrote:
> On 2017/04/13 17:29:29, David Trainor-ping if over 24h wrote:
> > On 2017/04/13 16:19:47, Ted C wrote:
> > > Drawable#setBounds (the only caller of onBoundsChange) calls
invalidateSelf,
> > so
> > > is this really needed?
> > 
> > We need this because the bounds might not change but the internal drawables
> > might.  The arrow might move from side to side or above/below.
> 
> Actually, I guess we should just be registering as the callback on those
> drawables... then propagating the results.  That should make this do what we
> want (although this drawable is by no means fully implemented).

setBounds unconditionally calls invalidateSelf.  If you got an onBoundsChange
then it did happen (unless it was the initial bounds setting call).

https://android.googlesource.com/platform/frameworks/base.git/+/master/graphi...

[David Trainor- moved to gerrit, 2017-04-13 20:48:15]

Yeah you're right my bad!  Uploaded the new CL with the Callback registered so
the invalidateSelf() would do stuff.  Thanks!

[Ted C, 2017-04-13 20:49:15]

lgtm

[David Trainor- moved to gerrit, 2017-04-13 21:00:21]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2017-04-13 21:00:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 21:55:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492117221614320,
"parent_rev": "fd8adbccb52a0baed704df75289567067745ec59", "commit_rev":
"da26a507e1367331f15769f75797fd70032a08b8"}

[commit-bot: I haz the power, 2017-04-13 21:56:10]

Description was changed from

==========
Improve (ViewAnchored)TextBubble logic

Make the TextBubble more robust and fix a few bugs:
- Make sure that moving the arrow actually triggers a drawable
invalidation.
- Make the ViewAnchoredTextBubble much more robust to View changes
(translate positioning, removing from hierarchy, visibility changes,
etc.).
- Make the TextBubble try to maintain it's orientation (above vs. below)
the anchor rect.
- Make the TextBubble properly swap orientations if the anchor rect
moves and requires it.
- Fix ViewAnchoredTextBubble to properly detach listeners on all dismiss
paths (instead of just an explicit call to dismiss() on the bubble).

BUG=711168
==========

to

==========
Improve (ViewAnchored)TextBubble logic

Make the TextBubble more robust and fix a few bugs:
- Make sure that moving the arrow actually triggers a drawable
invalidation.
- Make the ViewAnchoredTextBubble much more robust to View changes
(translate positioning, removing from hierarchy, visibility changes,
etc.).
- Make the TextBubble try to maintain it's orientation (above vs. below)
the anchor rect.
- Make the TextBubble properly swap orientations if the anchor rect
moves and requires it.
- Fix ViewAnchoredTextBubble to properly detach listeners on all dismiss
paths (instead of just an explicit call to dismiss() on the bubble).

BUG=711168

Review-Url: https://codereview.chromium.org/2818693002
Cr-Commit-Position: refs/heads/master@{#464561}
Committed:
https://chromium.googlesource.com/chromium/src/+/da26a507e1367331f15769f75797...
==========

[commit-bot: I haz the power, 2017-04-13 21:56:11]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/da26a507e1367331f15769f75797...