Closed Bug 1183588 Opened 9 years ago Closed 9 years ago

Update pull-to-refresh pattern to Material Design

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

defect
Not set
normal

Tracking

(firefox43 verified)

VERIFIED FIXED
Firefox 43
Tracking Status
firefox43 --- verified

People

(Reporter: tech4pwd, Assigned: vivek)

References

()

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150513191204

Steps to reproduce:

We currently use the ICS/JB/KK pattern, the platform has moved on. Let's switch over to Material Design pattern.
Summary: Update pull-to-refresh pattern → Update pull-to-refresh pattern to Material Design
Status: UNCONFIRMED → NEW
Ever confirmed: true
Sounds good. Would this work for our Panels Mike?
(In reply to Anthony Lam (:antlam) from comment #2)
> Sounds good. Would this work for our Panels Mike?

+1 from me.  I'm a little surprised this isn't just on by default.  Do we need to rev the support library version to get this styling?  Do we need to opt-in somewhere in our code?
(In reply to Nick Alexander :nalexander (PTO July 17-27) from comment #3)
> +1 from me.  I'm a little surprised this isn't just on by default.  Do we
> need to rev the support library version to get this styling?  Do we need to
> opt-in somewhere in our code?

Apparently we copied the source to GeckoSwipeToRefreshLayout and use that instead [1].

The reasons in the Javadoc are:

45  * GeckoSwipeRefreshLayout is mostly lifted from Android's support library (v4) with these
46  * modifications:
47  *  - Removes elastic "rubber banding" effect when overscrolling the child view.
48  *  - Changes the height of the progress bar to match the height of HomePager's page indicator.
49  *  - Uses a rectangle rather than a circle for the SwipeProgressBar indicator.

Rubber banding is no longer present, the height is no longer relevant, and I'm not sure what the rectangle vs. circle means, but it's probably not relevant.

[1]: https://mxr.mozilla.org/mozilla-central/search?string=swiperefreshlayout&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Assignee: nobody → michael.l.comella
Vivek said he'd handle this one on IRC.
Assignee: michael.l.comella → vivekb.balakrishnan
Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.

* Replaced GeckoSwipeRefreshLayout with SwipeRefreshLayout from support library.
* Handled refresh cancel in a UI Thread to avoid swipe refresh circle sticking around.
Attachment #8638853 - Flags: review?(michael.l.comella)
Sorry, Vivek - I'll get to this tomorrow.
Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

https://reviewboard.mozilla.org/r/14145/#review13101

::: mobile/android/base/home/PanelRefreshLayout.java:74
(Diff revision 1)
> -        setRefreshing(false);
> +                setRefreshing(false);

Why does this solve the problem of the refresh icon sticking around if the refresh is cancelled?

You should probably either add comments to each invocation, or extend SwipeRefreshLayout and override setRefreshing so that it always calls itself in a UIThread.
Attachment #8638853 - Flags: review?(michael.l.comella)
Attachment #8638853 - Flags: review?(michael.l.comella)
Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.

* Replaced GeckoSwipeRefreshLayout with SwipeRefreshLayout from support library.
* Annotated the method with appropriate thread annotations.

Support API SwipeRefreshLayout cancels the refresh view by scheduling a scale down animation.
At the end of the animation, the view is set to Gone state.
So, we must ensure that this view update happens in UiThread.

The deprecated GeckoSwipeRefreshLayout got away this because the it cleared
the progress bar with postInvalidate() which invalidates the view through
event loop.
Hi Nick,

As part of this bug I've added @WorkerThread annotation to onSyncStarted() and onSyncFinished(). I arrived at this conclusion through the logic of elimination that these callback are not run in MainThread. After digging through the code I find that requestSync is actually executed in a service [1]. Can you please correct me if I'm wrong with those annotations.

1 http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/5.1.1_r1/android/content/ContentResolver.java#1797
Flags: needinfo?(nalexander)
(In reply to Vivek Balakrishnan[:vivek] from comment #11)
> Hi Nick,
> 
> As part of this bug I've added @WorkerThread annotation to onSyncStarted()
> and onSyncFinished(). I arrived at this conclusion through the logic of
> elimination that these callback are not run in MainThread. After digging
> through the code I find that requestSync is actually executed in a service
> [1]. Can you please correct me if I'm wrong with those annotations.
> 
> 1
> http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/
> android/5.1.1_r1/android/content/ContentResolver.java#1797

The callback is invoked here:

http://androidxref.com/4.4.4_r1/xref/frameworks/base/services/java/com/android/server/content/SyncStorageEngine.java#547

Which is called all over the place, including in things like setBackoff().  It's not clear that setBackoff() couldn't be called from the main UI thread.

Sadly, there's no documentation guarantee of anything.  But we have a point we control: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncStatusHelper.java.

As a pre: patch, extract the interface from https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/FirefoxAccounts.java#72 to a new file; document that the methods are always called on a particular thread (which could be the UI thread, if you want that); and then make FxAccountSyncStatusHelper always invoke delegate methods in the right place.

I should have documented the thread assumptions and enforced the threading model from the very beginning.  That's my oversight.
Flags: needinfo?(nalexander)
Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

Clearing review – I think I'm waiting on the changes from comment 11 and comment 12.
Attachment #8638853 - Flags: review?(michael.l.comella)
Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.
Attachment #8638853 - Attachment description: MozReview Request: Bug 1183588 - Material design swipe refresh pattern changes r?mcomella. → MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback r?nalexander.
Attachment #8638853 - Flags: review?(nalexander)
Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.
Attachment #8650030 - Flags: review?(michael.l.comella)
Comment on attachment 8650030 [details]
MozReview Request: Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.

https://reviewboard.mozilla.org/r/16529/#review14963

Ship It!
Attachment #8650030 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

https://reviewboard.mozilla.org/r/14145/#review14965

If it works for you, it works for me: just nits.

::: mobile/android/base/fxa/SyncStatusListener.java:20
(Diff revision 3)
> +     * This is always called in UiThread.

...called on the UiThread.  (Both places.)

::: mobile/android/base/widget/ActivityChooserModel.java:1336
(Diff revision 3)
> -    private class SyncStatusListener implements FirefoxAccounts.SyncStatusListener {
> +    private class SyncStatusDelegate implements SyncStatusListener {

Consider InnerSyncStatusListener or something -- no sense using delegate and listener to refer to the same thing.
Attachment #8638853 - Flags: review?(nalexander) → review+
url:        https://hg.mozilla.org/integration/fx-team/rev/53da6578fad4f05df702ff737bc11a141eae9e0a
changeset:  53da6578fad4f05df702ff737bc11a141eae9e0a
user:       vivek <vivekb.balakrishnan@gmail.com>
date:       Wed Aug 19 21:16:10 2015 +0300
description:
Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r=nalexander.
url:        https://hg.mozilla.org/integration/fx-team/rev/720c7eff9ea0cafb08fe442311bb3d1c353ee977
changeset:  720c7eff9ea0cafb08fe442311bb3d1c353ee977
user:       vivek <vivekb.balakrishnan@gmail.com>
date:       Wed Aug 19 21:31:50 2015 +0300
description:
Bug 1183588 - Material design swipe refresh pattern changes r=mcomella.
https://hg.mozilla.org/mozilla-central/rev/53da6578fad4
https://hg.mozilla.org/mozilla-central/rev/720c7eff9ea0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Verified as fixed in build 43.0a1 2015-08-26;
Devices: 
 Asus Transformer Pad (Android 4.2.1).
 Google Nexus 9 (Android 5.1.1);
 Motorola Razr (Android 4.4.4).
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.