1152861 - Implement pull-to-refresh for reading list panel

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Awesomescreen, defect, P5)

Description

[:Margaret Leibovic]

Nick/Richard, is this even possible? It would be really nice to trigger a reading list sync to check for new items.

Comment 1

[Richard Newman [:rnewman]]

Yes, it's possible. We do the same thing for synced tabs.

Comment 2

[Nick Alexander :nalexander [he/him]]

Possible, and fun!  But multi-layered.  Not a good first bug, but a great mentor bug.  vivek is the expert here.  This works into vivek's unlanded patches for advertising a Firefox Account in the Reading List panel at Bug 1122710, but the two tickets could land in either order.

Here's what needs to happen:

1) wrap the existing Reading List panel in a GeckoSwipeRefreshLayout.
2) use an AccountLoader to determine if a Firefox Account is present on the device.  If there is no account, disable the swipe refresh layout entirely.
3) use FirefoxAccounts.requestSync to request a sync.  This will request both a Firefox Sync and a Reading List sync, but that's okay for now.
4) use a SyncStatusListener to wait for the end of the sync.  This multiplexes both Firefox Sync and the Reading List sync, waiting for both to complete; but that's okay for now too.
5) bask in glory.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/FirefoxAccounts.java#258

Comment 3

[Vivek Balakrishnan[:vivek]]

Attachment: 1152861.patch

Added pull to refresh layout for reading list.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8592455 [details] [diff] [review]
1152861.patch

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

Unfortunately this approach won't do the right thing when there's no Firefox Account.  (I think?  Tell me if I'm wrong.)  I outlined an approach using an AccountLoader that should work in https://bugzilla.mozilla.org/show_bug.cgi?id=1152861#c2.

::: mobile/android/base/home/ReadingListPanel.java
@@ +131,5 @@
>          super.onDestroyView();
>          mList = null;
>          mTopView = null;
>          mEmptyView = null;
> +        mRefreshLayout = null;

There's a little race here because mRefreshLayout could be null when mSyncStatusListener gets a "sync finished" message.  Maybe it doesn't happen because everything is on the main thread?  In any case, I'm not going to bother setting mRefreshLayout = null.

@@ +252,5 @@
> +
> +    protected class ReadingListRefreshListener implements GeckoSwipeRefreshLayout.OnRefreshListener {
> +        @Override
> +        public void onRefresh() {
> +            if (FirefoxAccounts.firefoxAccountsExist(getActivity())) {

This doesn't work because the GeckoSwipeRefreshLayout exists even when no Firefox Account exists.  You (correctly) took this from the Synced Tabs panel, except that panel only gets shown when there's an ambient account.  In this case, we don't want to allow a refresh action if there's no Account (otherwise you can drag the Reading List down when there's no syncing happening and it looks bad).

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: 1152861.patch

added account loader to enable swipe action iff there is valid account

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8593307 [details] [diff] [review]
1152861.patch

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

vivek: this looks really good, so I'm marking it r+.

However there's been a change in what's happening when for Reading List.  We're not going to ship Reading List syncing on Android in the 38-40 timeframe.  Bug 1155684 has disabled it for those releases.  Given this situation, we're not going to land this patch.  I'm sorry to have you work -- and do good work -- on a ticket that ends up not being used.  Especially 'cuz this is the second time this has happened with you and Reading List home panel code :/

Not everything about this situation is finalized; I'm happy to talk more on IRC.  Thanks again!

::: mobile/android/base/home/ReadingListPanel.java
@@ +211,5 @@
> +        if (account == null) {
> +            return false;
> +        }
> +
> +        if (SyncConstants.ACCOUNTTYPE_SYNC.equals(account.type)) {

In fact the Reading List will only sync against Firefox Accounts, so we can get rid of this.

@@ +241,5 @@
> +     * @param account
> +     *            Android Account (Sync or Firefox); may be null.
> +     */
> +    private void enableSwipeFromAccountState(final Account account) {
> +        // Early return when loader reset is called after the fragment is detached.

nice.

Comment 7

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

Without looking at the patch, I will say that some form of sync may happen soon (41 timeframe?) and supporting a refresh will still be handy.

This patch will see the light of day!

Comment 8

[Firefox Bug Husbandry Bot]

Bulk move: https://bugzilla.mozilla.org/show_bug.cgi?id=1399539

Comment 9

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 10

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.