Ensure no-hosts view doesn't appear while loading

Ensure no-hosts view doesn't appear while loading

Update the code handling switching out the various host-list-related
views to handle the three views (loading, no hosts, and host chooser)
directly instead of setting the no-hosts view as the chooser's empty
view. The two-state empty view functionality doesn't appear to be
designed to be used along side a third modal loading state.

This fixes an issue where both the loading spinner and the "There's
nothing to connect to" message could appear simultaneously under certain
conditions.

BUG=566177
Committed: https://crrev.com/cb4601ae1cecf4a7d33a65df53c5c35f180129fe
Cr-Commit-Position: refs/heads/master@{#371625}

[rkjnsn, 2016-01-20 18:55:43]

Patchset #1 (id:1) has been deleted

[rkjnsn, 2016-01-20 19:06:58]

rkjnsn@chromium.org changed reviewers:
+ joedow@chromium.org, lambroslambrou@chromium.org

[rkjnsn, 2016-01-20 19:06:58]

First attempt at fixing a bug at Google!

Please take a look.

[Lambros, 2016-01-20 19:23:46]

Thanks! I think this is a reasonable approach.

https://codereview.chromium.org/1608093002/diff/20001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/1608093002/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:158: private
void switchHostListView(boolean loading) {
This name doesn't indicate what the boolean parameter means, so the calling code
is not very readable.
Perhaps a better name is showHostListLoadingIndicator() or similar?

[rkjnsn, 2016-01-20 19:39:41]

https://codereview.chromium.org/1608093002/diff/20001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/1608093002/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:158: private
void switchHostListView(boolean loading) {
On 2016/01/20 19:23:46, Lambros wrote:
> This name doesn't indicate what the boolean parameter means, so the calling
code
> is not very readable.
> Perhaps a better name is showHostListLoadingIndicator() or similar?

My concern is that that doesn't reflect that it's actually switching between
three different views. I think I would be surprised to see the logic for
choosing between the empty view and the chooser in a function called
showHostListLoadingIndicator. Maybe a better name for the boolean, itself, would
help? Or maybe this method should be split into two and not take a bool, at all.

[Lambros, 2016-01-20 22:03:21]

On 2016/01/20 at 19:39:41, rkjnsn wrote:
>
https://codereview.chromium.org/1608093002/diff/20001/remoting/android/java/s...
> File remoting/android/java/src/org/chromium/chromoting/Chromoting.java
(right):
> 
>
https://codereview.chromium.org/1608093002/diff/20001/remoting/android/java/s...
> remoting/android/java/src/org/chromium/chromoting/Chromoting.java:158: private
void switchHostListView(boolean loading) {
> On 2016/01/20 19:23:46, Lambros wrote:
> > This name doesn't indicate what the boolean parameter means, so the calling
code
> > is not very readable.
> > Perhaps a better name is showHostListLoadingIndicator() or similar?
> 
> My concern is that that doesn't reflect that it's actually switching between
three different views. I think I would be surprised to see the logic for
choosing between the empty view and the chooser in a function called
showHostListLoadingIndicator. Maybe a better name for the boolean, itself, would
help? Or maybe this method should be split into two and not take a bool, at all.

Having two methods might not be a bad idea. Or have a member: boolean
mLoadingIndicatorShown;
Try to imagine what methods you would write if the loading-spinner *didn't*
obscure the whole list?
We'd like to implement pull-down-to-refresh for the host-list. In that case, the
loading-indicator would be a little circular spinner, and the previous host-list
(if any) would be visible underneath it.
It's out of scope for this CL, but try to bear this in mind anyway?

[rkjnsn, 2016-01-20 22:32:34]

On 2016/01/20 22:03:21, Lambros wrote:
> Try to imagine what methods you would write if the loading-spinner *didn't*
> obscure the whole list?

What about showHostListLoadingIndicator() and updateHostListView(), where the
former shows the loading indicator (which may or may not obscure the existing
list), and the latter hides the loading indicator and makes sure the appropriate
view is visible?

[Lambros, 2016-01-20 22:53:56]

On 2016/01/20 at 22:32:34, rkjnsn wrote:
> On 2016/01/20 22:03:21, Lambros wrote:
> > Try to imagine what methods you would write if the loading-spinner *didn't*
> > obscure the whole list?
> 
> What about showHostListLoadingIndicator() and updateHostListView(), where the
former shows the loading indicator (which may or may not obscure the existing
list), and the latter hides the loading indicator and makes sure the appropriate
view is visible?

SGTM

[rkjnsn, 2016-01-26 02:29:53]

On 2016/01/20 22:53:56, Lambros wrote:
> On 2016/01/20 at 22:32:34, rkjnsn wrote:
> > On 2016/01/20 22:03:21, Lambros wrote:
> > > Try to imagine what methods you would write if the loading-spinner
*didn't*
> > > obscure the whole list?
> > 
> > What about showHostListLoadingIndicator() and updateHostListView(), where
the
> former shows the loading indicator (which may or may not obscure the existing
> list), and the latter hides the loading indicator and makes sure the
appropriate
> view is visible?
> 
> SGTM

Okay. I uploaded a new patch set taking this approach.

[Lambros, 2016-01-26 18:28:07]

lgtm

[joedow, 2016-01-26 22:43:23]

lgtm

[rkjnsn, 2016-01-26 22:52:47]

The CQ bit was checked by rkjnsn@chromium.org

[commit-bot: I haz the power, 2016-01-26 22:54:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1608093002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1608093002/40001

[commit-bot: I haz the power, 2016-01-26 23:08:12]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-01-26 23:09:23]

Description was changed from

==========
Ensure no-hosts view doesn't appear while loading

Update the code handling switching out the various host-list-related
views to handle the three views (loading, no hosts, and host chooser)
directly instead of setting the no-hosts view as the chooser's empty
view. The two-state empty view functionality doesn't appear to be
designed to be used along side a third modal loading state.

This fixes an issue where both the loading spinner and the "There's
nothing to connect to" message could appear simultaneously under certain
conditions.

BUG=566177
==========

to

==========
Ensure no-hosts view doesn't appear while loading

Update the code handling switching out the various host-list-related
views to handle the three views (loading, no hosts, and host chooser)
directly instead of setting the no-hosts view as the chooser's empty
view. The two-state empty view functionality doesn't appear to be
designed to be used along side a third modal loading state.

This fixes an issue where both the loading spinner and the "There's
nothing to connect to" message could appear simultaneously under certain
conditions.

BUG=566177

Committed: https://crrev.com/cb4601ae1cecf4a7d33a65df53c5c35f180129fe
Cr-Commit-Position: refs/heads/master@{#371625}
==========

[commit-bot: I haz the power, 2016-01-26 23:09:24]

Patchset 2 (id:??) landed as
https://crrev.com/cb4601ae1cecf4a7d33a65df53c5c35f180129fe
Cr-Commit-Position: refs/heads/master@{#371625}