Closed Bug 987427 Opened 10 years ago Closed 10 years ago

[B2G][Calendar] Notifications for calendar alarms set for 2 hours before the event are incorrect.

Categories

(Firefox OS Graveyard :: Gaia::Calendar, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, b2g-v1.3 affected, b2g-v1.3T affected, b2g-v1.4 affected)

VERIFIED FIXED
2.0 S1 (9may)
tracking-b2g backlog
Tracking Status
b2g-v1.3 --- affected
b2g-v1.3T --- affected
b2g-v1.4 --- affected

People

(Reporter: demerick, Assigned: mmedeiros)

References

Details

(Whiteboard: Permafail, [priority][p=3])

Attachments

(3 files)

Attached file logcat.txt
Description:
If the user has an alarm set for 2 hours before an event the notification will incorrectly state 'starting in an hour'.

Repro Steps:
1) Update a Buri to BuildID: 20140324000202
2) Open Calendar app from the home screen
3) Tap '+' in the upper right hand corner
4) Set the start time to 2 hours and 5 minutes from the current time
5) Make sure the end time is after the start time
6) Set a reminder alarm for '2 hours before'
7) Exit the calendar app and wait 5 minutes
8) View the notification

Actual:
The notification incorrectly states 'starting in an hour'.

Expected:
The notification should say 'starting in 2 hours'.

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140324000202
Gaia: 730670951e40b2317a167fcd07c398bb662d6e87
Gecko: a44f8b39c2c8
Version: 30.0a2
Firmware Version: v1.2-device.cfg

Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/7500/
See attached: logcat, screenshot
Attached image Example screenshot
This issue also occurs on the Buri v 1.3.0 Mozilla RIL

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140324004001
Gaia: f7742fb4929cc57c9f72955ce5cebb8279745ac0
Gecko: e42b778a010f
Version: 28.0
Firmware Version: v1.2-device.cfg

The notification for an alarm set for 2 hours before the event states 'starting in an hour'.
Whiteboard: burirun1.4-2 → burirun1.4-2, 1.3tarakorun2
can we get triage to look at this?   displaying wrong information in notifications needs fixing.
blocking-b2g: --- → 1.4?
blocking-b2g: 1.4? → 1.4+
Thanks, Dylan! 
By the way, is it possible to find someone to look at this issue? Thank you.
Flags: needinfo?(doliver)
Is it possible if we move this bug to 2.0?
Renoming per comment 4.
blocking-b2g: 1.4+ → 1.4?
I'll take a look at this issue to understand better what is going on, just need to finish a couple tasks before. My bet (without looking at the code or trying to reproduce it) is that we might have some "rounding" issue (alarm triggered 1h59m50s before event rounded down to 1h). If anyone else have any "insight" of other potential causes, please let me know.
Flags: needinfo?(doliver) → needinfo?(mmedeiros)
confirmed that it's a rounding issue. Any value smaller than 2 hours returns "in an hour"... and you guys wont believe on the difference that is causing the rounding error. `Date.now()` returned `1397684100109` and the event start was `1397691300000`, the difference between these values is `1.9999697222222221` hours.

  var df = navigator.mozL10n.DateTimeFormat();
  df.fromNow(new Date(Date.now() + 1.99 * 60 * 60 * 1000)) === "in an hour"

now we have a "big" dilemma, should we "fix" the gaia/shared/js/l10n_date.js (round difference to closest minute) or should we simply do a quick hack on calendar (controllers/alarm.js)?

the hack on calendar can be as simple as adding `begins.setSeconds(25)` - 25 seconds is more than enough to avoid rounding issues, difference is usually less than 1 second.
Flags: needinfo?(mmedeiros)
(In reply to Kevin Hu [:khu] from comment #4)
> Is it possible if we move this bug to 2.0?

Agreed that we won't block 1.4 for it so 1.4+ was the wrong call. Marking it priority for a quick fix. The rounding bug happens with all of the time options but it's clearly worst if you choose the 2 hour reminder as the resulting "one hour" notification text is just wrong. 

Other options are less severe in terms of how far off we are -- a 1 hour reminder becomes "starting in 59 minutes" and a 1 day reminder is "starting in 23 hours".
blocking-b2g: 1.4? → backlog
Whiteboard: burirun1.4-2, 1.3tarakorun2 → burirun1.4-2, 1.3tarakorun2, [priority]
Thank you, Dylan! This is super helpful.
Assignee: nobody → mmedeiros
Whiteboard: burirun1.4-2, 1.3tarakorun2, [priority] → burirun1.4-2, 1.3tarakorun2, [priority][p=3]
Target Milestone: --- → 1.4 S6 (25apr)
quick hack on calendar app that fixes the problem (update seconds and milliseconds to match current time).. unsure if we should change the l10n lib instead.
Attachment #8410780 - Flags: review?(gaye)
Comment on attachment 8410780 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561

After lots of being unsure, I am ok with this. Thanks Miller!
Attachment #8410780 - Flags: review?(gaye) → review+
Let's also go ahead and suggest that the shared date code do something smarter.
Comment on attachment 8410780 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561

Gareth, sorry for the extra work but based on your comments I decided to change the logic and "fix" the l10n_date code instead. If the hack needed that much explanation it means it wasn't a simple "fix"...
Attachment #8410780 - Flags: review+ → review?(gaye)
Comment on attachment 8410780 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561

Fabien, can you please review my changes? I initially did a quick hack on the calendar app itself but I feel this logic should be handled by the l10n_date instead. It will probably avoid headaches. Thanks.
Attachment #8410780 - Flags: review?(fabien)
See Also: → 875586
Comment on attachment 8410780 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561

Calendar part/test still looks great.
Attachment #8410780 - Flags: review?(gaye) → review+
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Comment on attachment 8410780 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561

Hi :stas, I need your review on this patch. Thanks!
Attachment #8410780 - Flags: review?(fabien) → review?(stas)
Comment on attachment 8410780 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561

r=me with a comment that I'd like to see addressed.  

l10n_date.js might be used by third-party app developers to create localized cross-browser webapps. For instance, it is mentioned on https://developer.mozilla.org/en-US/Apps/Build/Localization/Localizing_Firefox_OS_Apps#l10n_date_Script. As much as I like them and find them useful, I'd prefer to avoid using non-ES5 methods like Math.sign.

Maybe re-write this code like this?

if (Math.abs(secDiff) > 60) {
  secDiff = secDiff > 0 ? Math.ceil(secDiff) : Math.floor(secDiff);
}

Also, it looks like the test doesn't have to be in an else block and absDiff could be calculated closer to where it's used.
Attachment #8410780 - Flags: review?(stas) → review+
Whiteboard: burirun1.4-2, 1.3tarakorun2, [priority][p=3] → Permafail, [priority][p=3]
landed into master: https://github.com/mozilla-b2g/gaia/commit/badc73ee7f108fa631150bded0cc57e92aad810e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
[Environment]
Gaia      c1a8cbaac1d921cfb50e3a2600720b75cf5afabd
Gecko     https://hg.mozilla.org/mozilla-central/rev/20ca234fd62b
BuildID   20140511160202
Version   32.0a1
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013


[Result]
PASS
Status: RESOLVED → VERIFIED
See Also: → 1021833
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: