bluetooth: Use state_selected for changing text color and mark the TextView as selected.

bluetooth: Use state_selected for changing text color and mark the TextView as selected.

Changes item_chooser_row_text_color.xml use for the TextView's color
to use state_selected instead of state_activated and manually set the TextView
as selected when an item is selected.

This so that the way in which items change colors is consistent across an
items' views. In a follow up patch we will add an icon and that will
require the use of state_selected rather than state_activated.

BUG=543466
Review-Url: https://codereview.chromium.org/2800563003
Cr-Commit-Position: refs/heads/master@{#463465}
Committed: https://chromium.googlesource.com/chromium/src/+/f65d65d3fc6c0413d6f940980bd006654fd02bf8

[ortuno, 2017-04-05 04:27:00]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 04:27:10]

Dry run: 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-05 06:02:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 06:02:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ortuno, 2017-04-05 07:30:40]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 07:30:50]

Dry run: 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-05 08:16:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 08:16:25]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-04-06 05:36:31]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 05:36:50]

Dry run: 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-06 06:26:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 06:26:36]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-04-06 06:27:40]

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

[ortuno, 2017-04-06 06:27:41]

tedchoc: picking up this series of patches again. PTAL

https://codereview.chromium.org/2800563003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/color/item_chooser_row_text_color.xml (right):

https://codereview.chromium.org/2800563003/diff/80001/chrome/android/java/res...
chrome/android/java/res/color/item_chooser_row_text_color.xml:7: <item
android:color="@android:color/white" android:state_selected="true"/>
My intention was to change item_chooser_row_background[1] to state_selected as
well but I ran into some problems and I couldn't get it to work.

1. Calling setSelected() and using state_selected resulted in the row no longer
changing color even though getDrawableState() returned state_selected.
2. I was hoping that by calling setSelected() on the parent view the children
would also be marked as selected. They did get marked as selected as seen by
getDrawableState() but they didn't change color.

(╯°□°）╯︵ ┻━┻

In the end I opted for setting isSelected() manually on the children of the row
view and keeping the item_chooser_row_background with state_activated.

https://codereview.chromium.org/2800563003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2800563003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:171:
assertTrue(getDescriptionTextView(dialog, 1).isSelected());
An initial version of this patch (see patchset #1) had much more thorough
testing. But sadly http://crbug.com/708069 and http://crbug.com/708891 made it
really unreliable. So I just removed the rest of the tests :( If you have any
ideas about either of those two issues I would greatly appreciated them. Though
I would like to leave that as a TODO as I've already spent too much time (last
couple of days) on this patch.

[ortuno, 2017-04-07 01:21:06]

Going back to this. I'm seeing some performance issues with this approach. The
icons are updated ~60 times a second and since we create a new Drawable for each
update touch events are starting to get ignored D:

[Ted C, 2017-04-07 06:02:38]

On 2017/04/07 01:21:06, ortuno wrote:
> Going back to this. I'm seeing some performance issues with this approach. The
> icons are updated ~60 times a second and since we create a new Drawable for
each
> update touch events are starting to get ignored D:

Why are the icons being updated so frequently?  Is it just one icon or many?  Is
the goal
to have the icon pulse up and down waiting for connection?  You might find that
you'll
want to throttle to 30fps to avoid janking the app in general (the higher the
frame
rate, the less cpu time available for the website, so be cautious...running the
tablet
loading throbber at 60fps had massive page load performance issues
historically).

Drawable creation should still be "relatively" small assuming the underlying
bitmaps are reused.  You should be able to do some java profiling to see
where all your time is being used:

https://developer.android.com/studio/profile/traceview.html
https://developer.android.com/studio/profile/systrace.html

You should have monitor available from the android sdk in third_party
which should allow you to do this from that (without installing android studio).

[ortuno, 2017-04-07 06:28:08]

On 2017/04/07 at 06:02:38, tedchoc wrote:
> On 2017/04/07 01:21:06, ortuno wrote:
> > Going back to this. I'm seeing some performance issues with this approach.
The
> > icons are updated ~60 times a second and since we create a new Drawable for
each
> > update touch events are starting to get ignored D:
> 
> Why are the icons being updated so frequently?  Is it just one icon or many? 
Is the goal
> to have the icon pulse up and down waiting for connection?  You might find
that you'll
> want to throttle to 30fps to avoid janking the app in general (the higher the
frame
> rate, the less cpu time available for the website, so be cautious...running
the tablet
> loading throbber at 60fps had massive page load performance issues
historically).
> 

A follow up patch adds icons for Signal Strength. Signal Strength is updates
every time a
device is found which happens at couple of times a second. Because there are
many BLE
beacons around us the icons are updated very often.

> Drawable creation should still be "relatively" small assuming the underlying
> bitmaps are reused.  You should be able to do some java profiling to see
> where all your time is being used:
> 
> https://developer.android.com/studio/profile/traceview.html
> https://developer.android.com/studio/profile/systrace.html
> 
> You should have monitor available from the android sdk in third_party
> which should allow you to do this from that (without installing android
studio).

Thanks for the pointers I'll take a look.

[ortuno, 2017-04-10 00:24:00]

I would still appreciate a review of this patch given that the patch that is
causing problems is still 3 patches away.

[Ted C, 2017-04-10 23:35:53]

lgtm and sorry for not reviewing sooner

https://codereview.chromium.org/2800563003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2800563003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:171:
assertTrue(getDescriptionTextView(dialog, 1).isSelected());
On 2017/04/06 06:27:41, ortuno wrote:
> An initial version of this patch (see patchset #1) had much more thorough
> testing. But sadly http://crbug.com/708069 and http://crbug.com/708891 made it
> really unreliable. So I just removed the rest of the tests :( If you have any
> ideas about either of those two issues I would greatly appreciated them.
Though
> I would like to leave that as a TODO as I've already spent too much time (last
> couple of days) on this patch.

Both of those bugs look to me like we aren't calling notifyDatasetChanged on the
adapter, but I see that removeItemFromList is doing it.

Are you expecting the UI to be reflected synchronously?  First off, these tests
are running on the Instrumentation thread, so you are dealing with thread
contention issues if you aren't running things on the UI thread.  I would try
using a CriteriaHelper like you do above and see if you wait for things do they
eventually happen or does it never end up triggering.

I'm happy to sit down with you for a bit to investigate this further if you want
as well.

[ortuno, 2017-04-10 23:38:46]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-04-10 23:38:47]

Thanks! Once I get around fixing our tests I'll come by for help.

[commit-bot: I haz the power, 2017-04-10 23:39: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-11 00:34:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491867526461640,
"parent_rev": "598a769a9e1dc76058c61ccd0c9e2eb60ca776e5", "commit_rev":
"f65d65d3fc6c0413d6f940980bd006654fd02bf8"}

[commit-bot: I haz the power, 2017-04-11 00:35:49]

Description was changed from

==========
bluetooth: Use state_selected for changing text color and mark the TextView as
selected.

Changes item_chooser_row_text_color.xml use for the TextView's color
to use state_selected instead of state_activated and manually set the TextView
as selected when an item is selected.

This so that the way in which items change colors is consistent across an
items' views. In a follow up patch we will add an icon and that will
require the use of state_selected rather than state_activated.

BUG=543466
==========

to

==========
bluetooth: Use state_selected for changing text color and mark the TextView as
selected.

Changes item_chooser_row_text_color.xml use for the TextView's color
to use state_selected instead of state_activated and manually set the TextView
as selected when an item is selected.

This so that the way in which items change colors is consistent across an
items' views. In a follow up patch we will add an icon and that will
require the use of state_selected rather than state_activated.

BUG=543466

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

[commit-bot: I haz the power, 2017-04-11 00:35:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f65d65d3fc6c0413d6f940980bd0...