User Agent: Mozilla/5.0 (Windows NT 6.1; rv:22.0) Gecko/20130312 Firefox/22.0 Build ID: 20130312031046 Steps to reproduce: Send Tab to Device extension (desktop) is broken and the closest users can get is the ability to visit a page, click the sync button on then open sync'd tabs tab on their mobile device. However an overwhelming majority of the time, this hasn't been caught by the mobile browser and updating sync information is an incredibly long task, requiring uses to leave the app completely. Enabling Pull to Refresh as a means of updating these tabs is a huge win for the User Experience in regards to sync'd tabs and being able to present timely usable data.

Not a WONTFIX, but I don't know what our approach is to refreshable UI. Ian?

I agree that this might be a win for the user experience. I think that we alreday implement the following similar behaviour: tapping the "Synced Tabs" tab should trigger a tab sync (and correctly handle the asynchronous updates), but I might be wrong. CCing ibarlow for UI thoughts.

I've certainly caught myself wishing for a manual refresh button somewhere in cases where a synced tab didn't make it through. Pull-to-refresh might be a nice thing to add. Even if we auto-sync when you tap on the synced tabs section, giving a user more obvious/direct control with pull to refresh would be nice too.

Let's do it.

Any input needed from you, Ian, before someone knocks this out?

I'm a bit swamped so I don't want designs to block this. Feel free to start and I'll comment as we go. Basically it should look something like this when you pull down: [downward arrow] Pull down to refresh… and then [upward arrow] Release to refresh…

(In reply to Ian Barlow (:ibarlow) from comment #6) > I'm a bit swamped so I don't want designs to block this. Feel free to start > and I'll comment as we go. Basically it should look something like this when > you pull down: > > [downward arrow] Pull down to refresh… > > and then > > [upward arrow] Release to refresh… Android is moving toward the non-bouncy pull-to-refresh that's featured in Gmail and Google+ whereby as a user pulls down, the action bar displays "Pull to Refresh" and then you get the dashed line. A library implementing this method is featured here: https://github.com/chrisbanes/ActionBar-PullToRefresh We could use an orange dashed line?

I would also take that as an implementation option, yes.

Thanks, folks. Shovel-ready!

jdover: this might be a simpler starting point for your pull-to-refresh work.

Added the SwipeRefreshLayout to the synced tabs tray and it fires TabAccessor.getTabs(), however the content provider query doesn't seem to be returning updated data? This may just have to do with how infrequent synced tabs are posted from browsers to FxSync.

Comment on attachment 8401584 [details] [diff] [review] patch Review of attachment 8401584 [details] [diff] [review]: ----------------------------------------------------------------- Could we get a Part 0 that just does the splitting of RemoteTabs, then a Part 1 that implements the change? It's hard to see what's new. nalexander and margaret can continue the reviewing from here on in -- I'll be on the road. I'll check in when I can. ::: mobile/android/base/RemoteTabsContainer.java @@ +1,3 @@ > +/* 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/. */ Should be /* 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/. */ @@ +69,5 @@ > + } > + > + @Override > + public void onRefresh() { > + TabsAccessor.getTabs(context, this); You need to trigger a sync here, rather than just re-running the query. Waiting on https://github.com/mozilla-services/android-sync/pull/424 , but: Something like: if (SyncAccounts.syncAccountsExist(getContext())) { // TODO: figure this out. It won't be as neat as the FxA version. } else { Account account = FirefoxAccounts.getFirefoxAccount(getContext())); if (account == null) { // They don't use Sync. return; } EnumSet hints = EnumSet.of(FirefoxAccounts.SyncHint.NOW, FirefoxAccounts.SyncHint.IGNORE_LOCAL_RATE_LIMIT); String[] engines = new String[] {"tabs"}; new AndroidFxAccount(getContext(), account).requestSync(hints, engines); }

Flagging nalexander for feedback on the sync code.

Comment on attachment 8402076 [details] [diff] [review] 02 - Add swipe to refresh to sync tab list Review of attachment 8402076 [details] [diff] [review]: ----------------------------------------------------------------- This is really great, and will guide the interface we land for requesting and listening to sync results. In general, we want Fennec to only interact with Sync via the two classes SyncAccounts and FirefoxAccounts; you'll see that the PR you reference (which has landed) exposes FirefoxAccoutns.requestSync. What we haven't done is expose the SyncStatus helper yet. The requestSync call will be easy to update; the status observer stuff requires a little more thought on both of our parts. I'm going to ni myself on this ticket so I don't lose track. Sorry for the delayed reviews, busy times. ::: mobile/android/base/RemoteTabsContainer.java @@ +42,5 @@ > + setColorScheme(R.color.swipe_refresh_orange, R.color.background_tabs, > + R.color.swipe_refresh_orange, R.color.background_tabs); > + > + setOnRefreshListener(new RefreshListener()); > + ContentResolver.addStatusChangeListener(ContentResolver.SYNC_OBSERVER_TYPE_ACTIVE, The status change listener definitely needs to be added in onResume and removed in onPause, because the inner SyncObserver will keep the outer RemoteTabsContainer alive forever.

Comment on attachment 8402072 [details] [diff] [review] 01 - Split RemoteTabs into container and list Review of attachment 8402072 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good, I just think we can improve the layout files. As a for-the-future comment, this might have been easier to review if you did an hg rename from RemoteTabs->RemoteTabsList in one patch, then made changes on top of that in another patch. But I can't judge you too harshly because I basically did exactly what you did in another bug earlier today :) ::: mobile/android/base/RemoteTabsContainer.java @@ +24,5 @@ > +import java.util.List; > + > +public class RemoteTabsContainer extends FrameLayout > + implements TabsPanel.PanelView { > + private static final String LOGTAG = "GeckoRemoteTabs"; Get rid of this, it's unused. @@ +33,5 @@ > + public RemoteTabsContainer(Context context, AttributeSet attrs) { > + super(context, attrs); > + this.context = context; > + > + LayoutInflater.from(context).inflate(R.layout.remote_tabs_list, this); Instead of inflating this here and making a new layout file, why not just include the list in tabs_panel.xml? ::: mobile/android/base/RemoteTabsList.java @@ +15,5 @@ > +import java.util.ArrayList; > +import java.util.HashMap; > +import java.util.List; > + > +public class RemoteTabsList extends ExpandableListView This could be package access, since it should only be used by RemoteTabsContainer. Also, this is a good opportunity to add some class comments to both this class and RemoteTabsContainer to explain what they do (one sentence would be fine). @@ +19,5 @@ > +public class RemoteTabsList extends ExpandableListView > + implements ExpandableListView.OnGroupClickListener, > + ExpandableListView.OnChildClickListener, > + TabsAccessor.OnQueryTabsCompleteListener { > + private static final String LOGTAG = "GeckoRemoteTabs"; This is also unused. @@ +21,5 @@ > + ExpandableListView.OnChildClickListener, > + TabsAccessor.OnQueryTabsCompleteListener { > + private static final String LOGTAG = "GeckoRemoteTabs"; > + > + private Context mContext; I'm assuming you kept these m prefixes because this logic is copied over from RemoteTabs. However, I would r+ a follow-up patch that removes the prefixes for consistency with RemoteTabsContainer. ::: mobile/android/base/resources/layout/remote_tabs_list.xml @@ +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/. --> > + > +<merge xmlns:android="http://schemas.android.com/apk/res/android"> Why do you need this merge? It only has one child, so I think we can get rid of it.

(In reply to Nick Alexander :nalexander from comment #15) > The status change listener definitely needs to be added in onResume and > removed in onPause, because the inner SyncObserver will keep the outer > RemoteTabsContainer alive forever. Also to note that this listener doesn't seem to fire at the right time. When it fires, account.isSyncing() is still returning true and then it isn't firing again after the sync is complete. Maybe FxA needs its own status listening interface? It's also possible I'm using SyncObserver incorrectly, but it seems like what I have should be working, provided that FxASyncProvider uses the content provider normally.

(In reply to Josh Dover [:jdover] from comment #17) > (In reply to Nick Alexander :nalexander from comment #15) > > The status change listener definitely needs to be added in onResume and > > removed in onPause, because the inner SyncObserver will keep the outer > > RemoteTabsContainer alive forever. > > Also to note that this listener doesn't seem to fire at the right time. When > it fires, > account.isSyncing() is still returning true and then it isn't firing again > after the sync > is complete. Maybe FxA needs its own status listening interface? It's also > possible I'm using > SyncObserver incorrectly, but it seems like what I have should be working, > provided that > FxASyncProvider uses the content provider normally. Your SyncObserver code isn't right. (I saw it, and I should have pointed that out in my initial pass.) We'll end up using some code I wrote recently [1] to smooth this Android wart; the ni on myself was to come back to this and expose the listening interface through the FirefoxAccounts interface. [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncStatusHelper.java

Note: this only supports new sync. In IRC, nalexander and I decided that old sync was not worth the extra effort of setting up the SyncObserver if it is going to be deprecated as early as Fx 32.

(In reply to Josh Dover [:jdover] from comment #20) > Note: this only supports new sync. In IRC, nalexander and I decided that old > sync was not worth the extra effort of setting up the SyncObserver if it is > going to be deprecated as early as Fx 32. I think that is the right call. Even if we don't migrate until Fx33 or Fx34. New Sync is where we should be expending effort.

Comment on attachment 8405117 [details] [diff] [review] 01 - Split RemoteTabs into container and list Review of attachment 8405117 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/RemoteTabsList.java @@ +23,5 @@ > +class RemoteTabsList extends ExpandableListView > + implements ExpandableListView.OnGroupClickListener, > + ExpandableListView.OnChildClickListener, > + TabsAccessor.OnQueryTabsCompleteListener { > + private Context context; This could be final. @@ +26,5 @@ > + TabsAccessor.OnQueryTabsCompleteListener { > + private Context context; > + private TabsPanel tabsPanel; > + > + private static ArrayList <ArrayList <HashMap <String, String>>> mTabsList; Please write a follow-up patch to drop this prefix for consistency.

Comment on attachment 8405118 [details] [diff] [review] 02 - Add swipe to refresh to sync tab list Review of attachment 8405118 [details] [diff] [review]: ----------------------------------------------------------------- This is looking great, but I want to do one more pass after reworking the android-sync code a little to make this cleaner. I'm going to do that now, and will post a pre- patch for you to review. So r+, but please don't land until we go one more round. ::: mobile/android/base/RemoteTabsContainer.java @@ +18,3 @@ > > /** > * Serves as container to wrap the list of synced tabs and provide swipe-to-refresh support. The nit: provides, or implements. @@ +24,2 @@ > implements TabsPanel.PanelView { > + private static final String[] STAGES = new String[] { "tabs" }; nit: descriptive names, always. STAGES_TO_SYNC_ON_REFRESH, or similar. @@ +30,5 @@ > > public RemoteTabsContainer(Context context, AttributeSet attrs) { > super(context, attrs); > this.context = context; > + syncObserver = new SyncObserver(); nit: (and a matter of opinion): I generally prefer descriptive names, so that searching globally is easier. That is, RemoteTabsSyncObserver instead of SyncObserver; RemoteTabsRefreshListener instead of RefreshListener. This is a very personal preference; do what feels right to you, and keep consistent with Fennec style as much as possible. @@ +49,5 @@ > + > + > + @Override > + public boolean canChildScrollUp() { > + // We are not supporting swipe-to-refresh for old sync. This disabled the swipe gesture if nit: disables.

(In reply to :Margaret Leibovic from comment #22) > This could be final. Done. > Please write a follow-up patch to drop this prefix for consistency. Done.

(In reply to Nick Alexander :nalexander from comment #23) > This is looking great, but I want to do one more pass after reworking the > android-sync code a little to make this cleaner. I'm going to do that now, > and will post a pre- patch for you to review. So r+, but please don't land > until we go one more round. Fixed nits, just waiting on android-sync code to finish this. Can you add the dependent bug? This also now depends on the custom GeckoSwipeRefreshLayout in bug 970707 to address UX concerns.

rnewman: this does a few small things, all in service of keeping all FxAccount + Sync access via the FirefoxAccounts interface. 1) expose sync status observer stuff via FirefoxAccounts; 2) don't expose AndroidFxAccount in sync status observer; 3) requestSync on background thread. Sigh.

jdover: Updated to build on refactored FirefoxAccounts. I see I left a little logging in; it needs to go. But it reveals an issue: the fragment lasts a lot longer than we want, so it's continually refreshing the tabs, etc, etc, on background syncs. Can we start and stop the observer based on visibility events?

Comment on attachment 8406638 [details] [diff] [review] Part 00 - Refactor FirefoxAccounts Review of attachment 8406638 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/fxa/FirefoxAccounts.java @@ +231,5 @@ > Logger.info(LOG_TAG, "Requesting sync."); > logSyncHints(syncHints); > > + // We get strict mode warnings on some devices, so make the request on a > + // background thread. Can we just disable the strict mode warnings instead? ::: mobile/android/base/fxa/activities/FxAccountConfirmAccountActivity.java @@ +117,5 @@ > + } > + > + @Override > + public Account getAccount() { > + return fxAccount.getAndroidAccount(); Insanity edge case: what happens if you delete the account from Android Settings mid-way through using this? ::: mobile/android/base/fxa/sync/FxAccountSyncStatusHelper.java @@ +35,5 @@ > // Used to unregister this as a listener. > protected Object handle = null; > > // Maps delegates to whether their underlying Android account was syncing the > // last time we observed a status change. // Access to this map should be synchronized.

Comment on attachment 8406638 [details] [diff] [review] Part 00 - Refactor FirefoxAccounts Review of attachment 8406638 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/fxa/FirefoxAccounts.java @@ +249,5 @@ > + * Only a weak reference to <code>syncObserver</code> is held. > + * > + * @param syncObserver to start notifying. > + */ > + public static void startObservingSyncStatus(SyncStatusDelegate syncObserver) { What's the reasoning for using the 'SyncStatusDelegate' and 'startObservingSyncStatus' naming convention rather than some more Android, like 'SyncStatusListener' and 'addSyncStatusListener'?

Comment on attachment 8406638 [details] [diff] [review] Part 00 - Refactor FirefoxAccounts Somehow rnewman's r+ got removed...

Comment on attachment 8406640 [details] [diff] [review] Add swipe to refresh to sync tab list Review of attachment 8406640 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/RemoteTabsContainer.java @@ +60,5 @@ > + > + @Override > + public void onAttachedToWindow() { > + super.onAttachedToWindow(); > + FirefoxAccounts.startObservingSyncStatus(syncObserver); We could move the add/remove to #show and #hide to avoid leaking. #hide is always called on this panel when the tab tray is closed.

(In reply to Josh Dover [:jdover] from comment #31) > Comment on attachment 8406640 [details] [diff] [review] > Add swipe to refresh to sync tab list > > Review of attachment 8406640 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/RemoteTabsContainer.java > @@ +60,5 @@ > > + > > + @Override > > + public void onAttachedToWindow() { > > + super.onAttachedToWindow(); > > + FirefoxAccounts.startObservingSyncStatus(syncObserver); > > We could move the add/remove to #show and #hide to avoid leaking. #hide is > always called on this panel when the tab tray is closed. wfm.

(In reply to Josh Dover [:jdover] from comment #29) > Comment on attachment 8406638 [details] [diff] [review] > Part 00 - Refactor FirefoxAccounts > > Review of attachment 8406638 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/fxa/FirefoxAccounts.java > @@ +249,5 @@ > > + * Only a weak reference to <code>syncObserver</code> is held. > > + * > > + * @param syncObserver to start notifying. > > + */ > > + public static void startObservingSyncStatus(SyncStatusDelegate syncObserver) { > > What's the reasoning for using the 'SyncStatusDelegate' and > 'startObservingSyncStatus' naming convention rather than some more Android, > like 'SyncStatusListener' and 'addSyncStatusListener'? This is just an android-sync vs. Fennec distinction. Back in the mists of time, rnewman preferred delegate for callbacks of this sort, and we've been pretty consistent. Of course, this API is for Fennec's consumption, so I'll take your point and alter the names.

Landed part 0: https://hg.mozilla.org/integration/fx-team/rev/f2e5bc417f96 Merge viking, please keep this ticket open so the other parts can land.

Comment on attachment 8406428 [details] [diff] [review] 02 - Add swipe to refresh to sync tab list Review of attachment 8406428 [details] [diff] [review]: ----------------------------------------------------------------- This does need to be re-worked, since I renamed following your suggestions before landing Part 0, but: r=nalexander with those mechanical changes and with (un-)registering the listener in (hide/)show.

Updated for naming changes.

hide() isn't being called when the tabs tray is closed, leading to the sync listener still being subscribed to sync events. This patch fixes that.

Comment on attachment 8409130 [details] [diff] [review] 03 - Call hide() on TabPanel after tab tray is hidden lgtm

Note to sheriff: Part 00 has already been landed.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #40) > https://hg.mozilla.org/mozilla-central/rev/f2e5bc417f96 This is just part 0. Unfortunately, our flags got lost. I will land the remaining pieces now.

https://hg.mozilla.org/integration/fx-team/rev/1e27e759fa2c https://hg.mozilla.org/integration/fx-team/rev/3224b22c0f29 https://hg.mozilla.org/integration/fx-team/rev/ce5ba3c8eb47

Backed out last two pieces (which had bad commit messages, sorry!) for rc* failures. https://hg.mozilla.org/integration/fx-team/rev/62bedf537dfd jdover, this looks a lot like XML inflation didn't get updated. See log at https://tbpl.mozilla.org/php/getParsedLog.php?id=38149837&tree=Fx-Team&full=1#error0 Probably wants to hit try before re-landing.

Forgot to make changes to tablet layout. Only patches 1 and 2 need to be landed now.

Passed on try: https://tbpl.mozilla.org/?tree=Try&rev=6db7706a904c

Comment on attachment 8409945 [details] [diff] [review] 01 - Split RemoteTabs into container and list Review of attachment 8409945 [details] [diff] [review]: ----------------------------------------------------------------- Rubber stamp -- I'm sure the changes are reasonable, and try is green. Ship it!

https://hg.mozilla.org/integration/fx-team/rev/57c3be479931 https://hg.mozilla.org/integration/fx-team/rev/6fa987a53e37

https://hg.mozilla.org/mozilla-central/rev/57c3be479931 https://hg.mozilla.org/mozilla-central/rev/6fa987a53e37

I found a small bug in the current Nightly (2014-05-01). If you pull down only a bit and release your finger before the orange line reaches the left and right ends of the screen, the refresh doesn't start and the orange line 'shrinks back' to the middle until it disappears. This works fully as intended. But, if you instead pull down (also only a bit), then move your finger up a bit (so that the line shrinks a bit but not fully disappears) and then release your finger, the line will never go away. Summarized: If you release your finger while moving down it works and the line goes away. But if you instead release your finger while moving up again the line will never go away by itself. (PS: Thanks for implementing this great feature!)

I filed a new bug based on this (bug 1005072). The interaction still seems to be a work-in-progress as far as I can tell.

Should I file a separate bug for the fact that P2R doesn't fetch sent tabs?

(In reply to Paul [pwd] from comment #53) > Should I file a separate bug for the fact that P2R doesn't fetch sent tabs? Yes. In it, please provide a descriptive observation with steps to reproduce if possible. First I've heard of this.

Removing the relnote-firefox 31+ flag since this will be disabled in 31 via bug 1021123

(In reply to Aaron Train [:aaronmt] from comment #55) > Removing the relnote-firefox 31+ flag since this will be disabled in 31 via > bug 1021123 Change of plans! Bug 1021123 is now going to make the colors a bit better, better enough that UX is okay with us shipping this.

Aaron, it seems that is going to ship in 31 too. Can someone check this in 31? Added the release notes with Margaret proposed wording: "Added ability to refresh synced tabs on demand"