Closed Bug 977161 Opened 6 years ago Closed 5 years ago

Duplicate device removal and/or hide devices

Categories

(Firefox for Android :: Android Sync, enhancement)

All
Android
enhancement
Not set

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
firefox35 --- verified

People

(Reporter: aaronmt, Assigned: nalexander)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [qa+])

Attachments

(5 files)

Attached image screenshot.png
Screenshot says it all.
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.
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.
Depends on: 963446, 977164, 977167
Flags: needinfo?(rfkelly)
Summary: RFE: Duplicate device removal and or hide devices → Duplicate device removal and/or hide devices
TTLs are indeed enabled on the new sync, same as the old sync
Flags: needinfo?(rfkelly)
Whiteboard: [qa+]
Duplicate of this bug: 1005440
Duplicate of this bug: 1054582
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.
Attachment #8489077 - Flags: review?(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.
Attachment #8489079 - Flags: review?(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.
Attachment #8489080 - Flags: review?(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.
Attachment #8489081 - Flags: review?(rnewman)
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Depends on: 1064304
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.
Attachment #8489077 - Flags: review?(rnewman) → review+
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.
Attachment #8489079 - Flags: review?(rnewman) → review+
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.
Attachment #8489080 - Flags: review?(rnewman) → review+
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
Attachment #8489081 - Flags: review?(rnewman) → review+
Blocks: 1064304
No longer depends on: 963446, 1025131, 1064304
Flags: qe-verify?
Hardware: ARM → All
Version: Firefox 30 → unspecified
(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.
(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.
(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;
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.
Depends on: 1069159
Status: RESOLVED → VERIFIED
Flags: qe-verify?
Flags: qe-verify+
Flags: in-moztrap?(fennec)
Product: Android Background Services → Firefox for Android
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.