Closed Bug 1064304 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

35 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
firefox35 --- verified
fennec 35+ ---

People

(Reporter: aaronmt, Assigned: nalexander)

References

Details

(Whiteboard: [qa+])

Attachments

(6 files, 2 obsolete files)

Currently one can not collapse a device in the new synced tabs panel. This was a feature in the remote tabs pane.
tracking-fennec: --- → ?
Summary: Lost the ability to collapse a device's tab set in the synced tabs panel → Lost the ability to collapse (and then expand) a device's tab set in the synced tabs panel
(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?
Flags: needinfo?(markcapella)
Flags: needinfo?(alam)
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.
Flags: needinfo?(markcapella)
I very much want the ability to collapse Synced devices on my network I don't care about to shorten my list
(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.
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.
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?
Flags: needinfo?(alam)
Anthony, we need a design to move forward here. Up for it?
Assignee: nobody → alam
tracking-fennec: ? → 35+
(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.
rnewman: I'm asking you for something more involved, perhaps you'll
skim this as you go by.
Attachment #8488192 - Flags: review?(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.
Attachment #8488196 - Flags: review?(rnewman)
Attached image prev_mob_syncpanel_tabs1.png (obsolete) —
Are we talking about this design? or need something else?
Comment on attachment 8488233 [details]
prev_mob_syncpanel_tabs1.png

Yes, an updated version of that.  I'll upload a current screenshot.
We look like this now.  The chevrons are place-holders; I took them from some old patch.
Assignee: alam → nalexander
Status: NEW → ASSIGNED
Flags: needinfo?(alam)
Alright I can get on that.
Attachment #8488192 - Flags: review?(rnewman) → review+
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.
Attachment #8488196 - Flags: review?(rnewman) → review+
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.
Attachment #8488196 - Attachment is obsolete: true
Attachment #8489076 - Flags: review?(rnewman)
No longer blocks: 977161
Depends on: 977161
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.
Attachment #8489076 - Flags: review?(rnewman) → review+
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.
Attachment #8488233 - Attachment is obsolete: true
Flags: needinfo?(alam)
Attached file panel_sync_assets.zip
Attaching icons/graphics
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.
Keywords: qawanted
Keywords: qawanted
Whiteboard: [qa+]
QA Contact: aaron.train
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(fennec)
Depends on: 1072954
You need to log in before you can comment on or make changes to this bug.