959513 - [MADAI][Email] Email application should handle low battery scenarios

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Sharaf]

Email application should handle low battery scenarios as explained below 


1.Battery level reach <5% when the application is in foreground and the sync type is "automatic"
	* A popup should be shown to the user with message "Battery too low to update automatically" and the sync type should be changed to manual

2 When the application is launched, and the current battery level is <5% and the sync type is "automatic"
	* A popup should be shown to the user with message "Battery too low to update automatically" and the sync type should be changed to manual

Comment 1

[Wayne Chang [:wchang]]

Hi JuWei,

Can you help out with the UX spec here? Partner will continue the implementation base on your proposal.

Thanks!

Comment 2

[Juwei Huang[:juwei]]

It's okay with me to provide spec for low battery scenarios.
But I am not recommend to change the sync type to "manual" at the moment. I'd rather suspend the automatic sync when in low battery, otherwise users might need to set it back to auto-sync by themselves which is easy to forget.
Hi Andrew, just wondering do we have any implement for this kind of constraint right now? If not, I can deliver specs by next week.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

The e-mail app is not currently battery-aware.  The Battery API is unprivileged and available to us, but not available on the worker (which is not a particularly big deal given how we currently do periodic sync, although it is annoying.)

There are 2 ways the e-mail app's periodic sync uses power:

1) Waking up the e-mail app at all.  We use a timer to wake the e-mail app up at the given sync interval by scheduling the next alarm every time our existing alarm fires.  We close ourselves at the end of the sync process unless the user has explicitly displayed the e-mail app since it was opened.  There is a fair amount of JS to parse, so we shouldn't totally disregard the power usage, although I don't have numbers on this.

2) Performing the sync by using data and opening folders.


The major implementation wrinkles are that we:

A) Have no way to be woken up when the power comes back unless our app is active.  The Battery API generates events for open documents, but they won't cause an app to wake up.  (And there are open issues about scheduling such events if there would be a lot of apps interested in the event.)

B) Cannot request that our alarm only fire when the device has a sufficient battery charge.


Based on Juwei's existing comments, it seems like the right approach given existing platform limitations would be to:

- Reduce the rate at which we wake the app up to sync on lower power.  We could use multiple charge levels/discharge times (the device estimates how long it has until it runs out of juice) to clamp the interval to longer times as the power level decreases.  If we stopped scheduling wake-ups, then periodic sync would effectively be disabled until the user manually starts the e-mail app.

  A potential horrible hack would be to generate a notification that says "Periodic synchronization is disabled for power reasons.  Click or open e-mail app to start again

- If we wake up / are triggered, do not auto-trigger the sync if the power level is below a specific threshold.


It sounds like the two UI points would be:

- If/when to show an in-app/system-wide notification about disabling the sync interval.

- Do we show a message in the settings app to indicate that although the sync interval is set to something, we are ignoring it?  This probably needs to be a different l10n string than the notification.


I'm needinfoing :arog since I think this is also a program manager decision for behaviour.

Comment 4

[psingapati]

Attachment: Lowbattery_Patch.zip

hi Andrew,
As per the comment#3 2 A), B) since we do not have any scheduling events,
we can not clear the alarms and re enable them when the power is back.

How about doing the following things (as per the description)

1. Showing alert/toast to the user "Battery is too low to sync automatically" , when the user launches the application in case of low battery.

2. Checking for the battery level , when a wake up alarm fires. If the battery level is too low , then ignore the alarm ( just like ignoring the non sync type alarms)[as mentioned in the above comment : - If we wake up / are triggered, do not auto-trigger the sync if the power level is below a specific threshold.]

I am also attaching the test patch for the above two points.
Please suggest.

Comment 5

[psingapati]

In addition to the comment#4, in case of point 2 we also need to check: if the device put for charging or not.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(In reply to psingapati from comment #4)
> Created attachment 8373907 [details]
> Lowbattery_Patch.zip
> 
> hi Andrew,
> As per the comment#3 2 A), B) since we do not have any scheduling events,
> we can not clear the alarms and re enable them when the power is back.
> 
> How about doing the following things (as per the description)

It's a reasonable proposal, but UX and Product have the final decision.  :arog is on PTO today, but should be back tomorrow.  

One question I have is whether 5% is too low a threshold.  On most of the devices I have owned, once the device hits 10% the device ends up shutting itself down almost immediately.  (Because I suspect the transform matrix has insufficient data at the low-end about the voltage/time-curve; maybe your devices ship with more predictable batteries and better matrices.)

I'll discuss the patch for the rest; I realize this is a proof-of-concept:

> 1. Showing alert/toast to the user "Battery is too low to sync
> automatically" , when the user launches the application in case of low
> battery.

In mail_common.js we have a Toaster object which should probably be the basis for our in-app notification.  Right now it's a little bit biased towards MailAPI's UndoableOperation abstraction, but if we added a type "alert", we could have that just take an l10n id (or something we prefix "toaster-alert" onto like we do for the other cases).
 
> 2. Checking for the battery level , when a wake up alarm fires. If the
> battery level is too low , then ignore the alarm ( just like ignoring the
> non sync type alarms)[as mentioned in the above comment : - If we wake up /
> are triggered, do not auto-trigger the sync if the power level is below a
> specific threshold.]

The spot where you've added the early return will cause us to fail to reschedule our alarms and will cause the e-mail app to not automatically close.

Probably the simplest solution to avoid breakage is to modify cronsync.js in the back-end and have the alarm notification also indicate that we are in a low-power situation.  Then it can just bypass performing the synchronizations but reschedule the alarms and indicate the cronsync completed without finding any new messages.  This would also allow us to easily-ish adjust our ensureSync() scheduling so that we do more of a back-off.  I'm NI'ing James Burke since he implemented sync and may have even better ideas there and how to reduce battery usage without greatly complicating the control flow.

Is BatteryHelper something that is intended to be contributed to shared/js?

Comment 7

[James Burke [:jrburke]]

My first impulse is to have cronsync-main's ensureSync just check the battery level and expand out the time for next sync based on that value. It already tries to regulate the alarms, make sure they are on track, so this would be one more variable to consider.

The next time ensureSync is called (which is called for every email app startup, as part of CronSync.onUniverseReady), if battery levels are up, the checks ensureSync does now could be adjusted to remove previous alarm and set to normal sync time.

Maybe do an automatic 4 hour jump to sync time in the case of < 10 battery, and do not worry about prompting the user. The low battery indicators already in the phone should be enough to indicate things are shutting down. I expect other things, like getting messages and phone calls, will be a strong motivator for the user to recharge, and if the user is less than 10% and takes longer than two 4 hour intervals, there are bigger problems in play than email sync.

Of course, those calls are really for :arog and Juwei to make, but in general, I am hoping we can isolate any changes to cronsync-main's ensureSync method.

Comment 8

[Juwei Huang[:juwei]]

I'm ok with changing sync time to 4 hours when battery < 10%. So the workflow would be:

* Battery lower than 10%
* Open Email app, a toast indicate "Battery is too low to sync automatically"
* Extending sync time to 4 hrs a time in the background

If Adam also agree with it, I'll deliver the spec later

Comment 9

[psingapati]

Attachment: Bug_959513_Lowbattery_Patch (batteryjs + email changes + screenshot)

hi :asuth, jhuang

I have discussed with :jrburke on IRC, about the suggestions. From end user perspective, it is always good to go with same sync interval that he/she sets manually. It is better to ignore the cronSync for the same interval that user sets, even though it is costing some battery for the application start up.
If we change the value to 4hrs when low battery case , restoring the users previous value will be additional work.

@ :asuth,

I have updated the patch as per your suggestions.
Used Toaster of mail_common.js and added battery level check at back end cronsync.

yes, BatteryHelper is to be contributed at shared/js. Code may be changed , but basic idea will be the same.

Please check and give your opinion.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8375454 [details]
Bug_959513_Lowbattery_Patch (batteryjs + email changes + screenshot)

:jrburke implemented periodic sync and is front-end owner and so is the best reviewer for this. Since :arog hasn't weighed in yet, this can't be a review, so requesting feedback instead.  (I'd defer feedback/review except that we do expect the core of the code to look basically the same no matter what :arog says :)

Comment 11

[Adam Rogers (:arog)]

I agree with the last few comments here; notify the user that we can't auto sync and then skip that interval.   

It's probably best to not insert an artificial wait time for two reasons: 
1) it's more work and 
2) if the user plugs in their device immediately or moves above the low battery threshold, we should sync normally.

As long as we're not changing any user settings, I'm happy with this.

Comment 12

[James Burke [:jrburke]]

Comment on attachment 8375454 [details]
Bug_959513_Lowbattery_Patch (batteryjs + email changes + screenshot)

To most efficiently use our resources, I suggest not trying to do another code pass until we lock down the product and design requirements, since I think it will change the code significantly. Specifically:

I believe the "will not sync now" notification needs to be a system notification and not an in-app toast, as the most likely case where we will hit the low battery condition will be during background sync. 

So if the expectation is to let the user know that we are changing our behavior from what they are expecting, the notification seems the right way to do this.

However, it bothers me that notifications generate sound. This kind of notification is not of a priority that would require immediate user action, just more of a "hey we are changing our behavior because we are trying to be nice".

But right now there is no way to indicate a specific notification does not require sound, so we will just need to roll with this, and if product likes, ask for progress on bug 912645.

So, if the product requirement is to use a notification, then I would likely structure the code change in a way that provides the least amount of disruption to the user and existing code and saves as much battery:

* only generate this notification in the cronsync-main's _checkSyncDone function. This means that we test the battery level after a sync completes. We can just up the threshold to say 7% if the concern is completing a sync may take some battery. By doing the notification here, we can generate a "not syncing any more until battery higher" message, and since it is grouped around when the user may sync a new message, hopefully the noise from this notification is mitigated, can be part of any new mail notification grouping.

* Then, in html_cache_restore, if it has an alarm wakeup, test for low battery there and just stop the rest of the app from starting up, and resetting the alarm for next interval (may mean we need to add more data to the alarm). I want to do a similar check for navigator.onLine in this file, so the battery check would just be another "are conditions right to start up the full app". By doing the work there, we can save some battery by not starting up the main_app work.

* I would avoid using a battery helper for this case and just directly test for battery.level, as we do not need the callback to when the battery gets low, we do that sync, and it means the changes to html_cache_restore stay small for fast startup.

* We need to figure out where the notification goes in the app if the user clicks on it. I would think just the normal message_list startup, but want that explicitly called out in the UX design. And because the notification APIs and their triggering/keeping of old notifications is currently weird, we may need to be sure to explicitly clear this notification if we detect it has been closed.

But again, I think the engineers on this bug should hold off on attempting another code change and doing feedback passes until we have the final design sorted.

We also want to be aware of schedules: as I understand it, we are coming up on our last 2 week sprint for 1.4. If this is to get into that sprint, then we will need to have UX early in that sprint. If that is not set then, then I expect dev review and landing of this feature to slip into the 1.5 timeframe.

Flagging Juwei for design, as that seems like the next step for this ticket. Specifically, dev will need to know:

* Is notification used or in app-toast, and under what conditions it should appear, particularly if the flow and notification behavior described above is not desired.
* The messaging for the notification/toast.

Comment 13

[James Burke [:jrburke]]

Sorry on the schedule, I had it wrong (thanks :asuth):

https://wiki.mozilla.org/FirefoxOS/productivity

We still have two more 1.4 dev sprints.

Comment 14

[Juwei Huang[:juwei]]

Agree with Adam, the main purpose is not to change any user settings.


> Flagging Juwei for design, as that seems like the next step for this ticket.
> Specifically, dev will need to know:
> 
> * Is notification used or in app-toast, and under what conditions it should
> appear, particularly if the flow and notification behavior described above
> is not desired.
> * The messaging for the notification/toast.

Although it sounds reasonable to put string to the system notification, somehow I'm still prefer to app-toast since currently we are not support non-alert notification, and all the notification would be put into notification tray, which is not what we want. But I'm still open to this question if one day we have one type of notifications that is only shown in app and will not display on notification tray.

So my UX suggestion would be:
* Device hits low battery
* Open email app
* A toast pops out a warning "Battery is low, please sync manually."

I'm also happy to see that there's only little coding work for this feature so that we could build into 1.4 asap. Thus I'm quite open to how to implement it as long as the flow is matched and the string pops out properly :)

Comment 15

[psingapati]

hi :jrburke,

Will you please review the attached patch , since in-app toast is fine for the UX team.
and tell me the next course of action in case anything needs to be modified.

Thanks.

Comment 16

[James Burke [:jrburke]]

I believe Juwei has identified the best user experience, by not using an obtrusive system notification and using the in-app toast instead. 

However, it does imply some more involved changes for the email app:

* email sync detects low battery, below 5%

* email app could be closed, so persist a state, call it "syncBatterySuspended" (just a placeholder name for this design description), and set it to "show".

* next time app is visible (could be during that sync flow where low battery was detected, if "syncBatterySuspended" is "show", then show the toast.

* once toast is dismissed, set "syncBatterySuspended" to "shown". Need a state between "show" and "inactive" so that we know we have already shown the low battery warning and do not need to show it again while the battery is below the threshold.

* On next email sync:
  * if battery is below 5% do not do any work, just set the next sync alarm. If "syncBatterySuspended" is not "shown", set it to "show".
  * if battery is above 5%, set "syncBatterySuspended" to "inactive". 

* html_cache_restore should change to support just resetting the alarm and shutting down the app if the battery is below the marker, to help achieve the actual goal of saving energy while in a low battery state.

Some new things that need to be added to the email app:

1) We need a new storage bucket for "toasts that need to be shown", to hold this new "syncBatterySuspended" state.

Right now all IndexedDB access is isolated to the mail DB that is used behind the MailAPI. We have used cookie storage for the fast startup case, but I would not like to use it for this purpose. So perhaps it means pulling in the shared/js/async_storage.js library at this point.

2) We need to be able to display some toast notifications on a visibility change. Longer term, I can see using that same mechanism to notify the user of account problems.

mail_app already does some `document.addEventListener('visibilitychange'`, so triggering the toasts in there may make sense.

End result, I do not believe the existing patch fits the above design criteria. I also would prefer, as mentioned in comment 12, to not use a BatteryHelper, but instead just test battery.level at the points we care about. This will avoid the lazy loading complication in the current patch and it will also lighten the amount of code needed for html_cache_restore.js.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

(In reply to James Burke [:jrburke] from comment #16)
> 1) We need a new storage bucket for "toasts that need to be shown", to hold
> this new "syncBatterySuspended" state.
> 
> Right now all IndexedDB access is isolated to the mail DB that is used
> behind the MailAPI. We have used cookie storage for the fast startup case,
> but I would not like to use it for this purpose. So perhaps it means pulling
> in the shared/js/async_storage.js library at this point.

I'd like to propose an alternate strategy here.  Periodic sync is conceptually a back-end thing that should happen without the main page at all.  The worker implementation is very limited which compels us to involve the main page, and being non-silly we've also put some of that code in the front-end to avoid spinning up the back-end for it to decide to spin right back down.

IndexedDB database opening is effectively constant-cost.  Adding another one is going to delay the back-end's startup even further since startup-time is especially zero sum.  But we already open the database on the front-end anyways, so...

So I propose that we just modify mail-db main a little bit so that it assumes a singleton-ish DB instance that gets opened on first request, which can also be done locally.  In the event of closure, it should still actually close the database, though, as there are tests that open and close the database a few times.  The lazy upgrade stuff works for free as long as the front-end keeps its mitts off of the getConfig() call.

So we just have the front-end open that database and store state with that database.  We will either need to add another table with a specialized version upgrade that affects nothing else, or stick the data in the 'config' table and teach getConfig() to ignore obj.id === 'syncstate' or something like that.  The other account state is actually going to be pretty big, so better for a one-off lookup.


Or cramming it in the cookie is okay with me too.

Comment 18

[psingapati]

Attachment: Lowbattery_apr8.patch

Andrew,

Can you please look at the wip patch for the bug attached and give your feedback.

Thanks

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8403279 [details] [diff] [review]
Lowbattery_apr8.patch

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

Requesting feedback from :jrburke; I think his ideas here will impact my back-end opinions.   But in general :jrburke is still king of cronsync, so he has final call on this in general.  (I just have strong opinions on how to structure stuff in the back-end for long-term goals and how to make sure we can get to those long-term goals.)

::: data/lib/mailapi/cronsync.js
@@ +353,5 @@
> +      // Checking the battery level and returning if it is very low
> +      // and phone is not in charging mode
> +      var battery = navigator.battery || navigator.mozBattery;
> +      var value = Math.round(battery.level * 100);
> +      if(!navigator.battery.charging && (value > 0 && value <= 5)) {

I think :jrburke's proposal was that this specific logic that detects the "not enough battery power" situation would run in the front-end and try and run an optimized code path that does minimal spin-up of the rest of the app.  Really just enough to save the fact that we skipped a sync to the database.

He mentioned html_cache_restore.js.  I think the thing to do would be to modify the logic there that checks for mozHasPendingMessage('alarm') and have it check the battery level.  If the battery is low, then I figure maybe we do something like have it directly call startApp('lowpower').  The idea is then we set the 'data-main' to the contents of 'data-lowpower' (based on the argument).

We would have the lowpower.js thing just be a file that requires maildb-main directly and calls saveConfig directly.

@@ +364,5 @@
> +        debug('low battery done');
> +        return done();
> +      } else {
> +        debug('low battery else');
> +        var lowBattery = {

Conceptually this does seem like this might be the right place for this (in the back-end), but I don't think the battery API gets remoted to the worker.  We may need lightweight propagation of this like we do for online/offline.  James may have more thoughts on this.

::: data/lib/mailapi/mailuniverse.js
@@ +1996,4 @@
>      }
>    },
>  
> +  getConfigRecord: function(key, callback) {

In an ideal world, I don't think we want an explicit API to get this.  Right now the config is automatically populated at startup and is just known to be correct when the universe is created.  We could send the syncState along-side that at startup and (in the future) update it and provide explicit notifications when it changes.

Thinking about other similar situations, I think we have:

- transient server problems, especially as it relates to background message sending.  But those are account-specific and we already have an explicit account problems mechanism for that.

- being offline which will affect all accounts and the former too.  I guess this is pretty similar.

So I guess another question for :jrburke is whether it makes sense to create a concept of "global" problems and have a concept of tracking those problems as reported (perhaps tracking when they were reported so if they were reported long ago in the past, we do in fact report them again since the user may have forgotten.)  Or maybe we should just treat this as a very specific case.

Comment 20

[James Burke [:jrburke]]

Comment on attachment 8403279 [details] [diff] [review]
Lowbattery_apr8.patch

My thinking about html_cache_restore.js was to the following work directly in `html_cache_restore`, without booting into the main app:

* if document.hidden === true:
* if below low battery mark:
* reset the alarm, then quit the app, don't even bootstrap into any main app logic.

The idea was to limit usage of battery as much as possible. However, by needing to show an in-app toast for the "not syncing because low battery" message, that may have created a wrinkle.

Although, perhaps not: if document.hidden, that means we started up in a background process, so we don't really need to boot into the main app, the user would not see the message anyway.

The only wrinkle might be the case where the phone dropped below 5% battery while in the person's pocket, and we would have tried some syncs, but did the quick alarm reset and shutdown in html_cache_restore. If the user then plugs in to power:

* and it goes above 5% and the next sync happens before they open email, it is all OK. No need to tell the user.
* it does not get above 5% and the user opens email. In this case, we always confirm the sync alarm targets, and at that point can detect we are not syncing and we can show the toast.

So I think that all works out for the html_cache_restore. The unfortunate part is about inlining some alarm work, but I would like to see what a patch for that looks like before working out a pathway inside the app.

That brings us to why we need to persist some state:

* the above scenario, but the user opens email two or more times before the battery is above 5% when the email app is launched. In that case, we only want to show the toast once.
* user is running email in foreground or background, and the battery drops below 5% during that time. We need to inform them once email is back in a visible state of the toast, but only once.

For this, we need to persist state about either needing to show or have shown the low battery toast.

I leave it to :asuth for how that info should be stored and to what table. On the app level, my first impulse is to see a `model` module event triggered for 'accountAlert', or something like that. Perhaps `model` internally would look at any config info on the accounts info as it was seen by `model` and then `model` triggers fine grained events for any alerts on the account?

I would expect `mail_app` to listen for that type of alert, and then use the toast mechanism to use the toast section for it. I can see `mail_app` posting back to `model` when the toast has been dismissed, and then `model` can inform the db to change the config state.

Those are mostly question marks for :asuth, if that fits in with how he would want the data stored/surfaced from gelam. I know we want to move more of what `model` does into a main-script interface in gelam, but the above is how I would see it working for the immediate future without that larger refactor.

There may be some new UI/UX we need too for the toast mechanism, for showing which account the toast is for, and probably a way for the toast to trigger launching into that account if not the current one, but we can refine that once we have the plumbing hooked up and data flowing.

Comment 21

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8403279 [details] [diff] [review]
Lowbattery_apr8.patch

Given:
- the low relative importance of the feature from the MoCo productivity team perspective
- the high relative complexity of the UX desired given the current divide between back-end and front-end
- the effort expended so far and the future extrapolated additional effort required from the MoCo productivity team engineering staff to get this in a test-able and landed state

I think we need to shelve this for now.

Comment 22

[BMO Automation]

Firefox OS is not being worked on