See also Bug 850600, where code reuse might be possible. I just tried out Margaret's excellent Pocket add-on, and my first instinct was to pull to refresh. Can we add that?

Haha, I was about to file this bug! I think we could add something to the Home.panel.add API to let the add-on register a "force refresh" handler or something like that. Another issue with this, however, is that I'm not sure that the cursor will update to hold those new items, since the SQLiteBridge retuns a MatrixCursor.

This could be useful: http://developer.android.com/reference/android/support/v4/widget/SwipeRefreshLayout.html

Lets sick an intern on this...

This allows for an options API such as: title: "MyPanel", views: [{ type: Home.panels.View.LIST, dataset: DATASET_ID }], refreshHandler: function(done) { updateData(done); }

This assumes that you are using a FRAME layout with only 1 child view. This is because SwipeRefreshLayout only supports one child view.

FYI, this is going to conflict with my patch in bug 978991. We can race to see who lands first, but I think my patch will probably be ready to land tomorrow morning. Looking through your patches here now...

(In reply to Josh Dover [:jdover] from comment #4) > Created attachment 8399705 [details] [diff] [review] > 01 - Add refreshEnabled to PanelConfig > > This allows for an options API such as: > > title: "MyPanel", > views: [{ > type: Home.panels.View.LIST, > dataset: DATASET_ID > }], > refreshHandler: function(done) { > updateData(done); > } I don't think we want to rely on the consumer passing a `done` function along. Luckily, we shouldn't need to do this, because if the consumer makes any HomeStorage.save/deleteAll calls, we will call "HomePanels:RefreshDataset" ourselves.

Comment on attachment 8399708 [details] [diff] [review] 04 - Pass refresh to JS and stop after callback Review of attachment 8399708 [details] [diff] [review]: ----------------------------------------------------------------- Forgive me, I'm going in reverse here :) I want to take a step back to think about where we're implementing this functionality. I think it would make more sense for this to be part of DynamicPanel, rather than FramePanelLayout, and we should see if we can make that work. ::: mobile/android/base/home/FramePanelLayout.java @@ +41,5 @@ > > if (panelConfig.isRefreshEnabled()) { > final SwipeRefreshLayout refreshLayout = new SwipeRefreshLayout(context); > + final RefreshListener refreshListener = > + new RefreshListener(refreshLayout, panelConfig.getId()); I don't know that these changes belong in FramePanelLayout, since that's one of (not now but in the future) multiple types of layouts that can be in DynamicPanel. I think we should look into implementing this functionality as part of DynamicPanel. @@ +75,5 @@ > + > + public RefreshListener(SwipeRefreshLayout refreshLayout, String panelId) { > + this.refreshLayout = refreshLayout; > + this.panelId = panelId; > + GeckoAppShell.registerEventListener("HomePanels:RefreshDataset", this); Oh, I started looking at the JS, and I didn't realize that you were adding a new "HomePanels:RefreshDataset" listener here. We are already listening for this event in DynamicPanel, which is where we take care of refreshing the dataset, and that seems like it would be a better place to put this logic to set the status of the refreshLayout. ::: mobile/android/modules/Home.jsm @@ +306,5 @@ > }; > > + handlePanelsRefresh = function(id) { > + let options = _registeredPanels[id](); > + if (!options.refreshHandler) { You should also check to make sure refreshHandler is a function. @@ +310,5 @@ > + if (!options.refreshHandler) { > + throw "Home.panels: Invalid refreshHandler for panel.id = " + id; > + } > + > + let datasetId = options.views[0].datasetId; This looks suspicious... it's true that right now we only support one view in a panel (because we only support a frame layout), but we should prepare for a future where we support multiple views. @@ +318,5 @@ > + sendMessageToJava({ > + type: "HomePanels:RefreshDataset", > + datasetId: datasetId > + }); > + }); As I mentioned in my last comment, we shouldn't need to send this message, so we can just call options.refreshHandler() without any parameters here. I also wonder if we should name this method onRefresh instead, to be consistent with the onInstall/onUninstall handlers I'm adding in bug 978991 (or alternately I should rename those installHandler/uninstallHandler).

Comment on attachment 8399707 [details] [diff] [review] 03 - Wrap child view in SwipeRefreshLayout Review of attachment 8399707 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by. ::: mobile/android/base/home/FramePanelLayout.java @@ +46,5 @@ > + refreshLayout.setColorScheme(R.color.swipe_refresh_1, R.color.swipe_refresh_2, > + R.color.swipe_refresh_3, R.color.swipe_refresh_4); > + } else { > + addView(childView); > + } I feel like this should be done in createPanelView so that PanelLayout subclasses don't need to re-implement the same logic.

Comment on attachment 8399705 [details] [diff] [review] 01 - Add refreshEnabled to PanelConfig Review of attachment 8399705 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I think it would be simpler to just add a new flag, REFRESH_ENABLED or something like that, instead of making a whole new field on PanelConfig (since there's already a lot going on in this data type).

Comment on attachment 8399706 [details] [diff] [review] 02 - Use new variable naming convention in FramePanelLayout Review of attachment 8399706 [details] [diff] [review]: ----------------------------------------------------------------- r+, but given the feedback on other patches, you may not end up needing to modify FramePanelLayout.

Comment on attachment 8399707 [details] [diff] [review] 03 - Wrap child view in SwipeRefreshLayout Review of attachment 8399707 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/FramePanelLayout.java @@ +46,5 @@ > + refreshLayout.setColorScheme(R.color.swipe_refresh_1, R.color.swipe_refresh_2, > + R.color.swipe_refresh_3, R.color.swipe_refresh_4); > + } else { > + addView(childView); > + } I agree with what lucasr has to say, I think this should go somewhere where it can be shared by all different layout types. ::: mobile/android/base/resources/values/colors.xml @@ +95,5 @@ > + <!-- Swipe to refresh colors --> > + <color name="swipe_refresh_1">#CCFF9500</color> > + <color name="swipe_refresh_2">#FFFFFF</color> > + <color name="swipe_refresh_3">#CCFF9500</color> > + <color name="swipe_refresh_4">#FFFFFF</color> Unless we're planning on using 4 separate colors in the future, I think we should just declare 2 colors here, then use those 2 colors as desired in the setColorScheme call.

Changed to use a flag in the EnumSet for refreshEnabled.

Moved SwipeRefreshLayout wrapping to be inside of PanelLayout#createPanelView by using a subclass of SwipeRefreshLayout that uses the decorator pattern to pass the DatasetBacked methods to the child view. This also allows us to stop refreshing when the dataset is changed without having to listen for an event. One thing I also added that should be up for discussion is to start the "refreshing" animation when the view is created to give feedback while the panel is loading for the first time. In practice with the pocket panel, it is only there for maybe 1 second (on a good wifi connection) but this could be useful for: 1) the user has a bad connection, 2) the 3rd party service is down/slow, 3) feedback while developing an addon.

> I don't think we want to rely on the consumer passing a `done` function > along. > > Luckily, we shouldn't need to do this, because if the consumer makes any > HomeStorage.save/deleteAll calls, we will call "HomePanels:RefreshDataset" > ourselves. I didn't know that the RefreshDataset was called automatically, however there is the case for when the addon may check for updates and find there are none, so the dataset is not changed. In this case, we still need to stop the refresh animation, so this 'done' callback should be called. I designed it so that if the dataset is not changed, the callback must be called, but it does not technically have to be called if the dataset is changed (but still can be called with no harm). For documentation purposes it may be simpler to simply say this must be called after refreshing, not sure.

Comment on attachment 8400262 [details] [diff] [review] 03 - Pass refresh to JS and stop after callback Review of attachment 8400262 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by. ::: mobile/android/base/home/PanelRefreshLayout.java @@ +41,5 @@ > + > + // Show the loading animation until the dataset is loaded. > + setRefreshing(true); > + > + GeckoAppShell.registerEventListener("HomePanels:RefreshDone", this); You're leaking the listener here.

Comment on attachment 8400262 [details] [diff] [review] 03 - Pass refresh to JS and stop after callback Review of attachment 8400262 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by. ::: mobile/android/modules/Home.jsm @@ +317,5 @@ > + sendMessageToJava({ > + type: "HomePanels:RefreshDone", > + panelId: id > + }); > + }); We've been working with the assumption that a panel can display multiple views at the same time (even though this is not the case right now). The "HomePanels:RefreshDone" event applies to the panel as a whole, not to specific views/datasets. This would probably break with multiple views (bound to different datasets) being refreshed at the same time.

Comment on attachment 8400257 [details] [diff] [review] 01 - Add refreshEnabled to PanelConfig Review of attachment 8400257 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/HomeConfig.java @@ +316,5 @@ > } > > + public boolean isRefreshEnabled() { > + return mFlags.contains(Flags.REFRESH_ENABLED); > + } Nit: Put this up higher with the other flag-related methods. ::: mobile/android/modules/Home.jsm @@ +258,5 @@ > this.authConfig.imageUrl = options.authHandler.imageUrl; > } > } > + > + if (options.onRefresh) { Nit: Re-name this onrefresh to be consistent with the event handler APIs we have in the Home.banner API (I'm updating my patch in bug 978991 to be oninstall/onuninstall).

Comment on attachment 8400260 [details] [diff] [review] 02 - Wrap child view in SwipeRefreshLayout Review of attachment 8400260 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/PanelLayout.java @@ +284,5 @@ > datasetBacked.setFilterManager(new PanelFilterManager(state)); > + > + if (mPanelConfig.isRefreshEnabled()) { > + view = new PanelRefreshLayout(getContext(), mPanelConfig, datasetBacked); > + } I don't think this is what we want, since createPanelView is used to create children for the panel, and in a future world where there are mutliple view in a panel, this would be we would be creating multiple PanelRefreshLayouts. I know that in comment 11, lucasr suggested putting this logic in createPanelView, but I think it might be more appropriate to put it in DynamicPanel#createPanelLayout, since that is where we make the one container layout for the panel itself. So in the current case where we only support FramePanelLayout, you could be wrapping that in a SwipeRefreshLayout. Does this make sense? To summarize, a dynamic panel contains one PanelLayout (right now the only supported one is FramePanelLayout). And then a PanelLayout can have multiple views inside it, but right now we only ever put one inside, because that's all FramePanelLayout is designed to do. However, in the future, we may have something like a TabsPanelLayout, which contains multiple child views. As long as we want pull-to-refresh to apply to the whole panel, I think we should keep its implementation at the panel level. However, thinking about this more, pull-to-refresh really is a UX paradigm that applies to scrollable lists, so maybe the refresh action should be a per-view thing. I think we should take a step back to think about what kind of UX we actually want if in the future we support multiple views in a panel. Because maybe it wouldn't make sense to pull-to-refresh the whole panel in that case, since I imagine the different views would scroll separately, and it seems odd that overscrolling one would refresh all of them. But once again, it's hard to think about this without a concrete example of what this multi-view UI would look like. One benefit of making the refresh action per-view is that it would create a more explict relationship between datasets and refresh actions, which would be nice. In this case the API would look like: title: "MyPanel", views: [{ type: Home.panels.View.LIST, dataset: DATASET_ID, onrefresh: function() {...} }], I was wondering if we could make this part of the HomeProvider API instead, since it more directly relates to datasets, but I realized that would not work because we need to know whether or not to show refresh UI. So basically, I want us to discuss this more before moving forward in one direction or another... sorry about that!

Comment on attachment 8400262 [details] [diff] [review] 03 - Pass refresh to JS and stop after callback Review of attachment 8400262 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/modules/Home.jsm @@ +317,5 @@ > + sendMessageToJava({ > + type: "HomePanels:RefreshDone", > + panelId: id > + }); > + }); I still really don't like this idea of passing a function to the consumer that we require they call. Looking at our HomeProvider.jsm implementation, we send a "HomePanels:RefreshDataset" message regardless of whether or not new items were saved, so long as save() is called: http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/HomeProvider.jsm#331 So I think a better approach would be to require add-ons to call save() (with or without new items), in their onrefresh handlers. Either that, or have some sort of timer on the animation that kills it if there isn't an update.

(In reply to :Margaret Leibovic from comment #21) > Comment on attachment 8400260 [details] [diff] [review] > 02 - Wrap child view in SwipeRefreshLayout > > Review of attachment 8400260 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/PanelLayout.java > @@ +284,5 @@ > > datasetBacked.setFilterManager(new PanelFilterManager(state)); > > + > > + if (mPanelConfig.isRefreshEnabled()) { > > + view = new PanelRefreshLayout(getContext(), mPanelConfig, datasetBacked); > > + } > > I don't think this is what we want, since createPanelView is used to create > children for the panel, and in a future world where there are mutliple view > in a panel, this would be we would be creating multiple PanelRefreshLayouts. > > I know that in comment 11, lucasr suggested putting this logic in > createPanelView, but I think it might be more appropriate to put it in > DynamicPanel#createPanelLayout, since that is where we make the one > container layout for the panel itself. So in the current case where we only > support FramePanelLayout, you could be wrapping that in a SwipeRefreshLayout. I initially tried this and forgot to mention the problems I found with this approach. Unfortunately, the way SwipeRefreshLayout is implemented, it really only expects a ListView to be it's child and anything else actually doesn't work correctly in regards to scrolling. Basically what happens is that SwipeRefreshLayout listens to any scrolls in the up direction, and if the child view is scrolled to the top then it intercepts the touch. When you have a FrameLayout as the child (to be clear: SwipeRefreshLayout -> FrameLayout -> ListView), the FrameLayout is always scrolled to the top so any time you try to scroll the view up, the SwipeRefreshLayout intercepts the touch, and the ListView never receives the scroll. Scrolling down is not affected. However there is a way around this, by overriding onInterceptTouchEvent() and/or canChildScrollUp() which will take some more work. With this widget being so new, I haven't been able to find any posts on how this is achieved. > However, in the future, we may have something like a TabsPanelLayout, which > contains multiple child views. I know we've been designing this to make it very extendible, but do we have any data to support that 3rd parties even want to build such a feature-rich app within Firefox? Just curious on what we have heard from partners on this. To be clear, this isn't an argument to make our framework less extendable, just a reality check. We should definitely strive to keep things flexible. > However, thinking about this more, pull-to-refresh really is a UX paradigm > that applies to scrollable lists, so maybe the refresh action should be a > per-view thing. I think we should take a step back to think about what kind > of UX we actually want if in the future we support multiple views in a > panel. Because maybe it wouldn't make sense to pull-to-refresh the whole > panel in that case, since I imagine the different views would scroll > separately, and it seems odd that overscrolling one would refresh all of > them. But once again, it's hard to think about this without a concrete > example of what this multi-view UI would look like. I see how this makes sense from a functional point of view, but I feel it would be confusing having multiple swipe-to-refresh lists on the screen at once. Also what if you wanted to refresh all the views at once? Why would a user only want to refresh one list? I don't know the answers but, like you said, I think that considering the use case for this is important before introducing something that could be wildly confusing. I've never seen any other app do this, though I could see it not being 100% awful on tablets. > One benefit of making the refresh action per-view is that it would create a > more explict relationship between datasets and refresh actions, which would > be nice. In this case the API would look like: > > title: "MyPanel", > views: [{ > type: Home.panels.View.LIST, > dataset: DATASET_ID, > onrefresh: function() {...} > }], If we decide to make this granular at the view level, then this totally makes sense.

Another thought: if an app owns multiple views, it can do whatever it likes with the refresh notification, even if we only do a whole-panel swipe -- refresh them all, only one, whatever.

(In reply to Josh Dover [:jdover] from comment #23) > (In reply to :Margaret Leibovic from comment #21) > > Comment on attachment 8400260 [details] [diff] [review] > > 02 - Wrap child view in SwipeRefreshLayout > > > > Review of attachment 8400260 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: mobile/android/base/home/PanelLayout.java > > @@ +284,5 @@ > > > datasetBacked.setFilterManager(new PanelFilterManager(state)); > > > + > > > + if (mPanelConfig.isRefreshEnabled()) { > > > + view = new PanelRefreshLayout(getContext(), mPanelConfig, datasetBacked); > > > + } > > > > I don't think this is what we want, since createPanelView is used to create > > children for the panel, and in a future world where there are mutliple view > > in a panel, this would be we would be creating multiple PanelRefreshLayouts. > > > > I know that in comment 11, lucasr suggested putting this logic in > > createPanelView, but I think it might be more appropriate to put it in > > DynamicPanel#createPanelLayout, since that is where we make the one > > container layout for the panel itself. So in the current case where we only > > support FramePanelLayout, you could be wrapping that in a SwipeRefreshLayout. > > I initially tried this and forgot to mention the problems I found with this > approach. Unfortunately, the way SwipeRefreshLayout is implemented, it > really only > expects a ListView to be it's child and anything else actually doesn't work > correctly in regards to scrolling. Basically what happens is that > SwipeRefreshLayout > listens to any scrolls in the up direction, and if the child view is scrolled > to the top then it intercepts the touch. When you have a FrameLayout as the > child > (to be clear: SwipeRefreshLayout -> FrameLayout -> ListView), the FrameLayout > is always scrolled to the top so any time you try to scroll the view up, the > SwipeRefreshLayout intercepts the touch, and the ListView never receives the > scroll. Scrolling down is not affected. What about a GridView? How does this work with panels that use a GRID view? The things you're saying here make me further think that SwipeRefreshLayout should wrap individual views, not the entire panel. > However there is a way around this, by overriding onInterceptTouchEvent() > and/or > canChildScrollUp() which will take some more work. With this widget being so > new, I haven't been able to find any posts on how this is achieved. This makes me nervous. I don't want us to end up with some highly customized touch code for a use case that we don't even know we need yet. > > However, in the future, we may have something like a TabsPanelLayout, which > > contains multiple child views. > > I know we've been designing this to make it very extendible, but do we have > any > data to support that 3rd parties even want to build such a feature-rich app > within Firefox? Just curious on what we have heard from partners on this. > > To be clear, this isn't an argument to make our framework less extendable, > just > a reality check. We should definitely strive to keep things flexible. I totally agree with a reality check! That's what I was trying to do with this discussion :) > > However, thinking about this more, pull-to-refresh really is a UX paradigm > > that applies to scrollable lists, so maybe the refresh action should be a > > per-view thing. I think we should take a step back to think about what kind > > of UX we actually want if in the future we support multiple views in a > > panel. Because maybe it wouldn't make sense to pull-to-refresh the whole > > panel in that case, since I imagine the different views would scroll > > separately, and it seems odd that overscrolling one would refresh all of > > them. But once again, it's hard to think about this without a concrete > > example of what this multi-view UI would look like. > > I see how this makes sense from a functional point of view, but I feel it > would > be confusing having multiple swipe-to-refresh lists on the screen at once. > Also > what if you wanted to refresh all the views at once? Why would a user only > want > to refresh one list? I don't know the answers but, like you said, I think > that > considering the use case for this is important before introducing something > that > could be wildly confusing. I've never seen any other app do this, though I > could see it not being 100% awful on tablets. I agree with what rnewman said in comment 24 - the add-on can do whatever it wants with the refresh notification. So if it gets a refresh notificaiton for one panel, it could refresh all of them. After hearing the points you had to say about how the SwipeRefreshLayout expects to be tied to a single view (which is what I was trying to get at with my comment about the UX interaction as well), I think we should implement this onrefresh callback as a per-view thing. And in the future, if we change our minds about refreshing a whole panel, we can bite the bullet and do whatever refactoring is necessary for that. But it seems to me like the UX for that wouldn't even make sense.

Comment on attachment 8400257 [details] [diff] [review] 01 - Add refreshEnabled to PanelConfig Review of attachment 8400257 [details] [diff] [review]: ----------------------------------------------------------------- Rescinding this r+, since I think we should go for a per-view approach, rather than per-panel.

Comment on attachment 8400260 [details] [diff] [review] 02 - Wrap child view in SwipeRefreshLayout Clearing review, since I think we should go with the per-view approach.

Cross-posting my comment from bug 976064 for clarity. ---------------- You don't need to identify specific views in order to implement a refresh request per view in bug 970707. What you need is just a way to connect the requester to the matching response somehow. You can identify the request with any kind of id, as long as: 1. The ID is unique across multiple parallel requests. 2. The response contains the request ID so that the caller can match the response with its request. In practice, this means you'd add a request ID parameter to both HomePanels:Refresh and HomePanels:RefreshDone. It would probably be analogous to how requestPanelsById is implemented. See: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelManager.java#74 http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Home.jsm#168

Comment on attachment 8400262 [details] [diff] [review] 03 - Pass refresh to JS and stop after callback Review of attachment 8400262 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by. ::: mobile/android/base/home/PanelRefreshLayout.java @@ +56,5 @@ > datasetBacked.setFilterManager(manager); > } > + > + @Override > + public void handleMessage(String event, final JSONObject message) { I'd rather register this message listener only once in PanelLayout. If the refresh requests are properly identified, you can easily know which view to update.

Changed to add this flag to ViewConfig rather than PanelConfig.

This wraps *each* view that is refreshEnabled with a SwipeRefreshLayout, rather than the whole panel. This assumes that the child view is a ListView or GridView (which are the only view types where pull to refresh makes sense).

This calls the onrefresh() callback on the viewconfig in JS. This code assumes that the ViewConfig#getIndex() method is available from lucasr's patch in bug 976064. Currently this requires the addon to call save() on the HomeProvider storage object to stop the refresh animation.

Comment on attachment 8402810 [details] [diff] [review] 02 - Wrap child view in SwipeRefreshLayout Review of attachment 8402810 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits addressed. ::: mobile/android/base/home/PanelLayout.java @@ +272,5 @@ > default: > throw new IllegalStateException("Unrecognized view type in " + getClass().getSimpleName()); > } > > final ViewState state = new ViewState(viewConfig); Nit: Move this declaration down below, right above where it's used. @@ -273,5 @@ > throw new IllegalStateException("Unrecognized view type in " + getClass().getSimpleName()); > } > > final ViewState state = new ViewState(viewConfig); > - // TODO: Push initial filter here onto ViewState Does this TODO still apply? If so, you should file a bug for it! @@ +282,5 @@ > if (view instanceof DatasetBacked) { > DatasetBacked datasetBacked = (DatasetBacked) view; > datasetBacked.setFilterManager(new PanelFilterManager(state)); > + > + if (viewConfig.isRefreshEnabled()) { In the future, if we ever have a view that doesn't implement DatasetBacked, we'll want to make sure consumers know that an onrefresh handler won't work. ::: mobile/android/base/home/PanelRefreshLayout.java @@ +7,5 @@ > + > +import org.mozilla.gecko.R; > + > +import org.mozilla.gecko.GeckoAppShell; > +import org.mozilla.gecko.GeckoEvent; Double check for unused imports. @@ +19,5 @@ > +import android.support.v4.widget.SwipeRefreshLayout; > +import android.support.v4.widget.SwipeRefreshLayout.OnRefreshListener; > +import android.view.View; > + > +class PanelRefreshLayout extends SwipeRefreshLayout implements DatasetBacked { Add a class comment explaining what this class does. @@ +22,5 @@ > + > +class PanelRefreshLayout extends SwipeRefreshLayout implements DatasetBacked { > + private final DatasetBacked datasetBacked; > + > + public PanelRefreshLayout(Context context, View childView) { Instead of casting childView to DatasetBacked, just make this constructor take a DatasetBacked type as a parameter. @@ +32,5 @@ > + > + this.datasetBacked = (DatasetBacked) childView; > + > + setOnRefreshListener(new RefreshListener()); > + addView(childView); Although I see now that you would then need to do a different cast for this addView call... so I guess the way you have it now is fine. I just try to avoid casts if possible, since compile-time errors are much better than run-time errors. At the very least, you should add some javadoc comments to this constructor and explain that childView should implement the DatasetBacked interface. @@ +34,5 @@ > + > + setOnRefreshListener(new RefreshListener()); > + addView(childView); > + > + // Must be called after the child view has been added. Good comment.

Comment on attachment 8402814 [details] [diff] [review] 03 - Pass refresh to JS callback Review of attachment 8402814 [details] [diff] [review]: ----------------------------------------------------------------- f+ for now, since you're going to have to change around the JS to account for changes in the tree. I like this more than the per-panel approach because it's more intuitive to me to be refreshing a view, not a whole dataset. ::: mobile/android/base/home/PanelRefreshLayout.java @@ +39,5 @@ > if (!(childView instanceof DatasetBacked)) { > throw new IllegalArgumentException("View must implement DatasetBacked to be refreshed"); > } > > + this.panelId = panelConfig.getId(); No need to pass in the whole panel config if you're only using the id. @@ +40,5 @@ > throw new IllegalArgumentException("View must implement DatasetBacked to be refreshed"); > } > > + this.panelId = panelConfig.getId(); > + this.viewId = viewConfig.getIndex(); Same for this index. Also, I would prefer we call this member variable viewIndex, to be consistent with where it's named elsewhere. @@ +71,5 @@ > } > > private class RefreshListener implements OnRefreshListener { > @Override > public void onRefresh() { Does SwipeRefreshLayout automatically set refreshing to true before this handler is called? @@ +77,5 @@ > + try { > + response.put("panelId", panelId); > + response.put("viewId", viewId); > + } catch (JSONException e) { > + Log.e(LOGTAG, "Could not create refresh message"); We should probably return early here, right? Otherwise "HomePanels:RefreshView" will run into a JS error. ::: mobile/android/chrome/content/browser.js @@ +130,5 @@ > }); > > // Lazily-loaded JS modules that use observer notifications > [ > + ["Home", ["HomePanels:Get", "HomePanels:Authenticate", "HomePanels:RefreshView"], "resource://gre/modules/Home.jsm"], You're going to need to update your tree and rebase.

Once this lands, you should also update the documentation here: https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Home.jsm/panels

(In reply to :Margaret Leibovic from comment #33) > Comment on attachment 8402810 [details] [diff] [review] > 02 - Wrap child view in SwipeRefreshLayout > > Review of attachment 8402810 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with nits addressed. > > ::: mobile/android/base/home/PanelLayout.java > @@ +272,5 @@ > > default: > > throw new IllegalStateException("Unrecognized view type in " + getClass().getSimpleName()); > > } > > > > final ViewState state = new ViewState(viewConfig); > > Nit: Move this declaration down below, right above where it's used. This state is used in the lines following, but the view used for the key may change if refreshEnabled, because we wrap the view in a PanelRefreshLayout. > Does this TODO still apply? If so, you should file a bug for it! Nope the bug for this was already closed. See bug 972503. > > @@ +282,5 @@ > > if (view instanceof DatasetBacked) { > > DatasetBacked datasetBacked = (DatasetBacked) view; > > datasetBacked.setFilterManager(new PanelFilterManager(state)); > > + > > + if (viewConfig.isRefreshEnabled()) { > > In the future, if we ever have a view that doesn't implement DatasetBacked, > we'll want to make sure consumers know that an onrefresh handler won't work. Will add to MDN docs when this lands. > > @@ +19,5 @@ > > +import android.support.v4.widget.SwipeRefreshLayout; > > +import android.support.v4.widget.SwipeRefreshLayout.OnRefreshListener; > > +import android.view.View; > > + > > +class PanelRefreshLayout extends SwipeRefreshLayout implements DatasetBacked { > > Add a class comment explaining what this class does. Done. > At the very least, you should add some javadoc comments to this constructor > and explain that childView should implement the DatasetBacked interface. Done.

(In reply to :Margaret Leibovic from comment #34) > > + this.panelId = panelConfig.getId(); > > No need to pass in the whole panel config if you're only using the id. > > Same for this index. Also, I would prefer we call this member variable > viewIndex, to be consistent with where it's named elsewhere. Done. > Does SwipeRefreshLayout automatically set refreshing to true before this > handler is called? Yes. > We should probably return early here, right? Otherwise > "HomePanels:RefreshView" will run into a JS error. Done.

Comment on attachment 8402996 [details] [diff] [review] 03 - Pass refresh to JS callback Review of attachment 8402996 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by. ::: mobile/android/base/home/PanelRefreshLayout.java @@ +59,5 @@ > > // Must be called after the child view has been added. > setColorScheme(R.color.swipe_refresh_orange, R.color.swipe_refresh_white, > R.color.swipe_refresh_orange, R.color.swipe_refresh_white); > + setRefreshing(true); This is conflating feedback for "loading current panel content" with "fetching new content for panel" which might be a bit misleading. We don't show progress feedback on any of the other panels when loading current content from disk. I'd prefer to keep a consistent behaviour across panels here i.e. only show progress feedback when the user explicitly requests refresh. Furthermore, this is likely to flash the progress feedback just before filling the list/grid with content (when the panel is first shown) which might feel a bit clunky. @@ +68,5 @@ > datasetBacked.setDataset(cursor); > + > + ThreadUtils.postToUiThread(new Runnable() { > + @Override > + public void run() { setDataset() should never be called off main thread. Why is this postToUiThread() call necessary here? @@ +87,5 @@ > + try { > + response.put("panelId", panelId); > + response.put("viewIndex", viewIndex); > + } catch (JSONException e) { > + Log.e(LOGTAG, "Could not create refresh message"); Add the Exception instance as the last parameter in the e() call. ::: mobile/android/modules/Home.jsm @@ +205,5 @@ > + "HomePanels:RefreshView": function handlePanelsRefreshView(data) { > + data = JSON.parse(data); > + > + let options = _registeredPanels[data.panelId](); > + let view = options.views[data.viewIndex]; Should probably throw a more meaningful error if the view is not found here.

Addressed lucasr's comments.

Comment on attachment 8403529 [details] [diff] [review] 03 - Pass refresh to JS callback Review of attachment 8403529 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, but let's have lucasr take a final look at it before we land it.

Comment on attachment 8403529 [details] [diff] [review] 03 - Pass refresh to JS callback Review of attachment 8403529 [details] [diff] [review]: ----------------------------------------------------------------- Noticed that the HomePanels:RefreshDone message is gone. Just want to understand why before giving r+. This patch, as is, will show progress forever if you try to refresh while offline for example (unless I'm missing something here?). ::: mobile/android/base/home/PanelRefreshLayout.java @@ +77,5 @@ > public void onRefresh() { > + final JSONObject response = new JSONObject(); > + try { > + response.put("panelId", panelId); > + response.put("viewIndex", viewIndex); nit: I'd turn the key names into constants for better visibility i.e. JSON_KEY_PANEL_ID & JSON_KEY_VIEW_INDEX ::: mobile/android/modules/Home.jsm @@ +207,5 @@ > + > + let options = _registeredPanels[data.panelId](); > + let view = options.views[data.viewIndex]; > + > + if (view === undefined) { I'd go with simply !view here. @@ +210,5 @@ > + > + if (view === undefined) { > + throw "Home.panels: Invalid view for panel.id = " + data.panelId > + + ", view.index = " + data.viewIndex; > + } nit: add empty line here.

(In reply to Lucas Rocha (:lucasr) from comment #41) > Noticed that the HomePanels:RefreshDone message is gone. Just want to > understand why before giving r+. This patch, as is, will show progress > forever if you try to refresh while offline for example (unless I'm missing > something here?). margaret and I decided that the addon shouldn't have call any callback to stop the refresh and that they can simply use HomeProvider.getStorage(ID).save(). We also talked about possibly just ending the refresh animation immediately and in a followup bug exploring an API that allows the addon to control the refresh animation.

(In reply to Josh Dover [:jdover] from comment #42) > Created attachment 8404108 [details] [diff] [review] > 03 - Pass refresh to JS callback > > (In reply to Lucas Rocha (:lucasr) from comment #41) > > Noticed that the HomePanels:RefreshDone message is gone. Just want to > > understand why before giving r+. This patch, as is, will show progress > > forever if you try to refresh while offline for example (unless I'm missing > > something here?). > > margaret and I decided that the addon shouldn't have call any callback to > stop the refresh and that they can simply use > HomeProvider.getStorage(ID).save(). > > We also talked about possibly just ending the refresh animation immediately > and in a followup bug exploring an API that allows the addon to control > the refresh animation. What about the case where the HTTP request to update the dataset fails for some reason (not a rare case in mobile devices) and add-on ends up not calling save() at all? I'm fine with working on this in a follow-up but not doing anything about this situation seems a bit too hand-wavy. Up to you and Margaret.

Comment on attachment 8404108 [details] [diff] [review] 05 - Pass refresh to JS callback Review of attachment 8404108 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Please file a follow-up to handle failed refreshes better in our add-on API to avoid showing progress forever. I'm assuming ibarlow has seen and given his thumbs up on the design side of things.

I agree with filing a follow-up to handle the error cases for ending the refresh animation. I think add-ons should be responsible for ending the animation, whether that is by making sure they make a save() call, or through some other explicit call to end the animation, since the animation will only start if the add-on has registered an onrefresh handler on that view anyway. As a fallback, we could also add a timeout that kills the animation after a certain amount of time. But let's do this in a follow-up.

Ian can you take a look at this pull to refresh UI? Should have pinged you earlier for input on this, but I think it should be pretty straight forward. This adds swipe-to-refresh to home panels that choose to support it. Something we could add in a follow-up is some sort of "Swipe down to refresh" helper text similar to what Gmail does. However some official Google apps don't even do this (ie. Google Now). I'm also not sure where the appropriate place to put that would be? Links to the build and an addon you'll need to install to test it: - APK: https://drive.google.com/file/d/0BwqqgfwYmRg4ZjE2ek81V0JrMGs/edit?usp=sharing - Addon: https://drive.google.com/file/d/0BwqqgfwYmRg4TDNNLWZiU1NGd1U/edit?usp=sharing

Thanks for posting this, Josh, great to see progress happening here. Couple comments below: 1. In this build, the refresh bar currently starts out as a little circle (screenshot: http://cl.ly/image/3x3B2c253h2j) which looks pretty weird -- it should always be squared off 2. Refresh bar should be the same height as the active tab indicator above it 3. Let's try a lighter orange colour for the refresh bar - try #ffc26c 4. Remove springy overscroll -- the only visual feedback should be the growing refresh bar. I did a quick mockup for points 2, 3 and 4 http://cl.ly/image/1U1F0Z2e0f0h

This patch adds a custom implementation for SwipeRefreshLayout to address ibarlow's changes. Unfortunately, I had to lift the source from android-support-v4 due to the changes needed to private methods that cannot be changed by a subclass. I tried simply overriding the constructor to modify mProgressBarHeight and override onTouch() to remove the call to updateContentOffsetTop(), but onTouch uses may of the private fields which would make subclassing complicated. Another benefit is this allows us to land this without having to rev the build bots with the new support library.

Only modifications were PanelRefreshLayout to subclass from GeckoSwipeRefreshLayout rather than SwipeRefreshLayout.

(In reply to Josh Dover [:jdover] from comment #48) > Created attachment 8406321 [details] [diff] [review] > 02 - Add custom SwipeRefreshLayout to remove rubber banding > > This patch adds a custom implementation for SwipeRefreshLayout to address > ibarlow's changes. Unfortunately, I had to lift the source from > android-support-v4 due to the changes needed to private methods that cannot > be changed by a subclass. > > I tried simply overriding the constructor to modify mProgressBarHeight and > override onTouch() to remove the call to updateContentOffsetTop(), but > onTouch uses may of the private fields which would make subclassing > complicated. Another benefit is this allows us to land this without having > to rev the build bots with the new support library. Would it be possible to break this into two patches - one that adds the Android source, followed by one that makes your changes to it? I would like to see exactly what you needed to change.

> Would it be possible to break this into two patches - one that adds the > Android source, followed by one that makes your changes to it? I would like > to see exactly what you needed to change. I totally meant to do this for you and forgot! Here you go!

Comment on attachment 8406390 [details] [diff] [review] 02 - Add GeckoSwipeRefreshLayout lifted from AOSP Review of attachment 8406390 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/widget/GeckoSwipeRefreshLayout.java @@ +47,5 @@ > + * - Uses a rectangle rather than a circle for the SwipeProgressBar indicator. > + * > + * This class also embeds package-access dependent classes SwipeProgressBar and > + * BakedBezierInterpolator. > + */ Technically, this comment should go in your next patch, as should the renaming to GeckoSwipeRefreshLayout. I'd also like to see a link to the original source code. Hopefully we wouldn't need to pull in upstream changes, but it would be good to maintain a reference to where we would want to do that. I'm trusting that besides the renaming, this is just a straight copy/paste of the Android widget.

Comment on attachment 8406393 [details] [diff] [review] 03 - Modify GeckoSwipeRefreshLayout for UX Review of attachment 8406393 [details] [diff] [review]: ----------------------------------------------------------------- Disclaimer: I didn't go through all of the Android widget code to understand the details, but these changes seem reasonable given your comment about what things needed to change. ::: mobile/android/base/widget/GeckoSwipeRefreshLayout.java @@ +52,5 @@ > public class GeckoSwipeRefreshLayout extends ViewGroup { > private static final long RETURN_TO_ORIGINAL_POSITION_TIMEOUT = 300; > private static final float ACCELERATE_INTERPOLATION_FACTOR = 1.5f; > private static final float DECELERATE_INTERPOLATION_FACTOR = 2f; > + private static final float PROGRESS_BAR_HEIGHT = 3; I think we should add comments where we changed something, just to make sure we have a reminder beyond hg blame. @@ -401,5 @@ > float offsetTop = yDiff; > if (mPrevY > eventY) { > offsetTop = yDiff - mTouchSlop; > } > - updateContentOffsetTop((int) (offsetTop)); Instead of deleting this method, I think we should just comment out this call site and explain why we don't want to do this.

Rebased onto fx-team

Rebased onto fx-team

Rebased onto fx-team

Rebased onto fx-team

Rebased onto fx-team

Used -M for creating patch to account for renames

https://hg.mozilla.org/integration/fx-team/rev/15ca8fe42018 https://hg.mozilla.org/integration/fx-team/rev/1e2c46a659c9 https://hg.mozilla.org/integration/fx-team/rev/a2fb529e5852 https://hg.mozilla.org/integration/fx-team/rev/8956eb0b642e https://hg.mozilla.org/integration/fx-team/rev/c1366bb92771

Comment on attachment 8406323 [details] [diff] [review] 04 - Wrap child view in SwipeRefreshLayout Review of attachment 8406323 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by. ::: mobile/android/base/home/PanelLayout.java @@ +352,5 @@ > datasetBacked.setFilterManager(new PanelFilterManager(viewState)); > + > + if (viewConfig.isRefreshEnabled()) { > + view = new PanelRefreshLayout(getContext(), view); > + } This will not play well with the empty view code from bug 965622. We want to allow swipe-to-refresh even when showing the empty view. Please file a follow-up to handle that.

https://hg.mozilla.org/mozilla-central/rev/15ca8fe42018 https://hg.mozilla.org/mozilla-central/rev/1e2c46a659c9 https://hg.mozilla.org/mozilla-central/rev/a2fb529e5852 https://hg.mozilla.org/mozilla-central/rev/8956eb0b642e https://hg.mozilla.org/mozilla-central/rev/c1366bb92771