Closed Bug 999756 Opened 5 years ago Closed 5 years ago

Empty view flashes while view is refreshing

Categories

(Firefox for Android :: Awesomescreen, defect, P2)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
fennec 31+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

In my add-on's onrefresh handler, I make a deleteAll call then save a new set of items, and I can see the empty view flash in between.

The solution to this bug may overlap with the solution to bug 999483.
(In reply to :Margaret Leibovic from comment #0)
> In my add-on's onrefresh handler, I make a deleteAll call then save a new
> set of items, and I can see the empty view flash in between.
> 
> The solution to this bug may overlap with the solution to bug 999483.

The flashing is most likely caused by the quick sequence of deleteAll() and save() calls on the add-on side (which will trigger two refreshes on the panel). An easy way to workaround that could be to delay the HomePanels:RefreshDataset message for a few milliseconds (on the HomeProvider.jsm side) to avoid unnecessary successive refreshes.
Priority: -- → P2
Assignee: nobody → margaret.leibovic
Went with the suggestion to delay sending the messages to Java. Testing with my Instagram panel, I tried 50ms, and found I was still seeing the flashing, so I went with 100ms, but this doesn't feel like a very robust solution, since it depends on how quickly the save transaction takes.

Maybe a better solution would be to allow add-ons to batch actions together, so that they can specify "this delete and save call are part of the same transaction".
Attachment #8419045 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8419045 [details] [diff] [review]
Delay sending "HomePanels:RefreshDataset" message to Java

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

Looks good with the suggested change.

(In reply to :Margaret Leibovic from comment #2)
> Created attachment 8419045 [details] [diff] [review]
> Delay sending "HomePanels:RefreshDataset" message to Java
> 
> Went with the suggestion to delay sending the messages to Java. Testing with
> my Instagram panel, I tried 50ms, and found I was still seeing the flashing,
> so I went with 100ms, but this doesn't feel like a very robust solution,
> since it depends on how quickly the save transaction takes.

Yeah, when I suggested the timer idea, I didn't account for the fact that these calls are asynchronous. I still think it makes sense to have this timer though as it will avoid redundant refresh requests in certain cases.

> Maybe a better solution would be to allow add-ons to batch actions together,
> so that they can specify "this delete and save call are part of the same
> transaction".

Agree. Maybe it would be simpler to just add a new method for the deleteall-and-savenew case? Your idea for an API to group operations together sounds good too.

::: mobile/android/modules/HomeProvider.jsm
@@ +289,5 @@
> + */
> +function refreshDataset(datasetId) {
> +  let timer = gRefreshTimers[datasetId];
> +  if (timer) {
> +    timer.cancel();

Given that the goal here is to simply avoid redundant successive refreshes, I'd expect the refreshDataset() call to simply bail if there's already a scheduled timer for the given datasetId instead of cancelling the existing timer and postponing the refresh a bit more.
Attachment #8419045 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #3)

> > Maybe a better solution would be to allow add-ons to batch actions together,
> > so that they can specify "this delete and save call are part of the same
> > transaction".
> 
> Agree. Maybe it would be simpler to just add a new method for the
> deleteall-and-savenew case? Your idea for an API to group operations
> together sounds good too.

I like the idea to add a new API for deleteall-and-savenew, since this pattern is something we're promoting in our demo add-ons (and a very common use case).

> ::: mobile/android/modules/HomeProvider.jsm
> @@ +289,5 @@
> > + */
> > +function refreshDataset(datasetId) {
> > +  let timer = gRefreshTimers[datasetId];
> > +  if (timer) {
> > +    timer.cancel();
> 
> Given that the goal here is to simply avoid redundant successive refreshes,
> I'd expect the refreshDataset() call to simply bail if there's already a
> scheduled timer for the given datasetId instead of cancelling the existing
> timer and postponing the refresh a bit more.

When implementing this, I was thinking of the solution as "only send a refresh message once a series of updates is done", so postponing the refresh a bit more would catch a series of more than 2 updates. However, since we're really just trying to optimize for this delete-then-save case, tweaking the solution to be more like "only send a refresh message once every N ms" would be fine as well (and make the refresh happen more quickly).
Instead of creating a whole new API, how about adding a parameter to the save method? This won't break exiting add-ons, but we can update our docs and boilerplate add-on to take advantage of this. This also has the benefit of being part of the same transaction as the save call, so if we fail to save items, the user won't be left with nothing.

I was also contemplating making this the default behavior, but that probably wouldn't be expected.
Attachment #8419537 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8419537 [details] [diff] [review]
Add parameter to HomeStorage.save to allow replacing existing items

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

Sounds like a good idea. Just wonder if a boolean argument is the best way to expose it. Maybe an options object with a 'replace' boolean key is more future-proof? No strong opinions here. Up to you.
Attachment #8419537 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 8419537 [details] [diff] [review]
> Add parameter to HomeStorage.save to allow replacing existing items
> 
> Review of attachment 8419537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sounds like a good idea. Just wonder if a boolean argument is the best way
> to expose it. Maybe an options object with a 'replace' boolean key is more
> future-proof? No strong opinions here. Up to you.

Sure, I'll update it to an options object, that's a good idea.
It would be nice to uplift this, since the flashing can look pretty bad.
tracking-fennec: --- → ?
https://hg.mozilla.org/mozilla-central/rev/6112f207dbc6
https://hg.mozilla.org/mozilla-central/rev/463a0f7fb61b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
tracking-fennec: ? → 30+
Comment on attachment 8419045 [details] [diff] [review]
Delay sending "HomePanels:RefreshDataset" message to Java

I realized this only need to go on aurora, not beta, since we don't have the onrefresh API on beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): home panels refresh API
User impact if declined: users can see empty views flash when data in lists refreshes
Testing completed (on m-c, etc.): landed on m-c 5/13
Risk to taking this patch (and alternatives if risky): low-risk, small change to how we refresh home panels, only affects add-on panels 
String or IDL/UUID changes made by this patch: none
Attachment #8419045 - Flags: approval-mozilla-aurora?
tracking-fennec: 30+ → 31+
Attachment #8419045 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.
Blocks: 1014030
You need to log in before you can comment on or make changes to this bug.