1064304 - Lost the ability to collapse (and then expand) a device's tab set in the synced tabs panel

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently one can not collapse a device in the new synced tabs panel. This was a feature in the remote tabs pane.

Comment 1

[Nick Alexander :nalexander [he/him]]

(In reply to Aaron Train [:aaronmt] from comment #0)
> Currently one can not collapse a device in the new synced tabs panel. This
> was a feature in the remote tabs pane.

This was a tactical choice to speed landing, but I'm not sure we even want this functionality any more.   AaronMT, do you have an opinion?  capella?  antlam?

Comment 2

[Mark Capella [:capella]]

For my part, I had provided this along with group open/close indicators as a simple addition during bug 1003610. I'd also set it to collapse by default, so the user gets an overview of devices on initial glance. (Like bookmarks has folders that you drill down into).

At that point I wasn't aware of the timing of the overall UI re-design in this area, so I wasn't really acting in concert with the bigger picture. I'd very much going to defer any opinion to nalexander here. I'm happy to help as I can, but cautious not to get in the way.

Comment 3

[Aaron Train [:aaronmt]]

I very much want the ability to collapse Synced devices on my network I don't care about to shorten my list

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Aaron Train [:aaronmt] from comment #3)
> I very much want the ability to collapse Synced devices on my network I
> don't care about to shorten my list

If we didn't show devices that had not synced in k (1? 3?) weeks, would you care as much?  We have a bug that keeps old devices around forever; I think we finally understand said bug and can address it.

Comment 5

[Aaron Train [:aaronmt]]

It's not even the duration, it's just that I sync Firefox on my two laptops and desktop, and my two daily (non-testing) Android phones. I sync them all fairly often, but I only care for the tabs on my desktop. Let me collapse the others.

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

From a UX stand point, I think it's definitely better to have a collapse ability here. If we can't get it done in time, could we land this with the next update?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Anthony, we need a design to move forward here. Up for it?

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Anthony, we need a design to move forward here. Up for it?

I will want visuals for expanded/collapsed, but we might profitably re-use existing icons.  I have an interaction design in mind that I will prototype and work with antlam to refine.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Land home group collapsed and expanded indicator resources. r=margaret

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Show group collapsed/expanded indicator as appropriate. r=margaret

rnewman: I'm asking you for something more involved, perhaps you'll
skim this as you go by.

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Persist group collapsed/expanded state in SharedPreferences. r=rnewman

rnewman: we talked about this once before: Bug 1025131.  I've had a
change of heart: can you read the class comment and provide feedback
about a) using SharedPrefs and b) the per-profile static singleton.

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_mob_syncpanel_tabs1.png

Are we talking about this design? or need something else?

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8488233 [details]
prev_mob_syncpanel_tabs1.png

Yes, an updated version of that.  I'll upload a current screenshot.

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: Remote.Tabs.Home.Panel.png

We look like this now.  The chevrons are place-holders; I took them from some old patch.

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Alright I can get on that.

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8488196 [details] [diff] [review]
Part 3: Persist group collapsed/expanded state in SharedPreferences. r=rnewman

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

r+ with fixes!

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +53,5 @@
>  
>      private static final String[] STAGES_TO_SYNC_ON_REFRESH = new String[] { "clients", "tabs" };
>  
> +    // Maintain group collapsed and hidden state.
> +    private static RemoteTabsExpandableListState sState;

// Only accessed from the UI thread.

@@ +175,5 @@
>      public void onActivityCreated(Bundle savedInstanceState) {
>          super.onActivityCreated(savedInstanceState);
>  
> +        // This races when multiple Fragments are created. That's okay: one
> +        // will win, and thereafter, all will be okay. If we create and then

Perhaps worth mentioning that it's all okay in part because they all run on the UI thread, so it doesn't even need to be volatile.

@@ +193,5 @@
>      }
>  
>      private void updateUiFromClients(List<RemoteClient> clients) {
>          if (clients != null && !clients.isEmpty()) {
>              for (int i = 0; i < mList.getExpandableListAdapter().getGroupCount(); i++) {

I'd pull out the getGroupCount call, and verify that it's <= clients.size(). Otherwise you'll get an out of bounds exception on 198. Better to degrade the experience than to crash.

::: mobile/android/base/home/RemoteTabsExpandableListState.java
@@ +46,5 @@
> +            throw new IllegalArgumentException("sharedPrefs must not be null");
> +        }
> +        this.sharedPrefs = sharedPrefs;
> +
> +        // Nota bena: it is not OK to modify the set returned by {@link

bene.

http://youtu.be/IIAdHEwiAy8?t=52s

@@ +58,5 @@
> +        this.hiddenClients = new HashSet<String>();
> +        final Set<String> hiddenClients = PrefUtils.getStringSet(sharedPrefs,
> +                PREF_HIDDEN_CLIENT_GUIDS, null);
> +        if (hiddenClients != null) {
> +            this.collapsedClients.addAll(hiddenClients);

Code review truism: in any sequence of copied-and-pasted code blocks, there is a bug in the last one. Here is yours.

@@ +83,5 @@
> +
> +        if (modified) {
> +            // This starts an asynchronous write. We don't care if we drop the
> +            // write, and we don't really care if we race between writes, since
> +            // we will return results from our in-memory cache.

Note that shared preferences are themselves a perfect wrapper for the write having occurred, so it's not even important that we can rely on the cache.

@@ +87,5 @@
> +            // we will return results from our in-memory cache.
> +            final Editor editor = sharedPrefs.edit();
> +            PrefUtils.putStringSet(editor, PREF_COLLAPSED_CLIENT_GUIDS,
> +                    collapsedClients);
> +            // The helper above actually calls apply, but that seems wrong.

File a bug; that does seem wrong! It should return the editor for chaining.

@@ +96,5 @@
> +        return modified;
> +    }
> +
> +    public synchronized boolean isClientCollapsed(String clientGuid) {
> +        return collapsedClients.contains(clientGuid);

null check first? It doesn't seem meaningful to query for a null client.

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Persist group collapsed/expanded state in SharedPreferences. r=rnewman

rnewman: you caught one copy-paste error; in response, I abstracted a
lot of that function out into helpers.  Worth taking a second look to
verify I didn't make a different mistake.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 8489076 [details] [diff] [review]
Part 3: Persist group collapsed/expanded state in SharedPreferences. r=rnewman

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

Might be time for a remotetabs package, as I mentioned in the other bug.

::: mobile/android/base/home/RemoteTabsExpandableListState.java
@@ +98,5 @@
> +            final Editor editor = sharedPrefs.edit();
> +            PrefUtils.putStringSet(editor, pref, clients);
> +            // The helper above actually calls apply, but that seems wrong.
> +            // Let's not rely on that for all time.
> +            editor.apply();

Presumably it's safe to apply the same editor twice.

But I have a vague fear that this might end up as an IllegalStateException.

And putStringSet's comment lies outright:

    // Cross version compatible way to save a string set to a pref.
    // NOTE: The editor that is passed in will not commit the transaction for you. It is up to callers to commit
    //       when they are done with any other changes to the database.

putStringSet is called in two places: getStringSet (!!!!!) which actually already calls .apply() on the result (!!!!!!), and GeckoPreferences.

Rather than putting those two comment lines here, please put an .apply() in MultiChoicePreference.persist, and remove the .apply from putStringSet.

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_mob_syncpanel_tabs6.png

Attaching update for the new sync tabs panel. 

Taking into consideration the new issues and limitations that were raised, I've updated the UI to match and hopefully address said issues. Let's give this a try for the first iteration. 

Icons to follow.

Comment 20

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: panel_sync_assets.zip

Attaching icons/graphics

Comment 21

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/a6b0b4b4c09e
https://hg.mozilla.org/integration/fx-team/rev/20585d800894
https://hg.mozilla.org/integration/fx-team/rev/bc600c098de3
https://hg.mozilla.org/integration/fx-team/rev/8685b21b7796

Comment 22

[Nick Alexander :nalexander [he/him]]

AaronMT: okay, let's get some testing on this.  Expand/collapse should persist across browser sessions (it's stored in shared prefs).  The icon is grayed out in one state; we're going to follow-up to match antlam's visual with the whole row grayed out, so that will look better.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6b0b4b4c09e
https://hg.mozilla.org/mozilla-central/rev/20585d800894
https://hg.mozilla.org/mozilla-central/rev/bc600c098de3
https://hg.mozilla.org/mozilla-central/rev/8685b21b7796