977161 - Duplicate device removal and/or hide devices

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, enhancement)

Description

[Aaron Train [:aaronmt]]

Attachment: screenshot.png

Screenshot says it all.

Comment 1

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8382298 [details]
screenshot.png

Yeah, on the Android side, this will be covered by Bug 963446.  These really do appear as different devices though (they have different Sync client IDs), so it's not trivial to mark them as dupes.

Leaving open to pursue making sure this works across desktop and Android, and for brainstorming UX work-arounds to duplicates.

Comment 2

[Richard Newman [:rnewman]]

Bug 963446 will stop the duplicates appearing when you disable Sync on your device.

We don't want to roll up all devices with the same name -- consider what happens when you have two Macs with the same username, and thus the same device name.

They'll expire on their own after three weeks. (rfkelly, TTLs are enabled on new Sync, right?)

about:home should be filtered out. I'll file a bug for that.

Beyond that, we're into Bug 821532.

Comment 3

[Ryan Kelly [:rfkelly]]

TTLs are indeed enabled on the new sync, same as the old sync

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Part 0: Make Remote{Client,Tab} implement Parcelable. r=rnewman

This is necessary to provide the list of clients to another Fragment:
the way to persist "final members" across configuration changes is to
provide the initial data in the Fragment's arguments bundle.

Note: I'd like to see RemoteClient replace ParcelableClientRecord, and
Fennec's clients table replace direct access to Sync's clients table;
but one step at a time.

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Add hide/show context menu items to remote clients. r=rnewman

We're not (yet) using show, but we may as well add it while we're here.

This is a little more involved than you might expect, but it's a well
worn trail: see TopSitesPanel and friends.  You could kick to margaret
if you prefer.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Add a clickable footer showing hidden device count. r=rnewman

This is quite delicate, due to the interactions between
ExpandableListView and the footer views.  At the end of the day, this
uses an Adapter that handles header and footer views as separate
view-group types.  That means working around Android's limitations; see
the comments in the code for the details.

The alternative would be to implement our own wrapping Adapter; or to
boil the specifics of our use case into RemoteTabsExpandableListAdapter.
This is certainly quicker than the former; and, I hope, less error prone
than the latter.

Final visuals will get hammered out with antlam before/immediately
after landing.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Allow unhiding hidden devices. r=rnewman

I'd like to make the DialogFragment show icons, last synced times,
number of tabs, etc; but AlertDialog does not handle multi-choice lists
backed by a custom list adapters.  (It does handle single-choice lists
backed by custom list adapters.)  We can implement our own dialog
fragment if and when we want better looks.

I've made this somewhat general so that we can use it from
ShareDialog/SendTab, which currently use AlertDialog.  Using
DialogFragment has two advantages.  First, it smoothly persists across
configuration changes; AlertDialog does not.  Second, it allow us to use
AlertDialog until we implement the improved UI above.

Technically this works fine locally.  But the actual UX experience is
a little awkward.  The dialog looks like:

Hidden Devices
-----------------------
X on Y            |   |
Nightly           | x |
-----------------------
Unhide selected devices

To start, no hidden devices are checked.  The user checks devices to
unhide, hits OK, and they return to the list.  This seemed to keep
most with "N devices hidden" in the button.  The hidden/unhide vs. not
shown/show vs something else is hard to comment on.

One reason I went with this approach is we could show *all* the
devices -- the visible ones checked, the hidden ones unchecked -- and
the user could update the whole list.  I didn't do this 'cuz I didn't
want to show "No devices hidden" at the bottom of the list all of the
time; and if it's not there, the UI is less accessible.  (Of course,
long tap for context and then Hide is not very accessible either.)

We'll see how antlam likes it, but suggestions wanted.  You'll
probably need to apply the patches to get a feel for it.

Brain dump over.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8489077 [details] [diff] [review]
Part 0: Make Remote{Client,Tab} implement Parcelable. r=rnewman

Review of attachment 8489077 [details] [diff] [review]:
-----------------------------------------------------------------

It's boilerplate-tastic!

Please file a bug re ParcelableClientRecord.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8489079 [details] [diff] [review]
Part 1: Add hide/show context menu items to remote clients. r=rnewman

Review of attachment 8489079 [details] [diff] [review]:
-----------------------------------------------------------------

If it works, I'm happy.

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +219,5 @@
> +        final boolean isHidden = sState.isClientHidden(info.client.guid);
> +        if (isHidden) {
> +            menu.findItem(R.id.home_remote_tabs_hide_client).setVisible(false);
> +        } else {
> +            menu.findItem(R.id.home_remote_tabs_show_client).setVisible(false);

This is so close to a typo line that I'd almost prefer a ternary to show the variant more clearly:

	
final boolean isHidden = sState.isClientHidden(info.client.guid);
final MenuItem item = menu.findItem(isHidden ? R.id.home_remote_tabs_hide_client :
                                               R.id.home_remote_tabs_show_client);
item.setVisible(false);

@@ +245,5 @@
> +        } else if (itemId == R.id.home_remote_tabs_show_client) {
> +            sState.setClientHidden(info.client.guid, false);
> +            return true;
> +        } else {
> +            return false;

Unnecessary else.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8489080 [details] [diff] [review]
Part 2: Add a clickable footer showing hidden device count. r=rnewman

Review of attachment 8489080 [details] [diff] [review]:
-----------------------------------------------------------------

Gnarly. Did I mention tests would fill me with joy?

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +219,5 @@
> +        // {add,remove}FooterView and setAdapter. setAdapter wraps the adapter
> +        // in a footer/header-managing adapter, which only happens (pre-KitKat)
> +        // if a footer/header is present. Therefore, we add our footer before
> +        // setting the adapter; and then we remove it afterward. From there on,
> +        // we can add/remove it at will.

Oh christ.

@@ +310,5 @@
> +            if (mList.getFooterViewsCount() < 1) {
> +                mList.addFooterView(mFooterView);
> +            }
> +        } else {
> +            mList.removeFooterView(mFooterView);

Can we invert these two clauses?

  if (hiddenClients == null || hiddenClients.isEmpty()) {
      mList.removeFooterView(mFooterView);
  } else {
      ...
  }

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +412,5 @@
>  
> +<!ENTITY home_remote_tabs_one_hidden_device "1 device hidden">
> +<!-- Localization note (home_remote_tabs_many_hidden_devices) : Number of
> +     hidden devices is always more than one.  We can't use Android
> +     plural forms, sadly. See Bug #753859. -->

Note that the argument is a digit?

::: mobile/android/base/resources/layout/home_remote_tabs_hidden_devices_footer.xml
@@ +4,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +-->
> +
> +<!-- This layout is actually necessary, because of an interaction

Lose the comma.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8489081 [details] [diff] [review]
Part 3: Allow unhiding hidden devices. r=rnewman

Review of attachment 8489081 [details] [diff] [review]:
-----------------------------------------------------------------

If it works, I'm happy.

::: mobile/android/base/RemoteClientsDialogFragment.java
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko;

This would be nice in org.mozilla.gecko.remotetabs.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +416,5 @@
>       plural forms, sadly. See Bug #753859. -->
>  <!ENTITY home_remote_tabs_many_hidden_devices "&formatD; devices hidden">
> +<!-- Localization note (home_remote_tabs_hidden_devices_title) : This is the
> +     title of a dialog; we expect more than one device. -->
> +<!ENTITY home_remote_tabs_hidden_devices_title "Hidden Devices">

"Hidden devices"

http://developer.android.com/design/building-blocks/dialogs.html
http://developer.android.com/guide/topics/ui/dialogs.html

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 8489077 [details] [diff] [review]
> Part 0: Make Remote{Client,Tab} implement Parcelable. r=rnewman
> 
> Review of attachment 8489077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's boilerplate-tastic!
> 
> Please file a bug re ParcelableClientRecord.

Filed Bug 1068097 for this and some other unification.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #11)
> Comment on attachment 8489079 [details] [diff] [review]
> Part 1: Add hide/show context menu items to remote clients. r=rnewman
> 
> Review of attachment 8489079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If it works, I'm happy.
> 
> ::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
> @@ +219,5 @@
> > +        final boolean isHidden = sState.isClientHidden(info.client.guid);
> > +        if (isHidden) {
> > +            menu.findItem(R.id.home_remote_tabs_hide_client).setVisible(false);
> > +        } else {
> > +            menu.findItem(R.id.home_remote_tabs_show_client).setVisible(false);
> 
> This is so close to a typo line that I'd almost prefer a ternary to show the
> variant more clearly:
> 
> 	
> final boolean isHidden = sState.isClientHidden(info.client.guid);
> final MenuItem item = menu.findItem(isHidden ?
> R.id.home_remote_tabs_hide_client :
>                                               
> R.id.home_remote_tabs_show_client);
> item.setVisible(false);

I've done this.  I was on the fence; generally, I'm not a ternary fan.

> @@ +245,5 @@
> > +        } else if (itemId == R.id.home_remote_tabs_show_client) {
> > +            sState.setClientHidden(info.client.guid, false);
> > +            return true;
> > +        } else {
> > +            return false;
> 
> Unnecessary else.

Not quite.  We need to return something; additional options are prevented by logic, but that's not available to the compiler.

Comment 16

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #15)

> Not quite.  We need to return something; additional options are prevented by
> logic, but that's not available to the compiler.

Oh, I mean just return, rather than returning inside an else:

  if (foo) {
    return true;
  } else if (bar) {
    return true;
  }

  return false;

Comment 17

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/00a5099d9f09
https://hg.mozilla.org/integration/fx-team/rev/82f14d208742
https://hg.mozilla.org/integration/fx-team/rev/160f3719128d
https://hg.mozilla.org/integration/fx-team/rev/b6cc5828cfd8

Comment 18

[Nick Alexander :nalexander [he/him]]

AaronMT: Hidden devices should persist across browser sessions (shared prefs, again).  Long click a group to "Hide" it.  If you have hidden devices, you should see a footer row (which does not yet match antlam's updated visuals) saying how many devices are hidden.  Tap the footer row to get a (rudimentary) unhide list.  If you unhide all devices, the bottom row should disappear.  If you have no devices/tabs, you should see a "Your remote tabs go here" filler.  If you hide all devices, you'll see that one ugly footer row.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00a5099d9f09
https://hg.mozilla.org/mozilla-central/rev/82f14d208742
https://hg.mozilla.org/mozilla-central/rev/160f3719128d
https://hg.mozilla.org/mozilla-central/rev/b6cc5828cfd8